Conversation
|
Thanks for the pull request, @rodmgwgu! 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. |
9f7350f to
d22ebbe
Compare
3a2f951 to
b73023f
Compare
d912d58 to
9ac87a4
Compare
9ac87a4 to
8fa7e9b
Compare
8fa7e9b to
a6abbef
Compare
mariajgrimaldi
left a comment
There was a problem hiding this comment.
Just a few comments for you to review!
| # Namespace prefixes for the internal Casbin form (schema objects never carry them). | ||
| ROLE_PREFIX = "role" | ||
| ACTION_PREFIX = "act" | ||
| SCOPE_WILDCARD = "*" | ||
| ALLOW = "allow" | ||
| POLICY_PTYPE = "p" |
There was a problem hiding this comment.
Don't have this already defined in data.py?
| def from_policy(cls, values: list[str]) -> "PolicyRow": | ||
| """Build from a stored ``p`` row (``[subject, action, scope, effect]``).""" | ||
| subject, action, scope, effect = (list(values) + ["", "", "", ""])[:4] | ||
| return cls(POLICY_PTYPE, subject, action, scope, effect) |
There was a problem hiding this comment.
[non-blocking] Could we reuse the existing policy parsing here? PolicyIndex already defines these field positions, and get_permission_from_policy() parses and validates the same list shape. It may be worth extracting shared parsing so the mapping and validation stay in one place.
| rows: list[PolicyRow] = field(default_factory=list) | ||
|
|
||
|
|
||
| def policy_row(role_id: str, permission_id: str, scope: str) -> PolicyRow: |
There was a problem hiding this comment.
This looks more than a constructor that should be part of PolicyRow?
Problem
A compiled schema carries bare identifiers; the enforcer stores namespaced Casbin
prows. Something has to translate between the two, and it has to be the single place that applies the namespacing — two places would drift, and a later comparison against the stored policy would silently stop matching (ADR 0018 §5).Approach
openedx_authz/engine/renderer.py—PolicyRow,RenderedPolicy,policy_row(),PolicyRendereropenedx_authz/tests/schema/test_renderer.pyrender()emits oneprow per (role, permission, supported scope), applies the internal form (role^,act^,<scope>^*) at this boundary, and is deterministic so the output can be diffed against a stored policy. It emits definition rows only — nevergassignments or legacyg2action inheritance — so data owned by other services is out of reach by construction.The render phase is pure: no Casbin, no Django, no database. The apply phase, which does touch both, is the next PR in the stack.
Manual testing instructions
Rollback plan
Revert this PR. Nothing consumes the renderer yet, so the revert is inert.
Retro compatibility
No authorization behavior changes. Rendering produces rows in memory and writes nothing.
AI Usage
Kiro was used to assist on feature planning and implementation. Implementation was done step by step with human guidance and validation, based on the ADRs.
Stack (6/8) — #446 split into reviewable pieces. Bases chain bottom-up; merge in order.
rod/authz-schema-models)rod/authz-schema-applier— schema applierload_authz_schemacommand, version bump and changelogMerge checklist: