From f12882b4d02ea871aa01a2e8edb4f3b8958cc247 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20M=C3=A9ndez?= Date: Wed, 23 Sep 2026 10:04:39 -0600 Subject: [PATCH 1/4] feat: add authz schema policy renderer --- src/openedx_authz/engine/renderer.py | 105 ++++++++++++++++++ .../tests/schema/test_renderer.py | 84 ++++++++++++++ 2 files changed, 189 insertions(+) create mode 100644 src/openedx_authz/engine/renderer.py create mode 100644 src/openedx_authz/tests/schema/test_renderer.py diff --git a/src/openedx_authz/engine/renderer.py b/src/openedx_authz/engine/renderer.py new file mode 100644 index 00000000..f3d15d9b --- /dev/null +++ b/src/openedx_authz/engine/renderer.py @@ -0,0 +1,105 @@ +"""Render compiled definitions to Casbin rows (ADR 0018 §1, §5). + +This is the Casbin-aware edge of the schema pipeline. It implements the +``render`` lifecycle step: + +* ``render`` builds the Casbin ``p`` rows for a :class:`CompiledSchema` in + memory, without touching the database. + +Key semantics: + * Definition rows only: ``render`` emits ``p`` rows and never ``g`` + (assignments) or ``g2`` (legacy action inheritance), so data owned by + other services is out of its reach by construction (ADR 0018 §3). + * Namespacing happens here, at the boundary: schema objects carry bare + identifiers, and the internal Casbin form (``role^``, ``act^``, + ``^*``) is applied on the way out. + * Deterministic output: rows are emitted in sorted order, so the rendered + set can be compared against a stored policy without spurious diffs. + +``render`` is pure — it imports nothing from Casbin or Django and performs no +database access. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field + +from openedx_authz.data import AUTHZ_POLICY_ATTRIBUTES_SEPARATOR as SEP +from openedx_authz.engine.schema.types import CompiledSchema, RoleDefinition + +# 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" + + +@dataclass(frozen=True) +class PolicyRow: + """A single Casbin ``p`` row rendered from a role-permission pair. + + Fields follow the ``p`` shape: subject (role), action (permission), scope + pattern, effect. Namespacing to the internal Casbin form (``role^``, + ``act^``, ``^*``) happens here, at the boundary — schema objects + never carry those prefixes. + """ + + ptype: str # always "p" for rendered definition rows + subject: str + action: str + scope: str + effect: str + + def as_policy(self) -> list[str]: + """Return the enforcer arg form: ``[subject, action, scope, effect]``.""" + return [self.subject, self.action, self.scope, self.effect] + + @classmethod + 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) + + +@dataclass +class RenderedPolicy: + """The full set of ``p`` rows for a compiled schema (no DB access).""" + + rows: list[PolicyRow] = field(default_factory=list) + + +def policy_row(role_id: str, permission_id: str, scope: str) -> PolicyRow: + """Build the Casbin ``p`` row for one ``(role, permission, scope)`` grant. + + The single place the internal namespacing is applied, so every producer and + consumer of a rendered row agrees on its exact shape. A drift between two + such places would silently stop a later comparison against the stored + policy from matching anything. + """ + return PolicyRow( + ptype=POLICY_PTYPE, + subject=f"{ROLE_PREFIX}{SEP}{role_id}", + action=f"{ACTION_PREFIX}{SEP}{permission_id}", + scope=f"{scope}{SEP}{SCOPE_WILDCARD}", + effect=ALLOW, + ) + + +class PolicyRenderer: + """Turns a :class:`CompiledSchema` into Casbin ``p`` rows in memory.""" + + def render(self, schema: CompiledSchema) -> RenderedPolicy: + """Produce one ``p`` row per (role, permission, supported scope). + + Emits definition (``p``) rows only — never ``g`` (assignments) or ``g2`` + (action inheritance). Applies the internal Casbin namespacing here. + Performs no database access. Output order is deterministic. + """ + rows: list[PolicyRow] = [] + for role_id in sorted(schema.roles): + role: RoleDefinition = schema.roles[role_id].definition + for scope in sorted(role.scopes): + for permission in sorted(role.permissions): + rows.append(policy_row(role.id, permission, scope)) + return RenderedPolicy(rows=rows) diff --git a/src/openedx_authz/tests/schema/test_renderer.py b/src/openedx_authz/tests/schema/test_renderer.py new file mode 100644 index 00000000..29f806a2 --- /dev/null +++ b/src/openedx_authz/tests/schema/test_renderer.py @@ -0,0 +1,84 @@ +"""Unit tests for the (pure) render step. + +Covers turning a compiled schema into Casbin ``p`` rows: one row per +role-permission-scope, the ``role^``/``act^``/``^*`` namespacing convention, +deterministic output, and scope fan-out. +""" + +from openedx_authz.engine.renderer import PolicyRenderer, PolicyRow +from openedx_authz.engine.schema.compilation import SchemaCompiler + +from .factories import category, make_document, permission, role + + +class TestPolicyRow: + """Constructing and round-tripping a single Casbin ``p`` row.""" + + def test_from_policy_round_trips_as_policy(self): + """A row rebuilt from ``as_policy`` output equals the original.""" + row = PolicyRow("p", "role^course_editor", "act^courses.view_course", "course-v1^*", "allow") + assert PolicyRow.from_policy(row.as_policy()) == row + + def test_from_policy_reads_a_stored_row(self): + """A stored ``[subject, action, scope, effect]`` row maps to its fields.""" + row = PolicyRow.from_policy(["role^r", "act^p", "course-v1^*", "allow"]) + assert row.ptype == "p" + assert (row.subject, row.action, row.scope, row.effect) == ("role^r", "act^p", "course-v1^*", "allow") + + def test_from_policy_pads_short_rows_with_empty_strings(self): + """A row with fewer than four values is padded rather than raising.""" + row = PolicyRow.from_policy(["role^r", "act^p"]) + assert (row.subject, row.action, row.scope, row.effect) == ("role^r", "act^p", "", "") + + +class TestPolicyRendering: + """Rendering a compiled schema into Casbin ``p`` policy rows.""" + + @staticmethod + def _schema(): + """Build a compiled schema fixture for renderer tests.""" + doc = make_document( + categories=[category("cat")], + permissions=[ + permission(name="view_course", cat="cat", scopes=("course-v1",)), + permission(name="edit_course_content", cat="cat", scopes=("course-v1",)), + ], + roles=[ + role( + rid="course_editor", + scopes=("course-v1",), + permissions=("courses.view_course", "courses.edit_course_content"), + ) + ], + ) + return SchemaCompiler().compile([doc]) + + def test_render_emits_one_p_row_per_role_permission_scope(self): + """Each role-permission-scope combination becomes one allow ``p`` row.""" + rendered = PolicyRenderer().render(self._schema()) + assert len(rendered.rows) == 2 + assert all(row.ptype == "p" and row.effect == "allow" for row in rendered.rows) + + def test_render_applies_casbin_namespacing(self): + """Subjects, actions, and scopes carry their Casbin namespace prefixes.""" + rendered = PolicyRenderer().render(self._schema()) + row = next(r for r in rendered.rows if r.action == "act^courses.view_course") + assert row.subject == "role^course_editor" + assert row.scope == "course-v1^*" + assert row.as_policy() == ["role^course_editor", "act^courses.view_course", "course-v1^*", "allow"] + + def test_render_is_deterministic(self): + """Rendering the same schema twice yields identical rows.""" + schema = self._schema() + assert PolicyRenderer().render(schema).rows == PolicyRenderer().render(schema).rows + + def test_multiple_scopes_multiply_rows(self): + """A permission spanning multiple scopes fans out into one row per scope.""" + doc = make_document( + categories=[category("cat")], + permissions=[permission(name="view_course", cat="cat", scopes=("course-v1", "ccx-v1"))], + roles=[role(rid="r", scopes=("course-v1", "ccx-v1"), permissions=("courses.view_course",))], + ) + rendered = PolicyRenderer().render(SchemaCompiler().compile([doc])) + scopes = {row.scope for row in rendered.rows} + assert scopes == {"course-v1^*", "ccx-v1^*"} From cfa5e666481d1fec61efc94a3a6f852ff8219dca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20M=C3=A9ndez?= Date: Mon, 5 Oct 2026 13:23:20 -0600 Subject: [PATCH 2/4] squash!: Refactor constants --- src/openedx_authz/data.py | 14 ++++++++++++-- src/openedx_authz/engine/renderer.py | 20 ++++++++++---------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/openedx_authz/data.py b/src/openedx_authz/data.py index 2df5af37..7be26cd5 100644 --- a/src/openedx_authz/data.py +++ b/src/openedx_authz/data.py @@ -11,6 +11,16 @@ AUTHZ_POLICY_ATTRIBUTES_SEPARATOR = "^" +# Shared authz vocabulary. These are the single source of truth for the namespace +# prefixes, scope wildcard, policy type, and default effect used across the authz +# data classes and the engine renderer, so every producer and consumer of a Casbin +# row agrees on its exact shape. +ROLE_NAMESPACE = "role" +ACTION_NAMESPACE = "act" +SCOPE_WILDCARD = "*" +POLICY_PTYPE = "p" +EFFECT_ALLOW = "allow" + class AuthzBaseClass: """Base class for all authz classes.""" @@ -59,7 +69,7 @@ class ActionData(AuthZData): 'Content Libraries > Delete Library' """ - NAMESPACE: ClassVar[str] = "act" + NAMESPACE: ClassVar[str] = ACTION_NAMESPACE @property def name(self) -> str: @@ -93,7 +103,7 @@ class PermissionData: """ action: ActionData = None - effect: Literal["allow", "deny"] = "allow" + effect: Literal["allow", "deny"] = EFFECT_ALLOW @property def identifier(self) -> str: diff --git a/src/openedx_authz/engine/renderer.py b/src/openedx_authz/engine/renderer.py index f3d15d9b..ed4afd90 100644 --- a/src/openedx_authz/engine/renderer.py +++ b/src/openedx_authz/engine/renderer.py @@ -24,16 +24,16 @@ from dataclasses import dataclass, field +from openedx_authz.data import ( + ACTION_NAMESPACE, + EFFECT_ALLOW, + POLICY_PTYPE, + ROLE_NAMESPACE, + SCOPE_WILDCARD, +) from openedx_authz.data import AUTHZ_POLICY_ATTRIBUTES_SEPARATOR as SEP from openedx_authz.engine.schema.types import CompiledSchema, RoleDefinition -# 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" - @dataclass(frozen=True) class PolicyRow: @@ -79,10 +79,10 @@ def policy_row(role_id: str, permission_id: str, scope: str) -> PolicyRow: """ return PolicyRow( ptype=POLICY_PTYPE, - subject=f"{ROLE_PREFIX}{SEP}{role_id}", - action=f"{ACTION_PREFIX}{SEP}{permission_id}", + subject=f"{ROLE_NAMESPACE}{SEP}{role_id}", + action=f"{ACTION_NAMESPACE}{SEP}{permission_id}", scope=f"{scope}{SEP}{SCOPE_WILDCARD}", - effect=ALLOW, + effect=EFFECT_ALLOW, ) From e531670869e7f62b99ec15daed386f6f16fefdaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20M=C3=A9ndez?= Date: Mon, 5 Oct 2026 13:37:05 -0600 Subject: [PATCH 3/4] squash!: Attend PR comments --- src/openedx_authz/api/data.py | 32 ++++---------- src/openedx_authz/api/permissions.py | 13 +++--- src/openedx_authz/data.py | 65 ++++++++++++++++++++++++++++ src/openedx_authz/engine/renderer.py | 47 +++++++++++--------- 4 files changed, 107 insertions(+), 50 deletions(-) diff --git a/src/openedx_authz/api/data.py b/src/openedx_authz/api/data.py index 20e3e153..a8724606 100644 --- a/src/openedx_authz/api/data.py +++ b/src/openedx_authz/api/data.py @@ -20,7 +20,14 @@ MANAGE_LIBRARY_TEAM, VIEW_LIBRARY_TEAM, ) -from openedx_authz.data import AUTHZ_POLICY_ATTRIBUTES_SEPARATOR, ActionData, AuthzBaseClass, AuthZData, PermissionData +from openedx_authz.data import ( + AUTHZ_POLICY_ATTRIBUTES_SEPARATOR, + ActionData, + AuthzBaseClass, + AuthZData, + PermissionData, + PolicyIndex, +) from openedx_authz.models.scopes import get_content_library_model, get_course_overview_model ContentLibrary = get_content_library_model() @@ -76,29 +83,6 @@ class GroupingPolicyIndex(Enum): # The rest of the fields are optional and can be ignored for now -class PolicyIndex(Enum): - """Index positions for fields in a Casbin policy (p). - - Policies define permissions by linking roles to actions within scopes with an effect. - Format: [role, action, scope, effect, ...] - - Attributes: - ROLE: Position 0 - The role identifier (e.g., 'role^instructor'). - ACT: Position 1 - The action identifier (e.g., 'act^read'). - SCOPE: Position 2 - The scope identifier (e.g., 'lib^lib:DemoX:CSPROB'). - EFFECT: Position 3 - The effect, either 'allow' or 'deny'. - - Note: - Additional fields beyond position 3 are optional and currently ignored. - """ - - ROLE = 0 - ACT = 1 - SCOPE = 2 - EFFECT = 3 - # The rest of the fields are optional and can be ignored for now - - class ScopeMeta(type): """Metaclass for ScopeData to handle dynamic subclass instantiation based on namespace.""" diff --git a/src/openedx_authz/api/permissions.py b/src/openedx_authz/api/permissions.py index 6448866a..aaf51d2e 100644 --- a/src/openedx_authz/api/permissions.py +++ b/src/openedx_authz/api/permissions.py @@ -22,14 +22,15 @@ def get_permission_from_policy(policy: list[str]) -> PermissionData: policy: A list representing a Casbin policy. Returns: - PermissionData: The corresponding PermissionData object or an empty PermissionData if the policy is invalid. - """ - if len(policy) < 4: # Do not count ptype - raise ValueError("Invalid policy format. Expected at least 4 elements.") + PermissionData: The corresponding PermissionData object. + Raises: + ValueError: If ``policy`` has fewer than ``PolicyIndex.required_width()`` elements. + """ + _role, action, _scope, effect = PolicyIndex.parse(policy) return PermissionData( - action=ActionData(namespaced_key=policy[PolicyIndex.ACT.value]), - effect=policy[PolicyIndex.EFFECT.value], + action=ActionData(namespaced_key=action), + effect=effect, ) diff --git a/src/openedx_authz/data.py b/src/openedx_authz/data.py index 7be26cd5..ab4faf4f 100644 --- a/src/openedx_authz/data.py +++ b/src/openedx_authz/data.py @@ -5,6 +5,7 @@ circular import between openedx_authz.api.data and openedx_authz.constants.permissions. """ +from enum import Enum from typing import ClassVar, Literal from attrs import define @@ -22,6 +23,70 @@ EFFECT_ALLOW = "allow" +class PolicyIndex(Enum): + """Index positions for fields in a Casbin policy (p). + + Policies define permissions by linking roles to actions within scopes with an effect. + Format: [role, action, scope, effect, ...] + + This is the single source of truth for the ``p`` row field layout, shared by + every producer and consumer of a Casbin row (the engine renderer and the + ``api`` data classes) so the mapping stays in one place. + + Attributes: + ROLE: Position 0 - The role identifier (e.g., 'role^instructor'). + ACT: Position 1 - The action identifier (e.g., 'act^read'). + SCOPE: Position 2 - The scope identifier (e.g., 'lib^lib:DemoX:CSPROB'). + EFFECT: Position 3 - The effect, either 'allow' or 'deny'. + + Note: + Additional fields beyond position 3 are optional and currently ignored. + """ + + ROLE = 0 + ACT = 1 + SCOPE = 2 + EFFECT = 3 + # The rest of the fields are optional and can be ignored for now + + @classmethod + def required_width(cls) -> int: + """Number of leading fields that make up a complete ``p`` row (4).""" + return len(cls) + + @classmethod + def pad(cls, values: list[str]) -> list[str]: + """Pad ``values`` with empty strings up to :meth:`required_width`. + + Callers that accept partially populated rows (e.g. the renderer + round-tripping an in-memory row) pad first so the shared, strict + :meth:`parse` does not reject them. + """ + return list(values) + [""] * (cls.required_width() - len(values)) + + @classmethod + def parse(cls, policy: list[str]) -> tuple[str, str, str, str]: + """Return ``(role, action, scope, effect)`` from a Casbin ``p`` row. + + The single place a ``p`` row is split into its fields, so every consumer + agrees on both the layout and the minimum shape. Rows shorter than + :meth:`required_width` are rejected; a caller that wants to tolerate a + partial row should :meth:`pad` it first. + + Raises: + ValueError: If ``policy`` has fewer than :meth:`required_width` + elements. + """ + if len(policy) < cls.required_width(): + raise ValueError(f"Invalid policy format. Expected at least {cls.required_width()} elements.") + return ( + policy[cls.ROLE.value], + policy[cls.ACT.value], + policy[cls.SCOPE.value], + policy[cls.EFFECT.value], + ) + + class AuthzBaseClass: """Base class for all authz classes.""" diff --git a/src/openedx_authz/engine/renderer.py b/src/openedx_authz/engine/renderer.py index ed4afd90..96248a89 100644 --- a/src/openedx_authz/engine/renderer.py +++ b/src/openedx_authz/engine/renderer.py @@ -30,6 +30,7 @@ POLICY_PTYPE, ROLE_NAMESPACE, SCOPE_WILDCARD, + PolicyIndex, ) from openedx_authz.data import AUTHZ_POLICY_ATTRIBUTES_SEPARATOR as SEP from openedx_authz.engine.schema.types import CompiledSchema, RoleDefinition @@ -57,10 +58,33 @@ def as_policy(self) -> list[str]: @classmethod 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] + """Build from a stored ``p`` row (``[subject, action, scope, effect]``). + + Parsing is delegated to the shared, strict + :meth:`~openedx_authz.data.PolicyIndex.parse`, so this stays in step + with the ``api`` layer. A partially populated row is padded first, so an + in-memory row round-trips instead of being rejected. + """ + subject, action, scope, effect = PolicyIndex.parse(PolicyIndex.pad(values)) return cls(POLICY_PTYPE, subject, action, scope, effect) + @classmethod + def from_grant(cls, role_id: str, permission_id: str, scope: str) -> "PolicyRow": + """Build the Casbin ``p`` row for one ``(role, permission, scope)`` grant. + + The single place the internal namespacing is applied, so every producer + and consumer of a rendered row agrees on its exact shape. A drift + between two such places would silently stop a later comparison against + the stored policy from matching anything. + """ + return cls( + ptype=POLICY_PTYPE, + subject=f"{ROLE_NAMESPACE}{SEP}{role_id}", + action=f"{ACTION_NAMESPACE}{SEP}{permission_id}", + scope=f"{scope}{SEP}{SCOPE_WILDCARD}", + effect=EFFECT_ALLOW, + ) + @dataclass class RenderedPolicy: @@ -69,23 +93,6 @@ class RenderedPolicy: rows: list[PolicyRow] = field(default_factory=list) -def policy_row(role_id: str, permission_id: str, scope: str) -> PolicyRow: - """Build the Casbin ``p`` row for one ``(role, permission, scope)`` grant. - - The single place the internal namespacing is applied, so every producer and - consumer of a rendered row agrees on its exact shape. A drift between two - such places would silently stop a later comparison against the stored - policy from matching anything. - """ - return PolicyRow( - ptype=POLICY_PTYPE, - subject=f"{ROLE_NAMESPACE}{SEP}{role_id}", - action=f"{ACTION_NAMESPACE}{SEP}{permission_id}", - scope=f"{scope}{SEP}{SCOPE_WILDCARD}", - effect=EFFECT_ALLOW, - ) - - class PolicyRenderer: """Turns a :class:`CompiledSchema` into Casbin ``p`` rows in memory.""" @@ -101,5 +108,5 @@ def render(self, schema: CompiledSchema) -> RenderedPolicy: role: RoleDefinition = schema.roles[role_id].definition for scope in sorted(role.scopes): for permission in sorted(role.permissions): - rows.append(policy_row(role.id, permission, scope)) + rows.append(PolicyRow.from_grant(role.id, permission, scope)) return RenderedPolicy(rows=rows) From f0252b742aa3fbfe1daf5e315da2271ad9f0ef05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20M=C3=A9ndez?= Date: Mon, 5 Oct 2026 15:05:54 -0600 Subject: [PATCH 4/4] squash!: Fix lint issues --- src/openedx_authz/data.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/openedx_authz/data.py b/src/openedx_authz/data.py index ab4faf4f..607e03e7 100644 --- a/src/openedx_authz/data.py +++ b/src/openedx_authz/data.py @@ -24,7 +24,8 @@ class PolicyIndex(Enum): - """Index positions for fields in a Casbin policy (p). + """ + Index positions for fields in a Casbin policy (p). Policies define permissions by linking roles to actions within scopes with an effect. Format: [role, action, scope, effect, ...] @@ -51,12 +52,13 @@ class PolicyIndex(Enum): @classmethod def required_width(cls) -> int: - """Number of leading fields that make up a complete ``p`` row (4).""" + """Return the number of leading fields that make up a complete ``p`` row (4).""" return len(cls) @classmethod def pad(cls, values: list[str]) -> list[str]: - """Pad ``values`` with empty strings up to :meth:`required_width`. + """ + Pad ``values`` with empty strings up to :meth:`required_width`. Callers that accept partially populated rows (e.g. the renderer round-tripping an in-memory row) pad first so the shared, strict @@ -66,7 +68,8 @@ def pad(cls, values: list[str]) -> list[str]: @classmethod def parse(cls, policy: list[str]) -> tuple[str, str, str, str]: - """Return ``(role, action, scope, effect)`` from a Casbin ``p`` row. + """ + Return ``(role, action, scope, effect)`` from a Casbin ``p`` row. The single place a ``p`` row is split into its fields, so every consumer agrees on both the layout and the minimum shape. Rows shorter than