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. |
9c964ac to
2c77167
Compare
| # Metadata fields an extension may replace on a role. | ||
| _METADATA_FIELDS = ("display_name", "description", "icon", "hidden") | ||
|
|
||
| # Singular labels for operator-facing messages, keyed by document attribute. | ||
| _KIND_LABELS = {"categories": "category", "permissions": "permission", "roles": "role"} |
There was a problem hiding this comment.
Nit: Could we apply this decorator to a shared build method, with each factory implementing only the part that differs?
| role_permission_sources=role_permission_sources, | ||
| ) | ||
|
|
||
| # ---- base collection -------------------------------------------------- |
There was a problem hiding this comment.
nit: do we need these in-line comments?
| if existing.definition == definition: | ||
| existing.sources.append(document.source) | ||
| elif document.priority > existing.priority: | ||
| # The loser is discarded; say so, otherwise the contributing | ||
| # file looks like it took effect (ADR 0017 §4). | ||
| self._warn_discarded( | ||
| kind, | ||
| identifier, | ||
| loser=existing.sources[0], | ||
| loser_priority=existing.priority, | ||
| winner=document.source, | ||
| winner_priority=document.priority, | ||
| ) | ||
| tracked[identifier] = _Tracked( | ||
| definition=definition, | ||
| sources=[document.source], | ||
| priority=document.priority, | ||
| ) | ||
| elif document.priority == existing.priority: | ||
| raise SchemaCompileError( | ||
| f"Conflicting {kind} definition for {identifier!r} at equal priority " | ||
| f"{document.priority} ({existing.sources[0].source_id} vs {document.source.source_id})." | ||
| ) | ||
| else: | ||
| self._warn_discarded( | ||
| kind, | ||
| identifier, | ||
| loser=document.source, | ||
| loser_priority=document.priority, | ||
| winner=existing.sources[0], | ||
| winner_priority=existing.priority, | ||
| ) | ||
| return tracked |
There was a problem hiding this comment.
Can we move this to a different method with a single responsibility of managing priority between documents? Could it be somehow reused to manage conflicts in extensions?
|
|
||
| # ---- roles + provenance ---------------------------------------------- | ||
|
|
||
| def _resolve_roles_and_provenance( |
There was a problem hiding this comment.
Could we use more descriptive names for md, pc, and rp_sources here? I had to trace them back to understand what they represent, and I think expanding them would make this flow easier to follow.
|
|
||
| # ---- roles + provenance ---------------------------------------------- | ||
|
|
||
| def _resolve_roles_and_provenance( |
There was a problem hiding this comment.
Also can we move this to a different method with a single responsibility? Resolve _resolve_roles_and_provenance could be
-> apply metadata changes
-> apply permission changes
-> change sources
|
|
||
| return rp_sources | ||
|
|
||
| def _gather_extension_changes(self, roles: dict[str, _Tracked], documents: list[SchemaDocument]): |
There was a problem hiding this comment.
Should this be split into two?
| pc["remove"].append((perm, document.priority, document.source)) | ||
| return metadata_changes, perm_changes | ||
|
|
||
| def _resolve_metadata(self, role_id: str, md: dict[str, list[tuple[object, int, SourceRecord]]]): |
There was a problem hiding this comment.
Can we reuse the mechanism of managing priority for all cases (base priority, this, etc)?
| provenance: dict[str, list[RelationshipSource]] = { | ||
| perm: [RelationshipSource(src, SchemaOriginKind.BASE, base_priority) for src in base_sources] | ||
| for perm in base | ||
| } |
There was a problem hiding this comment.
Should we have a property for this? We also use it in https://github.com/openedx/openedx-authz/pull/476/changes#diff-752f33fcceee317067b5f683d36400f9983f027126a7ab19d38514b21b3f337fR184-R187
| actions: dict[str, list[tuple[str, int, SourceRecord]]] = {} | ||
| for perm, priority, src in pc["add"]: | ||
| actions.setdefault(perm, []).append(("add", priority, src)) | ||
| for perm, priority, src in pc["remove"]: | ||
| actions.setdefault(perm, []).append(("remove", priority, src)) |
There was a problem hiding this comment.
I think we could have add/remove methods which could be called directly with a getattr, it could simplify this and the next lines
| winning_sources = [src for act, priority, src in entries if priority == max_priority and act == action] | ||
|
|
||
| for act, priority, src in entries: | ||
| if priority < max_priority: | ||
| self._warn_discarded( | ||
| f"role_extension {act} of {perm!r} on role", | ||
| role_id, | ||
| loser=src, | ||
| loser_priority=priority, | ||
| winner=winning_sources[0], | ||
| winner_priority=max_priority, | ||
| ) |
There was a problem hiding this comment.
Same question about reusability
2c77167 to
3ed5882
Compare
Problem
Several distributions can contribute to the same authorization schema, and one can extend a role defined by another. The loaded documents need to be resolved into a single set of definitions with per-contribution provenance, and role extensions merged deterministically (ADR 0018 §1, ADR 0023).
Approach
openedx_authz/engine/schema/compilation.py—SchemaCompileropenedx_authz/tests/schema/test_compilation.pyMerge rules follow ADR 0023: metadata replace, add/remove permissions, tri-state
hidden, andpriorityto resolve conflicts. An unresolvable equal-priority conflict raisesSchemaCompileErrorrather than picking a winner. Compilation is pure — no database, no Casbin.Manual testing instructions
Rollback plan
Revert this PR. Nothing consumes the compiler yet, so the revert is inert.
Retro compatibility
No authorization behavior changes. No models, no migration, no automatic code path.
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 (3/8) — #446 split into reviewable pieces. Bases chain bottom-up; merge in order.
rod/authz-schema-loader-only)rod/authz-schema-validator— schema validationrod/authz-schema-models— definition models + migrationrod/authz-schema-renderer— policy rendererrod/authz-schema-applier— schema applierload_authz_schemacommand, version bump and changelogMerge checklist: