[BUG FIX] [MER-5351] replace exact admin role checks with hierarchical permission helpers#6293
Open
andersweinstein wants to merge 1 commit intomasterfrom
Open
[BUG FIX] [MER-5351] replace exact admin role checks with hierarchical permission helpers#6293andersweinstein wants to merge 1 commit intomasterfrom
andersweinstein wants to merge 1 commit intomasterfrom
Conversation
Contributor
Contributor
AI Review — securityNo issues found |
Contributor
AI Review — performanceRepeated role map lookups in hot authorization checksfile: lib/oli/accounts.ex |
Contributor
AI Review — uiNo issues found |
Contributor
AI Review — elixirNo issues found |
Contributor
PrivSignal ReportTop-level result:
Links: artifact bundle
Reason Events
Sample Of All Events
|
Contributor
|
Preview deployed to: https://pr-6293.plasma.oli.cmu.edu |
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
1 participant
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.
This PR replaces exact admin-role checks with hierarchical permission helpers across Torus so higher admin levels inherit lower-level capabilities consistently.
Previously, a number of call sites used
has_admin_role?(..., :content_admin), which excluded account_admin users from behavior that was clearly intended to mean “content admin or higher.” This caused erroneous behavior such as account admins missing controls on pages they could otherwise access. This change standardizes permission checks on the level-based functionsat_least_content_admin?/1andat_least_account_admin?/1,addingis_system_admin?/1for the remaining system-admin-specific cases, and removeshas_admin_role?/2entirely.Tests
Tests were updated to reflect the level-based model, and a regression test was added for /authoring/projects to verify that account admins now see and can use the same “Show all projects” control available to content admins.
Validation:
Notes
is_admin?/1is currently equivalent toat_least_content_admin?/1, since both return true for content_admin, account_admin, and system_admin. This change still usesat_least_content_admin?/1at authorization sites where the intent appeared to be specifically “content admin or higher,” because that makes the minimum required privilege explicit and preserves the role-level semantics in the code instead of collapsing them into a generic “some admin” check.The problematic behavior was specifically with
has_admin_role?(..., :content_admin). Uses ofhas_admin_role?(..., :account_admin)were not causing incorrect authorization under the current role model, because system_admin was still included correctly. Those call sites are being replaced anyway withat_least_account_admin?/1to standardize on one consistent pattern for level-based admin checks and to make the hierarchical semantics of admin roles explicit everywhere.has_admin_role?(..., :system_admin)similarly worked correctly, but has been replaced in the interest of standardizing on one set of predicates and removing the potentially error-pronehas_admin_role?function.