fix(auth): reuse oauth clients during token exchange - #4918
Merged
Merged
Conversation
shepilov
marked this pull request as draft
September 14, 2026 14:02
shepilov
force-pushed
the
fix/reuse-token-exchange-clients
branch
from
September 14, 2026 15:52
930c17d to
5b40284
Compare
shepilov
force-pushed
the
fix/reuse-token-exchange-clients
branch
4 times, most recently
from
September 30, 2026 13:51
45a7bc2 to
064cd22
Compare
shepilov
marked this pull request as ready for review
September 30, 2026 13:51
Crash--
reviewed
Sep 30, 2026
|
|
||
| Reuse relies on the existing OIDC session bindings. If a binding is lost or | ||
| expires (after 31 days without renewal in Redis), another client can be created. | ||
| Existing duplicate clients are not automatically deleted by token exchange. |
Contributor
There was a problem hiding this comment.
Do we have a way to clean up old clients? I know that today we have a daily cron job to remove pending oauth client, but I don't know if we can make a distinction between a client created by token exchange and not usable anymore and other ones
Member
Author
There was a problem hiding this comment.
Unfortunately, no, only backchannel logout
shepilov
force-pushed
the
fix/reuse-token-exchange-clients
branch
from
October 1, 2026 09:21
064cd22 to
86ec13d
Compare
taratatach
reviewed
Oct 1, 2026
Repeated token exchanges within one OIDC session created a new OAuth client every time. Reuse the existing client when the OIDC context, instance, session (sid) and software_id match, including across allowed origins, so a session keeps a single client per application. A session-scoped lock serialises concurrent exchanges for the same session so they reuse one client instead of racing to create duplicates; the per-client lock still guards binding against refresh/revoke/update. Reused clients are re-read and re-validated under their lock, only newly-created clients are deleted on a binding failure, and the registration token is regenerated for reused clients. LockOAuthClient now returns lock-acquisition errors and every caller handles them instead of proceeding without the lock. Document the reuse semantics and their limits in docs/auth.md. Refs #4916
shepilov
force-pushed
the
fix/reuse-token-exchange-clients
branch
from
October 1, 2026 16:48
86ec13d to
958e364
Compare
taratatach
approved these changes
Oct 2, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
LockOAuthClientfor token exchange and propagate lock acquisition errors in all callers, with a regression test for a closed Redis connection.go test -count=1 -p 1 -timeout 10m ./web/auth ./web/settings ./web/oidc ./model/oauth ./model/oidc/bindingpassed all auth/OIDC packages; the settings RabbitMQ integration test failed because Docker could not bind a host port. The remaining settings tests passed withgo test -count=1 -v -skip '^TestForceInstanceDeletion$/^PublishDeletionMessageWithRealRabbitMQ$' ./web/settings.make unit-testsrun: 80 packages passed; the unchanged RabbitMQ connection tests failed because their Docker containers exited during startup or could not bind a host port.