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

PXP-4872 Fix usersync access revoke #743

Merged
merged 7 commits into from
Dec 30, 2019
Merged

PXP-4872 Fix usersync access revoke #743

merged 7 commits into from
Dec 30, 2019

Conversation

paulineribeyre
Copy link
Contributor

@paulineribeyre paulineribeyre commented Dec 12, 2019

addresses the following:

  • make sure access is revoked when a group is deleted from the user.yaml -> delete all groups that are in the arborist DB but not in the user.yaml
  • when a resource is renamed in the user.yaml, the old resource doesn't get deleted and users could still have access to it (e.g. user has access to /A and we renamed /A/B to /A/C) -> can only be reproduced by renaming top-level resources, because the "PUT resource" does its job of overriding all the subresources. So users could not still have access to it (the validation check makes sure the resource paths users have access to exist). We can still delete all obsolete resources in the future to clean up the DB but i didn't do it here
  • project access in the user.yaml overrides project access in the dbGaP files, but it should combine access instead -> comments that say "override" in the fence code created confusion, but in reality the access is already combined (there is a unit test that checks that). I updated/added comments
  • disable the option of adding a description to a group because Arborist doesn't allow that

relates to uc-cdis/gen3authz#26

Bug Fixes

  • Do not allow specifying descriptions for groups

Improvements

  • Properly revoke access given to users in a group when the group is deleted

Dependency updates

  • gen3authz to 0.3.0

@github-actions
Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@coveralls
Copy link

coveralls commented Dec 12, 2019

Pull Request Test Coverage Report for Build 8248

  • 5 of 8 (62.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.006%) to 69.45%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/sync/sync_users.py 5 8 62.5%
Totals Coverage Status
Change from base Build 8205: -0.006%
Covered Lines: 5099
Relevant Lines: 7342

💛 - Coveralls

@lgtm-com
Copy link

lgtm-com bot commented Dec 12, 2019

This pull request fixes 15 alerts when merging 250fda8 into f5a712b - view on LGTM.com

fixed alerts:

  • 10 for Missing call to __init__ during object initialization
  • 4 for Module is imported with 'import' and 'import from'
  • 1 for Flask app is run in debug mode

@paulineribeyre paulineribeyre requested review from vpsx, Avantol13 and itsJiaqi and removed request for vpsx December 13, 2019 00:13
Copy link
Contributor

@vpsx vpsx left a comment

Choose a reason for hiding this comment

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

  1. Digging the telemetry -> dbGaP authorization changes in docs :P
  2. Gotta update the gen3authz loose pin in setup.py also

@paulineribeyre paulineribeyre merged commit 3c5d95e into master Dec 30, 2019
@paulineribeyre paulineribeyre deleted the fix/group-sync branch December 30, 2019 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants