feat: implement permission validation API and hooks for authz - #269
feat: implement permission validation API and hooks for authz#269bra-i-am wants to merge 11 commits into
Conversation
|
Thanks for the pull request, @bra-i-am! 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. |
99425d5 to
2c9c885
Compare
rodmgwgu
left a comment
There was a problem hiding this comment.
Looking good, just added some comments for discussion. Thanks!
| * Consumers destructure permission keys directly — no nested `.permissions` object. | ||
| * | ||
| * @example | ||
| * const { enableAuthz } = useWaffleFlags(courseId); |
There was a problem hiding this comment.
Can you confirm if this would handle cases where the waffle flag is disabled globally but enabled for a specific course or org? It looks like it should, but just to be sure.
Can we add a comment specifying this?
There was a problem hiding this comment.
yees, it should! The hook is completely agnostic to how featureEnabled is calculated. It doesn't matter if it comes from a global or specific flag; if featureEnabled = true the hook will call the API
I'll be adding the comment to specify this
|
|
||
| ## Best Practices | ||
|
|
||
| - **Define permission constants** in your MFE (`COURSE_PERMISSIONS`, etc.) rather than |
There was a problem hiding this comment.
Would it make sense to include the constants for the existing permissions here in frontend-base?
There was a problem hiding this comment.
@rodmgwgu, thanks a lot for this call!
According to what I assumed, the permissions are MFE-specific. For example, in authoring MFE, we use these permissions to manage a course. I don't think they'll be needed anywhere else, and I tried to avoid coupling frontend-base to specific MFE permissions
The thing is, I agree there is a valid case for centralizing permissions: we should do it for those that will be needed across multiple MFEs, but I'm not sure either if this will be applied to other MFEs 😅 (I think it would be possible, for example, to frontend-app-instructor-dashboard) and, in case this will be, which permissions would qualify to be added in the repo
What do you think?
There was a problem hiding this comment.
I think we will need more high level discussion around this, currently the permissions in the backend are all defined in one place, openedx-authz, so having something similar in the frontend seems to make sense.
However, the plan is to extract those out of openedx-authz and instead define them on whatever module needs to implement them.
So keeping it per MFE makes sense for long term.
There was a problem hiding this comment.
[Done] We are not indicating consumers where to find the permissions they can use. I suggest to include https://docs.openedx.org/projects/openedx-authz/en/latest/concepts/core_roles_and_permissions/index.html here in the how to as well as the hook TSDoc.
| import { useWaffleFlags } from './waffleHooks'; // your MFE's waffle flag hook | ||
| import type { PermissionValidationQuery } from '@openedx/frontend-base'; | ||
|
|
||
| export const useResourcePermissions = <Query extends PermissionValidationQuery>( |
There was a problem hiding this comment.
This looks useful and generic enough, could we add it to frontend-base?
There was a problem hiding this comment.
This is intentionally a recommended pattern rather than a reusable hook, mainly because it references two things that are MFE-specific: the waffle flag name (enableAuthzCourseAuthoring in Authoring MFE), and the waffle flag hook (useWaffleFlags, which in this case hits to an endpoint in the studio). I think neither of those can live in frontend-base without coupling the library to a specific MFE's implementation
What do you think?
There was a problem hiding this comment.
You are right, although the flag is just one now (for course authoring), there could be more in the future, so keeping it as a recommendation is fine, thanks!
| const { isLoading, isError, data } = useQuery<PermissionValidationAnswer<Query>, Error>({ | ||
| queryKey: permissionsQueryKeys.validate(query, apiBaseUrl), | ||
| queryFn: featureEnabled ? () => validatePermissions(apiBaseUrl, query) : skipToken, | ||
| retry, |
|
Thanks for adding this to frontend-base! I'm going to defer to others for the review, but I'm happy to see this, and the approach is similar to the code you contributed to the authoring MFE which I liked. |
|
@bradenmacdonald and @rodmgwgu, thanks a lot for your time reviewing this! @rodmgwgu, I've already addressed the feedback you left. Looking forward to any additional comments! |
|
Making sure I understand this PR correctly: This is porting openedx/frontend-app-authoring#2685. I do think it likely makes sense to have this live in The main thing that concerns me is
The I agree with #150 's goal of creating "reusable utils to query for permissions from openedx-authz on frontend MFEs," but I think in the short term it likely makes sense for this to live in a standalone package to be consumed by both I'd also like to hear @arbrandes's take on this. |
Same. Having those three as simultaneous dependencies could lead to Unforeseen Consequences (tm).
Good idea. @bra-i-am, do you think that's doable? |
|
Thanks for the feedback @brian-smith-tcril and @arbrandes. I think the proposal to manage this as a standalone library makes sense, especially given the progressive migration of MFEs to frontend-base and the incremental rollout of RBAC. Since we're initially focusing on a limited set of scopes and roles, we won't need to update all MFEs at the same time. A shared library seems like a solid short- to medium-term solution. It allows both efforts to move forward in parallel. One question: what's the preferred process for creating the repository? Would you prefer to create it directly under the Open edX organization, or should we create it under our organization first and then transfer it over? |
I think currently the best path is to have the repository in the openedx org from the start. The first step of the process is to create a GH Request to axim-engineering. |
|
@brian-smith-tcril, @arbrandes, @dcoa: thanks for your comments. This is totally doable, and I agree it's the best path to follow! I've already created the GH Request: openedx/axim-engineering#1725 Do you think |
|
As I mentioned in my review of https://github.com/openedx/frontend-authz/pull/2, before we fully commit to a new temporary repository instead of implementing the feature here (or here and in frontend-platform), I'd like to understand which MFEs besides Authoring will use the feature, and in what timeframe. |
Hi @bra-i-am! Flagging this for you. |
|
As we agreed on having this on |
arbrandes
left a comment
There was a problem hiding this comment.
Have a couple of requests, but otherwise looking good so far.
| export { usePermissions, permissionsQueryKeys } from './hooks'; | ||
| export type { UsePermissionsOptions, UsePermissionsResult } from './hooks'; | ||
| export type { PermissionValidationQuery, PermissionValidationAnswer } from './types'; |
There was a problem hiding this comment.
validatePermissions should probably also be exported, no?
There was a problem hiding this comment.
I added it in the exports and also add a section in the 'how to' explaining when could be used over the hook. Please have a look and let me know if that make sense
718ab6f to
f035bb0
Compare
f035bb0 to
47da6df
Compare
|
thank you @arbrandes for helping us with the review I addressed your comments, could you validate? |
arbrandes
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous review. I have a couple more suggestions, if you don't mind.
| export interface PermissionValidationRequestItem { | ||
| action: string; | ||
| scope?: string; | ||
| } |
There was a problem hiding this comment.
Make scope required (scope: string), and fix the @example on line 18, which shows { canEdit: { action: 'courses.edit' } } with no scope.
PermissionValidationSerializer composes ScopeMixin, whose scope = serializers.CharField(max_length=255) is required and non-blank (serializers.py:14-33), and the view indexes perm["scope"] unconditionally. Omitting it 400s the whole request, and since validatePermissions sends one batch for the entire query, every key in that query then resolves to false - a silent deny across the hook, triggered by a type that says the field is optional.
| const { isLoading, isError, data } = useQuery<PermissionValidationAnswer<Query>, Error>({ | ||
| queryKey: permissionsQueryKeys.validate(query, apiBaseUrl), | ||
| queryFn: featureEnabled ? () => validatePermissions(apiBaseUrl, query) : skipToken, | ||
| retry, | ||
| staleTime, | ||
| }); | ||
|
|
||
| // Derived once: a disabled consumer must never be treated as loading, even when it | ||
| // shares a cache key with an enabled consumer whose fetch is in flight. Blanking the | ||
| // keys off the raw isLoading there would return isLoading: false with undefined keys. | ||
| const isPermissionsLoading = featureEnabled && isLoading; |
There was a problem hiding this comment.
Destructure isPending instead of isLoading:
const { isPending, isError, data } = useQuery(/* ... */);
// ...
const isPermissionsLoading = featureEnabled && isPending;isLoading is actually isPending && isFetching. With the default networkMode: 'online', a query that cannot start because the browser is offline sits at fetchStatus: 'paused', so isFetching is false and isLoading never goes true. A consumer following the documented isLoading / isError / if (!canView) deny sequence renders a permission denial instead of a spinner.
| export { validatePermissions } from './api'; | ||
| export { usePermissions, permissionsQueryKeys } from './hooks'; | ||
| export type { UsePermissionsOptions, UsePermissionsResult } from './hooks'; | ||
| export type { PermissionValidationQuery, PermissionValidationAnswer } from './types'; |
There was a problem hiding this comment.
Minor: consider also exporting PermissionValidationRequestItem and PermissionValidationResponseItem.
Now that validatePermissions is public, a caller typing the raw call's inputs or narrowing a response has no way to name those shapes.
| return { | ||
| isLoading: isPermissionsLoading, | ||
| isError: featureEnabled && isError, | ||
| isAuthzEnabled: featureEnabled, | ||
| ...permissionResults, | ||
| } as UsePermissionsResult<Query>; |
There was a problem hiding this comment.
Minor: consider returning error alongside isError.
With retry: false, a consumer cannot tell a 401/403 from a transient network failure - both produce the same all-false result.
Resolves #150 by porting openedx/frontend-app-authoring#2685 to
frontend-baseAdds a new
runtime/authzmodule that lets MFEs validate user permissions against theopenedx-authzbackend service without writing their own API layer.What's added:
validatePermissions(apiBaseUrl, query)— POSTs to/api/authz/v1/permissions/validate/mewith a batch of{ action, scope }pairs and maps the array response back to caller-defined keys. Missing keys default tofalse.usePermissions(query, featureEnabled, options?)— React hook backed by TanStack Query. Permission keys are spread at the top level (no nested.permissionsobject) alongsideisLoading,isError, andisAuthzEnabledflags.featureEnabledisfalse: no API call is made and all keys returntrue, preserving pre-authz behavior during gradual rollout via waffle flags.isErroristrueand all keys resolve tofalse.apiBaseUrloverride for MFEs still on@edx/frontend-platform.permissionsQueryKeys— exported query key factory for manual cache invalidation.PermissionValidationQuery,PermissionValidationAnswer<Query>, and related request/response item types.api.ts(5 cases) andhooks.tscovering the enabled/disabled paths, error states, and the flat return shape.docs/how_tos/permissions.mdcovering usage, caching, MFE-level wrapper pattern, best practices, and cache invalidation.Testing
edunext/frontend-app-authoringto your tutor envtutor mounts add ../path/to/clone/bc/generalize-authz-validation. This branch already updates the authoring MFE to consume theusePermissionshook fromfrontend-base./admin/waffle/flag/Nametoauthz.enable_course_authoring, setEveryonetoyesand save. If this isno, you should be able to use the legacy behavior without authz permissionsfrontend-authoring-mfeand switch to the branch of this PR, and runnpm i && npm run buildto update thedistfolderhttp://apps.local.openedx.io:2001/authoring/course/{_course_id_}Pages and Resourcespage by clickingContentin the headerhttp://apps.local.openedx.io:2025/admin-console/authz?scope={_course_id_},Pages and Resourcesshould be working as expected for each roleLLM usage notice
Built with assistance from Claude.