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

feat(auth): Add roles to policy engine validation logic #9178

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

pedro93
Copy link
Collaborator

@pedro93 pedro93 commented Nov 5, 2023

Adds roles as a new set of actor that is allowed to perform operations on DataHub.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the devops PR or Issue related to DataHub backend & deployment label Nov 5, 2023
@david-leifker
Copy link
Collaborator

Needs a test

@@ -126,11 +126,16 @@ private AuthorizedActors mergeAuthorizedActors(@Nullable AuthorizedActors origin
mergedGroups = new ArrayList<>(groups);
}

Set<Urn> roles = new HashSet<>(original.getRoles());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method seems like dead code? I don't see any references to it outside of this class and tracing back to Dexter's PR I don't see any actual usages of it. To get the actors authorized for something we go through different code unless I'm missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not super familiar with the code.
My understanding is that this merging logic is for when we have multiple chained authorizers.
Since the interface extended a plugin interface, this was something we supported for the community.

@@ -96,6 +100,9 @@ public PolicyActors getMatchingActors(
if (actorFilter.getGroups() != null) {
groups.addAll(actorFilter.getGroups());
}
if (actorFilter.getRoles() != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to only be used in the dead code above, this check is covered by isRoleMatch

@jjoyce0510
Copy link
Collaborator

Looks like just needs one conflict resolved. Once this is green LGTM. Nice work!

@pedro93 pedro93 force-pushed the ps-add-role-validation-to-auth-engine branch from 7604825 to 6651afe Compare November 8, 2023 17:28
@pedro93 pedro93 merged commit f38c808 into master Nov 8, 2023
33 checks passed
@pedro93 pedro93 deleted the ps-add-role-validation-to-auth-engine branch November 8, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops PR or Issue related to DataHub backend & deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants