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

Feat/arborist #138

Merged
merged 6 commits into from
Sep 16, 2019
Merged

Feat/arborist #138

merged 6 commits into from
Sep 16, 2019

Conversation

paulineribeyre
Copy link
Contributor

@paulineribeyre paulineribeyre commented Sep 11, 2019

  • Use Arborist + gen3authz for auth requests
    • Use arborist's auth/mapping endpoint to get all the resources the user has access to, and parse the result to get the programs and projects the user has "peregrine read" access to
    • Patch run.py so that gen3authz requests are mocked when we run peregrine locally
  • Unit tests:
    • Bump sheepdog to 2.0.0
    • Remove project_access from the test user tokens
    • Remove the random_user fixture (only keep submitter and admin) since authorization doesn't depend on project access in the token anymore
    • Delete unused file tests/auth_mock.py

Breaking Changes

  • Use Arborist + gen3authz for auth requests

Dependency updates

  • Add gen3authz dependency for auth requests

Deployment changes

  • Optional configuration variable ARBORIST (arborist base URL)
  • Requires Arborist to be deployed

@PlanXCyborg

This comment has been minimized.

@PlanXCyborg

This comment has been minimized.

@PlanXCyborg

This comment has been minimized.

@PlanXCyborg

This comment has been minimized.

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.

🎉 some minor comments + pending integration tests

"""
This fixture returns a function which you call to mock the call to
arborist client's methods.
Parameter "mapping" lets us specify the response for a call to
Copy link
Contributor

Choose a reason for hiding this comment

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

auth_mapping

mocked_response = MagicMock(requests.Response)

if function_name == "auth_mapping":
def mocked_items(*args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary

if function_name == "create_resource":
def mocked_get(*args, **kwargs):
return None
mocked_response.get = mocked_get
Copy link
Contributor

Choose a reason for hiding this comment

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

mocked_response.get = lambda *args, **kwargs: None ? :D :D


def resource_path_to_project_ids(resource_path):
parts = resource_path.strip('/').split('/')
if resource_path == "/" or (parts and parts[0] == "programs"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead make this if not blah: early return, so that the rest of the fn need not be indented --> control flow is easier to read?

Also: I don't think parts will ever be empty, since (1) the separator is specified and (2) resource_path is not None (otherwise previous line will fail). But not a big deal either way

So maybe

if not (parts[0] == "programs" or resource_path == "/"): 
    log bad resource path?
    return []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes thank you!!
except it's not a bad resource path, it's just ignored by peregrine (eg /data_upload or /consent/xxx)

@PlanXCyborg

This comment has been minimized.

@paulineribeyre paulineribeyre merged commit c9cafdd into master Sep 16, 2019
@paulineribeyre paulineribeyre deleted the feat/arborist branch September 16, 2019 22:01
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