-
Notifications
You must be signed in to change notification settings - Fork 5
[FC-0099] feat: add public API to interact with roles and permissions #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for the pull request, @mariajgrimaldi! 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. |
8d8b893 to
e06e4bd
Compare
7ce9d1b to
4c0d56e
Compare
BryanttV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to do one last review, but here are some initial comments.
| # in this case, ALL the policies, but that might not be the case | ||
|
|
||
|
|
||
| def get_permissions_for_roles( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing this function, but it is not working correctly. The implicit permissions are missing. For example, the library admin role returns these permissions:
{
"permissions": [
"delete_library",
"publish_library",
"manage_library_team",
"manage_library_tags",
"delete_library_content",
"publish_library_content",
"delete_library_collection",
"create_library",
"create_library_collection"
]
}They are the same ones assigned directly to the role in authz.policy file, but the implicit ones do not appear.
What is strange is that the enforcer does work correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 So we're on the same page, what should be the correct output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct output should be:
{
"permissions": [
# Direct permissions
"delete_library",
"publish_library",
"manage_library_team",
"manage_library_tags",
"delete_library_content",
"publish_library_content",
"delete_library_collection",
"create_library",
"create_library_collection"
# Implicit permissions
"view_library",
"view_library_team",
"view_library_tags",
"edit_library_content",
"view_library_content",
"edit_library_collection",
]
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it helps, for the MVP, we'll have all the permissions explicitly assigned to a role. We won't rely on permission inheritance. Perhaps this issue is something we can address later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good to know, thanks for the info!
I created the issue here so we can follow-up later: #91
openedx_authz/api/users.py
Outdated
| return user_role_assignments | ||
|
|
||
|
|
||
| def get_all_user_role_assignments_in_scope(scope_external_key: str) -> list[RoleAssignmentData]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try, thank you!
BryanttV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just minor corrections. Thank you very much for all your hard work!
|
I think from my perspective this is good once the other reviewers' comments are addressed. Thanks for all of the work! |
| @@ -1,11 +1,60 @@ | |||
| # ===== ACTION GROUPING (g2) ===== | |||
| ############################################ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bmtcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new comments are awesome! 🚀
MaferMazu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective, this is good to go. I tested it and it works as expected. When we address the rest of the comment, we can merge this. Thanks, @mariajgrimaldi, for all this work ✨
bmtcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work on a huge task!
BryanttV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks great! 🚀
Description
This PR introduces the public Python APIs for Open edX AuthZ. These modules expose role, permission, and user-assignment operations as an abstraction on top of the Casbin engine. The design follows a bottom-up approach: a type-safe data layer, low-level APIs for roles and permissions, and a user-facing convenience API (user-assignment).
Data Layer (
data.py)The foundation of the system. Provides type-safe data classes for users, subjects, scopes, actions, permissions, roles, and role assignments.
user^,role^,act^).attrsfor validation.This layer abstracts our internal naming conventions #65 which should not be exposed to API consumers. It validates inputs before they reach the Casbin engine, but they could also be useful to future serialization/deserialization operations.
Core APIs
Roles API (
roles.py)Permissions API (
permissions.py)PermissionData.get_all_permissions_in_scope).has_permission).Both APIs consume the data layer and interact with the engine directly. They build the shared vocabulary used throughout the library. These APIs shouldn't be directly consumed for authorization enforcement because of their abstraction. Further up layers with friendlier interfaces should be used instead.
Users API (
users.py)"alice","admin","lib:123") and handles namespacing internally.has_permissionso services can check access without going down to lower layers.This is the recommended entry point for most applications.
Layers & Dependencies
Each layer has a single responsibility. Services can stay simple with users.py, or use the lower APIs when more control is needed.
How To Test
A full test suite is included. It covers:
You can run it by: