-
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
Open
mesemus
wants to merge
2
commits into
inveniosoftware:master
Choose a base branch
from
mesemus:store-refresh-token
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,3 +69,6 @@ examples/*.crt | |
|
||
# Vscode | ||
.vscode/ | ||
|
||
# Pycharm venv | ||
.venv |
35 changes: 35 additions & 0 deletions
35
invenio_oauthclient/alembic/7def990b852e_add_expires_at_and_refresh_token_to_.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.""" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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")), | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Should we use unit of work here? See: docs
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Valid point, but I could consider using
db.session.begin_nested()
to allow for rollbacks on errors and maintain consistency with the existing codebase.Do you think this is a better approach?
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.
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 comment
The 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.
Let's commit the token immediately after the update to ensure it's saved, avoiding potential issues with nested transactions and token invalidation. This keeps token management straightforward.
would you agree on this?