Fixes #26774: expose custom properties in SpEL policy rule conditions#27218
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR extends the policy engine’s SpEL evaluation capabilities so policy conditions can evaluate entity Custom Properties (stored in the entity extension field), enabling ABAC-style rules based on those properties.
Changes:
- Add
getCustomProperties()toResourceContextInterfaceand implement it inResourceContextby deserializing the entityextension. - Update
ResourceContext.resolveEntity()to includeextensionin the resolved field list when supported, so custom properties are available during policy evaluation. - Add
getCustomProperties()andmatchCustomProperty(...)helpers toRuleEvaluator, plus unit tests covering the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContextInterface.java | Adds a default getCustomProperties() contract returning an empty map. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java | Implements custom property extraction from extension and attempts to ensure extension is fetched during entity resolution. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/RuleEvaluator.java | Adds SpEL-callable accessors/helpers for custom properties. |
| openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java | Adds unit tests for custom property access and matching behavior. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
PubChimps
added
the
safe to test
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
…he customProperties, use isSupportsExtension()
mohitjeswani01
force-pushed
the
feat/26774-custom-properties-spel-policy-conditions
branch
from
e51d312 to
2e365ad
Compare
|
Hi @PubChimps 👋 I've just applied the spotless formatting fix — Java checkstyle should I noticed the related PR #27033 was closed and Issue #26774 was closed
Could you let me know if this PR is still being considered , Thank you! 🙏 |
|
Hi @mohitjeswani01 thanks for reaching out, this issue was closed to prevent further PR's from being opened on it, could you please take a lot a the failed checks that were run? |
yes @PubChimps thanks for informing! I will make sure to update you shortly on the failed check , thanks ! |
🟡 Playwright Results — all passed (12 flaky)✅ 3970 passed · ❌ 0 failed · 🟡 12 flaky · ⏭️ 86 skipped
🟡 12 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
Hii @PubChimps sir , thanks for informing about the CI the Ci's are passing now and i will actively monitor those failing checks also and address the copilot comments shortly 👍! |
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
| } | ||
|
|
||
| if (actual instanceof List<?> list) { | ||
| return list.stream().anyMatch(item -> expectedValue.equals(item.toString())); |
There was a problem hiding this comment.
matchCustomProperty(...) can throw an NPE when the custom property value is a list that contains a null element (it does item.toString() inside anyMatch). Since the helper is documented as “handles all null/missing cases gracefully”, guard against null list items (and/or handle NullNode -> null) so the function returns false instead of failing evaluation.
| return list.stream().anyMatch(item -> expectedValue.equals(item.toString())); | |
| return list.stream().anyMatch(item -> item != null && expectedValue.equals(item.toString())); |
| if (entity == null || entity.getExtension() == null) { | ||
| cachedCustomProperties = Collections.emptyMap(); | ||
| return cachedCustomProperties; | ||
| } | ||
| try { | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> props = JsonUtils.convertValue(entity.getExtension(), Map.class); |
There was a problem hiding this comment.
getCustomProperties() caches Collections.emptyMap() as soon as entity.getExtension() is null. When this ResourceContext is constructed with a pre-loaded entity (constructor (resource, entity, repository)), resolveEntity() will not fetch missing fields, so an entity that supports extensions but was loaded without them will permanently appear to have no custom properties for the rest of the evaluation (because the empty map is cached). Consider lazily fetching the extension when missing (e.g., via entityRepository.getExtension(entity) / a lightweight reload with FIELD_EXTENSION) before caching the empty result, and only cache empty after that attempt.
| if (entity == null || entity.getExtension() == null) { | |
| cachedCustomProperties = Collections.emptyMap(); | |
| return cachedCustomProperties; | |
| } | |
| try { | |
| @SuppressWarnings("unchecked") | |
| Map<String, Object> props = JsonUtils.convertValue(entity.getExtension(), Map.class); | |
| if (entity == null) { | |
| cachedCustomProperties = Collections.emptyMap(); | |
| return cachedCustomProperties; | |
| } | |
| Object extension = entity.getExtension(); | |
| if (extension == null) { | |
| try { | |
| extension = entityRepository.getExtension(entity); | |
| } catch (Exception e) { | |
| LOG.warn( | |
| "Failed to fetch custom properties for entity {}: {}", | |
| entity.getId(), | |
| e.getMessage()); | |
| } | |
| } | |
| if (extension == null) { | |
| cachedCustomProperties = Collections.emptyMap(); | |
| return cachedCustomProperties; | |
| } | |
| try { | |
| @SuppressWarnings("unchecked") | |
| Map<String, Object> props = JsonUtils.convertValue(extension, Map.class); |
|
Describe your changes:
Fixes #26774
I worked on the Policy engine's SpEL evaluation context because entity
Custom Properties (e.g., "Data Sensitivity", "Department Owner") were
completely invisible to Policy Rule conditions — forcing teams to use
overly broad roles or Tags as a workaround instead of proper
attribute-based access control (ABAC).
Root Cause
ResourceContextnever fetched theextensionfield (where customproperties are stored) when resolving entities for policy evaluation.
Even if extension was available, no SpEL-callable method exposed it
to rule conditions.
What I Changed — 3 files, 1 commit:
ResourceContextInterface.javagetCustomProperties()default method returningMap<String, Object>— safe default of empty mapResourceContext.javagetCustomProperties()— fetches entity extension,deserializes via
JsonUtils, returns empty map on any failureresolveEntity()field list to includeFIELD_EXTENSIONwhen the entity type supports it — so custom properties are
actually loaded from DB during policy evaluation
RuleEvaluator.javagetCustomProperties()bridge method — returnsemptyMap()(not null) when
resourceContextis null, preventing NPE in SpELmatchCustomProperty(String propertyName, String expectedValue)— a convenient boolean helper for policy rule conditions
Usage in Policy Rule Conditions
After this change, policy rules can use:
Why matchCustomProperty() matters
The issue explicitly requested a helper method to simplify syntax.
Without it, policy authors must use verbose map access syntax.
matchCustomProperty()handles all null/missing cases gracefullyand works correctly in both validation mode and runtime mode.
How I Tested
Added 6 unit tests to
RuleEvaluatorTest.javacovering:getCustomProperties()returns empty map when no extensiongetCustomProperties()returns correct map when extension is setmatchCustomProperty()returns true when property matchesmatchCustomProperty()returns false when value doesn't matchmatchCustomProperty()returns false when property is missingmatchCustomProperty()returns false when entity has no extensionType of change:
Checklist:
Fixes #26774: expose custom properties in SpEL policy rule conditionsSummary by Gitar
EntityRepositoryread bundlesThis will update automatically on new commits.