feat(policy): expose custom properties in SpEL policy rule conditions (#26774) #27033
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! |
|
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! |
SaaiAravindhRaja
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to expose entity custom properties (stored in extension) to the SpEL evaluation context so Policy Rule conditions can reference them via resource.customProperties.
Changes:
- Added
getCustomProperties()default method toResourceContextInterface. - Implemented
getCustomProperties()inResourceContextand attempted to ensureextensionis fetched when resolving entities.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContextInterface.java | Adds a default getCustomProperties() API for SpEL access. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ResourceContext.java | Implements custom properties retrieval and requests extension during entity resolution. |
| if (entityRepository.isSupportsReviewers()) { | ||
| fields = EntityUtil.addField(fields, Entity.FIELD_REVIEWERS); | ||
| } | ||
| fields = EntityUtil.addField(fields, FIELD_EXTENSION); |
There was a problem hiding this comment.
resolveEntity() always adds extension to the requested fields (EntityUtil.addField(fields, FIELD_EXTENSION)). For entity types that don't support extensions (e.g., User schema has no extension field), entityRepository.getFields(fields) will throw invalidField, which can break authorization checks for those resources. Guard this addition with a support check (e.g., entityRepository.getAllowedFields().contains(FIELD_EXTENSION)), or only request extension lazily when getCustomProperties() is actually invoked.
| fields = EntityUtil.addField(fields, FIELD_EXTENSION); | |
| if (entityRepository.getAllowedFields().contains(FIELD_EXTENSION)) { | |
| fields = EntityUtil.addField(fields, FIELD_EXTENSION); | |
| } |
| Object extension = entity.getExtension(); | ||
| if (extension instanceof Map) { | ||
| return (Map<String, Object>) extension; | ||
| } | ||
| return Collections.emptyMap(); |
There was a problem hiding this comment.
entity.getExtension() is typically returned as a Jackson ObjectNode (see EntityRepository#getExtension), not a Map, so the extension instanceof Map check will almost always be false and this will return an empty map even when custom properties exist. Use JsonUtils.getMap(entity.getExtension()) (or handle JsonNode explicitly) and consider returning an unmodifiable map to prevent SpEL expressions from mutating the backing object.
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| public Map<String, Object> getCustomProperties() { | ||
| resolveEntity(); | ||
| if (entity == null) { | ||
| return Collections.emptyMap(); | ||
| } | ||
| Object extension = entity.getExtension(); | ||
| if (extension instanceof Map) { |
There was a problem hiding this comment.
No tests currently cover referencing custom properties via SpEL (e.g., resource.customProperties.get('sensitivity') == 'PII'). Add a policy evaluator/rule evaluator unit test that sets an entity extension (custom properties) and asserts the expression evaluates correctly (including the empty-map case when no extension is present).
…tils for extension (open-metadata#27033)
|
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! |
SaaiAravindhRaja
changed the title
SaaiAravindhRaja
force-pushed
the
feat/issue-26774-custom-props-policy-rules
branch
from
a5edaac to
b8cca80
Compare
|
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}
| parseExpression("customProperties.get('sensitivity') == 'PII'") | ||
| .getValue(context, Boolean.class); | ||
| assertTrue(result != null && result); | ||
|
|
||
| RuleEvaluator validationEvaluator = new RuleEvaluator(false); | ||
| StandardEvaluationContext validationContext = new StandardEvaluationContext(validationEvaluator); | ||
| Boolean validationResult = | ||
| parseExpression("customProperties.get('sensitivity') == 'PII'") |
There was a problem hiding this comment.
CompiledRule.parseExpression() runs ExpressionValidator.validateExpressionSafety(), which rejects any method calls not in the allow-list. The expression customProperties.get('sensitivity') will be parsed as a get(...) function call and should fail validation (since get is not an allowed function), so this test (and the documented usage) won’t work as written. Consider switching to SpEL map index syntax (e.g., customProperties['sensitivity'] == 'PII') or adding a dedicated @Function helper on RuleEvaluator (e.g., customProperty('sensitivity')) so it’s allow-listed.
| parseExpression("customProperties.get('sensitivity') == 'PII'") | |
| .getValue(context, Boolean.class); | |
| assertTrue(result != null && result); | |
| RuleEvaluator validationEvaluator = new RuleEvaluator(false); | |
| StandardEvaluationContext validationContext = new StandardEvaluationContext(validationEvaluator); | |
| Boolean validationResult = | |
| parseExpression("customProperties.get('sensitivity') == 'PII'") | |
| parseExpression("customProperties['sensitivity'] == 'PII'") | |
| .getValue(context, Boolean.class); | |
| assertTrue(result != null && result); | |
| RuleEvaluator validationEvaluator = new RuleEvaluator(false); | |
| StandardEvaluationContext validationContext = new StandardEvaluationContext(validationEvaluator); | |
| Boolean validationResult = | |
| parseExpression("customProperties['sensitivity'] == 'PII'") |
| @SuppressWarnings("unused") | ||
| public Map<String, Object> getCustomProperties() { | ||
| if (resourceContext == null) { | ||
| return Collections.emptyMap(); | ||
| } | ||
| return resourceContext.getCustomProperties(); | ||
| } |
There was a problem hiding this comment.
This exposes customProperties as a Map, but with the current ExpressionValidator any customProperties.get(...) usage will fail allow-list checks (it sees get(...) as a function call). To make custom properties usable in policy conditions without weakening validation, consider either documenting/supporting index access (customProperties['key']) or adding an @Function helper (e.g., customProperty(String key) / matchCustomProperty(...)) that can be allow-listed.
| if (supportsExtension()) { | ||
| fields = EntityUtil.addField(fields, FIELD_EXTENSION); | ||
| } |
There was a problem hiding this comment.
Adding FIELD_EXTENSION to the default fetched fields means every lazy resolveEntity() call will now load the entity extension for all entities that support it, even when no policy condition references custom properties. This can increase DB payload/serialization overhead for all authorization checks. Consider loading the extension lazily only when getCustomProperties() is actually used (e.g., a flag to include extension in the first fetch, or a targeted refetch of only the extension when needed).
| if (supportsExtension()) { | |
| fields = EntityUtil.addField(fields, FIELD_EXTENSION); | |
| } | |
| // Do not include FIELD_EXTENSION in the default lazy fetch path. | |
| // Extension/custom properties should only be loaded when explicitly requested | |
| // or when repository-specific PATCH/PUT field sets require them. |
| try { | ||
| return JsonUtils.getMap(entity.getExtension()); | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to get custom properties: {}", e.getMessage()); |
There was a problem hiding this comment.
getCustomProperties() logs only e.getMessage() and drops the stack trace, which makes diagnosing malformed extension payloads difficult in production. Consider logging the exception itself (or at least at debug level) so the root cause is preserved.
| LOG.warn("Failed to get custom properties: {}", e.getMessage()); | |
| LOG.warn("Failed to get custom properties", e); |
| try { | ||
| entityRepository.getFields(FIELD_EXTENSION); | ||
| return true; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
supportsExtension() uses catch (Exception) to detect whether FIELD_EXTENSION is allowed. This can mask unexpected failures (e.g., repository initialization issues) and silently skip loading extensions. Consider catching the specific exception thrown for invalid fields (e.g., IllegalArgumentException) and letting other exceptions propagate or be logged.
| } catch (Exception e) { | |
| } catch (IllegalArgumentException e) { |
|
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! |
Code Review ✅ Approved 5 resolved / 5 findingsExposes custom properties in SpEL policy rule conditions via getCustomProperties() on the RuleEvaluator root object, with fixes for null safety, lazy extension field loading, and control flow improvements. All findings resolved. ✅ 5 resolved✅ Bug: PR contains no functional changes — only a trailing newline
✅ Bug: getCustomProperties() not exposed via RuleEvaluator SpEL root object
✅ Bug: getCustomProperties() returns null, risks NPE in SpEL expressions
✅ Performance: Extension field fetched for all entities, not just when needed
✅ Quality: supportsExtension() uses exceptions for control flow
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
PubChimps
added
the
safe to test
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
|
Hi @SaaiAravindhRaja, thank you for your pr, but this issue has been assigned to a fix here, please make sure that you are assigned to an issue before submitting a fix for it, we will reopen this pr if necessary. |
|
🟡 Playwright Results — all passed (25 flaky)✅ 3622 passed · ❌ 0 failed · 🟡 25 flaky · ⏭️ 209 skipped
🟡 25 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 |
Fixes #26774
Exposes entity custom properties in the SpEL evaluation context so Policy Rule conditions can reference them (e.g.
resource.customProperties.get('sensitivity') == 'PII').