feat(auth) - New privilege for Associate tags #8644
Conversation
github-actions
bot
added
docs
| @@ -4,6 +4,7 @@ public enum DataHubGraphQLErrorCode { | |||
| BAD_REQUEST(400), | |||
| UNAUTHORIZED(403), | |||
| NOT_FOUND(404), | |||
| UNAUTHORIZED_TAG_ERROR(405), | |||
There was a problem hiding this comment.
HTTP 405 is well defined in general tech. Let's not override the standard HTTP codes
There was a problem hiding this comment.
Reverted back the error code to 403 with different error message
maggiehays
added
the
community-contribution
|
Is this an urgent ask from your side? We've not heard this request from anyone else yet, so it's unclear to me that we will deliver broad, generalizable value to the community in exchange the non-trivial complexity that this change introduces for the system. Perhaps the use case behind the change can be elaborated upon so we can better understand? I understand WHAT the change is, but I don't fully understand the WHY. My hunch: There is a group of users who are able to apply specific tags across all entities - data stewards - in which case I'd recommend granting data stewards the ability to edit ANY tags for ANY asset - why does this not work? |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes enhance the authorization and tag management capabilities within the DataHub system. New checks ensure that only users with the appropriate privileges can associate or remove tags, reinforcing security and control. Additionally, a new condition, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TagService
participant AuthService
User->>AuthService: Request permission to associate tag
AuthService-->>User: Permission granted
User->>TagService: Associate tag with entity
TagService-->>User: Tag associated successfully
sequenceDiagram
participant User
participant TagService
participant AuthService
User->>AuthService: Request permission to associate tag
AuthService-->>User: Permission denied
User->>TagService: Attempt to associate tag
TagService-->>User: AuthorizationException thrown
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
metadata-service/restli-api/src/main/snapshot/com.linkedin.platform.platform.snapshot.json (1)
5462-5465: Correct the documentation forNOT_EQUALScondition.The documentation for the
NOT_EQUALScondition in the JSON files is currently identical toEQUALS. It should be updated to accurately reflect its functionality, which is to ensure that a field does not match a given value, as indicated by its usage inPolicyEngine.java.
- File:
metadata-service/restli-api/src/main/snapshot/com.linkedin.platform.platform.snapshot.json- File:
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.jsonAnalysis chain
Verify the documentation for
NOT_EQUALScondition.The
NOT_EQUALScondition has been added to thePolicyMatchConditionenum. Ensure that the documentation accurately describes its functionality and that it is consistently applied across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage and documentation of `NOT_EQUALS` condition across the codebase. # Test: Search for `NOT_EQUALS` usage in the codebase. Expect: Consistent usage and documentation. rg --type json --type js --type ts --type java 'NOT_EQUALS'Length of output: 1274
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (21)
- datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/AddTagResolver.java (1 hunks)
- datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/AddTagsResolver.java (1 hunks)
- datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchAddTagsResolver.java (2 hunks)
- datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchRemoveTagsResolver.java (2 hunks)
- datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/RemoveTagResolver.java (1 hunks)
- datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/LabelUtils.java (1 hunks)
- datahub-graphql-core/src/main/resources/entity.graphql (1 hunks)
- datahub-web-react/src/app/entity/shared/utils.ts (1 hunks)
- datahub-web-react/src/app/permissions/policy/PolicyDetailsModal.tsx (3 hunks)
- datahub-web-react/src/app/permissions/policy/PolicyPrivilegeForm.tsx (10 hunks)
- datahub-web-react/src/app/permissions/policy/policyUtils.ts (4 hunks)
- datahub-web-react/src/app/shared/tags/AddTagsTermsModal.tsx (4 hunks)
- docs/authorization/policies.md (2 hunks)
- docs/authorization/roles.md (1 hunks)
- docs/tags.md (1 hunks)
- metadata-models/src/main/pegasus/com/linkedin/policy/PolicyMatchCondition.pdl (1 hunks)
- metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java (2 hunks)
- metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json (1 hunks)
- metadata-service/restli-api/src/main/snapshot/com.linkedin.platform.platform.snapshot.json (1 hunks)
- metadata-service/war/src/main/resources/boot/policies.json (3 hunks)
- metadata-utils/src/main/java/com/linkedin/metadata/authorization/PoliciesConfig.java (2 hunks)
Additional context used
LanguageTool
docs/authorization/roles.md
[duplication] ~132-~132: Possible typo: you repeated a word
Context: ...ark: | ❌ | The ability to to associate the tag to an entity. ...(ENGLISH_WORD_REPEAT_RULE)
Additional comments not posted (34)
metadata-models/src/main/pegasus/com/linkedin/policy/PolicyMatchCondition.pdl (1)
11-15: Addition ofNOT_EQUALScondition is appropriate.The new
NOT_EQUALScondition complements the existingEQUALScondition, enhancing the logical capabilities of the enum for policy matching.datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/RemoveTagResolver.java (1)
39-41: Authorization check for tag association is well-implemented.The addition of this check ensures that only users with the appropriate permissions can remove tags, enhancing security.
Verify the implementation of
LabelUtils.isAuthorizedToAssociateTagto ensure it correctly checks permissions.Verification successful
Authorization check implementation is accurate.
The method
isAuthorizedToAssociateTagcorrectly implements permission checks using privilege groups to ensure only authorized users can associate or remove tags.
- Located in
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/LabelUtils.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `isAuthorizedToAssociateTag`. # Test: Search for the method implementation. Expect: Correct logic for permission checks. ast-grep --lang java --pattern $'class LabelUtils { $$$ boolean isAuthorizedToAssociateTag($_, $_) { $$$ } $$$ }'Length of output: 126
Script:
#!/bin/bash # Description: Search for the method `isAuthorizedToAssociateTag` in Java files to verify its implementation. # Test: Use ripgrep to find the method definition in Java files. rg 'boolean isAuthorizedToAssociateTag\(' --glob '*.java' -A 5Length of output: 1078
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/AddTagResolver.java (1)
41-44: Authorization check for tag association is well-implemented.This check ensures that only users with the appropriate permissions can add tags, aligning with security best practices.
Verify the implementation of
LabelUtils.isAuthorizedToAssociateTagto ensure it correctly checks permissions.datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/AddTagsResolver.java (1)
46-50: Authorization check for tag association looks good.The addition of the authorization check for each tag enhances security by ensuring only authorized users can associate or remove tags.
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchRemoveTagsResolver.java (2)
42-44: Tag validation order is appropriate.Calling
validateTagsbefore other validations ensures that authorization checks are prioritized, enhancing security.
63-69: Modularization of authorization logic is effective.The
validateTagsmethod encapsulates the authorization logic, improving code modularity and maintainability.docs/tags.md (1)
20-20: Documentation update for "Associate Tags" privilege is clear.The addition of the "Associate Tags" privilege in the documentation is well-explained and aligns with the introduced code changes.
datahub-web-react/src/app/entity/shared/utils.ts (1)
100-105: Approved: Enhanced error handling for permission issues.The addition of a specific error message for permission-related errors improves user feedback. The existing comments clarify the need for this change.
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchAddTagsResolver.java (2)
48-48: Approved: Context handling update invalidateTags.The update to use
QueryContextinstead ofOperationContextaligns with the overall context handling strategy.
130-136: Approved: Enhanced authorization checks invalidateTags.The added authorization checks ensure that only users with the appropriate permissions can associate tags, enhancing security.
datahub-web-react/src/app/permissions/policy/policyUtils.ts (3)
88-92: Approved: Enhanced flexibility increateCriterion.The addition of the
conditionparameter allows for more flexible criterion creation, improving policy management.
122-127: Approved: NewgetFieldConditionfunction.The
getFieldConditionfunction enhances the ability to retrieve conditions for specific fields, facilitating more dynamic policy evaluations.
137-143: Approved: ImprovedsetFieldValuesfunction.The function now allows setting field values with a specified condition, enhancing the flexibility of policy evaluations.
datahub-web-react/src/app/permissions/policy/PolicyDetailsModal.tsx (3)
6-13: Imports look good.The new imports for
PolicyMatchConditionandgetFieldConditionare appropriate for the added functionality.
75-79: Logic for capturing policy match condition looks good.The use of
getFieldConditionto determine thepolicyMatchConditionis appropriate.
168-173: Asset condition rendering logic is correct.The rendering logic correctly displays "Excludes" or "Includes" based on the
policyMatchCondition.metadata-service/war/src/main/resources/boot/policies.json (1)
73-73: New privilege additions are consistent.The addition of "ASSOCIATE_TAGS_PRIVILEGE" across multiple policies is consistent and aligns with the PR objectives.
Also applies to: 244-244, 329-329
datahub-web-react/src/app/shared/tags/AddTagsTermsModal.tsx (1)
283-283: Error handling updates are correct.The replacement of
urnswithresourcesin error handling ensures accurate error messages.Also applies to: 313-313, 343-343, 373-373
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java (2)
248-253: LGTM! The NOT_EQUALS condition logic is well-implemented.The use of
allMatchensures that none of the specified values match the resource's field values, which is correct for theNOT_EQUALScondition.
263-264: LGTM! The NOT_EQUALS condition handling is correct.The method correctly returns the negation of the presence of the filter value within the field values set.
docs/authorization/policies.md (2)
175-175: LGTM! The "Associate Tags" privilege is correctly documented.The addition aligns with the system's authorization capabilities and enhances tag management.
149-149: LGTM! The updated note on "Share Entity" privilege is clear and informative.The clarification regarding its applicability to DataHub Cloud instances is well-placed.
datahub-web-react/src/app/permissions/policy/PolicyPrivilegeForm.tsx (8)
3-3: Import ofCheckboxcomponent.The addition of the
Checkboxcomponent is necessary for the new resource exclusion feature.
12-12: Addition ofPolicyMatchConditionimport.The
PolicyMatchConditionis crucial for managing the match condition logic in resource filtering.
109-110: InitializematchConditionstate.The
matchConditionstate is initialized usinggetFieldCondition, which aligns with the new logic for resource inclusion/exclusion.
Line range hint
186-200: Resource type selection logic.The logic for selecting and deselecting resource types now includes
PolicyMatchCondition.Equals, which is consistent with the new feature's requirements.
Line range hint
218-235: Resource selection logic update.The
onSelectResourceandonDeselectResourcefunctions now incorporatematchCondition, ensuring that the correct match condition is applied when resources are selected or deselected.
240-252: Update match condition in resources.The
updateMatchConditionInResourcesfunction updates the match condition based on the checkbox state, effectively toggling betweenEqualsandNotEquals.
351-366: Dynamic resource text based on match condition.The
getResourceTextfunction dynamically generates instructions based on the currentmatchCondition, enhancing user guidance.
535-583: Checkbox for resource exclusion.The checkbox allows users to toggle resource exclusion, updating the
matchConditionaccordingly. This addition provides a clear and interactive way to manage resource policies.datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/LabelUtils.java (1)
261-274: New methodisAuthorizedToAssociateTag.The method correctly sets up a
DisjunctivePrivilegeGroupwithASSOCIATE_TAGS_PRIVILEGEand callsAuthorizationUtils.isAuthorized. This enhances modularity by providing a specific check for tag association privileges.metadata-utils/src/main/java/com/linkedin/metadata/authorization/PoliciesConfig.java (2)
379-383: Addition ofASSOCIATE_TAGS_PRIVILEGE.The new privilege is defined with a type, display name, and description, aligning with existing privilege declarations. This addition enhances the control over tag association.
573-573: Integration ofASSOCIATE_TAGS_PRIVILEGEinto tag privileges.The new privilege is correctly added to the list of tag privileges, ensuring it is considered in access control checks.
datahub-graphql-core/src/main/resources/entity.graphql (1)
9146-9149: Addition ofNOT_EQUALScondition is well-implemented.The new
NOT_EQUALScondition in thePolicyMatchConditionenum is a useful addition, enhancing the expressiveness of policy match conditions. The documentation is clear and consistent with existing entries.
| "symbols" : [ "EQUALS", "NOT_EQUALS" ], | ||
| "symbolDocs" : { | ||
| "EQUALS" : "Whether the field matches the value" | ||
| "EQUALS" : "Whether the field matches the value", | ||
| "NOT_EQUALS" : "Whether the field matches the value" |
There was a problem hiding this comment.
Ensure consistent documentation for NOT_EQUALS.
The addition of the NOT_EQUALS symbol in the PolicyMatchCondition enum is consistent with the existing structure. However, the documentation for NOT_EQUALS is the same as EQUALS. It should be updated to reflect its actual purpose, which is to check if a field does not match a given value.
- "NOT_EQUALS" : "Whether the field matches the value"
+ "NOT_EQUALS" : "Whether the field does not match the value"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "symbols" : [ "EQUALS", "NOT_EQUALS" ], | |
| "symbolDocs" : { | |
| "EQUALS" : "Whether the field matches the value" | |
| "EQUALS" : "Whether the field matches the value", | |
| "NOT_EQUALS" : "Whether the field matches the value" | |
| "symbols" : [ "EQUALS", "NOT_EQUALS" ], | |
| "symbolDocs" : { | |
| "EQUALS" : "Whether the field matches the value", | |
| "NOT_EQUALS" : "Whether the field does not match the value" |
docs/authorization/roles.md
Outdated
| @@ -129,6 +129,7 @@ These privileges are common to both Self-Hosted DataHub and DataHub Cloud. | |||
| | Edit Dataset Column Glossary Terms | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to edit the column (field) glossary terms associated with a dataset schema. | | |||
| | Edit Dataset Column Descriptions | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to edit the column (field) descriptions associated with a dataset schema. | | |||
| | Edit Tag Color | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to change the color of a Tag. | | |||
| | Associate Tags | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to to associate the tag to an entity. | | |||
There was a problem hiding this comment.
Fix the typo in the privilege description.
There is a duplicated word "to" in the description of the "Associate Tags" privilege.
- | Associate Tags | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to to associate the tag to an entity. |
+ | Associate Tags | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to associate the tag to an entity. |Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Associate Tags | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to to associate the tag to an entity. | | |
| | Associate Tags | :heavy_check_mark: | :heavy_check_mark: | :x: | The ability to associate the tag to an entity. | |
Tools
LanguageTool
[duplication] ~132-~132: Possible typo: you repeated a word
Context: ...ark: | ❌ | The ability to to associate the tag to an entity. ...(ENGLISH_WORD_REPEAT_RULE)
We have some use-cases where only a specific group users are authorized to associate certain tags. For all other tags, we want to allow all users to be able to associate the tags to ANY entity. Edit Tag privilege is a privilege which allows ANY tag to be associated to an entity. The entity can be specified or restricted by ownership. But, there is no way to restrict association by tags. Also, there is only ways to specify policy by including resources but no way to specify policies by excluding resources. |
|
@jjoyce0510 and @chriscollins3456 Meenakshi synced up the code with master again after validation. Could one of you please review and merge the PR. Thanks in advance. |
|
@anshbansal , @chriscollins3456 and @jjoyce0510 Thanks and Regards. |
| LabelUtils.validateLabel( | ||
| context.getOperationContext(), tagUrn, Constants.TAG_ENTITY_NAME, _entityService); | ||
|
|
||
| if (!LabelUtils.isAuthorizedToAssociateTag(context, tagUrn)) { |
There was a problem hiding this comment.
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.
metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
| return ( | ||
| <Typography.Paragraph> |
There was a problem hiding this comment.
This is introducing a non-trivial amount of new risk into this system by adding a lot of new complexity.
Is there consideration about priority of exclusion inclusion rules and conflicting rules?
There was a problem hiding this comment.
Thanks for providing the detailed overview of the rationale behind associate Tags. I think it's safe to add this change, with the one caveat that the OpenAPI endpoints are not currently honoring this privilege - either that would need to be solved in this PR or a followup, I'll allow @david-leifker to chime in.
As for the exclusionary rules, I think this is a bit more concerning with larger implications. To date, we've fought long and hard to keep the policy system as simple as possible, to minimize the chance of user error when assigning privileges, by maintaining ONLY an inclusionary access model.
Exclusionary is a lot more risk because it means folks can do things by default. It is also not clear how to restrict privileges once it's been granted via an exclusionary policy has been defined - it's quite easy to accidentally grant broad access to many people without deeply understanding that you've done this.
I'm wondering if you folks are willing to drop this requirement, or to better understand why inclusionary access is not possible for your situation. This change will have security implications ALL users of DataHub across the globe, so it's quite important we get this one right.
…tion/PolicyEngine.java Co-authored-by: RyanHolstien <RyanHolstien@users.noreply.github.com>
In our organization, we want to allow all users to be able to create and associate tags for their own use cases. But, there are certain tags which are restricted. That is the reason we want to add this Exclusionary logic so that we can restrict access for these special tags alone. Would it be okay if we add this with a feature flag to reduce impact ? |
|
I think we definitely need to add a test around the policy and new privilege added to the existing tests around the policy engine. Note that the policy engine will grant access if any of the policies match, which means the NOT_EQUAL condition would not be given precedence. It is not a deny condition, but the naming might be a bit confusing based on the Specifically, I am not sure this logic allows this use case
|
New privilege to restrict association of tags. We have some use-cases where only a specific group users are authorized to associate certain tags. Also added a resource exclusion capability so that policy can be created for all resources excluding a certain set of resources using NOT_EQUALS policy condition.
To mimic behavior before this change, we need to create a policy which authorizes all users to the new privilege ASSOCIATE TAGS all tag resources.
Checklist
Summary by CodeRabbit
New Features
NOT_EQUALSto improve filtering capabilities in policy conditions.Bug Fixes
Documentation