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-4102 Feat/arb admin #319

Merged
merged 9 commits into from
Mar 24, 2020
Merged

PXP-4102 Feat/arb admin #319

merged 9 commits into from
Mar 24, 2020

Conversation

vpsx
Copy link
Contributor

@vpsx vpsx commented Feb 14, 2020

Switches Sheepdog authz handling of program/project CRUD to centralized auth/Arborist.

See https://github.com/uc-cdis/commons-users/pull/810 for new Sheepdog admin policy.

(For QAs' benefit) How to test:

  • See PR linked above for example of how to toggle for a given user the old admin: true/false and the new admin policy
  • Verify that the API behaves according to this matrix:
    User DOES have Sheepdog admin policy: All CRUD authorized.
        Admin is FALSE:
            Create program: Authorized
            Delete program: Authorized
            Create project: Authorized
            Delete project: Authorized
        Admin is TRUE:
            Create program: Authorized
            Delete program: Authorized
            Create project: Authorized
            Delete project: Authorized
    User does NOT have Sheepdog admin policy: All CRUD not authorized.
    (Also check that they are 403s in particular)
        Admin is FALSE:
            Create program: Unauthorized
            Delete program: Unauthorized
            Create project: Unauthorized
            Delete project: Unauthorized
        Admin is TRUE:
            Create program: Unauthorized
            Delete program: Unauthorized
            Create project: Unauthorized
            Delete project: Unauthorized

New Features

Switch checks guarding program/project CRUD to centralized auth (instead of checking for "admin" in JWT, check for Sheepdog admin policy in Arborist)

Breaking Changes

Having admin: true in user.yaml no longer allows a user to do program/project CRUD in Sheepdog. Users now need to have the Sheepdog admin policy in Arborist.

Deployment changes

The environment's user.yaml needs to be updated to add the new Sheepdog admin policy/resources/roles; for each user that has admin: true, grant them the new policy. See https://github.com/uc-cdis/commons-users/pull/810 for an example.

@PlanXCyborg
Copy link

PlanXCyborg commented Feb 14, 2020

The style in this PR agrees with black. ✔️

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

Copy link
Contributor

@paulineribeyre paulineribeyre left a comment

Choose a reason for hiding this comment

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

looks good!! i think you can clean up the various conftest files by

  • removing the admin fixture
  • removing is_admin from the tokens
  • using submitter in the tests that still user admin, with that warning you added to other tests about not checking authz

string: did
string: s3 url that you changed it to
Url:
PUT: /admin/<program>/<project>/files/<file_uuid>/reassign
Copy link
Contributor

Choose a reason for hiding this comment

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

this (updating URLs) should be supported in indexd, not sheepdog... and this code only supports S3 URLs, and it assumes there is only 1 stored URL 🤦‍♀ we should probably get rid of this and add an endpoint in indexd instead

cc @Avantol13 re:conversation about indexd not supporting URL updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we wait until indexd has the endpoint or shall I go ahead and remove this~

Copy link
Contributor

Choose a reason for hiding this comment

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

we can wait to be safe, though I don't think it's actually used anywhere? but if we change the public API we need a new major version of sheepdog

@vpsx
Copy link
Contributor Author

vpsx commented Feb 27, 2020

Thanku!! I completely forgot 🤦‍♂ . So that's all done except I didn't add the warning to the other tests--they didn't really claim to be checking authz anyway (whereas the first few either used to check authz, or were next to tests that checked the unauthorized case, so there was the implication). And it would have been a lot of noise :/

Copy link
Contributor

@paulineribeyre paulineribeyre left a comment

Choose a reason for hiding this comment

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

🎉

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.

4 participants