Skip to content

Commit

Permalink
Minor: Fix rbac for multiple rules (#18057)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
  • Loading branch information
@harshach @mohityadav766
harshach and mohityadav766 authored Oct 18, 2024
1 parent e708a32 commit c78cda6
Show file tree
Hide file tree
Showing 3 changed files with 347 additions and 85 deletions.
133 changes: 96 additions & 37 deletions ...ervice/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public class RBACConditionEvaluator {
private final ExpressionParser spelParser = new SpelExpressionParser();
private final StandardEvaluationContext spelContext;
private static final Set<MetadataOperation> 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;
Expand All @@ -36,38 +36,99 @@ public RBACConditionEvaluator(QueryBuilderFactory queryBuilderFactory) {
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<SubjectContext.PolicyContext> it =
subjectContext.getPolicies(List.of(user.getEntityReference()));
it.hasNext(); ) {
SubjectContext.PolicyContext context = it.next();

ConditionCollector policyCollector = new ConditionCollector(queryBuilderFactory);
List<OMQueryBuilder> allowRuleQueries = new ArrayList<>();
List<OMQueryBuilder> 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<MetadataOperation> 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;
}
Expand All @@ -82,35 +143,33 @@ private void preprocessExpression(SpelNode node, ConditionCollector collector) {
} else if (node instanceof OpOr) {
List<OMQueryBuilder> 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);
}
Expand All @@ -126,7 +185,7 @@ private void preprocessExpression(SpelNode node, ConditionCollector collector) {
} 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) {
Expand Down Expand Up @@ -165,7 +224,7 @@ private List<String> extractMethodArguments(MethodReference methodRef) {
List<String> 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;
Expand All @@ -181,27 +240,27 @@ public void matchAnyTag(List<String> tags, ConditionCollector collector) {
public void matchAllTags(List<String> 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<OMQueryBuilder> 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);
}

Expand Down Expand Up @@ -253,7 +312,7 @@ private OMQueryBuilder getIndexFilter(List<String> resources) {
List<String> indices =
resources.stream()
.map(resource -> Entity.getSearchRepository().getIndexOrAliasName(resource))
.collect(Collectors.toList());
.toList();

return queryBuilderFactory.termsQuery("_index", indices);
}
Expand Down
7 changes: 7 additions & 0 deletions openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
]
}
Loading

0 comments on commit c78cda6

Please sign in to comment.