Repository navigation
Conversation
07b8566 to
a900b7a
Compare
a900b7a to
eba98f2
Compare
eba98f2 to
3fa0f84
Compare
There was a problem hiding this comment.
@thomasdarimont Thanks, the additional details in the event are useful!
Added some minor comments inline, but I think that this also causes some test failures AccessTokenTest, OAuth2DeviceAuthorizationGrantTest, OAuthProofKeyForCodeExchangeTest, OID4VCIssuerEndpointTest (not sure about last one as it is flaky in general and it is failing in some other PRs as well...)
There was a problem hiding this comment.
Same concern like above (the fact that we're returning scopes inside the CorsErrorResponseException is likely also far from ideal, so if you can change in the PR when on this, it can be nice :-)
There was a problem hiding this comment.
Same as above. I'll remove the received scopes from the returned response. How about logging a warning here with the invalid scope?
We could also just sent out all allowed scopes with the response, e.g.: https://www.googlecloudcommunity.com/gc/Apigee/Invalid-Scope-Error/m-p/644418
There was a problem hiding this comment.
Thanks. Also sounds good to me to log the warning. Sending back the valid scopes is also a good idea.
There was a problem hiding this comment.
I updated the code, the error response now looks like this:
curl -s \
-d "grant_type=password" \
-d "username=admin" \
-d "password=admin" \
-d "client_id=admin-cli" \
-d "scope=unknown" \
http://localhost:8081/auth/realms/master/protocol/openid-connect/token {
"error":"invalid_scope",
"error_description":"Invalid scopes. Scopes must be contained in [address, email, microprofile-jwt, offline_access, organization, phone, profile]"
}This is a bit more telling than just invalid scopes. For now I filter the scopes to those that are also included in the token scope, but that might be not necessary. I could probably remove the filter and render out all allowed scopes. WDYT?
Without the "include in tokenscope" filter we get this:
{
"error":"invalid_scope",
"error_description":"Invalid scopes. Scopes must be contained in [acr, address, basic, email, microprofile-jwt, offline_access, organization, phone, profile, roles, web-origins]"
}Note that the artifical scopes roles, web-origins, basic, acr are now also added to the response.
3fa0f84 to
13c4955
Compare
|
@mposolda I revised the code as discussed and fixed the test failures, which were mostly checking for the errorDescription in the response. All tests pass now locally including |
There was a problem hiding this comment.
Here I used the more appropriate Errors.INVALID_REDIRECT_URI instead of Errors.INVALID_CODE
There was a problem hiding this comment.
Here I used the more appropriate Errors.INVALID_CLIENT instead of Errors.INVALID_CODE
f0d00ef to
6889ad5
Compare
There was a problem hiding this comment.
Here we collect all allowed scope names that can be requested (and represented in the token).
There was a problem hiding this comment.
Hi @thomasdarimont - thank you for improving the logging and the events.
Something I learned just recently: You'll need to call event.detail() before calling event.error(), as the event.error() will send out and store the event. If you do it the other way around, the details will get lost.
About the available scopes: I agree to store this information in the event, which will then be logged and depending on the configuration also stored in the database.
I then disagree to share the whole list of scopes with the caller, as I would consider this an information leak if this is shared with an unauthorized caller aka public client (CWE-200). Maybe we could have different messages for the event and returned to the caller.
I also see that you're adding log.warn() in some places. As the default setting is to log error events, this is IMHO redundant. Maybe you want to reconsider some of those log messages.
|
Thanks @ahus1 for taking a look and your helpful suggestions. I totally missed the subtle relationship between Regarding saving the elegible scopes in the events, I just reverted this for now to the previous state. ...
"scopes_supported": [
"openid",
"roles",
"organization",
"profile",
"web-origins",
"microprofile-jwt",
"phone",
"address",
"acr",
"email",
"basic",
"offline_access"
],
...Perhaps we should add a setting to the client scope configuration that allows to hide a client scope from the OIDC configuration document. @mposolda I just tested the initial suggestion in String errorMessage = "Invalid scopes: " + request.getScope();
event.detail(Details.REASON, errorMessage);A scope parameter like The current admin ui encodes the values shown in the events view, so I think we are fine with showing this in the table. I also reverted the changed log levels in |
d804da7 to
739ca5c
Compare
777320c to
a3cad80
Compare
|
@mposolda thanks for the review! I Adapted the PkceUtils to avoid info logging in favor of relying on the error events. There were two log messages in info in |
|
I just looked at the same problem for the SAML Endpoints #29238 and noticed that the Unfortunately the intent of |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.webauthn.AppInitiatedActionWebAuthnTest#proceedSetupWebAuthnLogoutOtherSessionsCheckedKeycloak CI - WebAuthn IT (chrome) org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeWithDynamicScopesEnabledorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testClientExchangeorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testIntrospectTokenAfterImpersonationorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testPublicClientNotAllowedorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeUsingServiceAccountorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationUsingPublicClientorg.keycloak.testsuite.client.ClientTypesTest#testUpdateClientWithClientType |
There was a problem hiding this comment.
@thomasdarimont Thanks! IMO this is sufficient. We can possibly have a follow-up to adjust Details.REASON in other places if needed.
@ahus1 WDYT? Are we ok with merge this?
…#29166) Fixes keycloak#29166 Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
- Adapted tests to reflect the change Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
- Adapted code to call event.detail(..) before event.error(..) - Adapted remaining OIDC endpoints - Reverted log levels to previous values in OAuth2GrantTypeBase Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
- Added Details.REASON to events - Removed info level logging in favor of event based logging - Changed info level logging in isValidPkceCodeVerifier method to debug The previously logged information in now contained in the error event. Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
42bf592 to
5d139d5
Compare
There was a problem hiding this comment.
Thank you - looks great. This will help users and admin a lot, especially when they first try out Keycloak.
Fixes #29166