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

feat(oauth2): add method to handle access token refresh process #3889

Closed

Conversation

Ahuahuachi
Copy link
Contributor

@Ahuahuachi Ahuahuachi commented Jun 12, 2024

Description

This PR adds a method to handle the process of refreshing an access token to the OAuth2 Adapter.
The purpose for this is to give developers an option to create custom workflows in their projects, and possibly create future middleware in allauth codebase to handle the most common use cases for this.

This was based on the solution provided by #420 (comment)

Submitting Pull Requests

General

  • Make sure you use semantic commit messages.
    Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
  • All Python code must formatted using Black, and clean from pep8 and isort issues.
  • JavaScript code should adhere to StandardJS.
  • If your changes are significant, please update ChangeLog.rst.
  • If your change is substantial, feel free to add yourself to AUTHORS.

@coveralls
Copy link

Coverage Status

coverage: 95.652% (-0.06%) from 95.71%
when pulling b6ea255 on Ahuahuachi:feat/refresh_access_token
into 8eb7afc on pennersr:main.

@pennersr
Copy link
Owner

This adds a new method to the OAuth2Adapter that is not used by allauth anywhere, nor is it covered by unit tests. So, I assume this is meant to be called by developers integrating allauth, but then the OAuth2Adapter is not the most logical place for that. You also mention:

possibly create future middleware...most common use cases for this

I think it is important to clear up what use case this is covering, and then it can be decided whether or not it is in scope and if so, what the best API would be to cover that.

But in its current form, this is not a viable PR.

@pennersr pennersr closed this Jun 16, 2024
@Ahuahuachi
Copy link
Contributor Author

Yes, it is meant to be called by developers.
I think the use case is completely covered on #420 discussion where some projects are in need of a refresh feature. As per your comment on 2013:

That's in scope, though I think it is best to integrate https://github.com/requests/requests-oauthlib to do the actual dirty work...

Please let me know if anything changed since

The common use case for the middleware I was thinking on, its when the site needs to have a valid access token on every page of the site. But I think this deserves a dedicated discussion when allauth has a refresh feature.

If this method being part of the OAuth2Adapter it's not logical, maybe I should try adding it to the OAuth2Client?

@pennersr
Copy link
Owner

That comment from 2013 did not age very well. In all this time, there has been no use case to have up-to-date access tokens for authentication purposes. You can even argue that storing the SocialToken unnecessary increases allauth scope.

when the site needs to have a valid access token on every page of the site.

Why would you need that? Likely for accessing resources on the side of the provider (e.g. posts, pictures and so on), but that is really all application specific, and not needed for authentication purposes at all.

@Ahuahuachi
Copy link
Contributor Author

The thing that got me into working this was precisely the need to access the resources of the providers.

So with the state of the project right now, a refresh feature of some kind is out of scope then? If so, maybe consider closing the discussion for that feature.

@pennersr
Copy link
Owner

@Ahuahuachi Fair enough, I've added a comment there...

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