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-7846): Visa refresh token expiration parameter #883

Merged
merged 10 commits into from
Mar 12, 2021

Conversation

BinamB
Copy link
Contributor

@BinamB BinamB commented Mar 10, 2021

This was an ask from ISB-CGC. It's mostly for testing purposes. Basically #848 but for RAS or any other Visa provider we might have in the future.

New Features

  • Added upstream_expires_in parameter in the /authorization endpoint to manually add refresh token expiration time.

Bug Fixes

  • Fixed Visa Update cronjob not clearing expired refresh tokens.

@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 Mar 10, 2021

Pull Request Test Coverage Report for Build 10561

  • 3 of 12 (25.0%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 69.484%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/resources/openid/idp_oauth2.py 0 1 0.0%
fence/job/visa_update_cronjob.py 1 3 33.33%
fence/blueprints/login/ras.py 2 8 25.0%
Files with Coverage Reduction New Missed Lines %
fence/blueprints/login/ras.py 1 33.33%
Totals Coverage Status
Change from base Build 10558: -0.05%
Covered Lines: 5722
Relevant Lines: 8235

💛 - Coveralls

@BinamB BinamB requested a review from paulineribeyre March 11, 2021 16:39
@BinamB BinamB changed the title Visa refresh token expiration parameter (PXP-7846): Visa refresh token expiration parameter Mar 11, 2021
fence/job/visa_update_cronjob.py Show resolved Hide resolved
fence/blueprints/login/ras.py Show resolved Hide resolved

# User definied RAS refresh token expiration time
parsed_url = urllib.parse.parse_qs(flask.redirect_url)
if parsed_url.get("visa_refresh_exp"):
Copy link
Contributor

Choose a reason for hiding this comment

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

You used visa_refresh_exp, the ticket says ras_expires_at, and other Fence endpoints use expires_in.
I'd suggest expires_in for consistency, unless the name in the ticket is a hard requirement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean use visa_expires_in or just expires_in? If i do expires_in then wouldn't this affect the fence refresh token as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

i was suggesting just expires_in but i didn't look closely, if expires_in is already used in this endpoint, then maybe upstream_expires_in? 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

or refresh_token_expires_in, but i think visa_expires_in might be confusing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same as well. Maybe upstream_expires_in

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 update PR description :p

fence/blueprints/login/ras.py Show resolved Hide resolved
fence/blueprints/login/ras.py Outdated Show resolved Hide resolved
BinamB and others added 3 commits March 11, 2021 14:58
Co-authored-by: Pauline Ribeyre <ribeyre@uchicago.edu>
@BinamB BinamB merged commit cc12808 into master Mar 12, 2021
@BinamB BinamB deleted the feat/ras_expiration_parameter branch March 12, 2021 23:03
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