#2325 add namespace support for policies#2368
#2325 add namespace support for policies#2368hu-ahmed wants to merge 3 commits intoeclipse-ditto:masterfrom
Conversation
|
update system test eclipse-ditto/ditto-testing#24 |
There was a problem hiding this comment.
QA Review — Namespace-Scoped Policy Entries
Clean design overall. The opt-in namespaces field on policy entries with backward-compatible empty-list semantics is well chosen. Enforcement, search indexing, event sourcing strategies, and JSON round-tripping all look correct. Good test coverage for the model layer.
Two findings to flag at the PR level:
CRITICAL: OpenAPI bundled file ditto-api-2.yml not regenerated
The PR adds documentation/src/main/resources/openapi/sources/schemas/policies/namespaces.yml and modifies policyEntry.yml, but the bundled ditto-api-2.yml is not regenerated. Per project convention, modifications to OpenAPI source files require running:
cd documentation/src/main/resources/openapi/sources && npm install && npm run buildand committing the regenerated output.
HIGH: forNamespace() returns unfiltered policy with a filtered enforcer
return new PolicyEnforcer(policy, filteredEnforcer);After forNamespace(), getEnforcer() sees only entries that match the namespace, but getPolicy() returns the full unfiltered policy (including all scoped entries). Any code path that reads getPolicy() on the filtered enforcer — e.g., to extract subjects, build response headers, or check entry labels — would see entries that the enforcer does NOT enforce.
This is currently safe because the existing code uses getEnforcer() for permission checks, but it's a foot-gun for future callers. Consider either:
- Documenting this invariant explicitly in
forNamespace()'s Javadoc ("the returned enforcer'sgetPolicy()is unfiltered") - Or building a filtered
Policyobject alongside the filteredEnforcer
System test coverage (ditto-testing PR eclipse-ditto/ditto-testing#24)
The two system tests in PR eclipse-ditto/ditto-testing#24 cover the core positive/negative scenarios well:
getThingWithNamespaceScopedPolicyEntry— direct policy with scoped entrygetThingWithNamespaceScopedAuthorizationInImportedPolicy— imported policy with scoped entry
Missing system-level coverage:
- Search visibility: a user granted access via namespace-scoped entry should find the allowed thing via search (
/api/2/search/things) but NOT the denied thing. TheEvaluatedPolicy.of()change looks correct, but there's no integration test verifying search results respect namespace scoping end-to-end.
policies/enforcement/src/main/java/org/eclipse/ditto/policies/enforcement/PolicyEnforcer.java
Outdated
Show resolved
Hide resolved
policies/model/src/main/java/org/eclipse/ditto/policies/model/PolicyEntry.java
Outdated
Show resolved
Hide resolved
...ies/enforcement/src/test/java/org/eclipse/ditto/policies/enforcement/PolicyEnforcerTest.java
Show resolved
Hide resolved
policies/model/src/main/java/org/eclipse/ditto/policies/model/ImmutablePolicyEntry.java
Outdated
Show resolved
Hide resolved
documentation/src/main/resources/openapi/sources/schemas/policies/policyEntry.yml
Outdated
Show resolved
Hide resolved
policies/model/src/main/java/org/eclipse/ditto/policies/model/ImmutablePolicyBuilder.java
Outdated
Show resolved
Hide resolved
policies/model/src/main/java/org/eclipse/ditto/policies/model/ImmutablePolicyBuilder.java
Outdated
Show resolved
Hide resolved
policies/model/src/main/java/org/eclipse/ditto/policies/model/ImmutablePolicyEntry.java
Outdated
Show resolved
Hide resolved
|
In general a thing I encountered: the I added some inline remarks about that - but as this covers the whole PR, it should be addressed everywhere :) Also, the recently added |
970f9e7 to
eca8bff
Compare
|
@hu-ahmed system tests run has 1 error in a CheckPermissionsIT - could you check, please? https://github.com/eclipse-ditto/ditto/actions/runs/23442796589#user-content-tr-6zeiug-r65 |
|
System tests passed: https://github.com/eclipse-ditto/ditto/actions/runs/23442796589 |
…cluding "namespaces"
91bb5d8 to
7ee81b2
Compare
7ee81b2 to
ddfd9ae
Compare
Resolves #2325
Add support for namespace-scoped policy entries via a new optional
namespacesfield onPolicyEntry.A scoped policy entry only applies to Things whose namespace matches at least one configured pattern.
If
namespacesis omitted or empty, the entry remains globally applicable for backward compatibility.Supported matching semantics:
com.acmematches only the exact namespacecom.acmecom.acme.*matches nested namespaces such ascom.acme.vehicles, but notcom.acmeitself