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

[PM-11476] Prevent parallel refreshToken calls #10799

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

Timshel
Copy link
Contributor

@Timshel Timshel commented Aug 29, 2024

📔 Objective

The goal of this PR is to prevent the clients from making parallel refresh token calls.
This can introduce issues if the provider is rolling the refresh token and invalidating the old ones after use.

This is simply done by keeping the Promise of the current refresh call as long as it's not finished and returning it again if another call is made.

Since I'm working with Vaultwarden I'm unsure how pertinent it is with the official server, but I believe it might make sense since it's probably way easier to handle in the client and not in the server.

First time I encountered the issue is with a refresh token validity set to 5 min or less (which the client consider expired then) which then trigger a flood to refresh calls which might result in invalidating the user session.
More recently someone had the issue with the browser extension and Authentik not liking receiving the same refresh_token twice.

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-11476

@bitwarden-bot bitwarden-bot changed the title Prevent parallel refreshToken calls [PM-11476] Prevent parallel refreshToken calls Aug 29, 2024
@Timshel
Copy link
Contributor Author

Timshel commented Oct 7, 2024

Any feedback ?

@benedikt-bartscher
Copy link

Thanks @Timshel for the fix!

Is there anything contributors can do to get a faster review/feedback on this PR?

@djsmith85 djsmith85 requested review from a team and rr-bw October 11, 2024 08:26
@derSoerrn95
Copy link

Anything new?

@trmartin4
Copy link
Member

Hello! Thank you for your contribution. The team will be taking this into review soon, and we'll be able to perform a full assessment at that time and provide feedback on any required changes.

@Timshel
Copy link
Contributor Author

Timshel commented Nov 28, 2024

Any news ?

@benedikt-bartscher
Copy link

@trmartin4 do you have a schedule for this PR review?

@trmartin4
Copy link
Member

@Timshel thank you for checking in. We have pulled this into our next sprint for review, so it should be soon. Thank you for your patience. We realize that this has been open for a while, but the team has had to balance a lot of different priorities in the interim.

@trmartin4 trmartin4 requested review from Patrick-Pimentel-Bitwarden and removed request for rr-bw January 13, 2025 18:38
@CLAassistant
Copy link

CLAassistant commented Jan 15, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details21490dee-fbf8-4315-84a2-e39ac08e9163

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM Client_Privacy_Violation /libs/tools/generator/components/src/username-generator.component.html: 3

Copy link
Contributor

@Patrick-Pimentel-Bitwarden Patrick-Pimentel-Bitwarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues foreseen with this code change. Tested locally the refresh token and the flow seems fine and nonbreaking.

@Patrick-Pimentel-Bitwarden
Copy link
Contributor

Handing this off to our QA team to finish the review process.

@Patrick-Pimentel-Bitwarden
Copy link
Contributor

Just passed through QA with all ✅

Thank you so much for taking the time to contributing. This should be rolled into our next release.

@Patrick-Pimentel-Bitwarden Patrick-Pimentel-Bitwarden merged commit 9a5ebf9 into bitwarden:main Jan 24, 2025
77 checks passed
@derSoerrn95
Copy link

Great 😍! Thanks @Timshel

@Timshel
Copy link
Contributor Author

Timshel commented Jan 24, 2025

@Patrick-Pimentel-Bitwarden thank you :)

@Timshel Timshel deleted the fix/refresh branch January 24, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants