feat(auth): add config-driven domain policy layer#152
Open
ironcommit wants to merge 5 commits into
Open
Conversation
Signed-off-by: Ryan Sadler <267728323+ironcommit@users.noreply.github.com>
ironcommit
requested review from
anastasia-nesterenko,
maxdubrinsky and
mckornfield
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAuthorization now extracts domain names from request paths, exposes domain metadata to Rego via get_domain_metadata/current_domain, synthesizes an authz domain registry at bundle time from endpoints plus discovered manifests, and adds tests for metadata wiring and collision handling. ChangesDomain metadata infrastructure for authorization
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/core/auth/src/nmp/core/auth/app/bundle.py`:
- Around line 47-52: The code currently lets manifests override core-inferred
domains by iterating over sorted(domain_names | manifests.keys()) and writing
into domains; change this to detect collisions first and fail fast: compute the
intersection between domain_names and manifests.keys() (e.g., conflicts =
domain_names & manifests.keys()) and if non-empty raise an exception
(ValueError/RuntimeError) with a clear message listing the conflicting names so
the PR fails rather than silently overwriting; keep the existing
domains-building logic (using manifests.get(name) and setting "extension" vs
"core") but only run it after verifying there are no conflicts so
common.get_domain_metadata(...) is never fed corrupted metadata.
In `@services/core/auth/src/nmp/core/auth/app/policies/domain.rego`:
- Around line 19-35: The rule domain_policy_checks_pass is currently a tautology
because its three branches cover every case (no current_domain, current_domain
without a policy, current_domain with a policy) so the domain layer never
enforces anything; change the third branch (the one matching when
has_domain_policy(domain.name) and binding policy :=
data.authz.domain_policies[domain.name]) to actually evaluate the policy outcome
instead of just returning the policy object — e.g., invoke or test the policy
decision (replace the unconditional "policy" success with a check like
policy.allow or calling a domain-specific decision rule such as
domain_policy_allows_request) so domain_policy_checks_pass only returns true
when there is no domain or when a domain exists and its policy explicitly
permits the request (this will allow the allow_request gating in authz.rego to
enforce domain policies).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c7af7318-db68-49ba-9070-542d4decb13f
📒 Files selected for processing (9)
services/core/auth/src/nmp/core/auth/app/bundle.pyservices/core/auth/src/nmp/core/auth/app/policies/authz.regoservices/core/auth/src/nmp/core/auth/app/policies/common.regoservices/core/auth/src/nmp/core/auth/app/policies/domain.regoservices/core/auth/src/nmp/core/auth/app/policies/extract.regoservices/core/auth/src/nmp/core/auth/app/policy_tests/authz_simple_test.regoservices/core/auth/src/nmp/core/auth/app/policy_tests/helpers_test.regoservices/core/auth/tests/test_bundle.pyservices/core/auth/tests/test_embedded_pdp.py
Comment on lines
+19
to
+35
| domain_policy_checks_pass if { | ||
| not current_domain | ||
| } | ||
|
|
||
| domain_policy_checks_pass if { | ||
| current_domain | ||
| not has_domain_policy(current_domain.name) | ||
| } | ||
|
|
||
| domain_policy_checks_pass if { | ||
| domain := current_domain | ||
| has_domain_policy(domain.name) | ||
| policy := data.authz.domain_policies[domain.name] | ||
| # First cut: policy presence activates the domain layer without | ||
| # adding additional deny conditions yet. | ||
| policy | ||
| } |
There was a problem hiding this comment.
domain_policy_checks_pass is a tautology.
One of these three branches matches every request: no current domain, current domain without a policy, or current domain with a policy. That makes domain_policy_checks_pass effectively always true, so the new allow_request gates in services/core/auth/src/nmp/core/auth/app/policies/authz.rego never enforce the domain layer at all.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/core/auth/src/nmp/core/auth/app/policies/domain.rego` around lines
19 - 35, The rule domain_policy_checks_pass is currently a tautology because its
three branches cover every case (no current_domain, current_domain without a
policy, current_domain with a policy) so the domain layer never enforces
anything; change the third branch (the one matching when
has_domain_policy(domain.name) and binding policy :=
data.authz.domain_policies[domain.name]) to actually evaluate the policy outcome
instead of just returning the policy object — e.g., invoke or test the policy
decision (replace the unconditional "policy" success with a check like
policy.allow or calling a domain-specific decision rule such as
domain_policy_allows_request) so domain_policy_checks_pass only returns true
when there is no domain or when a domain exists and its policy explicitly
permits the request (this will allow the allow_request gating in authz.rego to
enforce domain policies).
Signed-off-by: Ryan Sadler <267728323+ironcommit@users.noreply.github.com>
Signed-off-by: Ryan Sadler <267728323+ironcommit@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/core/auth/tests/test_embedded_pdp.py (1)
194-204: 💤 Low valueDuplicate of
test_authenticated_user_with_permission.Identical input and assertion (Lines 158-168). Since
minimal_authz_datanow always carriesdomains, every test in this class already exercises the "domain metadata present" path, so this adds no coverage. Either drop it or make it assert something the existing test doesn't (e.g. thatdomainsis actually in policy data / a domain-policy field).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/auth/tests/test_embedded_pdp.py` around lines 194 - 204, The test test_allow_keeps_working_when_domain_metadata_is_present is a duplicate of test_authenticated_user_with_permission; either remove it or change it to assert something unique: after calling set_policy_data(minimal_authz_data) and evaluate("allow", ...), also assert that the policy data contains the domains metadata (e.g., check minimal_authz_data or the policy store for a "domains" key or domain-specific field) so the test verifies domain metadata is present rather than duplicating the permission check; update the test name and assertions accordingly (refer to test_allow_keeps_working_when_domain_metadata_is_present and test_authenticated_user_with_permission).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@services/core/auth/tests/test_embedded_pdp.py`:
- Around line 194-204: The test
test_allow_keeps_working_when_domain_metadata_is_present is a duplicate of
test_authenticated_user_with_permission; either remove it or change it to assert
something unique: after calling set_policy_data(minimal_authz_data) and
evaluate("allow", ...), also assert that the policy data contains the domains
metadata (e.g., check minimal_authz_data or the policy store for a "domains" key
or domain-specific field) so the test verifies domain metadata is present rather
than duplicating the permission check; update the test name and assertions
accordingly (refer to test_allow_keeps_working_when_domain_metadata_is_present
and test_authenticated_user_with_permission).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73fdc833-4fb7-493e-90c7-17e9cca5fd75
📒 Files selected for processing (4)
services/core/auth/src/nmp/core/auth/app/bundle.pyservices/core/auth/src/nmp/core/auth/app/policies/domain.regoservices/core/auth/src/nmp/core/auth/app/policy_tests/authz_simple_test.regoservices/core/auth/tests/test_embedded_pdp.py
💤 Files with no reviewable changes (2)
- services/core/auth/src/nmp/core/auth/app/policies/domain.rego
- services/core/auth/src/nmp/core/auth/app/bundle.py
Contributor
|
mckornfield
approved these changes
Jun 3, 2026
Contributor
mckornfield
left a comment
mckornfield
left a comment
There was a problem hiding this comment.
domain doesn't feel like the right name to me because of priors, but I don't know that my other names are any better
| extract_domain_name(path) := domain if { | ||
| base_path := split(path, "?")[0] | ||
| parts := split(base_path, "/") | ||
| count(parts) >= 3 |
Contributor
There was a problem hiding this comment.
does this act as an assert?
| path := input.attributes.request.http.path | ||
| } | ||
|
|
||
| # Extract domain name from /apis/<domain>/... request paths. |
Contributor
There was a problem hiding this comment.
when I think domain I think of DNS like a TLD, is that the right word, versus "service" or "component"?
Signed-off-by: Ryan Sadler <267728323+ironcommit@users.noreply.github.com>
Signed-off-by: Ryan Sadler <267728323+ironcommit@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a domain metadata layer to the auth service. API domains (e.g.
models,agents) are automatically discovered from registered endpoint paths and extension plugin manifests, then injected into the OPA authorization data bundle as a unified registry. This gives policy authors a structuredcurrent_domainrule andget_domain_metadatahelper to key on when writing domain-specific authorization logic.Changes
OPA policies
domain.rego— exposescurrent_domainby resolving the request path's API domain against the registryextract.rego— addedextract_domain_name(path)to pull the domain segment from/apis/<domain>/...pathscommon.rego— addedget_domain_metadata(path)helper to look up domain name, version, and kind fromdata.authz.domainsBundle builder (
bundle.py)_extract_domain_names_from_endpointsdiscovers core API domain names from endpoint paths_build_domain_registrymerges core domains with extension plugin manifests (viadiscover_manifests)ValueErrorif an extension manifest name conflicts with a core API domaindomainsregistry into the authorization data bundle underauthz.domainsTests
allowauthorization is unaffected when domain metadata is presentSummary by CodeRabbit
Chores
Tests