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

Add refresh token implementation #1367

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Ae-Mc
Copy link
Contributor

@Ae-Mc Ae-Mc commented Mar 9, 2024

Add refresh token strategy, bearer token structure, database base class.
Need help with JWT and redis strategies.
Need help with cookie transport.
Need help with test writing (I will wrote it later, but tests must be reviewed by somebody more professional).
Need help with docs.

It can solve discussion #350.
In difference with pull request #1075 by @jtv8 this pull request doesn't change so much. Mostly it adds new classes, without changing old.

Ae-Mc and others added 21 commits March 9, 2024 05:13
…#1370)

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

---------

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Breaking change
---------------

The underlying password hashing library has been changed from `passlib` to `pwdlib`. This change is breaking only if you were using a custom `CryptContext`. Otherwise, you can upgrade without any changes.

Improvements
------------

* Python 3.12 support
* Password are now hashed using the Argon2 algorithm by default. Passwords created with the previous default algorithm (bcrypt) will still be verified correctly and upgraded to Argon2 when the user logs in.
* Bump dependencies
  * `python-multipart ==0.0.9`
@hasB4K
Copy link

hasB4K commented Mar 22, 2024

I would love to see this PR merged into fastapi-users. The refresh token is something that is often necessary when a project start to scale. @frankie567 what do you think? :)

@Chiggy-Playz
Copy link

@Ae-Mc is this PR in a usable state right now? I'd like to clone it locally and try it out in my project

@Ae-Mc
Copy link
Contributor Author

Ae-Mc commented Mar 27, 2024

@Chiggy-Playz I'm using it right now on one of my projects

@Chiggy-Playz
Copy link

I see. If the repository is public can you link me where its used? I'd like to get an idea of how to set things up properly 😅

@Ae-Mc
Copy link
Contributor Author

Ae-Mc commented Mar 27, 2024

@Chiggy-Playz
Copy link

@Ae-Mc thanks for the links! I think i've managed to implement it properly in my project as well (hopefully), however I did find a bug. The DatabaseRefreshStrategy has a parameter refresh_lifetime_seconds but that is not used anywhere. So I added the following code

    def _refresh_get_max_age(self) -> Optional[datetime]:
        max_age = None
        if self.refresh_lifetime_seconds:
            max_age = datetime.now(timezone.utc) - timedelta(
                seconds=self.refresh_lifetime_seconds
            )
        return max_age

inside the DatabaseRefreshStrategy class and then inside the read_token_by_refresh I use _refresh_get_max_age instead of _get_max_age. I've opened a PR on your repository and I'd be glad if you could merge it 😁

@abdullah-alnahas
Copy link

Hello,

I was wondering if you have any estimated timelines for when the merge will be completed and the release will take place.

@hasB4K
Copy link

hasB4K commented Apr 18, 2024

@abdullah-alnahas It really depends if @frankie567 wants this feature or not. Since he didn't answer yet, it's not even sure that this PR will even be accepted.

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.

None yet

6 participants