refactor(authz): validate Studio button view access and use useCourseUserPermissions in course-scoped contexts#3151
Conversation
…avoid unscoped queries in HelpSidebar
|
Thanks for the pull request, @dcoa! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
c122bda to
26e451d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3151 +/- ##
========================================
Coverage 95.77% 95.77%
========================================
Files 1396 1398 +2
Lines 33399 33427 +28
Branches 7603 7866 +263
========================================
+ Hits 31988 32016 +28
Misses 1367 1367
Partials 44 44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } = updateMutation; | ||
|
|
||
| const isLoading = isPendingSettingsStatus || (isAuthzEnabled && isLoadingUserPermissions); | ||
| const isLoading = isPendingSettingsStatus || isLoadingUserPermissions; |
There was a problem hiding this comment.
Why are we removing isAuthzEnabled like in this like but some others like L107 we're not? What's special about this specific case?
There was a problem hiding this comment.
I think I understand. We're checking if it's loading we don't necessarily need to check if authz is enabled, other components might do this for us.
| } = useUserPermissions(permissions, isAuthzEnabled); | ||
| } = useUserPermissions(permissions, shouldValidatePermissions); | ||
|
|
||
| const isLoading = isWaffleFlagsLoading || (isAuthzEnabled && isLoadingUserPermissions); |
There was a problem hiding this comment.
Same question of L70, we aren't we removing isAuthzEnabled here?
| ); | ||
|
|
There was a problem hiding this comment.
This makes me wonder about https://github.com/eduNEXT/frontend-app-authoring/blob/26e451dd22b4cbea4e323508836e8ecc30bf5777/src/course-outline/outline-sidebar/info-sidebar/CourseInfoSidebar.tsx#L124. Can't we only ask for the permission and that will take the flag into account?
| } | ||
|
|
||
| if (isAuthzEnabled && getConfig().ADMIN_CONSOLE_URL) { | ||
| if (isAuthzEnabled && adminConsoleBaseUrl && canViewConsoleTeams) { |
There was a problem hiding this comment.
Do we need the isAuthzEnabled or the checks in L54-L66 are enough?
There was a problem hiding this comment.
I'm a bit confused on when to check fo the flag and when we don't.
Description
This PR updates all course-scoped permission checks to use
useCourseUserPermissions, ensuring a consistent permissions model across course-specific areas.Additionally, it guards the Roles & Permissions button on the Home page so it is only rendered for users with permission to view team members in at least one course or library.
Implemented changes
useCourseUserPermissions. Replaced the repeateduseUserPermissions + useWaffleFlags + manual isAuthzEnabled/loading/fallbackboilerplate in course-scoped contexts with theuseCourseUserPermissionshook, which folds the waffle-flag gate, loading state, and "authz-disabled ⇒ allow (legacy behavior)" fallback into one place. Applied to:view_course_teamon any course orview_library_teamon any library.useCourseUserPermissionsnow only issues the permission query when both AuthZ is enabled and a courseId is present (shouldValidatePermissions), avoiding unscoped permission lookups.Supporting information
This fixes
Testing instructions
validate/me, no scope in there.No permissions

Allowed view team

Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'