-
Notifications
You must be signed in to change notification settings - Fork 76
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
OAuth2 Token refresh implemented #328
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,3 +69,6 @@ examples/*.crt | |
|
||
# Vscode | ||
.vscode/ | ||
|
||
# Pycharm venv | ||
.venv |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
# | ||
# This file is part of Invenio. | ||
# Copyright (C) 2016-2018 CERN. | ||
# | ||
# Invenio is free software; you can redistribute it and/or modify it | ||
# under the terms of the MIT License; see LICENSE file for more details. | ||
|
||
"""Add expires_at and refresh_token to remote token.""" | ||
|
||
import sqlalchemy as sa | ||
import sqlalchemy_utils | ||
from alembic import op | ||
|
||
# revision identifiers, used by Alembic. | ||
revision = "7def990b852e" | ||
down_revision = "aaa265b0afa6" | ||
branch_labels = () | ||
depends_on = ("aaa265b0afa6",) | ||
|
||
|
||
def upgrade(): | ||
"""Upgrade database.""" | ||
op.add_column( | ||
"oauthclient_remotetoken", | ||
sa.Column("refresh_token", sqlalchemy_utils.EncryptedType(), nullable=True), | ||
) | ||
op.add_column( | ||
"oauthclient_remotetoken", sa.Column("expires_at", sa.DateTime(), nullable=True) | ||
) | ||
|
||
|
||
def downgrade(): | ||
"""Downgrade database.""" | ||
op.drop_column("oauthclient_remotetoken", "expires_at") | ||
op.drop_column("oauthclient_remotetoken", "refresh_token") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# -*- coding: utf-8 -*- | ||
# | ||
# This file is part of Invenio. | ||
# Copyright (C) 2016-2018 CERN. | ||
# | ||
# Invenio is free software; you can redistribute it and/or modify it | ||
# under the terms of the MIT License; see LICENSE file for more details. | ||
|
||
"""Alembic migrations for Invenio-OAuthClient.""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# -*- coding: utf-8 -*- | ||
# | ||
# This file is part of Invenio. | ||
# Copyright (C) 2024 CESNET z.s.p.o. | ||
# | ||
# Invenio is free software; you can redistribute it and/or modify it | ||
# under the terms of the MIT License; see LICENSE file for more details. | ||
|
||
"""Handler for refreshing access token.""" | ||
|
||
from flask_oauthlib.client import OAuthResponse | ||
from flask_oauthlib.utils import to_bytes | ||
|
||
from invenio_oauthclient.handlers.token import make_expiration_time | ||
|
||
from ..models import RemoteToken | ||
from ..proxies import current_oauthclient | ||
|
||
|
||
def refresh_access_token(token: RemoteToken): | ||
""" | ||
Internal method to refresh the access token. | ||
:param token: the remote token to be refreshed | ||
:returns tuple of (access_token, secret, refresh_token, expires_at) | ||
Note: the current access/refresh token are invalidated during this call | ||
""" | ||
remote_account = token.remote_account | ||
client_id = remote_account.client_id | ||
remote = next( | ||
x | ||
for x in current_oauthclient.oauth.remote_apps.values() | ||
if x.consumer_key == client_id | ||
) | ||
client = remote.make_client() | ||
refresh_token_request = client.prepare_refresh_token_request( | ||
remote.access_token_url, | ||
refresh_token=token.refresh_token, | ||
client_id=remote.consumer_key, | ||
client_secret=remote.consumer_secret, | ||
) | ||
resp, content = remote.http_request( | ||
refresh_token_request[0], | ||
refresh_token_request[1], | ||
data=to_bytes(refresh_token_request[2], remote.encoding), | ||
method="POST", | ||
) | ||
resp = OAuthResponse(resp, content, remote.content_type) | ||
return ( | ||
resp.data.get("access_token"), | ||
"", | ||
resp.data.get("refresh_token"), | ||
make_expiration_time(resp.data.get("expires_in")), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ | |
|
||
"""Models for storing access tokens and links between users and remote apps.""" | ||
|
||
import datetime | ||
|
||
from flask import current_app | ||
|
||
# UserIdentity imported for backward compatibility. UserIdentity was originally | ||
|
@@ -119,6 +121,14 @@ class RemoteToken(db.Model, Timestamp): | |
) | ||
"""Access token to remote application.""" | ||
|
||
refresh_token = db.Column( | ||
EncryptedType(type_in=db.Text, key=_secret_key), nullable=True | ||
) | ||
"""Refresh token to remote application.""" | ||
|
||
expires_at = db.Column(db.DateTime, nullable=True) | ||
"""Access token expiration date.""" | ||
|
||
secret = db.Column(db.Text(), default="", nullable=False) | ||
"""Used only by OAuth 1.""" | ||
|
||
|
@@ -130,6 +140,17 @@ class RemoteToken(db.Model, Timestamp): | |
) | ||
"""SQLAlchemy relationship to RemoteAccount objects.""" | ||
|
||
@property | ||
def is_expired(self): | ||
"""Check if access token has expired.""" | ||
if not self.expires_at: | ||
return False | ||
|
||
leeway = current_app.config.get("OAUTHCLIENT_TOKEN_EXPIRES_LEEWAY", 10) | ||
expiration_with_leeway = self.expires_at - datetime.timedelta(seconds=leeway) | ||
|
||
return expiration_with_leeway < datetime.datetime.now() | ||
|
||
def __repr__(self): | ||
"""String representation for model.""" | ||
return ( | ||
|
@@ -141,18 +162,37 @@ def token(self): | |
"""Get token as expected by Flask-OAuthlib.""" | ||
return (self.access_token, self.secret) | ||
|
||
def update_token(self, token, secret): | ||
def update_token(self, token, secret, refresh_token=None, expires_at=None): | ||
"""Update token with new values. | ||
:param token: The token value. | ||
:param secret: The secret key. | ||
:param refresh_token: The refresh token | ||
:param expires_at: Time when the access token expires | ||
""" | ||
if self.access_token != token or self.secret != secret: | ||
if ( | ||
self.access_token != token | ||
or self.secret != secret | ||
or self.refresh_token != refresh_token | ||
or self.expiration != expires_at | ||
): | ||
with db.session.begin_nested(): | ||
self.access_token = token | ||
self.secret = secret | ||
self.refresh_token = refresh_token | ||
self.expires_at = expires_at | ||
db.session.add(self) | ||
|
||
def refresh_access_token(self): | ||
"""Refresh the access token.""" | ||
if not self.refresh_token: | ||
raise ValueError("No refresh token available") | ||
from .handlers.refresh import refresh_access_token | ||
|
||
access_token, refresh_token, secret, expires_at = refresh_access_token(self) | ||
self.update_token(access_token, refresh_token, secret, expires_at) | ||
db.session.commit() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would make invenio-oauthclient dependent on invenio-records-resources (where the UnitOfWork is defined). I would personally stick with simple commit as the rest of the library uses that, but would document it in the pydoc to make sure that the caller knows that commit will occur - does it make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid point, but I could consider using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely better, but I still have a small issue with that :) The problem is that when the oauth token endpoint is called, it invalidates the previous access & refresh token and returns a new pair (that is at least the case for our perun aai implementation). Then, if the new access & refresh is not stored to database (for example, the refresh method is invoked from an external begin_nested which rolls back afterwards for some reason), the original values stored in remote_token are completely unusable and any subsequent call will fail. That's why I would rather commit the token as soon as possible, or if using the begin_nested I would at least document that the caller should commit it as soon as possible - what would you prefer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point, it will just introduce complexity regarding token management and state consistency. |
||
|
||
@classmethod | ||
def get(cls, user_id, client_id, token_type="", access_token=None): | ||
"""Get RemoteToken for user. | ||
|
@@ -197,7 +237,17 @@ def get_by_token(cls, client_id, access_token, token_type=""): | |
) | ||
|
||
@classmethod | ||
def create(cls, user_id, client_id, token, secret, token_type="", extra_data=None): | ||
def create( | ||
cls, | ||
user_id, | ||
client_id, | ||
token, | ||
secret, | ||
token_type="", | ||
extra_data=None, | ||
refresh_token=None, | ||
expires_at=None, | ||
): | ||
"""Create a new access token. | ||
.. note:: Creates RemoteAccount as well if it does not exists. | ||
|
@@ -209,6 +259,8 @@ def create(cls, user_id, client_id, token, secret, token_type="", extra_data=Non | |
:param token_type: The token type. (Default: ``''``) | ||
:param extra_data: Extra data to set in the remote account if the | ||
remote account doesn't exists. (Default: ``None``) | ||
:param refresh_token: The refresh token. | ||
:param expires_at: Expiration of the token | ||
:returns: A :class:`invenio_oauthclient.models.RemoteToken` instance. | ||
""" | ||
|
@@ -228,6 +280,8 @@ def create(cls, user_id, client_id, token, secret, token_type="", extra_data=Non | |
remote_account=account, | ||
access_token=token, | ||
secret=secret, | ||
refresh_token=refresh_token, | ||
expires_at=expires_at, | ||
) | ||
db.session.add(token) | ||
return token | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why not use
datetime.now(timezone.utc)
instead ofdatetime.now()
to ensure all datetime objects are timezone-aware for better consistency and clarity?(
utcnow()
will be deprecated in 3.12)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems that sqlalchemy removes the timezone even with db.DateTime(timezone=True), so that after db commit the expires_at is not timezone aware. Would you have a better solution than :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if you changed
sa.Column("expires_at", sa.DateTime(timezone=True), nullable=True)
and SQLAlchemy still removes the timezone information despite the change, the problem could be relying somewhere else right?WDYT @slint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the "aware" version just to see the test results for different databases.
It seems that both sqlite and mysql (https://dev.mysql.com/doc/refman/8.0/en/datetime.html) convert timestamp to utc before storing to db and then to local timezone upon retrieval - but "awareness" is lost in sqlalchemy.
Only postgres stores the timezone with the datetime ("timestamp with time zone" type) and subsequently sqlalchemy returns timezone aware datetime.
As invenio sources use utcnow() in many places (grep gave me more that 100) and even the maintrunk of sqlalchemy_utils uses utcnow(), I'd vote for keeping the datetime naive but in utc, using utcnow in the code. Later on when the invenio codebase will be upgraded to python 3.12++, then if not already solved by sqlalchemy itself we might introduce our own timestamp decorator in invenio-db (similar to https://mike.depalatis.net/blog/sqlalchemy-timestamps.html)
@Samk13 @slint What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
sa.DateTime(timezone=True)
strips timezone info, the issue is deeper.I would vote to stick with
utcnow()
for now and address this in Python 3.12 upgrades.@slint, I recommend this. Your call. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to utcnow in code