feat: add authz schema discovery by rodmgwgu · Pull Request #474 · openedx/openedx-authz

rodmgwgu · 2026-09-23T16:19:03Z

Problem

Authorization schema resources need to be located before anything can be loaded: from the authz.schema entry-point group so applications ship definitions with their code, and from explicit directories so operators can contribute them through deployment configuration (ADR 0019).

Approach

The discovery phase only — no parsing, no models, no database.

openedx_authz/engine/schema/discovery.py — SchemaDiscovery, DiscoveredResource, SchemaDiscoveryError
setup.py — registers this package's own YAML files under the authz.schema group
openedx_authz/tests/schema/test_discovery.py

Resources are located and identified (package anchor, resource path, module, origin); reading and parsing them is the next PR in the stack.

Manual testing instructions

pytest openedx_authz/tests/schema/test_discovery.py

Entry-point discovery is exercised against this package's real installed metadata, so it covers the setup.py change.

Rollback plan

Revert this PR. Nothing consumes discovery yet, so the revert is inert.

Retro compatibility

No authorization behavior changes. No models, no migration, and no code path runs unless SchemaDiscovery is called explicitly.

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 (1/8) — #446 split into reviewable pieces. Bases chain bottom-up; merge in order.

This PR — schema discovery (base: main)
rod/authz-schema-loader-only — schema loading
rod/authz-schema-compiler — schema compilation
rod/authz-schema-validator — schema validation
rod/authz-schema-models — definition models + migration
rod/authz-schema-renderer — policy renderer
rod/authz-schema-applier — schema applier
feat: add authz schema pipeline and load_authz_schema command #446 — pipeline + load_authz_schema command, version bump and changelog

Merging all eight produces a tree byte-identical to the original #446 branch.

Merge checklist:

Version bumped — deferred to 8/8 so the stack does not conflict on every restack
Changelog record added — deferred to 8/8
Documentation updated (not only docstrings) — covered by the already-merged ADRs
Fixup commits are squashed away
Unit tests added/updated
Manual testing instructions provided
Noted any: Concerns, dependencies, migration issues, deadlines, tickets — bottom of the stack, no dependencies, no migration

openedx-webhooks · 2026-09-23T16:19:09Z

Thanks for the pull request, @rodmgwgu!

This repository is currently maintained by @openedx/committers-openedx-authz.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where 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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

mariajgrimaldi · 2026-09-24T13:15:56Z

+            return []
+        discovered: list[DiscoveredResource] = []
+        for directory in directories:
+            discovered.extend(self._iter_directory(directory, origin="settings"))


It’s not clear from the setting name whether the directory should be relative or absolute. For example, Mako templates use absolute directories for their configuration. Based on that, I couldn’t tell at first glance whether this referred to a directory of an installed module or to an absolute path.

Also, can we use an origin class for the "settings" string?

Good point, about the directory, it is actually a importlib.resources anchor + path.

I'll add documentation to clarify this.

mariajgrimaldi

Few comments about testing :)

Also, do you think some of the decisions made here should be documented in an ADR or the general ADR in #422 is enough?

mariajgrimaldi · 2026-09-24T14:47:23Z

+        discovered: list[DiscoveredResource] = []
+        for entry in entries:
+            if not entry.name.endswith(SCHEMA_FILE_SUFFIXES):
+                continue
+            if not entry.is_file():
+                continue
+            resource_path = f"{subpath}/{entry.name}" if subpath else entry.name
+            discovered.append(
+                DiscoveredResource(package=anchor, resource_path=resource_path, module=module, origin=origin)
+            )
+        return discovered


Wouldn't this work as an iter?

Suggested change

discovered: list[DiscoveredResource] = []

for entry in entries:

if not entry.name.endswith(SCHEMA_FILE_SUFFIXES):

continue

if not entry.is_file():

continue

resource_path = f"{subpath}/{entry.name}" if subpath else entry.name

discovered.append(

DiscoveredResource(package=anchor, resource_path=resource_path, module=module, origin=origin)

)

return discovered

for entry in entries:

if not entry.name.endswith(SCHEMA_FILE_SUFFIXES):

continue

if not entry.is_file():

continue

resource_path = f"{subpath}/{entry.name}" if subpath else entry.name

yield DiscoveredResource(package=anchor, resource_path=resource_path, module=module, origin=origin)

only a nit 👀

mariajgrimaldi · 2026-09-24T15:01:24Z

+            return []
+        discovered: list[DiscoveredResource] = []
+        for directory in directories:
+            discovered.extend(self._iter_directory(directory, origin="settings"))


Also, can we use an origin class for the "settings" string?

mariajgrimaldi · 2026-09-24T15:01:56Z

+        assert {r.origin for r in resources} == {"settings"}
+
+    def test_missing_django_contributes_nothing(self):
+        """The Casbin-free steps must stay importable and runnable without Django."""


In which cases we won't have django available?

None, in reality what we wanted to test here was for when the settings module is not available, changed to reflect that.

mariajgrimaldi · 2026-09-24T15:05:16Z

@@ -0,0 +1,276 @@
+"""Tests for directory-based schema discovery (ADR 0019)."""


I think I'm missing consistent docstrings from this module so it's easier to understand what's expected from each test.

Refactored to classes and added docstrings.

mariajgrimaldi · 2026-09-24T15:05:54Z

+    def test_entry_point_origin_wins_over_explicit_duplicate(self):
+        """The same file from two routes is kept once, tagged with the first route."""
+        resources = SchemaDiscovery(explicit_directories=[SCHEMA_DIR]).discover()
+
+        assert {r.origin for r in resources} == {"entry_point"}
+        assert len(resources) == len(EXPECTED_FILES)


Is this precedence documented somewhere? Not sure if it's already in an ADR.

I think this is more of an implementation detail, as it doesn't affect functionality, it only changes how the origin is marked, but the origin is only for diagnostics.

BryanttV

LGTM, just a few suggestions. Also, we need to resolve the conflicts due to the latest changes in the repo (#371).

BryanttV · 2026-09-29T22:47:28Z

+        """
+        try:
+            return resources.files(self.package).joinpath(self.resource_path).read_bytes()
+        except (FileNotFoundError, ModuleNotFoundError, OSError) as exc:


FileNotFoundError is a subclass of OSError

Suggested change

except (FileNotFoundError, ModuleNotFoundError, OSError) as exc:

except (ModuleNotFoundError, OSError) as exc:

applied, thanks!

BryanttV · 2026-09-29T22:53:04Z

+            base = resources.files(anchor)
+            target = base.joinpath(subpath) if subpath else base
+            entries = sorted(target.iterdir(), key=lambda entry: entry.name)
+        except (FileNotFoundError, ModuleNotFoundError, NotADirectoryError, OSError) as exc:


Suggested change

except (FileNotFoundError, ModuleNotFoundError, NotADirectoryError, OSError) as exc:

except (ModuleNotFoundError, OSError) as exc:

applied, thanks!

BryanttV · 2026-09-29T23:00:29Z

+        try:
+            base = resources.files(anchor)
+            target = base.joinpath(subpath) if subpath else base
+            entries = sorted(target.iterdir(), key=lambda entry: entry.name)


Is it necessary to sort here, considering it will be sorted later in .discover()?

not necessary, removed, thanks!

BryanttV · 2026-09-29T23:09:59Z

+            ) from exc
+
+
+class SchemaDiscoveryError(Exception):


Should we move this error class to the exceptions module?

We don't currently have an exceptions module, perhaps we can consider this as an improvement later when we have more exception classes.

mariajgrimaldi

LGTM. I don't have any other comments left :), thanks a lot!

We can merge this once we address Bryann's questions!

mariajgrimaldi · 2026-09-30T13:24:20Z

+        return discovered
+
+    def _iter_directory(self, directory: str, *, origin: Origin) -> list[DiscoveredResource]:
+        """Resolve a directory path and yield a resource per ``.yaml`` file.


Nit: I got a bit confused because of this docstring. It's not yielding a resource but all resources in the dir instead, can we update the yielding part or update the iter to return a resource (yield resource)?

changed, thanks!

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Sep 23, 2026

openedx-webhooks added this to Contributions Sep 23, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Sep 23, 2026

rodmgwgu added this pull request to stack #481 September 23, 2026 16:41

rodmgwgu requested review from BryanttV, carlos-marquez-wgu and mariajgrimaldi September 23, 2026 17:18

carlos-marquez-wgu approved these changes Sep 23, 2026

View reviewed changes

mariajgrimaldi requested changes Sep 24, 2026

View reviewed changes

mariajgrimaldi reviewed Sep 24, 2026

View reviewed changes

rodmgwgu force-pushed the rod/authz-schema-discovery branch from b2ed726 to 3b467d7 Compare September 24, 2026 21:35

rodmgwgu requested a review from mariajgrimaldi September 24, 2026 21:57

rodmgwgu force-pushed the rod/authz-schema-discovery branch from 3b467d7 to c5853aa Compare September 29, 2026 15:12

mphilbrick211 added the mao-onboarding Reviewing this will help onboard devs from an Axim mission-aligned organization (MAO). label Sep 29, 2026

BryanttV reviewed Sep 29, 2026

View reviewed changes

mariajgrimaldi approved these changes Sep 30, 2026

View reviewed changes

rodmgwgu force-pushed the rod/authz-schema-discovery branch 2 times, most recently from 21f461a to 5c642be Compare September 30, 2026 18:56

feat: add authz schema discovery

498dd8e

rodmgwgu force-pushed the rod/authz-schema-discovery branch from c9c6afc to 498dd8e Compare September 30, 2026 21:52

rodmgwgu merged commit 11c4996 into main Sep 30, 2026
8 checks passed

rodmgwgu deleted the rod/authz-schema-discovery branch September 30, 2026 22:00

efortish mentioned this pull request Oct 1, 2026

feat: extract translatable strings from the authz schema #509

Open

BryanttV linked an issue Oct 2, 2026 that may be closed by this pull request

Plugin Discovery Mechanism and Aggregation #433

Open

		@@ -0,0 +1,276 @@
		"""Tests for directory-based schema discovery (ADR 0019)."""

	except (FileNotFoundError, ModuleNotFoundError, OSError) as exc:
	except (ModuleNotFoundError, OSError) as exc:

Conversation

rodmgwgu commented Sep 23, 2026

Problem

Approach

Manual testing instructions

Rollback plan

Retro compatibility

AI Usage

Uh oh!

openedx-webhooks commented Sep 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodmgwgu Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mariajgrimaldi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanttV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mariajgrimaldi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

rodmgwgu Sep 24, 2026 •

edited

Loading