-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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) - New privilege for Associate entities for entities tags and glossary terms #8644
base: master
Are you sure you want to change the base?
Changes from 3 commits
c923b8b
f60a413
0da4478
5067773
0ee58b0
e03e2af
67a991f
df12d4a
e5216d4
7f002d5
c35550a
2750e54
15d56b1
80efa94
c60d9ab
bf81623
193a093
74074fb
f37a807
fae9dd0
4c1aa72
f703c95
8b43d10
49977c1
b235f0d
e98fb1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package com.linkedin.datahub.graphql.exception; | ||
|
||
|
||
/** | ||
* Exception thrown when update fails due to ASSOCIATE_TAG privilege. | ||
*/ | ||
public class TagAuthorizationException extends DataHubGraphQLException { | ||
|
||
public TagAuthorizationException(String message) { | ||
super(message, DataHubGraphQLErrorCode.UNAUTHORIZED_TAG_ERROR); | ||
} | ||
|
||
public TagAuthorizationException(String message, Throwable cause) { | ||
super(message, DataHubGraphQLErrorCode.UNAUTHORIZED_TAG_ERROR); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
import com.linkedin.common.urn.UrnUtils; | ||
import com.linkedin.datahub.graphql.QueryContext; | ||
import com.linkedin.datahub.graphql.exception.AuthorizationException; | ||
import com.linkedin.datahub.graphql.exception.TagAuthorizationException; | ||
import com.linkedin.datahub.graphql.generated.BatchAddTagsInput; | ||
import com.linkedin.datahub.graphql.generated.ResourceRefInput; | ||
import com.linkedin.datahub.graphql.resolvers.mutate.util.LabelUtils; | ||
|
@@ -45,7 +46,7 @@ public CompletableFuture<Boolean> get(DataFetchingEnvironment environment) throw | |
return CompletableFuture.supplyAsync(() -> { | ||
|
||
// First, validate the batch | ||
validateTags(tagUrns); | ||
validateTags(tagUrns, context); | ||
|
||
if (resources.size() == 1 && resources.get(0).getSubResource() != null) { | ||
return handleAddTagsToSingleSchemaField(context, resources, tagUrns); | ||
|
@@ -117,9 +118,13 @@ private Boolean attemptBatchAddTagsWithSiblings( | |
} | ||
} | ||
|
||
private void validateTags(List<Urn> tagUrns) { | ||
private void validateTags(List<Urn> tagUrns, QueryContext context) { | ||
for (Urn tagUrn : tagUrns) { | ||
LabelUtils.validateLabel(tagUrn, Constants.TAG_ENTITY_NAME, _entityService); | ||
|
||
if (!LabelUtils.isAuthorizedToAssociateTag(context, tagUrn)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Auth calls should come first before sending validation operations to the entity service, ideally done as soon as possible before any business logic occurs to short circuit. |
||
throw new TagAuthorizationException("Only users granted permission to this tag can assign or remove it"); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -201,6 +201,21 @@ public static boolean isAuthorizedToUpdateTags(@Nonnull QueryContext context, Ur | |
orPrivilegeGroups); | ||
} | ||
|
||
public static boolean isAuthorizedToAssociateTag(@Nonnull QueryContext context, Urn targetUrn) { | ||
|
||
// Decide whether the current principal should be allowed to associate the tag | ||
final DisjunctivePrivilegeGroup orPrivilegeGroups = new DisjunctivePrivilegeGroup(ImmutableList.of( | ||
new ConjunctivePrivilegeGroup(ImmutableList.of(PoliciesConfig.ASSOCIATE_TAGS_PRIVILEGE.getType())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm I'm a little confused here - so what's the benefit of this new privilege when we have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please check my response to your other comment on the need for this new privilege |
||
)); | ||
|
||
return AuthorizationUtils.isAuthorized( | ||
context.getAuthorizer(), | ||
context.getActorUrn(), | ||
targetUrn.getEntityType(), | ||
targetUrn.toString(), | ||
orPrivilegeGroups); | ||
} | ||
|
||
public static boolean isAuthorizedToUpdateTerms(@Nonnull QueryContext context, Urn targetUrn, String subResource) { | ||
|
||
Boolean isTargetingSchema = subResource != null && subResource.length() > 0; | ||
|
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.
HTTP 405 is well defined in general tech. Let's not override the standard HTTP codes
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.
Reverted back the error code to 403 with different error message