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.

Sign up for GitHub

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

Jump to bottom

feat(auth): add group membership field resolver provider #8846

Merged
merged 17 commits into from
Oct 12, 2023
Merged

feat(auth): add group membership field resolver provider #8846

merged 17 commits into from
Oct 12, 2023

Conversation

amanda-her
Copy link
Contributor

@amanda-her amanda-her commented Sep 15, 2023 •
edited
Loading

In this pull request the following topics have been addressed:

  • The group membership field resolver has been added to return the groups and native groups of a user. This is required to develop a custom authorizer using the plugin approach.
  • Improvement in PolicyEngine class to delegate in the field resolver instead of manually invoking the EntityClient class.
  • Fix method isGroupMatch in PolicyEngine class to check that a user belongs to at least one group if policy applies to all groups.

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 Sep 15, 2023
@amanda-her amanda-her changed the title Add group membership field resolver provider feat(auth): add group membership field resolver provider Sep 15, 2023
@amanda-her amanda-her marked this pull request as ready for review September 19, 2023 06:47
jjoyce0510
jjoyce0510 reviewed Sep 20, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/DataHubAuthorizer.java Outdated

// 1. Fetch all policies
final List<DataHubPolicyInfo> policiesToEvaluate = _policyCache.getOrDefault(ALL, new ArrayList<>());

Urn actorUrn = UrnUtils.getUrn(actor);
final ResolvedResourceSpec actorResolvedResourceSpec = _resourceSpecResolver.resolve(new ResourceSpec(actorUrn.getEntityType(), actor));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the idea here.. nice!

Do we need to resolve the resource spec at this point, or can we do it later on once we absolutely know that group membership is required. I want to make sure that we avoid making any additional lookups until we absolutely have to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there are two options:

  • As it is implemented now, resolving the actor and pass it to the PolicyEngine. It is true that at this point we do not know if group membership is required. However, according to this line, no field is resolved until it is explicitly called/obtained.
  • The other option implies passing the _resourceSpecResolver to the PolicyEngine class. The PolicyEngine class is initialized in the DatahubAuthorizer constructor while the _resourceSpecResolver in the init function. So if we want to follow this approach, the PolicyEngine class should be initialized in the init method after the resolver. Not sure if this has any implications.

jjoyce0510
jjoyce0510 reviewed Sep 20, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/DataHubAuthorizer.java Outdated
return false;
}

final ResolvedResourceSpec actorResolvedResourceSpec = _resourceSpecResolver.resolve(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as above here - is there any way we can pass down the ResourceSpec and simple resolve once we need to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same answer applies here.

jjoyce0510
jjoyce0510 reviewed Sep 20, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/DataHubAuthorizer.java
request.getPrivilege(),
resourceSpec
);
return result.isGranted();
}

private Optional<Urn> getUrnFromRequestActor(String actor) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we confirm that this will always be an urn from the caller? I don't think we have any cases where this will NOT be an urn, but want to make sure I'm not missing anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was created as a result of moving logic that was already there. I think is highly improbable that this will NOT be an urn. If in agreement we can use UrnUtils.getUrn instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine - let's go with this.

I do think in future we will want to make the auth layer a bit more opinionated and work in the URN types instead of raw strings. That was probably a bad call initially

jjoyce0510
jjoyce0510 reviewed Sep 20, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java Outdated
final Set<Urn> groups = resolveGroups(actor, context);
return actorFilter.isAllGroups() || (actorFilter.hasGroups() && Objects.requireNonNull(actorFilter.getGroups())
.stream()
final Set<String> groups = actorResolvedResourceSpec.getGroupMembership();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-auth/auth-api/src/main/java/com/datahub/authorization/EntityFieldType.java Outdated
/**
* Groups of which the entity (only applies to corpUser) is a member
*/
GROUP_MEMBERSHIP
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java
}

public List<String> getGrantedPrivileges(
final List<DataHubPolicyInfo> policies,
final Urn actor,
final Optional<ResolvedResourceSpec> resource) {
final ResolvedEntitySpec resolvedActorSpec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

actor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same answer as for the previous comment.

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java
}

/**
* Returns true if the privilege portion of a DataHub policy matches a the privilege being evaluated, false otherwise.
*/
private boolean isPrivilegeMatch(
final String requestPrivilege,
final List<String> policyPrivileges,
final PolicyEvaluationContext context) {
final List<String> policyPrivileges) {
return policyPrivileges.contains(requestPrivilege);
Copy link
Collaborator

Choose a reason for hiding this comment

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

thank u!

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java
@@ -199,7 +175,7 @@ private boolean isResourceMatch(
// No resource defined on the policy.
return true;
}
if (!requestResource.isPresent()) {
if (requestResource.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java
@@ -218,31 +194,31 @@ private PolicyMatchFilter getFilter(DataHubResourceFilter policyResourceFilter)
}
PolicyMatchCriterionArray criteria = new PolicyMatchCriterionArray();
if (policyResourceFilter.hasType()) {
criteria.add(new PolicyMatchCriterion().setField(ResourceFieldType.RESOURCE_TYPE.name())
criteria.add(new PolicyMatchCriterion().setField(EntityFieldType.TYPE.name())
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice renaming. much cleane!

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java
final DataHubActorFilter actorFilter,
final Optional<ResolvedResourceSpec> resourceSpec,
final Optional<ResolvedEntitySpec> resourceSpec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

seeing this, i am thinking we should always say "actorSpec" and "resourceSpec"

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-service/auth-impl/src/test/java/com/datahub/authorization/PolicyEngineTest.java
when(_entityClient.batchGetV2(eq(CORP_USER_ENTITY_NAME), eq(Collections.singleton(unauthorizedUserUrn)), any(),
any())).thenReturn(unauthorizedEntityResponseMap);
when(_entityClient.batchGetV2(eq(CORP_USER_ENTITY_NAME), eq(Collections.singleton(unauthorizedUserUrn)),
eq(Collections.singleton(ROLE_MEMBERSHIP_ASPECT_NAME)), any())).thenReturn(unauthorizedEntityResponseMap);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice improvements to the test!

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
metadata-service/auth-impl/src/test/java/com/datahub/authorization/PolicyEngineTest.java
@@ -1184,21 +1170,6 @@ private EntityResponse createUnauthorizedEntityResponse() throws URISyntaxExcept
final EntityResponse entityResponse = new EntityResponse();
final EnvelopedAspectMap aspectMap = new EnvelopedAspectMap();

final CorpUserInfo userInfo = new CorpUserInfo();
Copy link
Collaborator

Choose a reason for hiding this comment

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

strange that we were fetching this

jjoyce0510
jjoyce0510 reviewed Oct 10, 2023
View reviewed changes
...c/main/java/com/datahub/authorization/fieldresolverprovider/EntityFieldResolverProvider.java
/**
* Field that this hydrator is hydrating
*/
EntityFieldType getFieldType();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice renames!

jjoyce0510
jjoyce0510 approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

Once the conflicts are resolved, let's merge.

Nice work, @amanda-her !

Really appreciate all the hard work on this.

@apertinez-tw
Merge branch 'master' into add-GroupMembershipFieldResolverProvider
8b8ce34 
# Conflicts:
#	metadata-auth/auth-api/src/main/java/com/datahub/authorization/ResolvedResourceSpec.java
#	metadata-auth/auth-api/src/main/java/com/datahub/authorization/ResourceFieldType.java
#	metadata-service/auth-impl/src/main/java/com/datahub/authorization/DefaultResourceSpecResolver.java
@apertinez-tw
Resolve merge conflicts
8424d99
@Khurzak
Copy link
Contributor

Khurzak commented Oct 11, 2023

Hello @jjoyce0510 . We have solved the merge conflicts and think the branch is ready to be merged.

cc @sgomezvillamor

@jjoyce0510
Copy link
Collaborator

Merging through the flakes on the final smoke test. Unrelated to these changes.

@jjoyce0510 jjoyce0510 merged commit c564abc into datahub-project:master Oct 12, 2023
36 of 37 checks passed
@HannesHil HannesHil mentioned this pull request Oct 24, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Khurzak Khurzak Khurzak left review comments

@jjoyce0510 jjoyce0510 jjoyce0510 approved these changes

Assignees
No one assigned
Labels
devops PR or Issue related to DataHub backend & deployment
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@amanda-her @Khurzak @jjoyce0510 @apertinez-tw