Revise Share token exchange handling

- retry if refresh of exchanged token yields invalid token (wrong
  audience - known case of apparently incorrect Keycloak behaviour)
- use custom header instead of redirect patch to have Repository tier not
  redirect to Keycloak login page on unauthenticated access from Share
- activate audience verification which is inactive with Keycloak class
  defaults

repository/src/main/globalConfig/subsystems/Authentication/keycloak/keycloak-authentication-context.xml

@@ -133,6 +133,7 @@
         <property name="handlePublicApi" value="${keycloak.authentication.sso.handlePublicApi}" />
         <property name="loginPageUrl" value="${keycloak.authentication.loginPageUrl}" />
         <property name="originalRequestUrlHeaderName" value="${keycloak.authentication.sso.originalRequestUrlHeaderName}" />
+        <property name="noKeycloakHandlingHeaderName" value="x-${moduleId}-no-keycloak-handling" />
         <property name="bodyBufferLimit" value="${keycloak.authentication.bodyBufferLimit}" />
         <property name="sslRedirectPort" value="${keycloak.authentication.sslRedirectPort}" />
         <property name="keycloakDeployment" ref="keycloakDeployment" />

repository/src/main/globalConfig/subsystems/Authentication/keycloak/keycloak-authentication.properties

@@ -19,7 +19,7 @@ keycloak.authentication.sslRedirectPort=8443
 keycloak.authentication.bodyBufferLimit=10485760
 
 # override for a direct route to the auth server host
-# useful primarily for Dockerized deployments where container running Alfresco cannot resolve the auth server via the public DNS name
+# useful primarily for Docker-ized deployments where container running Alfresco cannot resolve the auth server via the public DNS name
 keycloak.authentication.directAuthHost=
 
 keycloak.adapter.auth-server-url=http://localhost:8180/auth
@@ -29,6 +29,8 @@ keycloak.adapter.ssl-required=none
 keycloak.adapter.public-client=false
 keycloak.adapter.credentials.provider=secret
 keycloak.adapter.credentials.secret=
+# for some reason, this is not a sane default in Keycloak Adapter config
+keycloak.adapter.verify-token-audience=true
 
 # TODO default settings (identical to AdapterConfig defaults) to better align with default Alfresco subsystem property handling
 

repository/src/main/java/de/acosix/alfresco/keycloak/repo/authentication/KeycloakAuthenticationFilter.java

@@ -116,6 +116,8 @@ public class KeycloakAuthenticationFilter extends BaseAuthenticationFilter
 
     protected String originalRequestUrlHeaderName;
 
+    protected String noKeycloakHandlingHeaderName;
+
     protected int bodyBufferLimit = DEFAULT_BODY_BUFFER_LIMIT;
 
     // use 8443 as default SSL redirect based on Tomcat default server.xml configuration
@@ -146,6 +148,8 @@ public class KeycloakAuthenticationFilter extends BaseAuthenticationFilter
         PropertyCheck.mandatory(this, "keycloakTicketTokenCache", this.keycloakTicketTokenCache);
         PropertyCheck.mandatory(this, "publicApiRuntimeContainer", this.publicApiRuntimeContainer);
 
+        PropertyCheck.mandatory(this, "noKeycloakHandlingHeaderName", this.noKeycloakHandlingHeaderName);
+
         // parent class does not check, so we do
         PropertyCheck.mandatory(this, "authenticationService", this.authenticationService);
         PropertyCheck.mandatory(this, "authenticationComponent", this.authenticationComponent);
@@ -222,6 +226,15 @@ public class KeycloakAuthenticationFilter extends BaseAuthenticationFilter
         this.originalRequestUrlHeaderName = originalRequestUrlHeaderName;
     }
 
+    /**
+     * @param noKeycloakHandlingHeaderName
+     *            the noKeycloakHandlingHeaderName to set
+     */
+    public void setNoKeycloakHandlingHeaderName(final String noKeycloakHandlingHeaderName)
+    {
+        this.noKeycloakHandlingHeaderName = noKeycloakHandlingHeaderName;
+    }
+
     /**
      * @param bodyBufferLimit
      *            the bodyBufferLimit to set
@@ -710,6 +723,7 @@ public class KeycloakAuthenticationFilter extends BaseAuthenticationFilter
         boolean skip = false;
 
         final String authHeader = req.getHeader(HEADER_AUTHORIZATION);
+        final String noKeycloakLoginRedirectHeader = req.getHeader(this.noKeycloakHandlingHeaderName);
 
         final String servletPath = req.getServletPath();
         final String pathInfo = req.getPathInfo();
@@ -765,8 +779,7 @@ public class KeycloakAuthenticationFilter extends BaseAuthenticationFilter
 
                     // cannot rely on session.isNew() to determine if this is a fresh login
                     // consider "fresh" login if issued within age limit (implicitly include any token refreshes performed client-side)
-                    final boolean isFreshLogin = accessToken.getIssuedAt()
-                            * 1000l > (System.currentTimeMillis() - FRESH_TOKEN_AGE_LIMIT_MS);
+                    final boolean isFreshLogin = accessToken.getIat() * 1000l > (System.currentTimeMillis() - FRESH_TOKEN_AGE_LIMIT_MS);
                     this.keycloakAuthenticationComponent.handleUserTokens(accessToken, accessToken, isFreshLogin);
 
                     // sessionUser should be guaranteed here, but still check - we need it for the cache key
@@ -857,9 +870,9 @@ public class KeycloakAuthenticationFilter extends BaseAuthenticationFilter
                                 "Skipping processKeycloakAuthenticationAndActions as request is aimed at a Public v1 ReST API which does not require authentication");
                         skip = true;
                     }
-                    // check no-auth flag (derived e.g. from checking if target web script requires authentication) only after all
-                    // pre-emptive auth
-                    // request details have been checked
+
+                    // check no-auth flag (derived e.g. from checking if target web script requires authentication) as last resort to see if
+                    // we need to force authentication after invalidating session
                     else if (Boolean.TRUE.equals(req.getAttribute(NO_AUTH_REQUIRED)))
                     {
                         LOGGER.trace(
@@ -895,6 +908,13 @@ public class KeycloakAuthenticationFilter extends BaseAuthenticationFilter
                     "Skipping processKeycloakAuthenticationAndActions as filter higher up in chain determined authentication as not required");
             skip = true;
         }
+        else if (Boolean.parseBoolean(noKeycloakLoginRedirectHeader))
+        {
+            LOGGER.trace(
+                    "Skipping processKeycloakAuthenticationAndActions as client provided custom 'no Keycloak handling' header {} with value that resolves to 'true'",
+                    this.noKeycloakHandlingHeaderName);
+            skip = true;
+        }
         // TODO Check for login page URL (rarely configured since Repository by default has no login page since 5.0)
 
         return skip;

share/src/main/config/default-config.xml

@@ -50,6 +50,8 @@
             <credentials>
                 <provider>secret</provider>
             </credentials>
+            <!-- for some reason, this is not a sane default in Keycloak Adapter config -->
+            <verify-token-audience>true</verify-token-audience>
         </keycloak-adapter-config>
     </config>
     

share/src/main/config/module-context.xml

@@ -47,12 +47,17 @@
         </property>
     </bean>
 
-    <bean id="${moduleId}.maxRemoteClientRedirectPatch"
+    <bean id="${moduleId}.remoteClientNoKeycloakLoginRedirectHeaderPatch"
         class="de.acosix.alfresco.utility.common.spring.PropertyAlteringBeanFactoryPostProcessor">
         <property name="targetBeanName" value="connector.remoteclient.abstract" />
-        <property name="propertyName" value="maxRedirects" />
-        <property name="value" value="1" />
-        <property name="enabled" value="\${${moduleId}.surf.onlyOneRedirect.enabled}" />
+        <property name="propertyName" value="requestHeaders" />
+        <property name="valueMap">
+            <map>
+                <entry key="x-${moduleId}-no-keycloak-handling" value="true" />
+            </map>
+        </property>
+        <property name="enabled" value="true" />
+        <property name="merge" value="true" />
     </bean>
 
     <bean id="${moduleId}.SessionIdMapper" class="${project.artifactId}.web.DefaultSessionIdMapper">

share/src/main/config/share-global.properties

@@ -1 +1 @@
-${moduleId}.surf.onlyOneRedirect.enabled=true
+# no configuration properties necessary - empty file to mark this as a Share module 

share/src/main/java/de/acosix/alfresco/keycloak/share/remote/AccessTokenAwareSlingshotAlfrescoConnector.java

@@ -71,6 +71,7 @@ public class AccessTokenAwareSlingshotAlfrescoConnector extends SlingshotAlfresc
         final RefreshableAccessTokenHolder endpointSpecificAccessToken = (RefreshableAccessTokenHolder) (session != null
                 ? session.getAttribute(KeycloakAuthenticationFilter.BACKEND_ACCESS_TOKEN_SESSION_KEY)
                 : null);
+
         if (endpointSpecificAccessToken != null)
         {
             if (endpointSpecificAccessToken.isActive())

share/src/main/java/de/acosix/alfresco/keycloak/share/web/KeycloakAuthenticationFilter.java

@@ -83,6 +83,7 @@ import de.acosix.alfresco.keycloak.share.config.KeycloakAuthenticationConfigElem
 import de.acosix.alfresco.keycloak.share.config.KeycloakConfigConstants;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.KeycloakSecurityContext;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.OAuth2Constants;
+import de.acosix.alfresco.keycloak.share.deps.keycloak.TokenVerifier;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.AdapterDeploymentContext;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.AuthenticatedActionsHandler;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.BearerTokenRequestAuthenticator;
@@ -95,6 +96,7 @@ import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.PreAuthActionsHa
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.ServerRequest;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.authentication.ClientCredentialsProviderUtils;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.rotation.AdapterTokenVerifier;
+import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.rotation.AdapterTokenVerifier.VerifiedTokens;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.servlet.FilterRequestAuthenticator;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.servlet.OIDCFilterSessionStore;
 import de.acosix.alfresco.keycloak.share.deps.keycloak.adapters.servlet.OIDCServletHttpFacade;
@@ -832,7 +834,7 @@ public class KeycloakAuthenticationFilter implements DependencyInjectedFilter, I
             session.setAttribute(UserFactory.SESSION_ATTRIBUTE_EXTERNAL_AUTH, Boolean.TRUE);
             session.setAttribute(UserFactory.SESSION_ATTRIBUTE_KEY_USER_ID, userId);
 
-            this.handleAlfrescoResourceAccessToken(session);
+            this.handleAlfrescoResourceAccessToken(session, false);
         }
 
         if (facade.isEnded())
@@ -937,13 +939,17 @@ public class KeycloakAuthenticationFilter implements DependencyInjectedFilter, I
     protected void onKeycloakAuthenticationFailure(final ServletContext context, final HttpServletRequest req,
             final HttpServletResponse res, final FilterChain chain) throws IOException, ServletException
     {
-        LOGGER.warn("Keycloak authentication failed due to {}", req.getAttribute(AuthenticationError.class.getName()));
+        final Object authError = req.getAttribute(AuthenticationError.class.getName());
+        LOGGER.warn("Keycloak authentication failed due to {}",
+                authError != null ? authError : "<missing AuthenticationError details in request context>");
         LOGGER.debug("Resetting session and state cookie before continueing with filter chain");
 
         req.getSession().invalidate();
 
         this.resetStateCookies(context, req, res);
 
+        // TODO If error occurred as part of redirect back from Keycloak, strip state / code params from URL query
+        // TODO If login page may follow, see about providing error message
         this.continueFilterChain(context, req, res, chain);
     }
 
@@ -1091,9 +1097,11 @@ public class KeycloakAuthenticationFilter implements DependencyInjectedFilter, I
             }
             else
             {
-                // TODO Validate via custom /touch to check if session is still valid
-                // custom => handle potential 302 instead of 401 response from Keycloak-enabled backend
-                // custom => deal with redirect host being unknown (similar to our auth-server-url vs. directAuthHost case)
+                /*
+                 * Note: We could validate session with a custom call to /touch but we leave that to any remaining SSO filters. We patch
+                 * remoteClient to submit a custom HTTP header to backend to avoid 302 redirects to Keycloak which other SSO filters cannot
+                 * handle, and this also avoids any issues with (public) Keycloak auth server URL being unknown, e.g. in a Docker scenario
+                 */
                 LOGGER.debug(
                         "Skipping processKeycloakAuthenticationAndActions as non-Keycloak-authenticated session is already established");
                 skip = true;
@@ -1174,7 +1182,7 @@ public class KeycloakAuthenticationFilter implements DependencyInjectedFilter, I
         if (currentSession != null)
         {
             LOGGER.debug("Skipping processKeycloakAuthenticationAndActions as Keycloak-authentication session is still valid");
-            this.handleAlfrescoResourceAccessToken(currentSession);
+            this.handleAlfrescoResourceAccessToken(currentSession, false);
             skip = true;
         }
         else
@@ -1333,8 +1341,11 @@ public class KeycloakAuthenticationFilter implements DependencyInjectedFilter, I
      *
      * @param session
      *            the active session managing any persistent access token state
+     * @param retry
+     *            {@code true} if the invocation of the operation is a retry as the result of a (hopefully temporary) verification failure
+     *            in the current thread
      */
-    protected void handleAlfrescoResourceAccessToken(final HttpSession session)
+    protected void handleAlfrescoResourceAccessToken(final HttpSession session, final boolean retry)
     {
         final KeycloakAuthenticationConfigElement keycloakAuthConfig = (KeycloakAuthenticationConfigElement) this.configService
                 .getConfig(KeycloakConfigConstants.KEYCLOAK_CONFIG_SECTION_NAME).getConfigElement(KeycloakAuthenticationConfigElement.NAME);
@@ -1390,28 +1401,44 @@ public class KeycloakAuthenticationFilter implements DependencyInjectedFilter, I
                     }
 
                     final String tokenString = response.getToken();
-                    final AdapterTokenVerifier.VerifiedTokens tokens;
                     try
                     {
-                        tokens = AdapterTokenVerifier.verifyTokens(tokenString, response.getIdToken(), this.keycloakDeployment);
+                        // cannot use simple AdapterTokenVerifier.verifyTokens as it checks for wrong audience
+                        // we also do not care about any IDToken retrieved (implicitly) with token exchange
+                        final TokenVerifier<AccessToken> tokenVerifier = AdapterTokenVerifier.createVerifier(tokenString,
+                                this.keycloakDeployment, true, AccessToken.class);
+                        tokenVerifier.audience(alfrescoResourceName);
+                        tokenVerifier.issuedFor(this.keycloakDeployment.getResourceName());
+
+                        final AccessToken accessToken = tokenVerifier.verify().getToken();
+
+                        if ((accessToken.getExp() - this.keycloakDeployment.getTokenMinimumTimeToLive()) <= Time.currentTime())
+                        {
+                            throw new AlfrescoRuntimeException(
+                                    "Failed to retrieve / refresh the access token for the Alfresco backend with a longer time-to-live than the minimum");
+                        }
+
+                        token = new RefreshableAccessTokenHolder(response, new VerifiedTokens(accessToken, null));
+                        session.setAttribute(BACKEND_ACCESS_TOKEN_SESSION_KEY, token);
+                        LOGGER.debug("Successfully retrieved / refresh access token for Alfresco backend");
                     }
                     catch (final VerificationException vex)
                     {
-                        LOGGER.error("Verification of access token for Alfresco backend failed", vex);
-                        throw new AlfrescoRuntimeException("Failed to verify access token for Alfresco backend", vex);
+                        session.removeAttribute(BACKEND_ACCESS_TOKEN_SESSION_KEY);
+                        if (!retry && token != null && token.canRefresh())
+                        {
+                            LOGGER.warn(
+                                    "Verification of refreshed access token for Alfresco backend failed - removed previous token from the session before retrying token exchange from scratch");
+
+                            this.handleAlfrescoResourceAccessToken(session, true);
+                        }
+                        else
+                        {
+                            LOGGER.error("Verification of access token for Alfresco backend failed in retry", vex);
+                            throw new AlfrescoRuntimeException("Keycloak token exchange for access to backend yielded invalid access token",
+                                    vex);
+                        }
                     }
-
-                    final AccessToken accessToken = tokens.getAccessToken();
-
-                    if ((accessToken.getExpiration() - this.keycloakDeployment.getTokenMinimumTimeToLive()) <= Time.currentTime())
-                    {
-                        throw new AlfrescoRuntimeException(
-                                "Failed to retrieve / refresh the access token for the Alfresco backend with a longer time-to-live than the minimum");
-                    }
-
-                    token = new RefreshableAccessTokenHolder(response, tokens);
-                    session.setAttribute(BACKEND_ACCESS_TOKEN_SESSION_KEY, token);
-                    LOGGER.debug("Successfully retrieved / refresh access token for Alfresco backend");
                 }
             }
             else if (alfrescoResourceName == null)
@@ -1455,8 +1482,21 @@ public class KeycloakAuthenticationFilter implements DependencyInjectedFilter, I
         formParams.add(new BasicNameValuePair(OAuth2Constants.REQUESTED_TOKEN_TYPE, OAuth2Constants.REFRESH_TOKEN_TYPE));
 
         final OidcKeycloakAccount keycloakAccount = (OidcKeycloakAccount) session.getAttribute(KEYCLOAK_ACCOUNT_SESSION_KEY);
-        final String tokenString = keycloakAccount.getKeycloakSecurityContext().getTokenString();
-        formParams.add(new BasicNameValuePair(OAuth2Constants.SUBJECT_TOKEN, tokenString));
+        final RefreshableAccessTokenHolder accessToken = (RefreshableAccessTokenHolder) session.getAttribute(ACCESS_TOKEN_SESSION_KEY);
+        if (keycloakAccount != null)
+        {
+            final String tokenString = keycloakAccount.getKeycloakSecurityContext().getTokenString();
+            formParams.add(new BasicNameValuePair(OAuth2Constants.SUBJECT_TOKEN, tokenString));
+        }
+        else if (accessToken != null && accessToken.isActive())
+        {
+            formParams.add(new BasicNameValuePair(OAuth2Constants.SUBJECT_TOKEN, accessToken.getToken()));
+        }
+        else
+        {
+            throw new IllegalStateException(
+                    "Either an active security context or access token should be present in the session, or previous validations have caught their non-existence and prevented this operation form being called");
+        }
 
         ClientCredentialsProviderUtils.setClientCredentials(this.keycloakDeployment, post, formParams);
 

share/src/main/java/de/acosix/alfresco/keycloak/share/web/UserGroupsLoadFilter.java

@@ -245,7 +245,9 @@ public class UserGroupsLoadFilter implements DependencyInjectedFilter, Initializ
             }
             else
             {
-                LOGGER.warn("Failed to load user groups for {} with backend call resulting in HTTP {} response and message {}", userId,
+                // TODO Specific handling for expectable error codes (401 / 302)
+                LOGGER.warn("Failed to load user groups for {} with backend call resulting in HTTP response with status {} {}",
+                        userId,
                         res.getStatus().getCode(), res.getStatus().getMessage());
                 userGroupsCSVList = "";
             }