docs: define the static authorization schema - #421
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. |
rodmgwgu
left a comment
There was a problem hiding this comment.
Looking good, just added a couple of points for discussion. Thanks!
|
|
||
| permissions: | ||
| - namespace: courses | ||
| name: view_course |
There was a problem hiding this comment.
Should we specify a naming convention for namespaces, names and permission_categories id? Like ascii lower snake case only?
There was a problem hiding this comment.
YES!
I'm working on a reference file
There was a problem hiding this comment.
Here it is, I think we could handle the details there once we settle on this ADR: #431
| permissions: | ||
| - courses.view_course | ||
|
|
||
| role_extensions: |
| * invalid YAML, unknown fields, missing required fields, and values with the wrong type; | ||
| * unsupported schema versions; | ||
| * IDs that contain uppercase letters or unsupported punctuation; | ||
| * icon names that are unavailable from ``@openedx/paragon/icons``; and |
There was a problem hiding this comment.
How would we handle possible future changes to the paragon icons? Like naming changes or icon removals?
There was a problem hiding this comment.
I think it'd be the same as any consumer of paragon icons (MFEs, extensions and so on) they should follow a deprecation process we'd have to be attentive of
|
|
||
| * invalid YAML, unknown fields, missing required fields, and values with the wrong type; | ||
| * unsupported schema versions; | ||
| * IDs that contain uppercase letters or unsupported punctuation; |
There was a problem hiding this comment.
Should we enforce also snake case naming convention?
BryanttV
left a comment
There was a problem hiding this comment.
Thanks for this @mariajgrimaldi!
| Decision | ||
| ******** | ||
|
|
||
| #. Schema format and boundary |
There was a problem hiding this comment.
The automatic numbering isn't working, I'm not sure why.
There was a problem hiding this comment.
Mmm others ADRs might being affected by this, I'll look into it later
| ============================= | ||
|
|
||
| A permission contains: | ||
|
|
There was a problem hiding this comment.
Should the permissions schema also have a version and priority?
There was a problem hiding this comment.
I'd have to look into this in more detail to weigh between the two options. Here's a brain dump:
- Versioning I think should be the entire file, I don't think we should individually version each permission since I don't think they would change much. Also the configuration marks the current status of the authorization system, so I think they belong to the file itself
- Priority would be mainly for extension patches so we know how to resolve between them. For example, if two extension patches modify the same role and one adds a permission while another removes it, priority would determine which change takes effect.
What kind of use cases can we solve by adding priority to permissions themselves?
| * ``namespace`` and ``name``, which form the stable identifier used by application checks, such as ``courses.view_course``; | ||
| * the scope namespaces where it can apply; | ||
| * a normalized permission category; and | ||
| * ``display_name``, ``description``, and an optional Paragon icon name. |
There was a problem hiding this comment.
If the Paragon icon is optional, what would be shown by default? Is there currently an easy way to validate if a Paragon icon exists? Since it is mentioned as part of the initial validation.
There was a problem hiding this comment.
I think we can make some kind of request to render it (not sure how easy or out-of-the-box). I was thinking we could do a simple check for the name following paragon conventions. What do you think?
| #. Roles and role extensions | ||
| ============================ | ||
|
|
||
| A role contains a stable identifier, display name, description, supported scope namespaces, and a list of complete permission identifiers. When an application needs to change an existing role, it uses ``role_extensions``. An extension may add or remove permissions and may replace the role's display name or description. |
There was a problem hiding this comment.
Can we clearly define the properties that role_extensions accepts?
There was a problem hiding this comment.
Excellent! Could we include a reference to that ADR here?
|
|
||
| schema_version: "1.0" | ||
| priority: 100 | ||
|
|
There was a problem hiding this comment.
Is it possible to explain a bit about how priority would work? Would there be a convention or something similar? What should happen if two plugins define the same priority?
There was a problem hiding this comment.
I think I do it in the validation and conflicts section, let me know if it's not clear enough: https://github.com/openedx/openedx-authz/pull/421/changes#diff-323a833929851b16109620a7eee56dfe642c78e8c26d00a06cc6c08f321182ceR115
| - courses.view_course | ||
|
|
||
| role_extensions: | ||
| - role: course_admin |
There was a problem hiding this comment.
Can an extension be added to a role that doesn't exist? Or is this included in the initial validation?
There was a problem hiding this comment.
Should be included in the validation
There was a problem hiding this comment.
| Python constants and role mappings | ||
| ================================== | ||
|
|
||
| Definitions would remain split across backend and frontend code. Applications would also need to change ``openedx-authz`` to add definitions they own. |
There was a problem hiding this comment.
What happens to the current role and permission definitions in the Python modules? Will we keep using them? If I want to create a new permission, do I have to add it to the YAML and the Python constants file? Or will we do something different?
There was a problem hiding this comment.
We'd need to entirely migrate to a configuration file to avoid having definitions scattered across multiple places
be47fea to
ff64cae
Compare
Description
Static roles, permissions, and their display information are currently spread across Python, Casbin policy files, and frontend code. As a result, an application that adds a role or permission must update several representations and may also need changes in
openedx-authzand the frontend.This ADR defines one versioned YAML schema for static authorization definitions. Applications can declare permissions, categories, roles, role extensions, scopes, and UI metadata with stable identifiers, while deployment compiles those definitions into the rows Casbin needs.
Merge checklist:
Check off if complete or not applicable: