-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix OidcClient duplicating the client_id for the public client #26637
Fix OidcClient duplicating the client_id for the public client #26637
Conversation
@@ -109,7 +109,7 @@ public Uni<Tokens> get() { | |||
body.add(OidcConstants.CLIENT_ASSERTION, jwt); | |||
} | |||
} else if (!OidcCommonUtils.isClientSecretPostAuthRequired(oidcConfig.credentials)) { | |||
body.add(OidcConstants.CLIENT_ID, oidcConfig.clientId.get()); | |||
body = copyMultiMap(body).add(OidcConstants.CLIENT_ID, oidcConfig.clientId.get()); |
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.
Isn't changing to set
enough here?
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.
@gastaldi Sorry, forgot to add a comment relating to set
, since OidcClient
will be reused by multiple threads, I decided to avoid set
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.
Copying the MultiMap is fine, my question was more that if you call add
again you will end up with more than one ClientID in the Map?
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.
Hey @gastaldi, sorry for asking to review on Sunday :-).
I see, so, in the original map client id is not present, this copy is created per every request and I guess, either set
or add
will do. I've added a test, as you can see in the original issue the duplication starts from a 2nd request (double add
), so the test does 2 requests, I could confirm without the fix the 2nd request was failing.
if you'd like I can replace add
with set
, would not be a problem at all
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.
No biggie, just curious really ;)
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.
Hi George, just pushed a minor update, it is quiet today, so an extra CI run will not be a problem, set
reads better for setting a single value property :-), thanks for the time
7e704e6
to
8a00cfe
Compare
Fixes #26619.
OidcClient
has a cachedMultiMap
which keeps some properties which are reused across multiple token requests (ex, it has a client id and secret set if the client post authentication is required, and scopes) - but if the public client is used to request a token then the same client id is added to the sameMultiMap
.So this PR does a minor update, copies the map to have a request specific map if the client is public (as it does in other cases) and it fixes the problem, the test has been added.