Skip to content

feat: add support for course permission in authz rest api#274

Merged
MaferMazu merged 8 commits intomainfrom
mfmz/fix-roles-endpoint-permissions
Apr 30, 2026
Merged

feat: add support for course permission in authz rest api#274
MaferMazu merged 8 commits intomainfrom
mfmz/fix-roles-endpoint-permissions

Conversation

@MaferMazu
Copy link
Copy Markdown
Contributor

@MaferMazu MaferMazu commented Apr 29, 2026

Description

This PR adds support for course permissions to the authz REST API methods. For example, allowing someone with course team management permissions to call the API to create, remove, and list roles.

How to test

Authenticate as a course admin, and you should be able to perform team management actions like creating, removing, and listing roles over the course you are an admin on.
Here is a Postman collection for a quick test: Open edX AuthZ - Role management.postman_collection.json

Also, I added tests to the code to verify that users with manage team have the proper rights over the REST API methods, and tested bulk enrollment to ensure that a user with permissions over a specific scope can't make a bulk enrollment to a different scope.

Extra

This resolves #273

Implementation details

This PR

  • Adds a CoursePermission class in permission.py with the namespace course-v1, to avoid the fallback to BaseScopePermission (that only permits to staff users).
  • Changes the loop in validate_permissions from "all must pass" to any. We only need to comply with one permission.
    • The exception will be the bulk requests, because those are handled with DynamicScopePermission, and with the _has_bulk_permission, we are checking that the user has all permissions needed to perform the action within those scopes.
  • Adds an extraction of scope[0] in scopes. Because after, I could send scopes rather than a scope in the PUT method, so the scope could be None, allowing the namespace detection to fall through to "global".
  • Adds the permission to the decorators.

Merge checklist:
Check off if complete or not applicable:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings) N/A
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

Technical documentation and code structure assisted by Claude 4.6 Sonnet; all logic verified and tested manually.

@openedx-webhooks
Copy link
Copy Markdown

Thanks for the pull request, @MaferMazu!

This repository is currently maintained by @openedx/committers-openedx-authz.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To 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:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where 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:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@MaferMazu MaferMazu force-pushed the mfmz/fix-roles-endpoint-permissions branch from 9f9dd2d to c8d3d1c Compare April 29, 2026 02:55
Comment thread openedx_authz/rest_api/v1/permissions.py
Comment thread openedx_authz/rest_api/v1/permissions.py Outdated
Comment thread openedx_authz/rest_api/v1/permissions.py
Comment thread openedx_authz/rest_api/v1/permissions.py
@mariajgrimaldi
Copy link
Copy Markdown
Member

Thanks a lot for this!

A few questions:

(This doesn't apply to the bulk enrollment, because we are using all(); tests were added to the code to verify this).

What do we mean by bulk enrollment? Are we referring to role assignment? Can you help me understand why this is a special case?

@mphilbrick211 mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Apr 29, 2026
Comment thread openedx_authz/tests/rest_api/test_permissions.py
Comment thread openedx_authz/tests/rest_api/test_views.py
Comment on lines +197 to +198
if not isinstance(perm_instance, MethodPermissionMixin):
return False
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this validation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MethodPermissionMixin check is needed because the decorator is our only source of truth for which permission to verify against the actor. In a single-scope request, each handler manages this on its own — and some handlers intentionally default to True when no decorator is present (e.g., "if no permission is declared, allow it"). That fallback is safe for a single scope because the handler makes a conscious decision. But in a bulk request, defaulting to True across all scopes would be unsafe — we need an explicit permission declaration to verify that the actor has the required (e.g., read) permissions for each requested scope.

This is a refined answer with Claude's help; my first try wasn't as clear as this version. 🙈

Note: I copied and pasted a piece of code of a single-scope handler, you can check it here #274 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

Comment thread openedx_authz/rest_api/v1/permissions.py Outdated
Copy link
Copy Markdown
Contributor

@rodmgwgu rodmgwgu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the following scenarios integrated with the admin console (all done via the UI) and it's looking all good, thanks! Just pending the outstanding comments:

Non staff user with Course Admin role to a specific course:

  • User can access the admin console
  • User can see the role assignments for that course
  • User can successfully remove a role assignment for that course
  • User can successfully add a role assignment for that course

Non staff user with Course Admin role to a whole Org (glob Org):

  • User can access the admin console
  • User can see the course role assignments for that org
  • User can successfully remove a course role assignment for that org
  • User can successfully add a course role assignment for that org

Also tested with bulk assigning to a specific course and an org, and worked fine.

Thanks!

@MaferMazu
Copy link
Copy Markdown
Contributor Author

MaferMazu commented Apr 30, 2026

@rodmgwgu @mariajgrimaldi @BryanttV this is ready for a re-review.

@mariajgrimaldi, regarding your question:

What do we mean by bulk enrollment? Are we referring to role assignment? Can you help me understand why this is a special case?

Yes, I was referring to bulk assignments (requests with the same operation across multiple scopes).
This is a special case, because in a single-scope request, each permission handler (ContentLibraryPermission, CoursePermission, etc.) manages its own logic — some even default to True when no @authz_permissions decorator is present, which is a valid and conscious design choice for that handler.
But in a bulk request, DynamicScopePermission is the one iterating over all scopes, and it can't safely rely on that handler-level fallback. It needs an explicit declaration of which permission to check against the actor for each scope (so I am forcing the use of an authz decorator to know which permission to check for all scopes).
Related explanation: #274 (comment)

I hope that with the refactor I did here (4d637ac), it's clearer.

Copy link
Copy Markdown
Contributor

@rodmgwgu rodmgwgu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Copy link
Copy Markdown
Contributor

@BryanttV BryanttV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

Copy link
Copy Markdown
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for addressing all of my comments!

@MaferMazu MaferMazu merged commit b1e9b3d into main Apr 30, 2026
8 checks passed
@MaferMazu MaferMazu deleted the mfmz/fix-roles-endpoint-permissions branch April 30, 2026 19:16
@github-project-automation github-project-automation Bot moved this from In Eng Review to Done in Contributions Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Bug - RBAC AuthZ - Course Admin gets 403 when assigning or removing roles on course scopes

6 participants