fix(reports): authorize non-core report targets on create and update (24.05) - #7865
Open
ar2rsawseen wants to merge 3 commits into
Open
fix(reports): authorize non-core report targets on create and update (24.05)#7865ar2rsawseen wants to merge 3 commits into
ar2rsawseen wants to merge 3 commits into
Conversation
/i/reports/create and /i/reports/update authorized the target apps only when the effective report_type was "core". A non-core report_type names the plugin that owns the report, and those reports were inserted and updated without any authorization of the object they point at. The authorization path for this already existed but was unreachable. The dashboards plugin implements /report/authorize, which checks view access to the dashboard a report renders, while the reports-side caller that invoked it (validateNonCoreUser) had been commented out. Restores that caller and gates both create and update on its result. Update authorizes the merged stored-plus-payload report rather than the payload alone, so a partial update cannot leave an unauthorized target in place, and repointing an existing report at a different target is checked too. A copy is passed so the authorize flag, which is only how the dispatch returns its result, never reaches the written document. Fails closed, matching the original intent of the commented-out code: a report type whose plugin does not answer /report/authorize is not authorized. Only "dashboards" implements it today, in this repo and in countly-platform, so a plugin adding a new report type needs to implement the event for it. Also drops validateCoreUser, which was commented out and superseded by the inline per-app check that uses the reports feature permission rather than plain app membership. Co-Authored-By: Claude <noreply@anthropic.com>
Real end-to-end coverage through the HTTP endpoints with a real non-admin member, so the whole path is exercised rather than the branch in isolation: validateCreate, the report_type branch, the /report/authorize dispatch into the dashboards plugin, and the insert. - a member with reports rights but no view access to a private dashboard is refused when scheduling a report against it - the same member succeeds for a dashboard they own, which proves the authorize dispatch resolves rather than the request simply failing - the authorize flag does not appear on the stored report - a core report for an app the member has rights on still succeeds, so the restructuring did not change the core path Not executed locally: the harness needs COUNTLY_TEST_API_KEY_ADMIN and COUNTLY_TEST_APP_ID. CI runs them in test-api-plugins. Co-Authored-By: Claude <noreply@anthropic.com>
Dashboard permissions are deliberately separate from app permissions: a dashboard can be shared with a member who has no access to the apps its widgets reference, and they are meant to be able to view it and schedule a report for it. The suite proved a member is refused a dashboard they cannot view, and allowed their own, but nothing covered the case in between, which is the one a tightening change could plausibly break. This uses the same admin-owned dashboard the member was already refused, changing only the share, so the test isolates the share as the deciding factor rather than app rights. Also generalised the cleanup to delete a list of dashboards rather than two hard-coded ids. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #7864 to
release.24.05. Applied cleanly; only the CHANGELOG heading needed resolving.What changes
/i/reports/createand/i/reports/updateauthorized the target apps only when the effectivereport_typewas"core". A non-corereport_typenames the plugin that owns the report, and those reports were inserted and updated without any authorization of the object they point at.The authorization path already existed but was unreachable: the dashboards plugin implements
/report/authorize, which checks view access to the dashboard a report renders, while the reports-side caller that invoked it (validateNonCoreUser) had been commented out.Restores that caller and gates create and update on its result. Update authorizes the merged stored-plus-payload report, so a partial update cannot leave an unauthorized target in place and repointing an existing report is checked too. A copy is passed so the
authorizedflag never reaches the written document. Fails closed, matching the original intent: a report type whose plugin does not answer the event is not authorized, and only"dashboards"implements it today.Also drops
validateCoreUser, which was commented out and superseded by the inline per-app check.Tests
Real end-to-end coverage through the HTTP endpoints with a real non-admin member: refused for a private dashboard they cannot view, allowed for one they own, the
authorizedflag absent from the stored report, and a core report still succeeding as a control.Not executed locally, since the harness needs
COUNTLY_TEST_API_KEY_ADMINandCOUNTLY_TEST_APP_ID. CI runs them intest-api-plugins.🤖 Generated with Claude Code