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.

Sign up for GitHub

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

Jump to bottom

♻️ Store ACL in authz field with new rules #22

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

♻️ Store ACL in authz field with new rules #22

wants to merge 15 commits into from

Conversation

znatty22
Copy link
Member

@znatty22 znatty22 commented Jun 20, 2023 •
edited
Loading

Motivation

Authorization policy information for indexd files is currently stored in the acl list field. However, the acl field is being replaced with a new authz list field. The values in the authz list will correspond to policies created in Arborist.

Please see kids-first/kf-api-dataservice#624 for details

Approach

Details on exact changes can be found in the updated README.

Update ACL Rules Logic

  1. Change open ACL value from * to /open
  2. All hidden genomic files get the empty ACL [ ]
  3. All non-open ACL values are prepended with /programs
  4. Change the value of the "default acl" (see README for when default acl is assigned)
From: [ {study_kfid}, {root_phs_acl} ] (i.e. [ "SD_12345678", "phs001138 .c999"])

To: [ unique set of ( consent ACLs from each specimen that contributes to the GF )] 
(i.e. [ "/programs/phs001138 .c1", "/programs/phs001138 .c2" ])

Annotate Reason for Visibility

  1. Hidden specimens and any related entities will have a visibility_reason=Consent Hold
  2. Hidden specimens and any related entities will have a visibility_comment=Sample is not registered in dbGaP

@znatty22 znatty22 self-assigned this Jun 20, 2023
@znatty22 znatty22 force-pushed the authz-field branch 2 times, most recently from 1cab639 to 0348627 Compare June 22, 2023 18:41
@znatty22 znatty22 changed the title ♻️ Store ACL in authz field ♻️ Store ACL in authz field with new rules Jun 22, 2023
@znatty22 znatty22 marked this pull request as ready for review June 22, 2023 18:49
chris-s-friedman
chris-s-friedman previously approved these changes Jun 30, 2023
View reviewed changes
Copy link
Collaborator

@chris-s-friedman chris-s-friedman left a comment

Choose a reason for hiding this comment

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

looks great!

Christina-J-Diaz
Christina-J-Diaz previously approved these changes Sep 19, 2023
View reviewed changes
Copy link

@Christina-J-Diaz Christina-J-Diaz left a comment

Choose a reason for hiding this comment

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

agreed- looks good!

@Christina-J-Diaz Christina-J-Diaz dismissed their stale review September 19, 2023 18:13

I meant to approve once I was done testing the ACL tool in QA, sorry!

@znatty22
🐛 Don't filter out authz = [] patches when testing
a20adf7 
After implementing new rules which set authz = [] some tests fail
This is because values get filtered out of patches if they are equal to
what is already in dataservice (avoids extra API calls).

The authz field will always be = [] in dataservice during tests since it is
not connected to indexd. That means any GF patches which include authz =
[] will be filtered out causing tests to fail
Christina-J-Diaz
Christina-J-Diaz previously approved these changes Oct 30, 2023
View reviewed changes
Copy link

@Christina-J-Diaz Christina-J-Diaz left a comment

Choose a reason for hiding this comment

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

After weeks of testing/using this new tool - it worked as expected!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-s-friedman chris-s-friedman chris-s-friedman left review comments

@Christina-J-Diaz Christina-J-Diaz Christina-J-Diaz left review comments

@allisonheath allisonheath Awaiting requested review from allisonheath

@baileyckelly baileyckelly Awaiting requested review from baileyckelly

At least 1 approving review is required to merge this pull request.

Assignees

@znatty22 znatty22

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@znatty22 @chris-s-friedman @Christina-J-Diaz