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

PXP-6240 Support ga4gh passport claim in userinfo #792

Merged
merged 4 commits into from
Jul 9, 2020
Merged

PXP-6240 Support ga4gh passport claim in userinfo #792

merged 4 commits into from
Jul 9, 2020

Conversation

vpsx
Copy link
Contributor

@vpsx vpsx commented Jul 7, 2020 •
edited
Loading

(This needs to be rebased--currently branched off RAS IDP branch. To review, just select the last few commits in the differ)
(Not actually doc-only, but want to skip tests until rebase is done)

New Features

  • Add ga4gh_visa_v1 table to database
  • Save user's RAS visas to db upon RAS login
  • Include ga4gh_passport_v1 claim in userinfo response

@vpsx vpsx added the doc-only documentation only. Skip tests. label Jul 7, 2020
@github-actions
Copy link

github-actions bot commented Jul 7, 2020

The style in this PR agrees with black. ✔️

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

@vpsx vpsx changed the title [WIP] Feat/ras visas uinfo [WIP] PXP-6240 Support ga4gh passport claim in userinfo Jul 7, 2020
itsJiaqi
itsJiaqi reviewed Jul 7, 2020
View reviewed changes
setup.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 7, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 9211

  • 19 of 26 (73.08%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 69.527%
Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/resources/openid/ras_oauth2.py 1 2 50.0%
fence/blueprints/login/ras.py 4 10 40.0%
Totals Coverage Status
Change from base Build 9208: 0.004%
Covered Lines: 5259
Relevant Lines: 7564
💛 - Coveralls

@lgtm-com
Copy link

lgtm-com bot commented Jul 7, 2020

This pull request introduces 1 alert when merging fc9ea95 into 6c89664 - view on LGTM.com

new alerts:

  • 1 for Unused import

paulineribeyre
paulineribeyre reviewed Jul 7, 2020
View reviewed changes
fence/blueprints/login/ras.py Outdated Show resolved Hide resolved
fence/resources/user/__init__.py
@@ -128,6 +129,16 @@ def get_user_info(current_session, username):
optional_info = _get_optional_userinfo(user, requested_userinfo_claims)
info.update(optional_info)

# Include ga4gh passport visas
# TODO: Respect access token claims (only include if ga4gh_passport_v1 scope present)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not do this right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[discussed offline; for posterity:] No deliberate reason, but I haven't done it yet 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll do this in a future sprint (separate PR) - since we'd like to merge visas ASAP.

@vpsx vpsx removed the doc-only documentation only. Skip tests. label Jul 8, 2020
itsJiaqi
itsJiaqi reviewed Jul 8, 2020
View reviewed changes
fence/models.py
@@ -534,6 +534,27 @@ class ServiceAccountToGoogleBucketAccessGroup(Base):
)


class GA4GHVisaV1(Base):
Copy link
Contributor

Choose a reason for hiding this comment

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

@Avantol13 are we happy with these fields? I guess we can always add more later, but these are all the fields I specified in the design doc so far.

itsJiaqi
itsJiaqi approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@itsJiaqi itsJiaqi left a comment

Choose a reason for hiding this comment

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

LGTM

@lgtm-com
Copy link

lgtm-com bot commented Jul 8, 2020

This pull request introduces 1 alert when merging 5dba7b4 into c4e3114 - view on LGTM.com

new alerts:

  • 1 for Unused import

paulineribeyre
paulineribeyre approved these changes Jul 8, 2020
View reviewed changes
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.

lgtm too

@vpsx vpsx changed the title [WIP] PXP-6240 Support ga4gh passport claim in userinfo PXP-6240 Support ga4gh passport claim in userinfo Jul 9, 2020
@vpsx vpsx merged commit f174024 into master Jul 9, 2020
@vpsx vpsx deleted the feat/ras-visas-uinfo branch July 9, 2020 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsJiaqi itsJiaqi itsJiaqi approved these changes

@paulineribeyre paulineribeyre paulineribeyre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vpsx @coveralls @itsJiaqi @paulineribeyre