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/create presigned url passports #984

Merged
merged 16 commits into from
Nov 29, 2021

Conversation

BinamB
Copy link
Contributor

@BinamB BinamB commented Nov 23, 2021

New Features

  • Return aws signed urls from POST /ga4gh/drs/v1/objects/<guid>/access/<access method> endpoint with passports
  • Return google signed urls from POST /ga4gh/drs/v1/objects/<guid>/access/<access method> endpoint with passports

Improvements

  • install vim in dockerfile for live debugging

Dependency updates

  • gen3authz ^1.1.1 -> ^1.3.0 to support sending auth/request to arborist with user_id instead of jwt token

Deployment changes

@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.

@BinamB BinamB marked this pull request as ready for review November 23, 2021 21:10
@coveralls
Copy link

coveralls commented Nov 23, 2021

Pull Request Test Coverage Report for Build 12041

  • 34 of 34 (100.0%) changed or added relevant lines in 2 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.9%) to 72.401%

Files with Coverage Reduction New Missed Lines %
fence/blueprints/ga4gh.py 1 94.12%
fence/resources/ga4gh/passports.py 3 77.78%
Totals Coverage Status
Change from base Build 12026: 0.9%
Covered Lines: 6637
Relevant Lines: 9167

💛 - Coveralls

@@ -1434,21 +1460,28 @@ def delete(self, container, blob): # pylint: disable=R0201
return ("Failed to delete data file.", status_code)


def _get_user_info(sub_type=str):
def _get_user_info(sub_type=str, user=None):
"""
Attempt to parse the request for token to authenticate the user. fallback to
populated information about an anonymous user.
By default, cast `sub` to str. Use `sub_type` to override this behavior.
"""
# TODO Update to support POSTed passport
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 take this TODO comment out, right?

@johnfrancismccann
Copy link
Contributor

gen3authz-1.3.0.tar.gz can be removed.

Copy link
Contributor

@Avantol13 Avantol13 left a comment

Choose a reason for hiding this comment

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

Let's merge this, I left a few comments but we can address (if needed) in a new PR

raise Unauthorized(
f"Either you weren't logged in or you don't have "
f"{action_to_permission[action]} permission "
f"on authz resource: {self.index_document['authz']}"
)
authorized_user_id = (
authorized_user_id if isinstance(authorized_user_id, str) else None
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this necessary? I assume it's b/c the function below expects None and not something False-y? Is it even possible for the above function to return ""?

google_proxy_group,
primary_google_service_account,
cloud_manager,
google_signed_url,
monkeypatch,
):
Copy link
Contributor

Choose a reason for hiding this comment

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

In a new PR can we get some quick, small docstrings about what these tests are actually testing for in more detail? Don't worry about doing it in this PR since the tests are already passing

@BinamB BinamB merged commit 8883117 into feat/passport Nov 29, 2021
@BinamB BinamB deleted the feat/create_presigned_url_passports branch November 29, 2021 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants