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.

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

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
c923b8b
New privilege for Associate tags
Aug 16, 2023
f60a413
Add Associate_tags policy for admin and editors
Aug 16, 2023
0da4478
Merge branch 'master' into master
mkamalas Aug 16, 2023
5067773
Set Tag Authorization error as 403 error code
Aug 18, 2023
0ee58b0
Merge branch 'master' into master
mkamalas Aug 18, 2023
e03e2af
Merge branch 'master' into master
mkamalas Aug 20, 2023
67a991f
Merge branch 'master' into master
mkamalas Aug 22, 2023
df12d4a
Merge branch 'master' into master
mkamalas Aug 23, 2023
e5216d4
Merge branch 'master' into master
mkamalas Aug 23, 2023
7f002d5
Merge branch 'master' into master
mkamalas Aug 23, 2023
c35550a
Merge branch 'master' into master
mkamalas Aug 24, 2023
2750e54
Add Associate_Tags privilege for datahub
Aug 27, 2023
15d56b1
Merge branch 'master' into master
mkamalas Aug 27, 2023
80efa94
For NOT_EQUALS, check all resources match the condition
mkamalas Sep 11, 2023
c60d9ab
Merge branch 'master' into master
mkamalas Aug 8, 2024
bf81623
Merge from upstream
mkamalas Aug 27, 2024
193a093
Format and linting fixes
mkamalas Aug 30, 2024
74074fb
Merge branch 'datahub-project:master' into master
mkamalas Aug 30, 2024
f37a807
Fix documentation for NOT_EQUALS
mkamalas Aug 30, 2024
fae9dd0
Merge branch 'master' into master
mkamalas Sep 4, 2024
4c1aa72
Resolve merge conflict issue
mkamalas Sep 9, 2024
f703c95
Update metadata-service/auth-impl/src/main/java/com/datahub/authoriza…
mkamalas Sep 18, 2024
8b43d10
Merge branch 'master' into master
mkamalas Sep 19, 2024
49977c1
Merge from upstream fork
mkamalas Feb 14, 2025
b235f0d
Rename privilege to Associate Entity and apply to terms
mkamalas Feb 16, 2025
e98fb1e
Merge branch 'master' into master
mkamalas Mar 5, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ public CompletableFuture<Boolean> get(DataFetchingEnvironment environment) throw
"Unauthorized to perform this action. Please contact your DataHub administrator.");
}

if (!LabelUtils.isAuthorizedToAssociateTag(environment.getContext(), tagUrn)) {
throw new AuthorizationException("Only users granted permission to this tag can assign or remove it");
}

return GraphQLConcurrencyUtils.supplyAsync(
() -> {
LabelUtils.validateResourceAndLabel(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ public CompletableFuture<Boolean> get(DataFetchingEnvironment environment) throw
"Unauthorized to perform this action. Please contact your DataHub administrator.");
}

tagUrns.forEach((tagUrn) -> {
if (!LabelUtils.isAuthorizedToAssociateTag(environment.getContext(), tagUrn)) {
throw new AuthorizationException("Only users granted permission to this tag can assign or remove it");
}
});

LabelUtils.validateResourceAndLabel(
context.getOperationContext(),
tagUrns,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public CompletableFuture<Boolean> get(DataFetchingEnvironment environment) throw
() -> {

// First, validate the batch
validateTags(context.getOperationContext(), tagUrns);
validateTags(context, tagUrns);

if (resources.size() == 1 && resources.get(0).getSubResource() != null) {
return handleAddTagsToSingleSchemaField(context, resources, tagUrns);
Expand Down Expand Up @@ -127,9 +127,13 @@ private Boolean attemptBatchAddTagsWithSiblings(
}
}

private void validateTags(@Nonnull OperationContext opContext, List<Urn> tagUrns) {
private void validateTags(@Nonnull QueryContext context, List<Urn> tagUrns) {
for (Urn tagUrn : tagUrns) {
LabelUtils.validateLabel(opContext, tagUrn, Constants.TAG_ENTITY_NAME, _entityService);
LabelUtils.validateLabel(context.getOperationContext(), tagUrn, Constants.TAG_ENTITY_NAME, _entityService);

if (!LabelUtils.isAuthorizedToAssociateTag(context, tagUrn)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 AuthorizationException("Only users granted permission to this tag can assign or remove it");
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ public CompletableFuture<Boolean> get(DataFetchingEnvironment environment) throw
return GraphQLConcurrencyUtils.supplyAsync(
() -> {

// First, validate the tag urns
validateTags(tagUrns, context);

// First, validate the batch
validateInputResources(context.getOperationContext(), resources, context);

Expand All @@ -57,6 +60,14 @@ public CompletableFuture<Boolean> get(DataFetchingEnvironment environment) throw
"get");
}

private void validateTags(List<Urn> tagUrns, QueryContext context) {
for (Urn tagUrn : tagUrns) {
if (!LabelUtils.isAuthorizedToAssociateTag(context, tagUrn)) {
throw new AuthorizationException("Only users granted permission to this tag can assign or remove it");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

now in all these resolvers we'd be checking they have the EDIT_ENTITY_TAGS_PRIVILEGE as well as this new ASSOCIATE_TAGS_PRIVILEGE which seems confusing to me to require to privileges for the same action. In validateInputResource in these resolvers you can see us make these checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EDIT_ENTITY_TAGS_PRIVILEGE is a privilege for the entity to which a list of tags are associated. ASSOCIATE_TAGS_PRIVILEGE is for the list of tags that are being associated. That is the reason I have created 2 different methods.

}
}

private void validateInputResources(
@Nonnull OperationContext opContext, List<ResourceRefInput> resources, QueryContext context) {
for (ResourceRefInput resource : resources) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ public CompletableFuture<Boolean> get(DataFetchingEnvironment environment) throw
throw new AuthorizationException(
"Unauthorized to perform this action. Please contact your DataHub administrator.");
}
if (!LabelUtils.isAuthorizedToAssociateTag(environment.getContext(), tagUrn)) {
throw new AuthorizationException("Only users granted permission to this tag can assign or remove it");
}

return GraphQLConcurrencyUtils.supplyAsync(
() -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,21 @@ public static boolean isAuthorizedToUpdateTags(
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()))
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 EDIT_ENTITY_TAGS_PRIVILEGE privilege already that seems to be doing type of thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 EDIT_ENTITY_TAGS_PRIVILEGE privilege already that seems to be doing type of thing?

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) {

Expand Down
4 changes: 4 additions & 0 deletions datahub-graphql-core/src/main/resources/entity.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -9143,6 +9143,10 @@ enum PolicyMatchCondition {
Whether the field matches the value
"""
EQUALS
"""
Whether the field does not the value
"""
NOT_EQUALS
}

"""
Expand Down
6 changes: 6 additions & 0 deletions datahub-web-react/src/app/entity/shared/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ function getGraphqlErrorCode(e) {

export const handleBatchError = (urns, e, defaultMessage) => {
if (urns.length > 1 && getGraphqlErrorCode(e) === 403) {
if (e.message === 'Only users granted permission to this tag can assign or remove it') {
return {
content: `${e.message}. The bulk edit being performed will not be saved.`,
duration: 3,
};
}
Comment on lines +100 to +105
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this new message when we have a message about being unauthorized below?

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 new error message is for clarity to pinpoint which authorization is missing.

return {
content:
'Your bulk edit selection included entities that you are unauthorized to update. The bulk edit being performed will not be saved.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,14 @@ import { Link } from 'react-router-dom';
import { Button, Divider, Modal, Tag, Typography } from 'antd';
import styled from 'styled-components';
import { useEntityRegistry } from '../../useEntityRegistry';
import { Maybe, Policy, PolicyState, PolicyType } from '../../../types.generated';
import { Maybe, Policy, PolicyMatchCondition, PolicyState, PolicyType } from '../../../types.generated';
import { useAppConfig } from '../../useAppConfig';
import { convertLegacyResourceFilter, getFieldValues, mapResourceTypeToDisplayName } from './policyUtils';
import {
convertLegacyResourceFilter,
getFieldCondition,
getFieldValues,
mapResourceTypeToDisplayName,
} from './policyUtils';
import AvatarsGroup from '../AvatarsGroup';

type PrivilegeOptionType = {
Expand Down Expand Up @@ -67,9 +72,11 @@ export default function PolicyDetailsModal({ policy, visible, onClose, privilege
const isMetadataPolicy = policy?.type === PolicyType.Metadata;

const resources = convertLegacyResourceFilter(policy?.resources);

const resourceTypes = getFieldValues(resources?.filter, 'TYPE') || [];
const dataPlatformInstances = getFieldValues(resources?.filter, 'DATA_PLATFORM_INSTANCE') || [];
const resourceEntities = getFieldValues(resources?.filter, 'URN') || [];
const policyMatchCondition = getFieldCondition(resources?.filter, 'URN');
const domains = getFieldValues(resources?.filter, 'DOMAIN') || [];

const {
Expand Down Expand Up @@ -158,6 +165,13 @@ export default function PolicyDetailsModal({ policy, visible, onClose, privilege
);
})) || <PoliciesTag>All</PoliciesTag>}
</div>
<div>
<Typography.Title level={5}>Asset Condition</Typography.Title>
<ThinDivider />
<PoliciesTag>
{policyMatchCondition === PolicyMatchCondition.NotEquals ? 'Excludes' : 'Includes'}
</PoliciesTag>
</div>
<div>
<Typography.Title level={5}>Assets</Typography.Title>
<ThinDivider />
Expand Down
140 changes: 101 additions & 39 deletions datahub-web-react/src/app/permissions/policy/PolicyPrivilegeForm.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { useEffect, useMemo, useRef, useState } from 'react';
import { Link } from 'react-router-dom';
import { Form, Select, Tag, Tooltip, Typography, Tag as CustomTag } from 'antd';
import { Form, Select, Tag, Tooltip, Typography, Tag as CustomTag, Checkbox } from 'antd';
import styled from 'styled-components/macro';

import { useEntityRegistry } from '../../useEntityRegistry';
Expand All @@ -9,13 +9,14 @@ import {
useGetSearchResultsForMultipleLazyQuery,
useGetSearchResultsLazyQuery,
} from '../../../graphql/search.generated';
import { ResourceFilter, PolicyType, EntityType, Domain, Entity } from '../../../types.generated';
import { ResourceFilter, PolicyType, EntityType, Domain, Entity, PolicyMatchCondition } from '../../../types.generated';
import {
convertLegacyResourceFilter,
createCriterionValue,
createCriterionValueWithEntity,
EMPTY_POLICY,
getFieldValues,
getFieldCondition,
getFieldValuesOfTags,
mapResourceTypeToDisplayName,
mapResourceTypeToEntityType,
Expand Down Expand Up @@ -105,6 +106,8 @@ export default function PolicyPrivilegeForm({
const resourceTypes = getFieldValues(resources.filter, 'TYPE') || [];
const resourceEntities = getFieldValues(resources.filter, 'URN') || [];

const matchConditionInitial = getFieldCondition(resources.filter, 'RESOURCE_URN');
const [matchCondition, setMatchCondition] = useState(matchConditionInitial);
const getDisplayName = (entity) => {
if (!entity) {
return null;
Expand Down Expand Up @@ -180,7 +183,7 @@ export default function PolicyPrivilegeForm({
};
setResources({
...resources,
filter: setFieldValues(filter, 'TYPE', [...resourceTypes, createCriterionValue(selectedResourceType)]),
filter: setFieldValues(filter, 'TYPE', [...resourceTypes, createCriterionValue(selectedResourceType)], PolicyMatchCondition.Equals),
});
};

Expand All @@ -194,6 +197,7 @@ export default function PolicyPrivilegeForm({
filter,
'TYPE',
resourceTypes?.filter((criterionValue) => criterionValue.value !== deselectedResourceType),
PolicyMatchCondition.Equals,
),
});
};
Expand All @@ -211,7 +215,9 @@ export default function PolicyPrivilegeForm({
resource,
getEntityFromSearchResults(resourceSearchResults, resource) || null,
),
]),
],
matchCondition,
),
});
};

Expand All @@ -226,20 +232,40 @@ export default function PolicyPrivilegeForm({
filter,
'URN',
resourceEntities?.filter((criterionValue) => criterionValue.value !== resource),
matchCondition,
),
});
};

const updateMatchConditionInResources = (excludeResource) => {
const filter = resources.filter || {
criteria: [],
};
setResources({
...resources,
filter: setFieldValues(
filter,
'RESOURCE_URN',
resourceEntities,
excludeResource ? PolicyMatchCondition.NotEquals : PolicyMatchCondition.Equals,
),
});
};
// When a domain is selected, add its urn to the list of domains
const onSelectDomain = (domainUrn, domainObj?: Domain) => {
const filter = resources.filter || {
criteria: [],
};
const domainEntity = domainObj || getEntityFromSearchResults(domainSearchResults, domainUrn);
const updatedFilter = setFieldValues(filter, 'DOMAIN', [
const updatedFilter = setFieldValues(
filter,
'DOMAIN',
[
...domains,
createCriterionValueWithEntity(domainUrn, domainEntity || null),
]);
],
PolicyMatchCondition.Equals,
);
setResources({
...resources,
filter: updatedFilter,
Expand All @@ -262,6 +288,7 @@ export default function PolicyPrivilegeForm({
filter,
'DOMAIN',
domains?.filter((criterionValue) => criterionValue.value !== domain),
PolicyMatchCondition.Equals,
),
});
};
Expand Down Expand Up @@ -321,6 +348,23 @@ export default function PolicyPrivilegeForm({
: displayStr;
};

const getResourceText = (policyMatch) => {
if (policyMatch === PolicyMatchCondition.Equals) {
return (
<Typography.Paragraph>
Search for specific resources the policy should apply to. If <b>none</b> is selected, policy is
applied to <b>all</b> resources of the given type.
</Typography.Paragraph>
);
}
return (
<Typography.Paragraph>
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 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?

Search for specific resource(s) the policy exclusion should apply to. If <b>none</b> is selected, policy
is applied to <b>all</b> resources of the given type.
</Typography.Paragraph>
);
};

function handleCLickOutside() {
// delay closing the domain navigator so we don't get a UI "flash" between showing search results and navigator
setTimeout(() => setIsFocusedOnInput(false), 0);
Expand Down Expand Up @@ -488,39 +532,57 @@ export default function PolicyPrivilegeForm({
</Form.Item>
)}
{showResourceFilterInput && (
<Form.Item label={<Typography.Text strong>Resource</Typography.Text>}>
<Typography.Paragraph>
Search for specific resources the policy should apply to. If <b>none</b> is selected, policy is
applied to <b>all</b> resources of the given type.
</Typography.Paragraph>
<Select
notFoundContent="No search results found"
value={resourceSelectValue}
mode="multiple"
filterOption={false}
placeholder="Apply to ALL resources by default. Select specific resources to apply to."
onSelect={onSelectResource}
onDeselect={onDeselectResource}
onSearch={handleResourceSearch}
tagRender={(tagProps) => (
<Tag closable={tagProps.closable} onClose={tagProps.onClose}>
<Tooltip title={tagProps.value.toString()}>
{displayStringWithMaxLength(
resourceUrnToDisplayName[tagProps.value.toString()] ||
tagProps.value.toString(),
75,
)}
</Tooltip>
</Tag>
)}
>
{resourceSearchResults?.map((result) => (
<Select.Option key={result.entity.urn} value={result.entity.urn}>
{renderSearchResult(result)}
</Select.Option>
))}
</Select>
</Form.Item>
<>
<Form.Item label={<Typography.Text strong>Resource Condition</Typography.Text>}>
<Typography.Paragraph>
Selecting the checkbox below will exclude selected resource from the policy. If not
selected, resources selected will be included in the policy.
</Typography.Paragraph>
<Checkbox
checked={matchCondition === PolicyMatchCondition.NotEquals}
onChange={(value) => {
setMatchCondition(
value?.target?.checked
? PolicyMatchCondition.NotEquals
: PolicyMatchCondition.Equals,
);
updateMatchConditionInResources(value?.target?.checked);
}}
>
Exclude Resources
</Checkbox>
</Form.Item>
<Form.Item label={<Typography.Text strong>Resource</Typography.Text>}>
{getResourceText(matchCondition)}
<Select
notFoundContent="No search results found"
value={resourceSelectValue}
mode="multiple"
filterOption={false}
placeholder="Apply to ALL resources by default. Select specific resources to apply to."
onSelect={onSelectResource}
onDeselect={onDeselectResource}
onSearch={handleResourceSearch}
tagRender={(tagProps) => (
<Tag closable={tagProps.closable} onClose={tagProps.onClose}>
<Tooltip title={tagProps.value.toString()}>
{displayStringWithMaxLength(
resourceUrnToDisplayName[tagProps.value.toString()] ||
tagProps.value.toString(),
75,
)}
</Tooltip>
</Tag>
)}
>
{resourceSearchResults?.map((result) => (
<Select.Option key={result.entity.urn} value={result.entity.urn}>
{renderSearchResult(result)}
</Select.Option>
))}
</Select>
</Form.Item>
</>
)}
{showResourceFilterInput && (
<Form.Item label={<Typography.Text strong>Select Tags</Typography.Text>}>
Expand Down
Loading
Loading