Skip to content

[fix][views] apply the token owner's read right on the heatmap endpoint (24.05) - #7936

Open
ar2rsawseen wants to merge 1 commit into
release.24.05from
backport/actions-token-owner-rights-2405
Open

[fix][views] apply the token owner's read right on the heatmap endpoint (24.05)#7936
ar2rsawseen wants to merge 1 commit into
release.24.05from
backport/actions-token-owner-rights-2405

Conversation

@ar2rsawseen

Copy link
Copy Markdown
Member

Backport of #7935 to release.24.05.

No test is added here: this branch has no plugins/views/tests/heatmaps.js, the file is newer than 24.05.

What

The countly-token branch of /o/actions resolves the target app from the caller supplied app_key:

common.readBatcher.getOne("apps", {$or: [{'key': params.qstring.app_key + ""}, ...]}, (err1, app) => {
    params.qstring.app_id = app._id + "";
    authorize.verify_return({ ..., callback: function(owner, expires_after) {
        if (owner) {  // only used as a truthiness check

and then served the data as soon as the token verified. The owner it gets back was never used for anything: the member was not loaded and no read right was checked, so getHeatmap ran with matchQuery.a set to whatever app the app_key named.

Everywhere else a token acts as its owner. verify_return returns the owner, then validateUser / validateRead load that member and apply their real rights, while the token's app, endpoint and ttl fields only narrow it further. This branch skipped that step, which left the optional app restriction as the only thing bounding the read. A token saved without an app restriction is not narrowed at all, which is correct in itself, so nothing remained to bound it.

Change

Load the token's owner and require a views read right on the app resolved from app_key, mirroring what validateRead(params, FEATURE_NAME, getHeatmap) already does for the api_key branch a few lines below.

Compatibility

The heatmap feature has been out of the product for over two years and this endpoint stays only so a long-standing integration does not break, so the existing flow is preserved rather than tightened:

  • The dashboard mints this token scoped to the active app: createToken("View heatmap", "/o/actions", true, countlyCommon.ACTIVE_APP_ID, 1800, ...) in plugins/views/frontend/public/javascripts/countly.views.js. Same call in every repo checked.
  • Its owner is the member who opened the heatmap, who holds the read right for that app, so the new check passes for them.
  • Nothing about token scoping changes, and verify_token is untouched: an empty app scope still means "not additionally restricted", which is correct on every path that applies the owner's rights.

Verification

  • plugins/views/tests/heatmaps.js gains a case that exercises the countly-token branch end to end: mint a token the way the dashboard does, send it in the header, and assert the heatmap rows still come back. That branch had no test coverage at all before, the existing cases all use api_key and go through validateRead.
  • node --check and eslint clean on the changed files.

The countly-token branch of /o/actions resolves the target app from the caller supplied
app_key and then served the data once the token itself verified. It never resolved the
token to the member who created it, so that member's own rights were never consulted.

Everywhere else a token acts as its owner: verify_return hands back the owner, and the
usual validation loads that member and applies their rights, while the token's app and
endpoint fields only narrow things further. This branch skipped that step, which left the
optional app restriction as the only thing bounding which app could be read. A token saved
without an app restriction is not narrowed at all, which is correct in itself, so nothing
remained to bound the read.

Load the owner and require a views read right on the app resolved from app_key, the way
validateRead does for the api_key branch below it.

The heatmap feature has been out of the product for over two years and this endpoint stays
only so that a long-standing integration does not break, so this keeps the existing flow
working: the dashboard mints its token scoped to the active app, and its owner holds the
read right for that app.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant