-
Notifications
You must be signed in to change notification settings - Fork 12
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
ID-1306 CBAS Submission, Filtering by Resource Parent, and Getting Resource Creator #1457
base: develop
Are you sure you want to change the base?
Conversation
val parsedResourceTypes = resourceTypes.map(_.split(",").map(ResourceTypeName(_)).toSet).getOrElse(Set.empty) | ||
val parsedPolicies = policies.map(_.split(",").map(AccessPolicyName(_)).toSet).getOrElse(Set.empty) | ||
val parsedRoles = roles.map(_.split(",").map(ResourceRoleName(_)).toSet).getOrElse(Set.empty) | ||
val parsedActions = actions.map(_.split(",").map(ResourceAction(_)).toSet).getOrElse(Set.empty) | ||
val parsedParentResourceIds = parentResources |
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.
a little cleanup here to keep our code DRY
@@ -197,6 +197,7 @@ trait UserRoutesV2 extends SamUserDirectives with SamRequestContextDirectives { | |||
Set.empty, | |||
Set(ResourceRoleName("user")), | |||
Set.empty, | |||
Set.empty, |
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.
Listing all user resources doesn't need to filter on parent, so an empty set is provided here.
val resourceParentsConstraint = | ||
if (parentResourceIds.nonEmpty) samsqls"and ${parentResourceIds | ||
.map(parentResourceId => samsqls"(${resourceParent.resourceTypeId} = ${resourceTypePKsByName(parentResourceId.resourceTypeName)} and ${resourceParent.name} = ${parentResourceId.resourceId})") | ||
.reduce((acc, clause) => samsqls"$acc or $clause")}" | ||
else samsqls"" |
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.
A series of or
clauses is needed here because we're joining on the fully-qualified resource id, which is actually 2 values.
…com:broadinstitute/sam into tl_ID-1306_cbas_submission_filter_by_parent
|
||
<changeSet logicalFilePath="dummy" author="tlangs" id="resource_created_by"> | ||
<addColumn tableName="sam_resource"> | ||
<column name="created_by" type="VARCHAR"> |
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.
Adding the column created_by
isn't just useful for submissions. Its really odd that Sam doesn't know what user created a resource!
src/main/resources/reference.conf
Outdated
cbas-submission = { | ||
actionPatterns = { | ||
abort = { | ||
description = "Abort this cbas-submission" | ||
} | ||
read = { | ||
description = "read information about the submission" | ||
} | ||
create_with_parent = { | ||
description = "Enables creating the request object with a parent" | ||
} | ||
} | ||
ownerRoleName = "owner" | ||
roles = { | ||
owner = { | ||
roleActions = ["abort", "create_with_parent"] | ||
includedRoles = ["reader"] | ||
} | ||
reader = { | ||
roleActions = ["read", "read_creator"] | ||
} | ||
} | ||
allowLeaving = false | ||
reuseIds = false | ||
} |
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.
This will allow CBAS to no longer need to store Sam User IDs!
create_with_parent = { | ||
description = "Enables creating the request object with a parent" | ||
} | ||
read_creator = { |
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.
This resource action will allow those who have it to be able to get the resource creator's email. This is similar to read_policies
, where users are allowed to get the emails of people in policies on a resource.
@@ -1674,6 +1677,35 @@ resourceTypes = { | |||
allowLeaving = false | |||
reuseIds = true | |||
} | |||
|
|||
cbas-submission = { |
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.
This will allow CBAS to no longer need to store Sam User IDs!
owner = { | ||
roleActions = ["abort", "create_with_parent"] | ||
includedRoles = ["reader"] | ||
} |
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.
How hard is it to change these after creation if we wanted to (say) have a "canceller" role separate from the owner/creator?
@@ -2976,6 +2985,44 @@ paths: | |||
application/json: | |||
schema: | |||
$ref: '#/components/schemas/ErrorReport' | |||
/api/resources/v2/{resourceTypeName}/{resourceId}/creator: |
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.
This is fine but it's probably more helpful to have a way to batch the lookup of resource => createdBy
. Either here or in the listResources
response perhaps?
37f86f6
to
655a99d
Compare
Quality Gate passedIssues Measures |
Ticket: https://broadworkbench.atlassian.net/browse/ID-1306
To help our Workflows friends, we need to add a
cbas-submissions
resource type, withowner
andreader
roles. This will allow CBAS to not need to store Sam User IDs, and instead let Sam do what Sam does best!We also need to augment the
listResourcesV2
endpoint so that it can take in aparentResourceId
and constrain its results to resources with only thatparentResourceId
.I've added an optional query parameter to the
listResourcesV2
endpoint, which takes in a list of resource parents to filter on. The resource parents are formatted asresourceTypeName:resourceId
, since we need both to be able to do a proper DB join. The:
character is not allowed in resource names, so its an OK character to use as a separator.The response from
listResourcesV2
has not changed, as doing so might break any consumers of the endpoint. If there are no consumers that would break, it may be worth it to include this parent information in the response.I've added a new endpoint:
getResourceCreator
. If a user has theread_creator
action, they'll be able to get the email address of the creator of the resource. Its odd that Sam wasn't storing this before!PR checklist