Conversation
Define the response of GET /api/authz/v1/roles/ queried by scope_type, returning paginated roles with the categories and permissions catalogs needed by the Admin Console roles and permissions matrix. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the pull request, @BryanttV! 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. |
| translated. Since the body depends on the request language, the response must vary on | ||
| ``Accept-Language``. | ||
|
|
||
| Icons are Paragon icon names (validated by the schema, see `ADR 0026`_). The client maps |
There was a problem hiding this comment.
Just to keep in mind: There is already a ADR 0026 that is not related to paragon icons, I'll change the number on my PR, this will need updating once my PR is merged.
rodmgwgu
left a comment
There was a problem hiding this comment.
Overall looking good, Added some comments for consideration. Thanks!
| ``library`` (``ScopesTypeField``). The catalog describes what a scope type offers, not a | ||
| particular course or library, so a concrete scope is not needed. | ||
|
|
||
| * ``scope_type`` is required. Unlike ``/scopes/``, a request without it is invalid (400) |
There was a problem hiding this comment.
I'm thinking that perhaps we should support a "scope_types" param instead, that accepts a list of scope types.
This for supporting future requirements, like listing the roles dynamically for assignments, when we enable support for roles with multiple scope types.
There was a problem hiding this comment.
Good point, thanks. I agree it's cheaper to settle this now, while no released client calls the endpoint, than to introduce a second breaking change when roles with multiple scope types arrive.
I'd update the ADR as follows:
- The query parameter becomes
scope_types: a required list with at least one value, each one ofcourseorlibrary(e.g.?scope_types=course,library). Empty values and unknown values return 400. - Semantics: a role is returned if it has grants in any of the requested scope types (OR).
permissionsandcategoriesare the union of those used by the requested types, and a role'spermissionslists its grants in those namespaces. - Authorization: the user must hold the corresponding view-team permission for each requested scope type (in any scope, as today), so a request can't expose roles of a type the user can't see.
user_count: counts the users assigned to the role across the requested scope types.- Response:
scope_typebecomesscope_types: Array<"course" | "library">. - The Roles and Permissions tab keeps sending a single value, since one matrix still can't mix course and library roles. The list is what the dynamic role listing for assignments will use.
What do you think?
|
|
||
| * ``scope_type`` is required. Unlike ``/scopes/``, a request without it is invalid (400) | ||
| because one matrix cannot mix course and library roles. | ||
| * The scope type is mapped to its scope namespace (``course`` to ``course-v1``, ``library`` |
There was a problem hiding this comment.
Not necessarily for now, but to keep in mind: How would we handle this mapping if we introduce new scopes in the future? specially when the scope is not defined by authz itself?
There was a problem hiding this comment.
Good question. I think ideally we should pull the scope types from the existing scope_registry, maybe using the NAMESPACE as the key. That way, any plugin could contribute a scope type without modifying authz. What do you think?
| Authorization | ||
| ============= | ||
|
|
||
| The permission needed depends on the requested ``scope_type``, so a user cannot read the |
There was a problem hiding this comment.
If we change this to "scope_types", then should we check for permissions on all the requested scope types inclusive? or just filter the output to only what the user can view?
There was a problem hiding this comment.
I mentioned this in this comment. What do you think?
| * A new permission class is needed for the per-scope-type authorization. | ||
| * ``definition_kind`` is reserved for user-defined roles. Their storage, the translation of | ||
| their names and any source detail remain out of scope. | ||
| * A role present in the Casbin policy but with no stored definition cannot be described. It |
There was a problem hiding this comment.
I think if a role is present in Casbin but not stored, then we should not show it. Otherwise the filtering and pagination will become much more complex as we will need to query Casbin in addition to the DB.
Record that scope types could later be resolved from scope_registry so plugins can contribute new ones, while course and library stay explicit for now. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Replace scope_type with a comma-separated scope_types parameter so the catalog can later serve roles with multiple scope types. A role is returned if it has grants in any requested type, and the user needs the view-team permission of each one. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Roles that exist only in the Casbin policy are no longer listed, so the catalog reads only from the database and filtering and pagination stay in a single query. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Description
Adds ADR 0028, which defines the response contract of
GET /api/authz/v1/roles/when queried byscope_types. ADR 0021 decided that this endpoint returns the definitions stored in the authz model, but left the exact response shape open. This ADR defines it.The Roles and Permissions tab of the Admin Console renders a matrix (roles × permissions grouped by category) for one or more scope types at a time. Today the frontend hardcodes all of this (
course/constants.ts,library/constants.ts). The matrix needs every permission of the requested scope types, including the ones a role does not grant, so a list of roles that only carries the permissions each one grants is not enough.Key decisions
scope_types(required): a comma-separated list ofcourseand/orlibrary, with at least one value. Duplicates are collapsed. An empty item or an unsupported value returns a 400. Thescopeparameter is removed.courses.view_course_teamforcourseandcontent_libraries.view_library_teamforlibrary, in any scope. When several scope types are requested, the user must hold the permission for each one. This needs a new permission class, sinceDynamicScopePermissionandAnyScopePermissioncannot express it.permissionscontains the permissions supported by any requested scope type. A role is listed if it has grants in any of them, and itspermissionsare the ones it grants across them.categoriesandpermissionscatalogs are sent once, and each role references permissions by id. The response echoes the requestedscope_types. The client can build each matrix cell directly.user_countis calculated across the requested scope types and counts distinct users, not a single scope.hiddenroles (ADR 0023) are excluded, and roles without a stored definition are not listed.Manual testing
Documentation only. To review, read
docs/decisions/0028-api-contract-for-role-catalog.rstor check the docs.Merge checklist