Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate rules into elasticsearch result #15734

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HesamoddinMonfared
Copy link

I worked on applying rules to search results.
The current implementation is for elasticsearch only.

The implementation has been tested with different rules, policies and roles and the results show the correctness.

@HesamoddinMonfared HesamoddinMonfared requested a review from a team as a code owner March 27, 2024 16:00
Copy link

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

return searchSourceBuilder;
}

private BoolQueryBuilder createQueryBuilderBasedOnRule(SpelNode ast) throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be better if we codify for each expression/rule to return the search condition for it.
Example if the condition says isOwner() only allow, then we should return the condition.getSearchCondition(SecurityContext) -> return 'owner.name=%s'.format(SecurityContext.getUsername() or owner.name=(getTeams(SecurityContext.getUsername())

BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery();
var keyword = ast2keyword(ast);

if (ast.getChildCount() == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complexity involved enforcing these on the search side is

  1. Hierarchy of Rule processing
  2. We process Deny rules first before the allow rules
  3. Enforcing the Resource side attribute conditions example Allow Edit only if the table contains certain tags

@harshach
Copy link
Collaborator

@HesamoddinMonfared Thanks for taking this up. Lets visit the goals of this integration

  1. Users who doesn't have view access to certain data assets shouldn't be able to see it as part of the search results
  2. We are not going to worry about the Edit Access conditions as they will be taken care by the APIs itself
  3. Complexity of View conditions Processing
    4. A team can set a condition that only their team members should be able to view all of it assets
    5. A team can set a condition that only their team members should be able to view all of it assets except if it contains certain tags
    6. Admin can set a condition at Org level, any data in a domain should only viewable by users with-in that domain except for assets that are part of Data Products

who will build the conditions, the delegation should be part of the rule itself . i.e each rule should have method to equivalent to returning true or false . Example isOwner() takes the security context and returns true or false if the user who logged in is a owner of the asset or part of the team that is owner of the asset.
Similarly it should return a condition that can be applied to search query instead of this logic outside. This way any new conditions added can be implemented methodically rather adding this logic in another place which can easily introduce gaps

Condition evaluation

  1. Go through hierarchy of the conditions similar to what we are doing in authorizer.
  2. Process the Deny rule first
  3. All of these rules should have an operator with AND, if there are multiple rules at the same state we should look at the condition specified for that rule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants