diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java index 60b09817a814..5cbb3c4bee8f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java @@ -252,6 +252,10 @@ private void handleMethodReference(SpelNode node, ConditionCollector collector) hasAnyRole(roles, collector); } case "hasDomain" -> hasDomain(collector); + case "matchAnyDomain" -> { + List domainFqns = extractMethodArguments(methodRef); + matchAnyDomain(domainFqns, collector); + } case "inAnyTeam" -> { List teams = extractMethodArguments(methodRef); inAnyTeam(teams, collector); @@ -406,6 +410,22 @@ private void matchAnyServiceType(List serviceTypes, ConditionCollector c collector.addMust(queryBuilderFactory.boolQuery().should(clauses)); } + public void matchAnyDomain(List domainFqns, ConditionCollector collector) { + List domainQueries = new ArrayList<>(); + for (String domainFqn : domainFqns) { + domainQueries.add(queryBuilderFactory.termQuery("domains.fullyQualifiedName", domainFqn)); + } + switch (domainQueries.size()) { + case 0 -> + // No domains requested: the condition can never match, so make the rule deny-all instead + // of adding an empty bool query (Elasticsearch/OpenSearch treat an empty bool query as + // match_all, which would turn an ALLOW rule into a full index grant). + collector.setMatchNothing(true); + case 1 -> collector.addMust(domainQueries.get(0)); + default -> collector.addMust(queryBuilderFactory.boolQuery().should(domainQueries)); + } + } + public void matchAnyCertification( List certificationLabels, ConditionCollector collector) { List certificationQueries = new ArrayList<>(); @@ -413,13 +433,14 @@ public void matchAnyCertification( certificationQueries.add( queryBuilderFactory.termQuery("certification.tagLabel.tagFQN", certificationLabel)); } - OMQueryBuilder certificationQueriesCombined; - if (certificationQueries.size() == 1) { - certificationQueriesCombined = certificationQueries.get(0); - } else { - certificationQueriesCombined = queryBuilderFactory.boolQuery().should(certificationQueries); + switch (certificationQueries.size()) { + case 0 -> + // No certifications requested: deny-all instead of emitting an empty bool query, which + // Elasticsearch/OpenSearch evaluate as match_all and would turn an ALLOW into a grant. + collector.setMatchNothing(true); + case 1 -> collector.addMust(certificationQueries.get(0)); + default -> collector.addMust(queryBuilderFactory.boolQuery().should(certificationQueries)); } - collector.addMust(certificationQueriesCombined); } public void isOwner(User user, ConditionCollector collector) { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/RuleEvaluator.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/RuleEvaluator.java index e9e3f2a182d9..b16041a31323 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/RuleEvaluator.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/RuleEvaluator.java @@ -247,6 +247,43 @@ public boolean noDomain() { return nullOrEmpty(resourceContext.getDomains()); } + @Function( + name = "matchAnyDomain", + input = "List of comma separated Domain fully qualified names", + description = + "Returns true if the entity being accessed belongs to at least one of the domains given as input", + examples = {"matchAnyDomain('Domain.Finance', 'Domain.Procurement')"}) + @SuppressWarnings("unused") + public boolean matchAnyDomain(String... domainFQNs) { + if (expressionValidation) { + for (String domainFQN : domainFQNs) { + validateEntityReference(Entity.DOMAIN, domainFQN); + } + return false; + } + if (resourceContext == null) { + return false; + } + List domains = resourceContext.getDomains(); + if (nullOrEmpty(domains)) { + LOG.debug("No Domains found for resource"); + return false; + } + LOG.debug( + "matchAnyDomain {} resourceDomains {}", + Arrays.toString(domainFQNs), + Arrays.toString(domains.toArray())); + for (String domainFQN : domainFQNs) { + boolean found = + domains.stream() + .anyMatch(d -> domainFQN.equals(d.getFullyQualifiedName())); + if (found) { + return true; + } + } + return false; + } + @Function( name = "matchAllTags", input = "List of comma separated tag or glossary fully qualified names", diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java index 3f2111eb82be..95fa73683037 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java @@ -350,6 +350,45 @@ void testHasDomain() { "The query should contain the user's domain ID."); } + @Test + void testMatchAnyDomain() { + setupMockPolicies( + "matchAnyDomain('Domain.Finance', 'Domain.Procurement')", "ALLOW"); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + Query elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = serializeQueryToJson(elasticQuery); + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + + assertTrue( + generatedQuery.contains("domains.fullyQualifiedName"), + "The query should contain 'domains.fullyQualifiedName'."); + assertFieldExists( + jsonContext, + "$.bool.should[?(@.term['domains.fullyQualifiedName'].value=='Domain.Finance')]", + "Domain.Finance should be in a should (OR) clause"); + assertFieldExists( + jsonContext, + "$.bool.should[?(@.term['domains.fullyQualifiedName'].value=='Domain.Procurement')]", + "Domain.Procurement should be in a should (OR) clause"); + } + + @Test + void testMatchAnyDomainWithSingleDomain() { + setupMockPolicies("matchAnyDomain('Domain.Finance')", "ALLOW"); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + Query elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = serializeQueryToJson(elasticQuery); + + assertTrue( + generatedQuery.contains("domains.fullyQualifiedName"), + "The query should contain 'domains.fullyQualifiedName'."); + assertTrue( + generatedQuery.contains("Domain.Finance"), + "The query should contain the requested domain FQN."); + } + @Test void testHasDomainWithMultipleDomains() { setupMockPolicies("hasDomain()", "ALLOW"); @@ -1606,4 +1645,36 @@ void testStaticNestedQueryDoesNotFailForUnmappedFields() { json.contains("\"ignore_unmapped\":true"), "must set ignore_unmapped so query works on indexes without this nested field"); } + + /** + * Regression for the empty-condition ALLOW leak: {@code matchAnyDomain()} with zero arguments used + * to emit an empty bool query, which Elasticsearch evaluates as match_all, so an ALLOW rule + * granted access to every searchable resource. The generated query must fail closed (match_none) + * and must not contain an empty must/should clause. + */ + @Test + void testMatchAnyDomainWithNoArgumentsDoesNotGrantAccess() { + setupMockPolicies("matchAnyDomain()", "ALLOW"); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + Query elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = serializeQueryToJson(elasticQuery); + + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + // RBACConditionEvaluator fails closed by returning ConditionCollector's match_none query, which + // serializes as {"bool":{"must_not":[{"match_all":{}}]}} — a query that can never match. + assertFieldExists( + jsonContext, + "$.bool.must_not[?(@.match_all)]", + "an ALLOW rule with an empty matchAnyDomain() must fail closed to a never-matching query"); + assertFalse( + generatedQuery.contains("domains.fullyQualifiedName"), + "no domain clause should be emitted for an empty argument list"); + assertFalse( + generatedQuery.contains("\"should\":[]"), + "no empty bool clause may be emitted, it is treated as match_all by Elasticsearch"); + // Safe against PathNotFoundException: a match_none query omits `must` entirely. + assertFieldDoesNotExist( + jsonContext, "$.bool.must", "must (an empty bool is treated as match_all by Elasticsearch)"); + } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/OpenSearchRBACConditionEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/OpenSearchRBACConditionEvaluatorTest.java index a0ff785e2074..8ae82201f88e 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/OpenSearchRBACConditionEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/OpenSearchRBACConditionEvaluatorTest.java @@ -162,6 +162,28 @@ void testOpenSearchRoleAndDomainCheck() { "domains.id"); } + @Test + void testMatchAnyDomain() { + setupMockPolicies("matchAnyDomain('Domain.Finance', 'Domain.Procurement')", "ALLOW"); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + Query openSearchQuery = ((OpenSearchQueryBuilder) finalQuery).build(); + String generatedQuery = openSearchQuery.toJsonString(); + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + + assertTrue( + generatedQuery.contains("domains.fullyQualifiedName"), + "The query should contain 'domains.fullyQualifiedName'."); + assertFieldExists( + jsonContext, + "$..bool.should[?(@.term['domains.fullyQualifiedName'].value=='Domain.Finance')]", + "Domain.Finance should be in a should (OR) clause"); + assertFieldExists( + jsonContext, + "$..bool.should[?(@.term['domains.fullyQualifiedName'].value=='Domain.Procurement')]", + "Domain.Procurement should be in a should (OR) clause"); + } + @Test void testHasDomainWithMultipleDomains() { setupMockPolicies("hasDomain()", "ALLOW"); @@ -426,4 +448,36 @@ void testStaticNestedQueryDoesNotFailForUnmappedFields() { json.contains("\"ignore_unmapped\":true"), "must set ignore_unmapped so query works on indexes without this nested field"); } + + /** + * Regression for the empty-condition ALLOW leak: {@code matchAnyDomain()} with zero arguments used + * to emit an empty bool query, which OpenSearch evaluates as match_all, so an ALLOW rule granted + * access to every searchable resource. The generated query must fail closed (match_none) and must + * not contain an empty must/should clause. + */ + @Test + void testMatchAnyDomainWithNoArgumentsDoesNotGrantAccess() { + setupMockPolicies("matchAnyDomain()", "ALLOW"); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + Query openSearchQuery = ((OpenSearchQueryBuilder) finalQuery).build(); + String generatedQuery = openSearchQuery.toJsonString(); + + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + // RBACConditionEvaluator fails closed by returning ConditionCollector's match_none query, which + // serializes as {"bool":{"must_not":[{"match_all":{}}]}} — a query that can never match. + assertFieldExists( + jsonContext, + "$.bool.must_not[?(@.match_all)]", + "an ALLOW rule with an empty matchAnyDomain() must fail closed to a never-matching query"); + assertFalse( + generatedQuery.contains("domains.fullyQualifiedName"), + "no domain clause should be emitted for an empty argument list"); + assertFalse( + generatedQuery.contains("\"should\":[]"), + "no empty bool clause may be emitted, it is treated as match_all by OpenSearch"); + // Safe against PathNotFoundException: a match_none query omits `must` entirely. + assertFieldDoesNotExist( + jsonContext, "$.bool.must", "must (an empty bool is treated as match_all by OpenSearch)"); + } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java index b466974cc44c..522375668703 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java @@ -482,6 +482,79 @@ void test_matchAnyTag() { assertTrue(evaluateExpression("!matchAnyTag('tag4')")); } + @Test + void test_matchAnyDomain() { + EntityReference finance = + new EntityReference() + .withId(UUID.randomUUID()) + .withName("Finance") + .withFullyQualifiedName("Domain.Finance"); + EntityReference procurement = + new EntityReference() + .withId(UUID.randomUUID()) + .withName("Procurement") + .withFullyQualifiedName("Domain.Procurement"); + + CreateResourceContext contextWithDomains = mock(CreateResourceContext.class); + Mockito.when(contextWithDomains.getDomains()).thenReturn(listOf(finance, procurement)); + StandardEvaluationContext ctx = + new StandardEvaluationContext(new RuleEvaluator(null, subjectContext, contextWithDomains)); + + // Resource belongs to Finance; both the first and the second input match + assertTrue( + parseExpression("matchAnyDomain('Domain.Finance', 'Domain.Engineering')") + .getValue(ctx, Boolean.class)); + assertTrue( + parseExpression("matchAnyDomain('Domain.Engineering', 'Domain.Procurement')") + .getValue(ctx, Boolean.class)); + assertTrue(parseExpression("matchAnyDomain('Domain.Finance')").getValue(ctx, Boolean.class)); + assertFalse(parseExpression("!matchAnyDomain('Domain.Finance')").getValue(ctx, Boolean.class)); + + // Domain.Finance is not present in the resource's domains + assertFalse( + parseExpression("matchAnyDomain('Domain.Engineering')").getValue(ctx, Boolean.class)); + assertTrue( + parseExpression("!matchAnyDomain('Domain.Engineering')").getValue(ctx, Boolean.class)); + assertFalse( + parseExpression("matchAnyDomain('Domain.Engineering', 'Domain.Marketing')") + .getValue(ctx, Boolean.class)); + + // Resource with no domains never matches + CreateResourceContext contextNoDomains = mock(CreateResourceContext.class); + Mockito.when(contextNoDomains.getDomains()).thenReturn(new ArrayList<>()); + StandardEvaluationContext emptyCtx = + new StandardEvaluationContext(new RuleEvaluator(null, subjectContext, contextNoDomains)); + assertFalse( + parseExpression("matchAnyDomain('Domain.Finance')").getValue(emptyCtx, Boolean.class)); + assertTrue( + parseExpression("!matchAnyDomain('Domain.Finance')").getValue(emptyCtx, Boolean.class)); + + // getDomains() returning null must not blow up + CreateResourceContext contextNullDomains = mock(CreateResourceContext.class); + Mockito.when(contextNullDomains.getDomains()).thenReturn(null); + StandardEvaluationContext nullCtx = + new StandardEvaluationContext(new RuleEvaluator(null, subjectContext, contextNullDomains)); + assertFalse( + parseExpression("matchAnyDomain('Domain.Finance')").getValue(nullCtx, Boolean.class)); + + // A null resource context never matches + StandardEvaluationContext noResourceCtx = + new StandardEvaluationContext(new RuleEvaluator(null, subjectContext, null)); + assertFalse( + parseExpression("matchAnyDomain('Domain.Finance')").getValue(noResourceCtx, Boolean.class)); + } + + @Test + void test_matchAnyDomain_duringExpressionValidation() { + // The validating RuleEvaluator has no resource context; the function must only resolve the + // referenced domains and return false instead of throwing or NPE-ing. + StandardEvaluationContext ctx = new StandardEvaluationContext(new RuleEvaluator(false)); + assertFalse( + parseExpression("matchAnyDomain('Domain.Finance', 'Domain.Procurement')") + .getValue(ctx, Boolean.class)); + assertTrue(parseExpression("!matchAnyDomain('Domain.Finance')").getValue(ctx, Boolean.class)); + } + @Test void test_matchAnyCertification() { // Certification is not Present @@ -779,8 +852,8 @@ private Boolean evaluateUserResourceCondition(User target, String condition) { * A ResourceContext built with neither an id nor a name never resolves an entity, so every * attribute a policy condition reads comes back empty - and nothing reports that it did. A tag * condition then answers the same way whether or not the tag is present, which makes a Deny fire - * on every entity in one polarity and on none in the other. This is the root cause of #31941; - * the fix is that callers holding a specific entity must pass its identity in. + * on every entity in one polarity and on none in the other. This is the root cause of #31941; the + * fix is that callers holding a specific entity must pass its identity in. */ @Test void test_bareResourceContextCannotSeeTags() { @@ -1058,13 +1131,15 @@ void test_hasDomain_edgeCases_realFQNFormat() { table.setDomains(List.of(corpSubDomain.getEntityReference())); assertTrue( evaluateExpression("hasDomain()"), - "User with '\"Corp.Domain\"' should have access to '\"Corp.Domain\".\"Sub.Division\"' resources"); + "User with '\"Corp.Domain\"' should have access to '\"Corp.Domain\".\"Sub.Division\"'" + + " resources"); user.setDomains(List.of(corpDomain.getEntityReference())); table.setDomains(List.of(corpDifferent.getEntityReference())); assertTrue( evaluateExpression("hasDomain()"), - "User with '\"Corp.Domain\"' should have access to '\"Corp.Domain\".\"Other.Division\"' resources"); + "User with '\"Corp.Domain\"' should have access to '\"Corp.Domain\".\"Other.Division\"'" + + " resources"); // Edge Case 2: Similar but different root domains Domain engineering = createDomain("Engineering", "Engineering");