security(rights): require app admin access to be granted, not merely unsaid (24.05) - #7873
Open
ar2rsawseen wants to merge 1 commit into
Open
security(rights): require app admin access to be granted, not merely unsaid (24.05)#7873ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
…unsaid
hasAdminAccess started from `true` and only cleared when an entry for the app
existed with `all` falsy. An app the member had no entry for never cleared it,
so every unlisted app came back as one they administer. Master already starts
from false and requires every operation to carry `all`, so this branch had been
diverging on the meaning of app admin access.
Everything reached through this helper inherited the fail-open. hasCreateRight,
hasReadRight, hasUpdateRight and hasDeleteRight all fall through to it, so they
passed for apps nobody had granted. Proven on this branch before the change:
for a member holding only feature-scoped alerts rights on appA,
hasUpdateRight("alerts", "appB") and hasReadRight("alerts", "appB") both
returned true. After it they are false, while appA stays true.
The practical effect was that the cross-app authorization work on this branch
was inert. The hooks fix merged in #7863 calls these helpers at four sites, and
the alerts create guard calls hasCreateRight; all of them were being answered
"yes" for apps the caller held nothing on. This repairs those in place, and is
a prerequisite for the alerts stored-app backport.
Note this does not widen anything. Access that was explicitly granted is
unaffected: an app listed in permission._.a returns true from the early return,
untouched here, and an app carrying all:true across c/r/u/d still returns true.
What stops passing is admin access the permission document never expressed,
either no entry at all or a partial grant that does not cover all four
operations. permission._.a is what the UI writes when somebody is made an app
admin, so the grant path users actually go through is unchanged.
Verified against the full unit suite: 101 passing / 5 failing before, 112
passing / the same 5 failing after. Those 5 (validateArgs extra types, countly
request) are pre-existing and unrelated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an authorization fail-open in rights.hasAdminAccess on the release.24.05 branch so that app-admin access must be explicitly granted (matching master), rather than being implicitly assumed when no permission entry exists for an app. This prevents feature-scoped rights checks (hasCreateRight/hasReadRight/hasUpdateRight/hasDeleteRight) from incorrectly succeeding on unrelated apps.
Changes:
- Change
hasAdminAccessto default to “not admin” and require explicitall: truegrants (or existing admin mechanisms like_.a, legacyadmin_of, orglobal_admin). - Add unit tests covering explicitly granted admin access vs. ungranted/partial/feature-scoped access scenarios.
- Add unit tests demonstrating the corrected behavior of per-feature rights helpers that depend on
hasAdminAccess.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| api/utils/rights.js | Makes hasAdminAccess fail-closed unless admin access is explicitly granted, aligning branch behavior with master. |
| test/unit-tests/api.utils.rights.hasAdminAccess.js | Adds regression tests for hasAdminAccess and downstream per-feature rights helpers to prevent reintroducing the fail-open. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1153
to
+1156
| if (passesAllRules) { | ||
| isAdmin = true; | ||
| } | ||
|
|
Comment on lines
+72
to
+90
| var member = { | ||
| permission: { | ||
| _: {a: [], u: [["appA"]]}, | ||
| c: {appA: {all: false, allowed: {alerts: true}}}, | ||
| r: {appA: {all: false, allowed: {alerts: true}}}, | ||
| u: {appA: {all: false, allowed: {alerts: true}}}, | ||
| d: {} | ||
| } | ||
| }; | ||
| it("keeps the right on the app the member holds it for", function() { | ||
| Boolean(rights.hasUpdateRight("alerts", "appA", member)).should.equal(true); | ||
| Boolean(rights.hasReadRight("alerts", "appA", member)).should.equal(true); | ||
| Boolean(rights.hasCreateRight("alerts", "appA", member)).should.equal(true); | ||
| }); | ||
| it("refuses the right on an app the member holds nothing for", function() { | ||
| Boolean(rights.hasUpdateRight("alerts", "appB", member)).should.equal(false); | ||
| Boolean(rights.hasReadRight("alerts", "appB", member)).should.equal(false); | ||
| Boolean(rights.hasCreateRight("alerts", "appB", member)).should.equal(false); | ||
| }); |
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.
hasAdminAccesson this branch starts fromtrueand only clears when an entry for the app exists withallfalsy. An app the member has no entry for never clears it, so every unlisted app comes back as one they administer.masteralready starts fromfalseand requires every operation to carryall, so the two branches had drifted apart on what "app admin access" means.Everything reached through this helper inherits the fail-open:
hasCreateRight,hasReadRight,hasUpdateRightandhasDeleteRightall fall through to it.Measured on this branch, for a member holding only feature-scoped
alertsrights onappA:hasUpdateRight("alerts", "appB")hasReadRight("alerts", "appB")hasAdminAccess(member, "appB", "u")hasUpdateRight("alerts", "appA")Why this matters beyond the helper
The cross-app authorization work on this branch is currently inert. The hooks fix merged in #7863 calls these helpers at four sites, and the alerts create guard calls
hasCreateRight. All of them were being answered "yes" for apps the caller holds nothing on. This repairs them in place, and is a prerequisite for the alerts stored-app backport.This does not widen anything, and does not remove granted access
The early return for
permission._.ais untouched, and that is what the UI writes when somebody is made an app admin. So the grant path users actually go through is unchanged:_.a(real app-admin grant)all: trueallfalsyadmin_ofr, no c/u/d)What stops passing is admin access the permission document never expressed. On upgrade, the members affected are exactly those who were being treated as admins of apps nobody granted them.
Verification
validateArgs extra types,Countly Request) are pre-existing and unrelated, and fail identically on an unmodified checkout of this branch.test/unit-tests/api.utils.rights.hasAdminAccess.js, covering both halves: every shape of explicitly granted access still passes, and every shape of ungranted access is refused. Included deliberately, because the risk here is over-tightening just as much as under-tightening.The implementation is byte-identical to master's, rather than a reimplementation, so the two branches stop diverging on this.
Follow-up
The alerts stored-app backport for this branch depends on this landing first; its tests fail against the current
hasAdminAccess.🤖 Generated with Claude Code