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

(PXP-7846): Visa refresh token expiration parameter #883

Merged
merged 10 commits into from
Mar 12, 2021
Merged

(PXP-7846): Visa refresh token expiration parameter #883

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8c3c6d4
RAS Expiratrion Parameter
BinamB Mar 9, 2021
bb4ff0b
fix things
BinamB Mar 10, 2021
9283304
refresh token fix
BinamB Mar 10, 2021
78d0edc
refresh token remove
BinamB Mar 10, 2021
3b130f7
black
BinamB Mar 10, 2021
32a33cb
Merge branch 'master' into feat/ras_expiration_parameter
BinamB Mar 10, 2021
94cfd48
changes
BinamB Mar 11, 2021
fa8688e
Merge branch 'feat/ras_expiration_parameter' of github.com:uc-cdis/fe…
BinamB Mar 11, 2021
05c0488
Update fence/job/visa_update_cronjob.py
BinamB Mar 11, 2021
cc4de53
Merge branch 'master' into feat/ras_expiration_parameter
BinamB Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions fence/blueprints/login/ras.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
import jwt
import os
from flask_sqlalchemy_session import current_session
import urllib.request, urllib.parse, urllib.error

from fence.models import GA4GHVisaV1, IdentityProvider

from fence.blueprints.login.base import DefaultOAuth2Login, DefaultOAuth2Callback

from fence.config import config
from fence.scripting.fence_create import init_syncer
from fence.utils import get_valid_expiration


class RASLogin(DefaultOAuth2Login):
Expand Down Expand Up @@ -66,8 +68,22 @@ def post_login(self, user, token_result):
refresh_token = flask.g.tokens.get("refresh_token")
id_token = flask.g.tokens.get("id_token")
decoded_id = jwt.decode(id_token, verify=False)

# Add 15 days to iat to calculate refresh token expiration time
expires = int(decoded_id.get("iat")) + config["RAS_REFRESH_EXPIRATION"]

# User definied RAS refresh token expiration time
parsed_url = urllib.parse.parse_qs(flask.redirect_url)
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved
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

custom_refresh_expiration = int(decoded_id.get("iat")) + int(
parsed_url.get("visa_refresh_exp")[0]
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved
)
expires = get_valid_expiration(
paulineribeyre marked this conversation as resolved.
Show resolved Hide resolved
custom_refresh_expiration,
expires,
expires,
)

flask.current_app.ras_client.store_refresh_token(
user=user, refresh_token=refresh_token, expires=expires
)
Expand Down
4 changes: 4 additions & 0 deletions fence/job/visa_update_cronjob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ async def updater(self, name, updater_queue, db_session):
)
client.update_user_visas(user, db_session)
else:
BinamB marked this conversation as resolved.
Show resolved Hide resolved
if user.upstream_refresh_tokens:
user.upstream_refresh_tokens = []
db_session.commit()

self.logger.info(
"User {} doesnt have visa. Skipping . . .".format(user.username)
)
Expand Down
4 changes: 2 additions & 2 deletions fence/resources/openid/idp_oauth2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,11 @@ def get_access_token(self, user, token_endpoint, db_session=None):
proxies=self.get_proxies(),
refresh_token=refresh_token,
)
new_refresh_token = token_response["refresh_token"]
refresh_token = token_response["refresh_token"]

self.store_refresh_token(
user,
refresh_token=new_refresh_token,
refresh_token=refresh_token,
expires=expires,
db_session=db_session,
)
Expand Down