-
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
Changes from 40 commits
e0ad49f
ed670a0
62dc5c3
65b3a87
9c2657e
ebce9b6
7d02aae
0940909
ea10c6e
755ac7f
feaba4f
3918a2a
8bcd8e0
cc2be41
362578b
dc5d719
f9a4e9c
35f7c62
330d919
4cb5d84
a1a2f3a
7442aae
1606138
4f20bb1
002e774
9e01299
7641023
d57319a
43e1abb
4088fb1
ba949fe
74364d0
cc8517c
1560bc0
b40b885
df952c1
daa80f6
319b17e
30f1a44
dcaa6c3
85a97c1
eed5d05
e221525
3077809
d77d110
3d8cf6a
bbe4457
9b780fa
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 |
---|---|---|
@@ -1,14 +1,15 @@ | ||
package com.linkedin.datahub.graphql.resolvers.policy.mappers; | ||
|
||
import com.linkedin.common.UrnArray; | ||
import com.linkedin.common.urn.Urn; | ||
import com.linkedin.datahub.graphql.generated.ActorFilter; | ||
import com.linkedin.datahub.graphql.generated.Policy; | ||
import com.linkedin.datahub.graphql.generated.PolicyMatchCondition; | ||
import com.linkedin.datahub.graphql.generated.PolicyMatchCriterion; | ||
import com.linkedin.datahub.graphql.generated.PolicyMatchCriterionValue; | ||
import com.linkedin.datahub.graphql.generated.PolicyMatchFilter; | ||
import com.linkedin.datahub.graphql.generated.PolicyState; | ||
import com.linkedin.datahub.graphql.generated.PolicyType; | ||
import com.linkedin.datahub.graphql.generated.ActorFilter; | ||
import com.linkedin.datahub.graphql.generated.ResourceFilter; | ||
import com.linkedin.datahub.graphql.types.common.mappers.UrnToEntityMapper; | ||
import com.linkedin.datahub.graphql.types.mappers.ModelMapper; | ||
|
@@ -53,6 +54,10 @@ private ActorFilter mapActors(final DataHubActorFilter actorFilter) { | |
result.setAllGroups(actorFilter.isAllGroups()); | ||
result.setAllUsers(actorFilter.isAllUsers()); | ||
result.setResourceOwners(actorFilter.isResourceOwners()); | ||
UrnArray resourceOwnersTypes = actorFilter.getResourceOwnersTypesUrns(); | ||
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. I'd prefer if we refer to this simply as "resourceOwnerTypes" since we do not use the "urns" suffix for the others here - users, groups, etc. |
||
if (resourceOwnersTypes != null) { | ||
result.setResourceOwnersTypesUrns(resourceOwnersTypes.stream().map(Urn::toString).collect(Collectors.toList())); | ||
} | ||
if (actorFilter.hasGroups()) { | ||
result.setGroups(actorFilter.getGroups().stream().map(Urn::toString).collect(Collectors.toList())); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7965,6 +7965,10 @@ type ActorFilter { | |
""" | ||
resourceOwners: Boolean! | ||
|
||
resourceOwnersTypesUrns: [String!] | ||
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 add comments at parity with the other fields! 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. Let's also namethis "resourceOwnerTypes" I think given the new OwnershipTypes feature we will assume this is an URN. You can also place this in the field comment 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. Added comments and changed name to |
||
|
||
resolvedOwnershipTypes: [OwnershipTypeEntity!] | ||
|
||
""" | ||
Whether the filter should apply to all users | ||
""" | ||
|
@@ -8093,6 +8097,8 @@ input ActorFilterInput { | |
""" | ||
resourceOwners: Boolean! | ||
|
||
resourceOwnersTypesUrns: [String!] | ||
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. Same comments here. 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. Done |
||
|
||
""" | ||
Whether the filter should apply to all users | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
import React from 'react'; | ||
import { Form, Select, Switch, Tag, Typography } from 'antd'; | ||
import styled from 'styled-components'; | ||
import { Maybe } from 'graphql/jsutils/Maybe'; | ||
|
||
import { useEntityRegistry } from '../../useEntityRegistry'; | ||
import { ActorFilter, CorpUser, EntityType, PolicyType, SearchResult } from '../../../types.generated'; | ||
import { useGetSearchResultsLazyQuery } from '../../../graphql/search.generated'; | ||
import { useListOwnershipTypesQuery } from '../../../graphql/ownership.generated'; | ||
import { CustomAvatar } from '../../shared/avatar'; | ||
|
||
type Props = { | ||
|
@@ -36,6 +38,10 @@ const SearchResultContent = styled.div` | |
align-items: center; | ||
`; | ||
|
||
const OwnershipWrapper = styled.div` | ||
margin-top: 12px; | ||
`; | ||
|
||
/** | ||
* Component used to construct the "actors" portion of a DataHub | ||
* access Policy by populating an ActorFilter object. | ||
|
@@ -46,12 +52,39 @@ export default function PolicyActorForm({ policyType, actors, setActors }: Props | |
// Search for actors while building policy. | ||
const [userSearch, { data: userSearchData }] = useGetSearchResultsLazyQuery(); | ||
const [groupSearch, { data: groupSearchData }] = useGetSearchResultsLazyQuery(); | ||
|
||
const { data: ownershipData } = useListOwnershipTypesQuery({ | ||
variables: { | ||
input: {}, | ||
}, | ||
}); | ||
const ownershipTypes = | ||
ownershipData?.listOwnershipTypes?.ownershipTypes.filter((type) => type.urn !== 'urn:li:ownershipType:none') || | ||
[]; | ||
const ownershipTypesMap = Object.fromEntries(ownershipTypes.map((type) => [type.urn, type.info?.name])); | ||
// 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
}); | ||
}; | ||
|
||
const onSelectOwnershipTypeActor = (newType: string) => { | ||
const newResourceOwnersTypesUrns: Maybe<string[]> = [...(actors.resourceOwnersTypesUrns || []), newType]; | ||
setActors({ | ||
...actors, | ||
resourceOwnersTypesUrns: newResourceOwnersTypesUrns, | ||
}); | ||
}; | ||
|
||
const onDeselectOwnershipTypeActor = (type: string) => { | ||
const newResourceOwnersTypesUrns: Maybe<string[]> = actors.resourceOwnersTypesUrns?.filter( | ||
(u: string) => u !== type, | ||
); | ||
setActors({ | ||
...actors, | ||
resourceOwnersTypesUrns: newResourceOwnersTypesUrns?.length ? newResourceOwnersTypesUrns : null, | ||
}); | ||
}; | ||
|
||
|
@@ -175,6 +208,7 @@ export default function PolicyActorForm({ policyType, actors, setActors }: Props | |
// Select dropdown values. | ||
const usersSelectValue = actors.allUsers ? ['All'] : actors.users || []; | ||
const groupsSelectValue = actors.allGroups ? ['All'] : actors.groups || []; | ||
const ownershipTypesSelectValue = actors.resourceOwnersTypesUrns || []; | ||
|
||
const tagRender = (props) => { | ||
// eslint-disable-next-line react/prop-types | ||
|
@@ -215,6 +249,36 @@ export default function PolicyActorForm({ policyType, actors, setActors }: Props | |
selected privileges. | ||
</Typography.Paragraph> | ||
<Switch size="small" checked={actors.resourceOwners} onChange={onToggleAppliesToOwners} /> | ||
{actors.resourceOwners && ( | ||
<OwnershipWrapper> | ||
<Typography.Paragraph> | ||
List of types of ownership which will be used to match owners. If empty, the policies | ||
will applied to any type of ownership. | ||
</Typography.Paragraph> | ||
<Select | ||
value={ownershipTypesSelectValue} | ||
mode="multiple" | ||
placeholder="Ownership types" | ||
onSelect={(asset: any) => onSelectOwnershipTypeActor(asset)} | ||
onDeselect={(asset: any) => onDeselectOwnershipTypeActor(asset)} | ||
tagRender={(tagProps) => { | ||
return ( | ||
<Tag closable={tagProps.closable} onClose={tagProps.onClose}> | ||
{ownershipTypesMap[tagProps.value.toString()]} | ||
</Tag> | ||
); | ||
}} | ||
> | ||
{ownershipTypes.map((resOwnershipType) => { | ||
return ( | ||
<Select.Option value={resOwnershipType.urn}> | ||
{resOwnershipType?.info?.name} | ||
</Select.Option> | ||
); | ||
})} | ||
</Select> | ||
</OwnershipWrapper> | ||
)} | ||
</Form.Item> | ||
)} | ||
<Form.Item label={<Typography.Text strong>Users</Typography.Text>}> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,19 @@ | |
|
||
import com.datahub.authentication.Authentication; | ||
import com.google.common.collect.ImmutableSet; | ||
import com.linkedin.common.Owner; | ||
import com.linkedin.common.Ownership; | ||
import com.linkedin.common.urn.Urn; | ||
import com.linkedin.common.urn.UrnUtils; | ||
import com.linkedin.data.template.StringArray; | ||
import com.linkedin.entity.EntityResponse; | ||
import com.linkedin.entity.EnvelopedAspect; | ||
import com.linkedin.entity.EnvelopedAspectMap; | ||
import com.linkedin.entity.client.EntityClient; | ||
import com.linkedin.identity.GroupMembership; | ||
import com.linkedin.identity.NativeGroupMembership; | ||
import com.linkedin.identity.RoleMembership; | ||
import com.linkedin.metadata.Constants; | ||
import com.linkedin.metadata.authorization.PoliciesConfig; | ||
import com.linkedin.policy.DataHubActorFilter; | ||
import com.linkedin.policy.DataHubPolicyInfo; | ||
|
@@ -28,6 +32,7 @@ | |
import java.util.Optional; | ||
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
import javax.annotation.Nullable; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
|
@@ -303,11 +308,34 @@ private boolean isOwnerMatch( | |
if (!actorFilter.isResourceOwners() || !requestResource.isPresent()) { | ||
return false; | ||
} | ||
return isActorOwner(actor, requestResource.get(), context); | ||
List<Urn> ownershipTypesUrns = actorFilter.getResourceOwnersTypesUrns(); | ||
return isActorOwner(actor, requestResource.get(), ownershipTypesUrns, context); | ||
} | ||
|
||
private boolean isActorOwner(Urn actor, ResolvedResourceSpec resourceSpec, PolicyEvaluationContext context) { | ||
Set<String> owners = resourceSpec.getOwners(); | ||
private Set<String> getOwnersForType(ResourceSpec resourceSpec, List<Urn> ownershipTypesUrns) { | ||
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 commentThe 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 |
||
Collections.singleton(Constants.OWNERSHIP_ASPECT_NAME), _systemAuthentication); | ||
if (response == null || !response.getAspects().containsKey(Constants.OWNERSHIP_ASPECT_NAME)) { | ||
return Collections.emptySet(); | ||
} | ||
ownershipAspect = response.getAspects().get(Constants.OWNERSHIP_ASPECT_NAME); | ||
} catch (Exception e) { | ||
log.error("Error while retrieving ownership aspect for urn {}", entityUrn, e); | ||
return Collections.emptySet(); | ||
} | ||
Ownership ownership = new Ownership(ownershipAspect.getValue().data()); | ||
Stream<Owner> ownersStream = ownership.getOwners().stream(); | ||
if (ownershipTypesUrns != null) { | ||
ownersStream = ownersStream.filter(owner -> ownershipTypesUrns.contains(owner.getTypeUrn())); | ||
} | ||
return ownersStream.map(owner -> owner.getOwner().toString()).collect(Collectors.toSet()); | ||
} | ||
|
||
private boolean isActorOwner(Urn actor, ResolvedResourceSpec resourceSpec, List<Urn> ownershipTypesUrns, PolicyEvaluationContext context) { | ||
Set<String> owners = this.getOwnersForType(resourceSpec.getSpec(), ownershipTypesUrns); | ||
if (isUserOwner(actor, owners)) { | ||
return true; | ||
} | ||
|
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.
I think it does, how else would UI work properly? Here is query and the output screensho (please let me know if I misunderstood you)t: