-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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): Fine grained ownership policies #7499
feat(auth): Fine grained ownership policies #7499
Conversation
visible in UI and enforced for users (no groups)
Thank you for the contribution! Let me discuss this with the core DataHub team and then get back to you :) |
Appreciate it, let me know if you need any input from me. |
Hi there, after talking with the core DataHub team, we're thinking of doing the following: We're actually introducing the ability to have custom Ownership Types very soon (cc @pedro93), and we want to make sure this contribution covers that case as well. Would it be okay with you if we held off on merging this in until custom Ownership Types are supported? At that point either we can work with you on accounting for that case, or we can take over getting this to the finish line. Let me know your thoughts and thank you so much for this contribution! |
Hello @aditya-radhakrishnan, thank you for monitoring this issue. Let's wait for the custom Ownership Types then and please let me know once the PR can be updated. |
Sounds good, will definitely do so! |
Hello @aditya-radhakrishnan I wanted to ask how is the dependency (custom Ownership Types) doing? It seems it is implemented by this PR: #7623 ? Is it blocked by something? |
@@ -1622,6 +1622,9 @@ private void configurePolicyResolvers(final RuntimeWiring.Builder builder) { | |||
})).dataFetcher("resolvedRoles", new LoadableTypeBatchResolver<>(dataHubRoleType, (env) -> { | |||
final ActorFilter filter = env.getSource(); | |||
return filter.getRoles(); | |||
})).dataFetcher("resolvedOwnershipTypes", new LoadableTypeBatchResolver<>(ownershipType, (env) -> { | |||
final ActorFilter filter = env.getSource(); | |||
return filter.getResourceOwnersTypesUrns(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm that we will resolve the entire "OwnershipType" entity from this? It appears that we are just mapping to the URNs themselves, as opposed to the OwnershipType objects that are required in the data model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -8093,6 +8097,8 @@ input ActorFilterInput { | |||
""" | |||
resourceOwners: Boolean! | |||
|
|||
resourceOwnersTypesUrns: [String!] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// Toggle the "Owners" switch | ||
const onToggleAppliesToOwners = (value: boolean) => { | ||
setActors({ | ||
...actors, | ||
resourceOwners: value, | ||
resourceOwnersTypesUrns: value ? actors.resourceOwnersTypesUrns : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - we can remove the "urns" suffix in all of these names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@jjoyce0510 thank you for the review. I applied requested changes and provided the screenshot with a proof that ownership types resolution works. Is there anything left? |
I run some performance tests with this feature using
@jjoyce0510 are there still any concerns about the feature? |
Change copy in the details modal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to address comments! The PR is very nearly ready. There is one thing I'd missed in my previous review, specifically the comment about moving the logic for "resolving" the specific owner types for the resource from the PolicyEngine into a ResolvedResourceSpec object.
This basically allows us to lazily do the lookup in a way that is not visible to the Policy Engine, keeping the responsibility of evaluating policies and fetching the facts required to evaluate a policy separate
Once this change is addressed, we will be ready to merge this PR!
Cheers
John
@@ -8130,6 +8140,11 @@ input ActorFilterInput { | |||
""" | |||
resourceOwners: Boolean! | |||
|
|||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding these!
Urn entityUrn = UrnUtils.getUrn(resourceSpec.getResource()); | ||
EnvelopedAspect ownershipAspect; | ||
try { | ||
EntityResponse response = _entityClient.getV2(entityUrn.getEntityType(), entityUrn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The final comment from me - I'd really prefer to not see this logic in the Policy Engine. This can be abstracted away to the ResolvedResourceSpec and handled internally.
The policy engine should simply ask for the ownership types for the resouce and get it back without knowing how that happens
Also note that I made a small copy change in one of the files, you may need to pull-then-push your changes! |
Thanks @jjoyce0510 for the review Correct me if I’m wrong basically, you are proposing to move this logic here datahub/metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java Lines 323 to 332 in 3077809
to the by adding a sort of method That makes sense and actually this is something we already considered when starting the implementation, however we found a limitation in the current implementation of the ResolverSpecs abstraction: the return type is limited to the datahub/metadata-auth/auth-api/src/main/java/com/datahub/authorization/FieldResolver.java Lines 45 to 53 in 3077809
So, we are kindly requesting you to release the feature as it is in this PR and consider the refactor as pending technical debt that goes beyond the scope of this PR. There are several reasons why we think such a refactor should be managed in the future as part of the generalization of the
Thanks |
Hi @jjoyce0510 , about the failing tests, there are two checks failing:
|
CI failures are unrelated. Will be ignoring those! Thanks for the hard work on this PR, and apologies for the delay! Cheers |
This PR introduces a new optional field for DataHubPolicyInfo -
resourceOwnersType
. It can only be used withresourceOwners
flag set to True. Currently, any owner of the resource will match policy withresourceOwners
, this change introduces possibility to specify which particular "types" (i.e.TECHNICAL_OWNER
,BUSINESS_OWNER
) of ownership will be considered for particular policy - making it possible to, for example, grant rights to change tags of datasets to only their technical owners but not business owners. This is an actual user requirement described here: https://feature-requests.datahubproject.io/p/authorization-policy-restricted-to-_eg_-technical_owners-ownersBeside changes in the
PolicyEngine
and other parts of backend also UI was amended to allow for new capability to be set as show below:Policy view pop-up also shows new capabilities: