From dd37e02140535ce6e2f5e87ca6715ded0a8b46ea Mon Sep 17 00:00:00 2001
From: Daniel Fesenmeyer <daniel.fesenmeyer@bosch.com>
Date: Wed, 4 Oct 2023 14:23:28 +0200
Subject: [PATCH] Improve logging in case of OIDC Identity provider errors:

- log the full Redirection URL, when it contains an error parameter, or does not contain the state or code parameter
- log the token endpoint URL (without - possibly confidential - params) and the response body, when the token endpoint does not return a success response

Closes #23690
---
 .../broker/provider/util/SimpleHttp.java      |  7 ++
 .../oidc/AbstractOAuth2IdentityProvider.java  | 65 +++++++++++++------
 .../keycloak/services/messages/Messages.java  |  2 +
 .../login/messages/messages_en.properties     |  1 +
 4 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/util/SimpleHttp.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/util/SimpleHttp.java
index fe743fef203..d40274c159f 100755
--- a/server-spi-private/src/main/java/org/keycloak/broker/provider/util/SimpleHttp.java
+++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/util/SimpleHttp.java
@@ -248,6 +248,13 @@ public class SimpleHttp {
         }
     }
 
+    /**
+     * @return the URL without params
+     */
+    public String getUrl() {
+        return url;
+    }
+
     private Response makeRequest() throws IOException {
 
         HttpRequestBase httpRequest = createHttpRequest();
diff --git a/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java b/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java
index 1fb4d684dfe..72242391d48 100755
--- a/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java
+++ b/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java
@@ -479,7 +479,10 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
         public Response authResponse(@QueryParam(AbstractOAuth2IdentityProvider.OAUTH2_PARAMETER_STATE) String state,
                                      @QueryParam(AbstractOAuth2IdentityProvider.OAUTH2_PARAMETER_CODE) String authorizationCode,
                                      @QueryParam(OAuth2Constants.ERROR) String error) {
+            OAuth2IdentityProviderConfig providerConfig = provider.getConfig();
+            
             if (state == null) {
+                logErroneousRedirectUrlError("Redirection URL does not contain a state parameter", providerConfig);
                 return errorIdentityProviderLogin(Messages.IDENTITY_PROVIDER_MISSING_STATE_ERROR);
             }
 
@@ -487,10 +490,8 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
                 AuthenticationSessionModel authSession = this.callback.getAndVerifyAuthenticationSession(state);
                 session.getContext().setAuthenticationSession(authSession);
 
-                OAuth2IdentityProviderConfig providerConfig = provider.getConfig();
-
                 if (error != null) {
-                    logger.error(error + " for broker login " + providerConfig.getProviderId());
+                    logErroneousRedirectUrlError("Redirection URL contains an error", providerConfig);
                     if (error.equals(ACCESS_DENIED)) {
                         return callback.cancelled(providerConfig);
                     } else if (error.equals(OAuthErrorException.LOGIN_REQUIRED) || error.equals(OAuthErrorException.INTERACTION_REQUIRED)) {
@@ -500,23 +501,39 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
                     }
                 }
 
-                if (authorizationCode != null) {
-                    String response = generateTokenRequest(authorizationCode).asString();
-
-                    BrokeredIdentityContext federatedIdentity = provider.getFederatedIdentity(response);
-
-                    if (providerConfig.isStoreToken()) {
-                        // make sure that token wasn't already set by getFederatedIdentity();
-                        // want to be able to allow provider to set the token itself.
-                        if (federatedIdentity.getToken() == null)federatedIdentity.setToken(response);
-                    }
-
-                    federatedIdentity.setIdpConfig(providerConfig);
-                    federatedIdentity.setIdp(provider);
-                    federatedIdentity.setAuthenticationSession(authSession);
-
-                    return callback.authenticated(federatedIdentity);
+                if (authorizationCode == null) {
+                    logErroneousRedirectUrlError("Redirection URL neither contains a code nor error parameter",
+                            providerConfig);
+                    return errorIdentityProviderLogin(Messages.IDENTITY_PROVIDER_MISSING_CODE_OR_ERROR_ERROR);
                 }
+
+                SimpleHttp simpleHttp = generateTokenRequest(authorizationCode);
+                String response;
+                try (SimpleHttp.Response simpleResponse = simpleHttp.asResponse()) {
+                    int status = simpleResponse.getStatus();
+                    boolean success = status >= 200 && status < 400;
+                    response = simpleResponse.asString();
+
+                    if (!success) {
+                        logger.errorf("Unexpected response from token endpoint %s. status=%s, response=%s",
+                                simpleHttp.getUrl(), status, response);
+                        return errorIdentityProviderLogin(Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR);
+                    }
+                }
+
+                BrokeredIdentityContext federatedIdentity = provider.getFederatedIdentity(response);
+
+                if (providerConfig.isStoreToken()) {
+                    // make sure that token wasn't already set by getFederatedIdentity();
+                    // want to be able to allow provider to set the token itself.
+                    if (federatedIdentity.getToken() == null)federatedIdentity.setToken(response);
+                }
+
+                federatedIdentity.setIdpConfig(providerConfig);
+                federatedIdentity.setIdp(provider);
+                federatedIdentity.setAuthenticationSession(authSession);
+
+                return callback.authenticated(federatedIdentity);
             } catch (WebApplicationException e) {
                 return e.getResponse();
             } catch (IdentityBrokerException e) {
@@ -524,10 +541,18 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
                     return errorIdentityProviderLogin(e.getMessageCode());
                 }
                 logger.error("Failed to make identity provider oauth callback", e);
+                return errorIdentityProviderLogin(Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR);
             } catch (Exception e) {
                 logger.error("Failed to make identity provider oauth callback", e);
+                return errorIdentityProviderLogin(Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR);
             }
-            return errorIdentityProviderLogin(Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR);
+        }
+
+        private void logErroneousRedirectUrlError(String mainMessage, OAuth2IdentityProviderConfig providerConfig) {
+            String providerId = providerConfig.getProviderId();
+            String redirectionUrl = session.getContext().getUri().getRequestUri().toString();
+
+            logger.errorf("%s. providerId=%s, redirectionUrl=%s", mainMessage, providerId, redirectionUrl);
         }
 
         private Response errorIdentityProviderLogin(String message) {
diff --git a/services/src/main/java/org/keycloak/services/messages/Messages.java b/services/src/main/java/org/keycloak/services/messages/Messages.java
index 687cb0c3e40..41b68c78d20 100755
--- a/services/src/main/java/org/keycloak/services/messages/Messages.java
+++ b/services/src/main/java/org/keycloak/services/messages/Messages.java
@@ -185,6 +185,8 @@ public class Messages {
 
     public static final String IDENTITY_PROVIDER_MISSING_STATE_ERROR = "identityProviderMissingStateMessage";
 
+    public static final String IDENTITY_PROVIDER_MISSING_CODE_OR_ERROR_ERROR = "identityProviderMissingCodeOrErrorMessage";
+
     public static final String IDENTITY_PROVIDER_INVALID_RESPONSE = "identityProviderInvalidResponseMessage";
 
     public static final String IDENTITY_PROVIDER_INVALID_SIGNATURE = "identityProviderInvalidSignatureMessage";
diff --git a/themes/src/main/resources/theme/base/login/messages/messages_en.properties b/themes/src/main/resources/theme/base/login/messages/messages_en.properties
index 937e1a0d9fb..91262ea18ad 100755
--- a/themes/src/main/resources/theme/base/login/messages/messages_en.properties
+++ b/themes/src/main/resources/theme/base/login/messages/messages_en.properties
@@ -343,6 +343,7 @@ cookieNotFoundMessage=Cookie not found. Please make sure cookies are enabled in
 insufficientLevelOfAuthentication=The requested level of authentication has not been satisfied.
 identityProviderUnexpectedErrorMessage=Unexpected error when authenticating with identity provider
 identityProviderMissingStateMessage=Missing state parameter in response from identity provider.
+identityProviderMissingCodeOrErrorMessage=Missing code or error parameter in response from identity provider.
 identityProviderInvalidResponseMessage=Invalid response from identity provider.
 identityProviderInvalidSignatureMessage=Invalid signature in response from identity provider.
 identityProviderNotFoundMessage=Could not find an identity provider with the identifier.