feat(privileges) Create privileges to allow for managing children of entities #6346

chriscollins3456 · 2022-11-02T21:20:35Z

This PR does a few things, all with the goal of allowing users to manage children of entities using our permissions framework. Specifically, the desire was to allow a way for users to allow owners of Glossary Nodes to create, delete, and move Terms and Nodes underneath it. So this PR creates the generic privileges than apply to Nodes but could be applied to other entities. Same thing with a new GraphQL privileges object on an entity. Broken down:

Create a new Manage Children privilege that's applied to Glossary Nodes right now
Create utility class for Glossary entities to check privileges and other shared methods (including privileges for managing children)
Create new EntityPrivileges object that can be returned on an entity itself
Create new EntityPrivilegesResolver to resolve privileges of different entity types
Update the frontend for Nodes and Terms to allow for creating, deleting, and moving child Terms and Nodes if a user has permissions.

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

… terms and nodes with a resolver

…ate and delete

github-actions · 2022-11-02T21:26:33Z

Unit Test Results (build & test)

613 tests +11 609 ✔️ +11 12m 3s ⏱️ -1s
151 suites +  2     4 💤 ±  0
151 files +  2     0 ❌ ±  0

Results for commit bae42e2. ± Comparison against base commit aa06f31.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-11-03T04:45:21Z

...re/src/main/java/com/linkedin/datahub/graphql/resolvers/entity/EntityPrivilegesResolver.java

+        case Constants.GLOSSARY_NODE_ENTITY_NAME:
+          return getGlossaryNodePrivileges(urn, context);
+        default:
+          log.warn("Tried to get entity privileges for entity type {} but nothing is implemented for it yet", urn.getEntityType());


jjoyce0510 · 2022-11-03T04:46:13Z

...re/src/main/java/com/linkedin/datahub/graphql/resolvers/entity/EntityPrivilegesResolver.java

+    }
+    Urn parentNodeUrn = GlossaryUtils.getTermParentUrn(termUrn, context, _entityClient);
+    if (parentNodeUrn != null) {
+      Boolean canManage = GlossaryUtils.canManageGlossaryEntity(context, parentNodeUrn);


Should this check really be... "can manage children entities" ?

jjoyce0510 · 2022-11-03T05:03:10Z

...re/src/main/java/com/linkedin/datahub/graphql/resolvers/entity/EntityPrivilegesResolver.java

+      result.setCanManageChildren(true);
+      return result;
+    }
+    Boolean canManageChildren = GlossaryUtils.canManageGlossaryEntity(context, nodeUrn);


NIt: somewhat confusing when reading that canManageGlossaryEntity is related to whether we can manage the children

jjoyce0510 · 2022-11-03T05:04:40Z

...rc/main/java/com/linkedin/datahub/graphql/resolvers/glossary/CreateGlossaryTermResolver.java


    return CompletableFuture.supplyAsync(() -> {
-      if (AuthorizationUtils.canManageGlossaries((context))) {
+      if (GlossaryUtils.canManageGlossaryEntity(context, parentNode)) {


Yeah seeing it once more I would expeet to pass the current entity urn into this method

yeah these comments make total sense. I think it either needs to be canManageChildrenEntities and pass in the parentNode or it should still be canManageGlossaryEntity and pass in the current entity and an optional parentNode, then the method gets the parent node if none is provided (because sometimes we have access to it like in create while other times we need to fetch it like for delete)

I'm going to go with canManageChildrenEntities for now since that's how it's currently set up and I think makes the privileges relationship in the glossary a bit more obvious - we look to owners of the parent for permissions and the name of the method tells us that.

If you like the other option though, let me know! i could be convinced.

jjoyce0510 · 2022-11-03T05:05:46Z

...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/UpdateNameResolver.java

@@ -62,7 +63,7 @@ private Boolean updateGlossaryTermName(
      UpdateNameInput input,
      QueryContext context
  ) {
-    if (AuthorizationUtils.canManageGlossaries(context)) {
+    if (GlossaryUtils.canManageGlossaries(context)) {


So in order to change the name of the entity I've created, I might need "manage glossaries"? What if we just revert back to the same check we ran before around whether we can manage the node?

yup definitely like this more. if you can delete the entity you should be able to rename it...

jjoyce0510 · 2022-11-03T05:05:52Z

...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/UpdateNameResolver.java

@@ -86,7 +87,7 @@ private Boolean updateGlossaryNodeName(
      UpdateNameInput input,
      QueryContext context
  ) {
-    if (AuthorizationUtils.canManageGlossaries(context)) {
+    if (GlossaryUtils.canManageGlossaries(context)) {


jjoyce0510 · 2022-11-03T05:07:23Z

...re/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/UpdateParentNodeResolver.java

-      if (AuthorizationUtils.canManageGlossaries(environment.getContext())) {
+      Urn currentParentUrn = GlossaryUtils.getParentUrn(targetUrn, context, _entityClient);
+      // need to be able to manage current parent node and new parent node
+      if (GlossaryUtils.canManageGlossaryEntity(context, currentParentUrn) && GlossaryUtils.canManageGlossaryEntity(context, parentNodeUrn)) {


Pretty awesome! Nice catch on this check.

jjoyce0510 · 2022-11-03T05:07:43Z

...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/GlossaryUtils.java

+
+  private GlossaryUtils() { }
+
+  public static boolean canManageGlossaries(@Nonnull QueryContext context) {


Minor: i tend to try to put a java method doc on every public method

jjoyce0510 · 2022-11-03T05:08:07Z

...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/GlossaryUtils.java

+  }
+
+  /**
+   * Returns true if the current user is able to create, delete, or move Glossary Terms and Nodes under a parent Node.


Based on description, definitely think this is due for a rename!

jjoyce0510 · 2022-11-03T05:09:34Z

...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/GlossaryUtils.java

+    }
+  }
+
+  public static Urn getNodeParentUrn(Urn nodeUrn, QueryContext context, EntityClient entityClient) {


minor: java doc.

We can also annotate this method as @nullable.

Also prefer to annotate input args where possible

jjoyce0510 · 2022-11-03T05:09:43Z

...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/GlossaryUtils.java

+    }
+  }
+
+  public static Urn getParentUrn(Urn urn, QueryContext context, EntityClient entityClient) {


jjoyce0510 · 2022-11-03T05:11:20Z

datahub-web-react/src/app/entity/shared/EntityDropdown/EntityDropdown.tsx

@@ -197,14 +192,14 @@ function EntityDropdown(props: Props) {
                        {menuItems.has(EntityMenuItems.DELETE) && (
                            <StyledMenuItem
                                key="5"
-                                disabled={isDeleteDisabled || !canManageGlossaries}
+                                disabled={entityHasChildren || !canManageGlossaryEntity}


jjoyce0510 · 2022-11-03T05:12:01Z

datahub-web-react/src/app/entity/shared/EntityDropdown/EntityDropdown.tsx

                                onClick={onDeleteEntity}
                            >
                                <Tooltip
                                    title={`Can't delete ${entityRegistry.getEntityName(
                                        entityType,
                                    )} with child entities.`}
-                                    overlayStyle={isDeleteDisabled ? {} : { display: 'none' }}
+                                    overlayStyle={entityHasChildren ? {} : { display: 'none' }}


Should I be able to see this if I am not allowed to delete?

great point! i'll make it so we only show this tooltip if you would be able to delete if there were not any children, but there are children.

jjoyce0510 · 2022-11-03T05:13:41Z

metadata-utils/src/main/java/com/linkedin/metadata/authorization/PoliciesConfig.java

@@ -249,6 +249,12 @@ public class PoliciesConfig {
      "Edit Contact Information",
      "The ability to change the contact information such as email & chat handles.");

+  // Glossary Node Privileges
+  public static final Privilege MANAGE_CHILDREN_PRIVILEGE = Privilege.of(


I know we are calling this a "Glossary Node Privilege", but then naming it something really generic (Manage Children). What is the risk of going too generic with this? Would it be better to go more specific with something like "MANAGE_GLOSSARY_CHILDREN"? Disclaimer- I have not deeply thought through it

hmm so I think we could certainly reuse this privilege for other things if we start needing to. The risk is that MANAGE_CHILDREN could mean slightly different things in different contexts. So in the future - does MANAGE_CHILDREN mean the same thing for Glossary Nodes, Domains, and Containers? maybe? maybe not.

so maybe the least risky move here is to make this more specific and call if MANAGE_GLOSSARY_CHILDREN so that if we want to have slightly different meanings for different parent contexts.

It would also give more flexibility when creating policies to be more fine-grained - maybe you want to create a policy where users can manage children of nodes they create, but only edit domains they own, but not necessarily their children - they could then add the MANAGE_GLOSSARY_CHILDREN privilege but omit the MANAGE_DOMAIN_CHILDREN privilege.

The bummer then is if we start having a lot of these, it feels repetitive. However, I don't really see that happening. Also we're pretty repetitive in our privileges per entity type anyways, this kind of fits in with the current approach of giving very fine-grained control over things.

After talking this out I'm going to go ahead and change the name to be more specific! However if you feel different let me know and I'll be happy to discuss.

jjoyce0510 · 2022-11-03T05:14:08Z

...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/GlossaryUtils.java

+import static com.linkedin.datahub.graphql.resolvers.AuthUtils.ALL_PRIVILEGES_GROUP;
+
+@Slf4j
+public class GlossaryUtils {


I think it'd be great to have a couple unit tests. Usually these static classes are quite easy to test!

jjoyce0510 · 2022-11-03T18:39:46Z

datahub-web-react/src/app/entity/shared/containers/profile/__tests__/EntityHeader.test.tsx


        expect(canEditName).toBe(false);
    });

    it('should return false for an unsupported entity', () => {
-        const canEditName = getCanEditName(EntityType.Chart, platformPrivileges);
+        const canEditName = getCanEditName(EntityType.Chart, entityDataWithManagePrivileges, platformPrivileges);


Nice! Thanks for adding these :)

jjoyce0510

Overall LGTM. Once CI is green we can go on it

…entities (datahub-project#6346) Co-authored-by: Chris Collins <[email protected]> Co-authored-by: Chris Collins <[email protected]>

Chris Collins and others added 9 commits November 2, 2022 17:12

add new manage_children privilege and create new privileges object on…

d7530e1

… terms and nodes with a resolver

add frontend menu changes and update graphql queries on FE

6dd48dd

create GlossaryUtils and add correct permissions checks on BE for cre…

2fc4ee0

…ate and delete

update FE for move

8097270

remove unused imports - FE

38de292

make utils function for getting parent node urn

9ef8e4b

update permissions for moving

5abecd2

fix BE failing formatting and build issues

372fe74

add tests for resolver

4f79a26

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Nov 2, 2022

jjoyce0510 reviewed Nov 3, 2022

View reviewed changes

Chris Collins added 2 commits November 3, 2022 11:58

PR edits except for glossary utils tests - coming in next commit

74fe091

add tests for glossary utils

bae42e2

jjoyce0510 reviewed Nov 3, 2022

View reviewed changes

jjoyce0510 approved these changes Nov 3, 2022

View reviewed changes

jjoyce0510 merged commit 8c783da into datahub-project:master Nov 3, 2022

HannesHil mentioned this pull request Jul 11, 2023

Data Domain - Ownership inheritence #8395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(privileges) Create privileges to allow for managing children of entities #6346

feat(privileges) Create privileges to allow for managing children of entities #6346

chriscollins3456 commented Nov 2, 2022

github-actions bot commented Nov 2, 2022 •

edited

Loading

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

chriscollins3456 Nov 3, 2022

jjoyce0510 Nov 3, 2022

chriscollins3456 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

chriscollins3456 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022 •

edited

Loading

jjoyce0510 Nov 3, 2022

chriscollins3456 Nov 3, 2022

jjoyce0510 Nov 3, 2022

chriscollins3456 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 Nov 3, 2022

jjoyce0510 left a comment


		private GlossaryUtils() { }

		public static boolean canManageGlossaries(@Nonnull QueryContext context) {

feat(privileges) Create privileges to allow for managing children of entities #6346

feat(privileges) Create privileges to allow for managing children of entities #6346

Conversation

chriscollins3456 commented Nov 2, 2022

Checklist

github-actions bot commented Nov 2, 2022 • edited Loading

Unit Test Results (build & test)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 2, 2022 •

edited

Loading

jjoyce0510 Nov 3, 2022 •

edited

Loading