From c78cda67570d6822cf155794c1fd8f8c03d34857 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 17 Oct 2024 19:15:06 -0700 Subject: [PATCH] Minor: Fix rbac for multiple rules (#18057) * Minor: Search RBAC, fix condition evaluation for single policy multiple rules * Minor: Search RBAC, fix condition evaluation for single policy multiple rules * add test for complex policies * Add viewAll policy to organization --------- Co-authored-by: Mohit Yadav <105265192+mohityadav766@users.noreply.github.com> --- .../security/RBACConditionEvaluator.java | 133 +++++--- .../json/data/policy/OrganizationPolicy.json | 7 + ...asticSearchRBACConditionEvaluatorTest.java | 292 +++++++++++++++--- 3 files changed, 347 insertions(+), 85 deletions(-) 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 6bcb33bc874..d61b9dfc206 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 @@ -26,7 +26,7 @@ public class RBACConditionEvaluator { private final ExpressionParser spelParser = new SpelExpressionParser(); private final StandardEvaluationContext spelContext; private static final Set SEARCH_RELEVANT_OPS = - Set.of(MetadataOperation.VIEW_BASIC, MetadataOperation.VIEW_ALL); + Set.of(MetadataOperation.VIEW_BASIC, MetadataOperation.VIEW_ALL, MetadataOperation.ALL); public RBACConditionEvaluator(QueryBuilderFactory queryBuilderFactory) { this.queryBuilderFactory = queryBuilderFactory; @@ -36,38 +36,99 @@ public class RBACConditionEvaluator { public OMQueryBuilder evaluateConditions(SubjectContext subjectContext) { User user = subjectContext.user(); spelContext.setVariable("user", user); - ConditionCollector collector = new ConditionCollector(queryBuilderFactory); + + ConditionCollector finalCollector = new ConditionCollector(queryBuilderFactory); for (Iterator it = subjectContext.getPolicies(List.of(user.getEntityReference())); it.hasNext(); ) { SubjectContext.PolicyContext context = it.next(); + + ConditionCollector policyCollector = new ConditionCollector(queryBuilderFactory); + List allowRuleQueries = new ArrayList<>(); + List denyRuleQueries = new ArrayList<>(); + for (CompiledRule rule : context.getRules()) { - if (rule.getCondition() != null - && rule.getOperations().stream().anyMatch(SEARCH_RELEVANT_OPS::contains)) { - ConditionCollector ruleCollector = new ConditionCollector(queryBuilderFactory); - if (!rule.getResources().isEmpty() && !rule.getResources().contains("All")) { - OMQueryBuilder indexFilter = getIndexFilter(rule.getResources()); - ruleCollector.addMust(indexFilter); - } - SpelExpression parsedExpression = - (SpelExpression) spelParser.parseExpression(rule.getCondition()); - preprocessExpression(parsedExpression.getAST(), ruleCollector); - OMQueryBuilder ruleQuery = ruleCollector.buildFinalQuery(); - if (rule.getEffect().toString().equalsIgnoreCase("DENY")) { - collector.addMustNot(ruleQuery); - } else { - collector.addMust(ruleQuery); - } + boolean isDenyRule = rule.getEffect().toString().equalsIgnoreCase("DENY"); + List mappedOperations = + rule.getOperations().stream() + .map( + op -> { + if (op.toString().equalsIgnoreCase("Create") + || op.toString().equalsIgnoreCase("Delete") + || op.toString().toLowerCase().startsWith("edit")) { + return MetadataOperation.VIEW_BASIC; + } + return op; + }) + .collect(Collectors.toList()); + + if (isDenyRule && SEARCH_RELEVANT_OPS.stream().noneMatch(mappedOperations::contains)) { + continue; } + + OMQueryBuilder ruleQuery = buildRuleQuery(rule, user); + if (ruleQuery == null || ruleQuery.isEmpty()) { + continue; + } + if (isDenyRule) { + denyRuleQueries.add(ruleQuery); + } else { + allowRuleQueries.add(ruleQuery); + } + } + + if (!denyRuleQueries.isEmpty()) { + if (denyRuleQueries.size() == 1) { + policyCollector.addMustNot(denyRuleQueries.get(0)); + } else { + OMQueryBuilder denyQuery = queryBuilderFactory.boolQuery().should(denyRuleQueries); + policyCollector.addMustNot(denyQuery); + } + } + + if (!allowRuleQueries.isEmpty()) { + if (allowRuleQueries.size() == 1) { + policyCollector.addMust(allowRuleQueries.get(0)); + } else { + OMQueryBuilder allowQuery = queryBuilderFactory.boolQuery().should(allowRuleQueries); + policyCollector.addMust(allowQuery); + } + } else { + policyCollector.addMust(queryBuilderFactory.matchAllQuery()); + } + + OMQueryBuilder policyFinalQuery = policyCollector.buildFinalQuery(); + if (policyFinalQuery != null && !policyFinalQuery.isEmpty()) { + finalCollector.addMust(policyFinalQuery); } } - return collector.buildFinalQuery(); + return finalCollector.buildFinalQuery(); + } + + private OMQueryBuilder buildRuleQuery(CompiledRule rule, User user) { + ConditionCollector ruleCollector = new ConditionCollector(queryBuilderFactory); + spelContext.setVariable("user", user); + + // Apply index filtering if resources are specified and not "All" + if (!rule.getResources().isEmpty() && !rule.getResources().contains("All")) { + OMQueryBuilder indexFilter = getIndexFilter(rule.getResources()); + ruleCollector.addMust(indexFilter); + } + + if (rule.getCondition() != null && !rule.getCondition().trim().isEmpty()) { + SpelExpression parsedExpression = + (SpelExpression) spelParser.parseExpression(rule.getCondition()); + preprocessExpression(parsedExpression.getAST(), ruleCollector); + } else { + ruleCollector.addMust(queryBuilderFactory.matchAllQuery()); + } + + return ruleCollector.buildFinalQuery(); } private void preprocessExpression(SpelNode node, ConditionCollector collector) { - // Delay this check until after processing necessary expressions if (collector.isMatchNothing()) { return; } @@ -82,35 +143,33 @@ public class RBACConditionEvaluator { } else if (node instanceof OpOr) { List orQueries = new ArrayList<>(); boolean allMatchNothing = true; - boolean hasTrueCondition = false; // Track if any condition evaluated to true + boolean hasTrueCondition = false; for (int i = 0; i < node.getChildCount(); i++) { ConditionCollector childCollector = new ConditionCollector(queryBuilderFactory); preprocessExpression(node.getChild(i), childCollector); if (childCollector.isMatchNothing()) { - continue; // If this child evaluates to match nothing, skip it + continue; } if (childCollector.isMatchAllQuery()) { - hasTrueCondition = true; // If any condition evaluates to true, mark it - break; // Short-circuit: if any condition in OR evaluates to true, the whole OR is true + hasTrueCondition = true; + break; } OMQueryBuilder childQuery = childCollector.buildFinalQuery(); if (childQuery != null) { - allMatchNothing = - false; // If at least one child query is valid, it’s not all match nothing + allMatchNothing = false; orQueries.add(childQuery); } } if (hasTrueCondition) { - collector.addMust(queryBuilderFactory.matchAllQuery()); // OR is true, add match_all + collector.addMust(queryBuilderFactory.matchAllQuery()); } else if (allMatchNothing) { - collector.setMatchNothing(true); // OR is false + collector.setMatchNothing(true); } else { - // Add the valid OR queries to the collector for (OMQueryBuilder orQuery : orQueries) { collector.addShould(orQuery); } @@ -126,7 +185,7 @@ public class RBACConditionEvaluator { } else { OMQueryBuilder subQuery = subCollector.buildFinalQuery(); if (subQuery != null && !subQuery.isEmpty()) { - collector.addMustNot(subQuery); // Add must_not without extra nesting + collector.addMustNot(subQuery); } } } else if (node instanceof MethodReference) { @@ -165,7 +224,7 @@ public class RBACConditionEvaluator { List args = new ArrayList<>(); for (int i = 0; i < methodRef.getChildCount(); i++) { SpelNode childNode = methodRef.getChild(i); - String value = childNode.toStringAST().replace("'", ""); // Remove single quotes + String value = childNode.toStringAST().replace("'", ""); args.add(value); } return args; @@ -181,27 +240,27 @@ public class RBACConditionEvaluator { public void matchAllTags(List tags, ConditionCollector collector) { for (String tag : tags) { OMQueryBuilder tagQuery = queryBuilderFactory.termQuery("tags.tagFQN", tag); - collector.addMust(tagQuery); // Add directly to the collector's must clause + collector.addMust(tagQuery); } } public void isOwner(User user, ConditionCollector collector) { List ownerQueries = new ArrayList<>(); - ownerQueries.add(queryBuilderFactory.termQuery("owner.id", user.getId().toString())); + ownerQueries.add(queryBuilderFactory.termQuery("owners.id", user.getId().toString())); if (user.getTeams() != null) { for (EntityReference team : user.getTeams()) { - ownerQueries.add(queryBuilderFactory.termQuery("owner.id", team.getId().toString())); + ownerQueries.add(queryBuilderFactory.termQuery("owners.id", team.getId().toString())); } } for (OMQueryBuilder ownerQuery : ownerQueries) { - collector.addShould(ownerQuery); // Add directly to the collector's should clause + collector.addShould(ownerQuery); } } public void noOwner(ConditionCollector collector) { - OMQueryBuilder existsQuery = queryBuilderFactory.existsQuery("owner.id"); + OMQueryBuilder existsQuery = queryBuilderFactory.existsQuery("owners.id"); collector.addMustNot(existsQuery); } @@ -253,7 +312,7 @@ public class RBACConditionEvaluator { List indices = resources.stream() .map(resource -> Entity.getSearchRepository().getIndexOrAliasName(resource)) - .collect(Collectors.toList()); + .toList(); return queryBuilderFactory.termsQuery("_index", indices); } diff --git a/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json b/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json index 3ad245e38b5..5036e370e01 100644 --- a/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json +++ b/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json @@ -22,6 +22,13 @@ "operations": ["EditOwners"], "effect": "allow", "condition": "noOwner()" + }, + { + "name": "OrganizationPolicy-ViewAll-Rule", + "description" : "Allow all users to discover data assets.", + "resources" : ["all"], + "operations": ["ViewAll"], + "effect": "allow" } ] } \ No newline at end of file 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 30da785f07d..a7a015b6e16 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 @@ -2,7 +2,6 @@ package org.openmetadata.service.search.security; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; @@ -16,7 +15,11 @@ import com.jayway.jsonpath.JsonPath; import es.org.elasticsearch.index.query.QueryBuilder; import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.UUID; import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.AfterEach; @@ -55,6 +58,14 @@ class ElasticSearchRBACConditionEvaluatorTest { return resource.toLowerCase(); }); Entity.setSearchRepository(mockSearchRepository); + mockSubjectContext = mock(SubjectContext.class); + mockUser = mock(User.class); + EntityReference mockUserReference = mock(EntityReference.class); + when(mockUser.getEntityReference()).thenReturn(mockUserReference); + when(mockUserReference.getId()).thenReturn(UUID.randomUUID()); + when(mockUser.getId()).thenReturn(UUID.randomUUID()); + when(mockUser.getName()).thenReturn("testUser"); + when(mockSubjectContext.user()).thenReturn(mockUser); } @AfterEach @@ -67,26 +78,15 @@ class ElasticSearchRBACConditionEvaluatorTest { String effect, List> resourcesList, List> operationsList) { - // Mock the user - mockUser = mock(User.class); - EntityReference mockUserReference = mock(EntityReference.class); - when(mockUser.getEntityReference()).thenReturn(mockUserReference); - when(mockUserReference.getId()).thenReturn(UUID.randomUUID()); - when(mockUser.getId()).thenReturn(UUID.randomUUID()); - when(mockUser.getName()).thenReturn("testUser"); - // Mock the policy context SubjectContext.PolicyContext mockPolicyContext = mock(SubjectContext.PolicyContext.class); when(mockPolicyContext.getPolicyName()).thenReturn("TestPolicy"); - // Create a list of mock rules List mockRules = new ArrayList<>(); for (int i = 0; i < expressions.size(); i++) { CompiledRule mockRule = mock(CompiledRule.class); when(mockRule.getOperations()).thenReturn(List.of(MetadataOperation.VIEW_BASIC)); when(mockRule.getCondition()).thenReturn(expressions.get(i)); - - // Use resources from the corresponding index in resourcesList List resources = (resourcesList.size() > i) ? resourcesList.get(i) : List.of("All"); when(mockRule.getResources()).thenReturn(resources); @@ -100,11 +100,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } when(mockPolicyContext.getRules()).thenReturn(mockRules); - - // Mock the subject context with this policy - mockSubjectContext = mock(SubjectContext.class); when(mockSubjectContext.getPolicies(any())).thenReturn(List.of(mockPolicyContext).iterator()); - when(mockSubjectContext.user()).thenReturn(mockUser); } private void setupMockPolicies(String expression, String effect, List resources) { @@ -127,7 +123,7 @@ class ElasticSearchRBACConditionEvaluatorTest { QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); String generatedQuery = elasticQuery.toString(); - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'."); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owners.id'."); assertTrue( generatedQuery.contains(mockUser.getId().toString()), "The query should contain the user's ID."); @@ -144,8 +140,8 @@ class ElasticSearchRBACConditionEvaluatorTest { assertTrue( generatedQuery.contains("must_not"), "The query should contain 'must_not' for negation."); assertTrue( - generatedQuery.contains("owner.id"), - "The query should contain 'owner.id' in the negation."); + generatedQuery.contains("owners.id"), + "The query should contain 'owners.id' in the negation."); assertTrue( generatedQuery.contains(mockUser.getId().toString()), "The negation should contain the user's ID."); @@ -182,7 +178,7 @@ class ElasticSearchRBACConditionEvaluatorTest { generatedQuery.contains("PII.Sensitive"), "The query should contain 'PII.Sensitive' tag."); assertTrue(generatedQuery.contains("Test.Test1"), "The query should contain 'Test.Test1' tag."); assertTrue(generatedQuery.contains("Test.Test2"), "The query should contain 'Test.Test2' tag."); - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'."); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owners.id'."); assertTrue( generatedQuery.contains("must_not"), "The query should contain a negation (must_not)."); @@ -221,13 +217,13 @@ class ElasticSearchRBACConditionEvaluatorTest { // Check for the presence of owner.id in the "must_not" clause for the negation assertFieldExists( jsonContext, - "$.bool.should[2].bool.must_not[0].bool.should[?(@.term['owner.id'])]", - "owner.id in must_not"); + "$.bool.should[2].bool.must_not[0].bool.should[?(@.term['owners.id'])]", + "owners.id in must_not"); // Check for the presence of must_not for the case where there is no owner assertFieldExists( jsonContext, - "$.bool.should[3].bool.must_not[?(@.exists.field=='owner.id')]", + "$.bool.should[3].bool.must_not[?(@.exists.field=='owners.id')]", "no owner must_not clause"); // Count the number of bool clauses @@ -388,7 +384,7 @@ class ElasticSearchRBACConditionEvaluatorTest { // Check for owner ID and Public tag in the query assertFieldExists( jsonContext, - "$.bool.should[?(@.term['owner.id'].value=='" + mockUser.getId().toString() + "')]", + "$.bool.should[?(@.term['owners.id'].value=='" + mockUser.getId().toString() + "')]", "owner.id"); assertFieldExists( jsonContext, "$.bool.should[?(@.term['tags.tagFQN'].value=='Public')]", "Public tag"); @@ -415,7 +411,7 @@ class ElasticSearchRBACConditionEvaluatorTest { assertTrue(generatedQuery.contains("tags.tagFQN"), "The query should contain 'tags.tagFQN'."); assertTrue(generatedQuery.contains("Sensitive"), "The query should contain 'Sensitive' tag."); - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'."); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owners.id'."); assertFalse(generatedQuery.contains("match_none"), "The query should not be match_none."); } @@ -476,7 +472,7 @@ class ElasticSearchRBACConditionEvaluatorTest { // Ensure that the query does not check for noOwner since inAnyTeam('HR') is false assertFieldDoesNotExist( - jsonContext, "$.bool.must_not[?(@.exists.field=='owner.id')]", "noOwner clause"); + jsonContext, "$.bool.must_not[?(@.exists.field=='owners.id')]", "noOwner clause"); // Ensure the query does not contain a match_none condition assertFieldDoesNotExist(jsonContext, "$.bool[?(@.match_none)]", "match_none"); @@ -505,8 +501,8 @@ class ElasticSearchRBACConditionEvaluatorTest { "Internal"); assertFieldExists( jsonContext, - "$.bool.must_not[0].bool.should[1].bool.should[?(@.term['owner.id'])]", - "owner.id"); + "$.bool.must_not[0].bool.should[1].bool.should[?(@.term['owners.id'])]", + "owners.id"); } @Test @@ -573,7 +569,7 @@ class ElasticSearchRBACConditionEvaluatorTest { assertTrue( generatedQuery.contains("\"domain.id\""), "The query should contain 'domain.id' term."); assertTrue( - generatedQuery.contains("\"owner.id\""), "The query should contain 'owner.id' term."); + generatedQuery.contains("\"owners.id\""), "The query should contain 'owners.id' term."); assertFalse( generatedQuery.contains("\"must_not\" : [ { \"exists\""), "The query should not contain 'must_not' clause for 'noOwner()'."); @@ -634,7 +630,7 @@ class ElasticSearchRBACConditionEvaluatorTest { // Check for the presence of owner.id in the second should block assertFieldExists( - jsonContext, "$.bool.should[1].bool.should[?(@.term['owner.id'])]", "owner.id"); + jsonContext, "$.bool.should[1].bool.should[?(@.term['owners.id'])]", "owners.id"); } @Test @@ -740,7 +736,7 @@ class ElasticSearchRBACConditionEvaluatorTest { "Confidential"); // Ownership (isOwner condition) should be in `should` - assertFieldExists(jsonContext, "$.bool.should[?(@.term['owner.id'])]", "owner.id"); + assertFieldExists(jsonContext, "$.bool.should[?(@.term['owners.id'])]", "owners.id"); } @Test @@ -761,8 +757,8 @@ class ElasticSearchRBACConditionEvaluatorTest { "must_not for hasDomain"); assertFieldExists( jsonContext, - "$.bool.should[?(@.term['owner.id'].value=='" + mockUser.getId().toString() + "')]", - "owner.id"); + "$.bool.should[?(@.term['owners.id'].value=='" + mockUser.getId().toString() + "')]", + "owners.id"); assertFieldDoesNotExist(jsonContext, "$.bool[?(@.match_none)]", "match_none should not exist"); } @@ -861,33 +857,33 @@ class ElasticSearchRBACConditionEvaluatorTest { // Check for the "Table" index condition assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "$.bool.should[0].bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", "Index filtering for 'Table' resource"); assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.match_all)]", + "$.bool.should[0].bool.must[?(@.match_all)]", "match_all for hasAnyRole 'Admin'"); assertFieldExists( jsonContext, - "$.bool.must[0].bool.should[?(@.term['tags.tagFQN'].value=='Sensitive')]", + "$.bool.should[0].bool.should[?(@.term['tags.tagFQN'].value=='Sensitive')]", "Sensitive tag"); // Check for the "Dashboard" index condition assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.terms._index && @.terms._index[?(@ == 'database')])]", + "$.bool.should[1].bool.must[?(@.terms._index && @.terms._index[?(@ == 'database')])]", "Index filtering for 'Database' resource"); assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.match_all)]", + "$.bool.should[1].bool.must[?(@.match_all)]", "match_all for inAnyTeam 'Engineering'"); assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.term['tags.tagFQN'].value=='Confidential')]", + "$.bool.should[1].bool.must[?(@.term['tags.tagFQN'].value=='Confidential')]", "Confidential tag"); assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.term['tags.tagFQN'].value=='Internal')]", + "$.bool.should[1].bool.must[?(@.term['tags.tagFQN'].value=='Internal')]", "Internal tag"); } @@ -929,7 +925,7 @@ class ElasticSearchRBACConditionEvaluatorTest { // Check for domain filtering in the "Table" index clause assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.term['domain.id'].value=='" + "$.bool.should[0].bool.must[?(@.term['domain.id'].value=='" + domain.getId().toString() + "')]", "user's domain ID"); @@ -937,7 +933,7 @@ class ElasticSearchRBACConditionEvaluatorTest { // Check for the matchAnyTag clause for Public tag in the "Table" index clause assertFieldExists( jsonContext, - "$.bool.must[0].bool.should[?(@.term['tags.tagFQN'].value=='Public')]", + "$.bool.should[0].bool.should[?(@.term['tags.tagFQN'].value=='Public')]", "Public tag"); } @@ -979,7 +975,7 @@ class ElasticSearchRBACConditionEvaluatorTest { // Adjust the assertion for the hasDomain clause assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.term['domain.id'].value=='" + "$.bool.should[0].bool.must[?(@.term['domain.id'].value=='" + domain.getId().toString() + "')]", "user's domain ID"); @@ -987,13 +983,13 @@ class ElasticSearchRBACConditionEvaluatorTest { // Check for the inAnyTeam('Engineering') clause assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.match_all)]", + "$.bool.should[1].bool.must[?(@.match_all)]", "match_all for inAnyTeam 'Engineering'"); // Check for the matchAnyTag clause for Critical tag assertFieldExists( jsonContext, - "$.bool.must[1].bool.should[?(@.term['tags.tagFQN'].value=='Critical')]", + "$.bool.should[1].bool.should[?(@.term['tags.tagFQN'].value=='Critical')]", "Critical tag"); } @@ -1007,11 +1003,16 @@ class ElasticSearchRBACConditionEvaluatorTest { List.of(List.of(MetadataOperation.EDIT_DESCRIPTION))); // Mock user ownership - when(mockUser.getId()).thenReturn(UUID.randomUUID()); + UUID userId = UUID.randomUUID(); + when(mockUser.getId()).thenReturn(userId); // Evaluate the condition OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); - assertNull(finalQuery, "The query should be null since the rule is ignored"); + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owner.id'"); + assertTrue( + generatedQuery.contains(userId.toString()), "The query should contain the user's ID"); } @Test @@ -1033,8 +1034,203 @@ class ElasticSearchRBACConditionEvaluatorTest { String generatedQuery = elasticQuery.toString(); // The rule should affect the search query - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'"); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owner.id'"); assertTrue( generatedQuery.contains(userId.toString()), "The query should contain the user's ID"); } + + @Test + void testDenyAllOperationsOnTableResource() { + setupMockPolicies( + List.of(""), "DENY", List.of(List.of("Table")), List.of(List.of(MetadataOperation.ALL))); + + UUID userId = UUID.randomUUID(); + when(mockUser.getId()).thenReturn(userId); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + + assertFieldExists( + jsonContext, + "$.bool.must_not[0].bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "must_not clause excluding 'table'"); + + // Assertions to ensure 'table' is not included in 'must' or 'should' clauses + assertFieldDoesNotExist( + jsonContext, + "$.bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "'table' should not be included in must clauses"); + assertFieldDoesNotExist( + jsonContext, + "$.bool.should[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "'table' should not be included in should clauses"); + } + + @Test + void testUserSeesOwnedAndUnownedResourcesIncludingTeamOwnership() { + // Set up a policy with three rules + setupMockPolicies( + List.of( + "noOwner()", // Rule 1 condition (ViewBasic) + "isOwner()" // Rule 2 condition (All operations) + ), + "ALLOW", + List.of( + List.of("All"), // Rule 1 resources + List.of("All") // Rule 2 resources + ), + List.of(List.of(MetadataOperation.EDIT_OWNERS), List.of(MetadataOperation.ALL))); + + UUID userId = UUID.randomUUID(); + when(mockUser.getId()).thenReturn(userId); + + UUID teamId1 = UUID.randomUUID(); + UUID teamId2 = UUID.randomUUID(); + EntityReference team1 = new EntityReference(); + team1.setId(teamId1); + team1.setName("TeamA"); + + EntityReference team2 = new EntityReference(); + team2.setId(teamId2); + team2.setName("TeamB"); + + when(mockUser.getTeams()).thenReturn(List.of(team1, team2)); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + + assertFieldExists( + jsonContext, + "$.bool.should[1].bool.should[?(@.term['owners.id'].value=='" + userId.toString() + "')]", + "The query should allow resources where the user is the owner"); + + assertFieldExists( + jsonContext, + "$.bool.should[1].bool.should[?(@.term['owners.id'].value=='" + teamId1.toString() + "')]", + "The query should allow resources where TeamA is the owner"); + assertFieldExists( + jsonContext, + "$.bool.should[1].bool.should[?(@.term['owners.id'].value=='" + teamId2.toString() + "')]", + "The query should allow resources where TeamB is the owner"); + + assertFalse( + generatedQuery.contains("noOwner()"), + "The query should not contain 'noOwner()' as a string"); + } + + @Test + void testPoliciesProcessing() { + List compiledRules = new ArrayList<>(); + List> policies = getPolicies(); + + for (Map policy : policies) { + CompiledRule rule = createCompiledRule(policy); + compiledRules.add(rule); + } + + SubjectContext.PolicyContext mockPolicyContext = mock(SubjectContext.PolicyContext.class); + when(mockPolicyContext.getRules()).thenReturn(compiledRules); + Iterator policyContextIterator = + Collections.singletonList(mockPolicyContext).iterator(); + when(mockSubjectContext.getPolicies(anyList())).thenReturn(policyContextIterator); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + + assertFalse( + generatedQuery.contains("Create"), "The query should not contain 'Create' operation"); + assertFalse( + generatedQuery.contains("Delete"), "The query should not contain 'Delete' operation"); + assertFalse(generatedQuery.contains("Edit"), "The query should not contain 'Edit' operation"); + + boolean containsOwnerCondition = generatedQuery.contains("owners.id"); + boolean containsTagCondition = generatedQuery.contains("tags.tagFQN"); + assertTrue( + containsOwnerCondition || containsTagCondition, + "The query should include 'isOwner' or 'matchAnyTag' conditions where applicable"); + } + + // Helper methods + private CompiledRule createCompiledRule(Map policyDef) { + CompiledRule rule = mock(CompiledRule.class); + when(rule.getName()).thenReturn((String) policyDef.get("name")); + when(rule.getDescription()).thenReturn((String) policyDef.get("description")); + when(rule.getEffect()) + .thenReturn(CompiledRule.Effect.valueOf(((String) policyDef.get("effect")).toUpperCase())); + when(rule.getOperations()) + .thenReturn(mapOperations((List) policyDef.get("operations"))); + when(rule.getResources()).thenReturn((List) policyDef.get("resources")); + when(rule.getCondition()).thenReturn((String) policyDef.getOrDefault("condition", "")); + return rule; + } + + private List mapOperations(List operations) { + List mappedOperations = new ArrayList<>(); + for (String op : operations) { + if (op.equalsIgnoreCase("Create") + || op.equalsIgnoreCase("Delete") + || op.toLowerCase().startsWith("edit")) { + mappedOperations.add(MetadataOperation.VIEW_BASIC); + } else if (op.equalsIgnoreCase("All")) { + mappedOperations.add(MetadataOperation.ALL); + } else { + mappedOperations.add(MetadataOperation.fromValue(op)); + } + } + return mappedOperations; + } + + private List> getPolicies() { + List> policyDefs = new ArrayList<>(); + + Map policy1 = new HashMap<>(); + policy1.put("name", "APP_ALLOW"); + policy1.put("description", "APP MARKET PLACE DEFINITION ALLOW AS OWNER"); + policy1.put("effect", "allow"); + policy1.put("operations", List.of("ViewAll")); + policy1.put("resources", List.of("appMarketPlaceDefinition")); + policy1.put("condition", "matchAnyTag('tags.a') && isOwner()"); + policyDefs.add(policy1); + + Map policy2 = new HashMap<>(); + policy2.put("name", "APP_ALLOW_2"); + policy2.put("description", "APP MARKET PLACE DEFINITION ALLOW"); + policy2.put("effect", "allow"); + policy2.put("operations", List.of("ViewBasic")); + policy2.put("resources", List.of("appMarketPlaceDefinition")); + policyDefs.add(policy2); + + Map policy3 = new HashMap<>(); + policy3.put("name", "APP_DENY"); + policy3.put("description", "APP MARKET PLACE DEFINITION DENY"); + policy3.put("effect", "deny"); + policy3.put("operations", List.of("Create", "Delete", "EditAll")); + policy3.put("resources", List.of("appMarketPlaceDefinition")); + policyDefs.add(policy3); + + Map policy4 = new HashMap<>(); + policy4.put("name", "APP_ALLOW_2"); + policy4.put("description", "APP ALLOW"); + policy4.put("effect", "allow"); + policy4.put("operations", List.of("ViewBasic")); + policy4.put("resources", List.of("app")); + policyDefs.add(policy4); + + Map policy5 = new HashMap<>(); + policy5.put("name", "APP_ALLOW_3"); + policy5.put("description", "APP DENY"); + policy5.put("effect", "deny"); + policy5.put("operations", List.of("Create", "Delete", "EditAll")); + policy5.put("resources", List.of("app")); + policyDefs.add(policy5); + + return policyDefs; + } }