-
Notifications
You must be signed in to change notification settings - Fork 79
OAuth2 Token refresh implemented #328
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
Conversation
00a3663 to
e8be1a8
Compare
cd180a5 to
1527c5e
Compare
palkerecsenyi
left a comment
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.
Thank you so much for creating this PR @mesemus, it looks like quite a bit of work and will improve security a lot while also adding support for more modern and spec-compliant OAuth servers. I'm aware it's been over a year, I'm coming back to this as I need refresh token support for the upcoming GitLab integration (inveniosoftware/invenio-github#190) and this PR is 99% of the way there. I apologise for the large number of suggestions, these are mostly just nitpicks :)
I'm also very happy to fix them myself in a new PR if you're busy. Thanks!
invenio_oauthclient/alembic/7def990b852e_add_expires_at_and_refresh_token_to_.py
Outdated
Show resolved
Hide resolved
| self.expires_at = expires_at | ||
| db.session.add(self) | ||
|
|
||
| def refresh_access_token(self): |
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.
Just so I understand how to use this, is the intention that users of this module manually check is_expired and then call refresh_access_token if needed? I personally agree with this approach (automating this would make everything a bit more confusing and blackbox-y), just want to check this is what you meant.
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.
You marked this comment as resolved, but I had the same question :)
What is the answer?
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.
Ah yes sorry, should have explained here! Yes the intention is that users manually check this, which lets us avoid a potentially confusing blackbox design where the token refresh happens internally.
For example, here is how it's used in Invenio-VCS:
@cached_property
def remote_token(self):
token = RemoteToken.get(self.user_id, self.factory.remote.consumer_key)
if token is None:
return None
if token.is_expired:
token.refresh_access_token()
return token1527c5e to
790e7cd
Compare
6e0959a to
721f869
Compare
Uncomment the code when inveniosoftware/invenio-oauthclient#328 is merged
Uncomment the code when inveniosoftware/invenio-oauthclient#328 is merged
Uncomment the code when inveniosoftware/invenio-oauthclient#328 is merged
Uncomment the code when inveniosoftware/invenio-oauthclient#328 is merged
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
Uncomment the code when inveniosoftware/invenio-oauthclient#328 is merged
Uncomment the code when inveniosoftware/invenio-oauthclient#328 is merged
1e2e750 to
038ff8a
Compare
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
slint
left a comment
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.
LGTU (with @palkerecsenyi), some extra warnings/guidance for the db.session.commit() and we need to update copyright headers.
invenio_oauthclient/alembic/7def990b852e_add_expires_at_and_refresh_token_to_.py
Outdated
Show resolved
Hide resolved
| values = session.get(session_key, None) | ||
| if values: | ||
| # Continue supporting the old tuple for backwards-compatibility with existing sessions | ||
| if len(values) == 2: |
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.
does this mean we plan to support old style tokens indefinitely or we plan to phase them out at some point?
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.
We phase them out immediately. When the user logs in with OAuth and returns to the authorized endpoint, we check if we need extra sign up info (which is usually the case on Zenodo). If so, we need to store their credentials in the session until they complete their account, which is when we do the token exchange with the identity provider. The only case where the old token would be returned from the session is if a user was in the middle of completing the extra info form when we did the deploy, and here we are ensuring that this edge case is handled gracefully.
38de277 to
5bf8125
Compare
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Added support for refresh tokens (RFC 6749 Section 6) * Created two new columns in the oauthclient_remotetoken to store the encrypted refresh token and the expiration date of the latest stored access token. * Extended the authorized handler to store the refresh token and expiration date when received from the server. * Added methods to the RemoteToken to check whether the token is expired, and to refresh the token. * Added unit tests to cover new functionality. Co-authored-by: Mirek Simek <miroslav.simek@gmail.com>
5bf8125 to
a1fa09c
Compare
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
* Bumping the major version of invenio-oauthclient to 6.0.0 * This includes inveniosoftware/invenio-oauthclient#360 which is not necessarily breaking but requires a manual DB migration for very large instances, as documented in the module's upgrade guide. * Further changes are yet come in v6 before RDM v14, such as inveniosoftware/invenio-oauthclient#328.
Important: we are aiming to release this at the same time as #360. When we merge, we need to align the DB migrations, but still keep them as 2 separate migrations so that one can be run manually. This migration does not need to be run manually in any scenario.
Description
Closes #68.
Added fields on
RemoteTokenmodel:refresh_tokenexpires_atAdded properties on
RemoteToken:is_expiredNew methods on
RemoteToken:refresh_access_tokenChecklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: