This is an automated email from the ASF dual-hosted git repository. rombert pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git
commit 6f6061bf50a684199210203bbf01eca5513f994b Author: Robert Munteanu <[email protected]> AuthorDate: Wed Sep 18 17:41:12 2024 +0200 oidc-rp: remove id token persistence, that should never happen Additionally, make the OidcCallbackServlet use the token store API rather than requiring the JCR impl. --- .../sling/extensions/oidc_rp/OidcTokenStore.java | 2 - .../oidc_rp/impl/JcrUserHomeOidcTokenStore.java | 62 ++++------------------ .../oidc_rp/impl/OidcCallbackServlet.java | 12 +++-- .../servlets/oidc_rp/AuthorizationCodeFlowIT.java | 4 -- .../oidc_rp/impl/OidcConnectionFinderImplTest.java | 43 ++++----------- 5 files changed, 27 insertions(+), 96 deletions(-) diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/OidcTokenStore.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/OidcTokenStore.java index 603ddc52..b5a56297 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/OidcTokenStore.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/OidcTokenStore.java @@ -43,7 +43,5 @@ public interface OidcTokenStore { OidcToken getRefreshToken(OidcConnection connection, ResourceResolver resolver) throws OidcException; - OidcToken getIdToken(OidcConnection connection, ResourceResolver resolver) throws OidcException; - void persistTokens(OidcConnection connection, ResourceResolver resolver, OidcTokens tokens) throws OidcException; } diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/JcrUserHomeOidcTokenStore.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/JcrUserHomeOidcTokenStore.java index 70d59a8e..f4c578d3 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/JcrUserHomeOidcTokenStore.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/JcrUserHomeOidcTokenStore.java @@ -16,10 +16,9 @@ */ package org.apache.sling.extensions.oidc_rp.impl; -import java.time.LocalDateTime; +import java.time.Instant; import java.time.ZoneId; import java.time.ZonedDateTime; -import java.time.temporal.ChronoUnit; import java.util.Calendar; import java.util.GregorianCalendar; @@ -40,19 +39,13 @@ import org.osgi.service.component.annotations.Component; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.nimbusds.oauth2.sdk.token.BearerAccessToken; -import com.nimbusds.oauth2.sdk.token.RefreshToken; -import com.nimbusds.oauth2.sdk.token.Tokens; -import com.nimbusds.openid.connect.sdk.token.OIDCTokens; - -@Component(service = { OidcTokenStore.class, JcrUserHomeOidcTokenStore.class } ) +@Component(service = { OidcTokenStore.class } ) public class JcrUserHomeOidcTokenStore implements OidcTokenStore { // TODO - expires_at private static final String PROPERTY_NAME_EXPIRES_AT = "expiresAt"; private static final String PROPERTY_NAME_ACCESS_TOKEN = "access_token"; private static final String PROPERTY_NAME_REFRESH_TOKEN = "refresh_token"; - private static final String PROPERTY_NAME_ID_TOKEN = "id_token"; private final Logger logger = LoggerFactory.getLogger(getClass()); @@ -102,32 +95,18 @@ public class JcrUserHomeOidcTokenStore implements OidcTokenStore { } @Override - public OidcToken getIdToken(OidcConnection connection, ResourceResolver resolver) { - try { - User user = resolver.adaptTo(User.class); - - return getToken(connection, user, PROPERTY_NAME_ID_TOKEN); - } catch (RepositoryException e) { - throw new OidcException(e); - } - } - - public void persistTokens(OidcConnection connection, ResourceResolver resolver, OIDCTokens tokens) { - persistTokens0(connection, resolver, tokens); - } - - private void persistTokens0(OidcConnection connection, ResourceResolver resolver, Tokens tokens) { + public void persistTokens(OidcConnection connection, ResourceResolver resolver, OidcTokens tokens) { try { User currentUser = resolver.adaptTo(User.class); Session session = resolver.adaptTo(Session.class); ZonedDateTime expiry = null; - long expiresIn = tokens.getAccessToken().getLifetime(); - if ( expiresIn > 0 ) { - expiry = LocalDateTime.now().plus(expiresIn, ChronoUnit.SECONDS).atZone(ZoneId.systemDefault()); + long expiresAt = tokens.expiresAt(); + if ( expiresAt > 0 ) { + expiry = ZonedDateTime.ofInstant(Instant.ofEpochMilli(expiresAt), ZoneId.systemDefault()); } - String accessToken = tokens.getAccessToken().getValue(); + String accessToken = tokens.accessToken(); currentUser.setProperty(propertyPath(connection, PROPERTY_NAME_ACCESS_TOKEN), session.getValueFactory().createValue(accessToken)); if ( expiry != null ) { Calendar cal = GregorianCalendar.from(expiry); @@ -135,42 +114,19 @@ public class JcrUserHomeOidcTokenStore implements OidcTokenStore { } else currentUser.removeProperty(propertyPath(connection, PROPERTY_NAME_EXPIRES_AT)); - if ( tokens.getRefreshToken() != null ) { - String refreshToken = tokens.getRefreshToken().getValue(); + if ( tokens.refreshToken() != null ) { + String refreshToken = tokens.refreshToken(); if ( refreshToken != null ) currentUser.setProperty(propertyPath(connection, PROPERTY_NAME_REFRESH_TOKEN), session.getValueFactory().createValue(refreshToken)); else currentUser.removeProperty(propertyPath(connection, PROPERTY_NAME_REFRESH_TOKEN)); } - if ( tokens instanceof OIDCTokens oidcTokens) { - // don't touch the id token if we don't have an OIDC token, e.g. when refreshing the access token - String idToken = oidcTokens.getIDTokenString(); - if ( idToken != null ) - currentUser.setProperty(propertyPath(connection, PROPERTY_NAME_ID_TOKEN), session.getValueFactory().createValue(idToken)); - else - currentUser.removeProperty(propertyPath(connection, PROPERTY_NAME_ID_TOKEN)); - } - session.save(); } catch (RepositoryException e) { throw new OidcException(e); } } - - @Override - public void persistTokens(OidcConnection connection, ResourceResolver resolver, OidcTokens tokenPair) { - OIDCTokens nimbusTokens; - RefreshToken nimbusRefreshToken = new RefreshToken(tokenPair.refreshToken()); - BearerAccessToken nimbusAccessToken = new BearerAccessToken(tokenPair.accessToken(), tokenPair.expiresAt(), null); - if ( tokenPair.idToken() != null ) { - nimbusTokens = new OIDCTokens(tokenPair.idToken(), nimbusAccessToken, nimbusRefreshToken); - } else { - nimbusTokens = new OIDCTokens(nimbusAccessToken, nimbusRefreshToken); - } - - persistTokens(connection, resolver, nimbusTokens); - } private String propertyPath(OidcConnection connection, String propertyName) { return nodePath(connection) + "/" + propertyName; diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/OidcCallbackServlet.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/OidcCallbackServlet.java index e1635ae3..565058ce 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/OidcCallbackServlet.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oidc_rp/impl/OidcCallbackServlet.java @@ -39,6 +39,8 @@ import org.apache.sling.api.SlingHttpServletResponse; import org.apache.sling.api.servlets.SlingAllMethodsServlet; import org.apache.sling.auth.core.AuthConstants; import org.apache.sling.extensions.oidc_rp.OidcConnection; +import org.apache.sling.extensions.oidc_rp.OidcTokenStore; +import org.apache.sling.extensions.oidc_rp.OidcTokens; import org.apache.sling.servlets.annotations.SlingServletPaths; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; @@ -72,7 +74,7 @@ public class OidcCallbackServlet extends SlingAllMethodsServlet { private final Logger logger = LoggerFactory.getLogger(getClass()); private final Map<String, OidcConnection> connections; - private final JcrUserHomeOidcTokenStore tokenStore; + private final OidcTokenStore tokenStore; private final OidcProviderMetadataRegistry metadataRegistry; static String getCallbackUri(HttpServletRequest request) { @@ -86,7 +88,7 @@ public class OidcCallbackServlet extends SlingAllMethodsServlet { } @Activate - public OidcCallbackServlet(@Reference(policyOption = GREEDY) List<OidcConnection> connections, @Reference JcrUserHomeOidcTokenStore tokenStore, + public OidcCallbackServlet(@Reference(policyOption = GREEDY) List<OidcConnection> connections, @Reference OidcTokenStore tokenStore, @Reference OidcProviderMetadataRegistry metadataRegistry) { this.connections = connections.stream() .collect(Collectors.toMap( OidcConnection::name, Function.identity())); @@ -159,8 +161,10 @@ public class OidcCallbackServlet extends SlingAllMethodsServlet { // - does the 'aud' claim match the client id of our connection // - nonce validation (?) // - iat/exp validation (?) - - tokenStore.persistTokens(connection, request.getResourceResolver(), tokenResponse.getOIDCTokens()); + + OidcTokens tokens = Converter.toApiOidcTokens(tokenResponse.getOIDCTokens()); + + tokenStore.persistTokens(connection, request.getResourceResolver(), tokens); if ( redirect.isEmpty() ) { response.setStatus(HttpServletResponse.SC_OK); diff --git a/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/AuthorizationCodeFlowIT.java b/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/AuthorizationCodeFlowIT.java index c3543319..8396215d 100644 --- a/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/AuthorizationCodeFlowIT.java +++ b/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/AuthorizationCodeFlowIT.java @@ -182,10 +182,6 @@ class AuthorizationCodeFlowIT { // validate that the JWT is valid; we trust what keycloak has returned but just want to ensure that // the token was stored correctly SignedJWT.parse(accesToken); - - // in a similar manner, validate the id_token - String idToken = keycloakToken.get("id_token").asText(); - SignedJWT.parse(idToken); } private String getUserPath(SlingClient sling, String authorizableId) throws ClientException { diff --git a/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionFinderImplTest.java b/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionFinderImplTest.java index 5d26a5e7..54de7f7c 100644 --- a/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionFinderImplTest.java +++ b/org.apache.sling.servlets.oidc-rp/src/test/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionFinderImplTest.java @@ -23,12 +23,12 @@ import java.util.concurrent.TimeUnit; import javax.jcr.RepositoryException; import org.apache.jackrabbit.api.security.user.User; -import org.apache.sling.api.resource.LoginException; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ValueMap; import org.apache.sling.extensions.oidc_rp.OidcConnection; import org.apache.sling.extensions.oidc_rp.OidcToken; import org.apache.sling.extensions.oidc_rp.OidcTokenState; +import org.apache.sling.extensions.oidc_rp.impl.Converter; import org.apache.sling.extensions.oidc_rp.impl.JcrUserHomeOidcTokenStore; import org.apache.sling.jackrabbit.usermanager.impl.AuthorizableAdapterFactory; import org.apache.sling.testing.mock.sling.ResourceResolverType; @@ -56,12 +56,12 @@ class OidcConnectionFinderImplTest { } @Test - void persistTokens_accessTokenOnly() throws LoginException, RepositoryException { + void persistTokens_accessTokenOnly() throws RepositoryException { OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12), null); JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); - connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); + connectionFinder.persistTokens(connection, context.resourceResolver(), Converter.toApiOidcTokens(tokens)); Resource connectionResource = getConnectionResource(connection); @@ -73,19 +73,20 @@ class OidcConnectionFinderImplTest { } @Test - void persistTokens_accessAndIdToken() throws LoginException, RepositoryException { + void persistTokens_accessAndIdToken() throws RepositoryException { OIDCTokens tokens = new OIDCTokens(new PlainJWT(new JWTClaimsSet.Builder().issuer("example.com").build()), new BearerAccessToken(12), null); JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); - connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); + connectionFinder.persistTokens(connection, context.resourceResolver(), Converter.toApiOidcTokens(tokens)); Resource connectionResource = getConnectionResource(connection); + // we explicitly do not store the id token, that is only useful if we want an authentication handler ValueMap connectionProps = connectionResource.getValueMap(); assertThat(connectionProps) .as("stored tokens for connection") - .containsOnlyKeys("jcr:primaryType", "access_token", "id_token") + .containsOnlyKeys("jcr:primaryType", "access_token") .containsEntry("access_token", tokens.getAccessToken().getValue()); } @@ -109,7 +110,7 @@ class OidcConnectionFinderImplTest { OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12), null); JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); - connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); + connectionFinder.persistTokens(connection, context.resourceResolver(), Converter.toApiOidcTokens(tokens)); OidcToken accessToken = connectionFinder.getAccessToken(connection, context.resourceResolver()); assertThat(accessToken).as("access token") @@ -125,7 +126,7 @@ class OidcConnectionFinderImplTest { OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12, lifetimeSeconds, null), null); JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); - connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); + connectionFinder.persistTokens(connection, context.resourceResolver(), Converter.toApiOidcTokens(tokens)); // wait for the token to expire Thread.sleep( TimeUnit.SECONDS.toMillis( 2 * lifetimeSeconds ) ); @@ -142,7 +143,7 @@ class OidcConnectionFinderImplTest { OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12), new RefreshToken(12)); JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); - connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); + connectionFinder.persistTokens(connection, context.resourceResolver(), Converter.toApiOidcTokens(tokens)); OidcToken refreshToken = connectionFinder.getRefreshToken(connection, context.resourceResolver()); assertThat(refreshToken).as("refresh token") @@ -163,30 +164,6 @@ class OidcConnectionFinderImplTest { .isEqualTo( OidcTokenState.MISSING); } - @Test - void getIdToken_missing() { - JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); - - OidcToken refreshToken = connectionFinder.getIdToken(connection, context.resourceResolver()); - assertThat(refreshToken).as("id token") - .isNotNull() - .extracting( OidcToken::getState ) - .isEqualTo( OidcTokenState.MISSING); - } - - @Test - void getIdToken_valid() { - OIDCTokens tokens = new OIDCTokens(new PlainJWT(new JWTClaimsSet.Builder().issuer("example.com").build()), new BearerAccessToken(12), null); - JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); - connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); - - OidcToken refreshToken = connectionFinder.getIdToken(connection, context.resourceResolver()); - assertThat(refreshToken).as("id token") - .isNotNull() - .extracting( OidcToken::getState, OidcToken::getValue ) - .containsExactly( OidcTokenState.VALID, tokens.getIDTokenString() ); - } - private Resource getConnectionResource(OidcConnection connection) throws RepositoryException { String userPath = context.resourceResolver().adaptTo(User.class).getPath(); Resource userHomeResource = context.resourceResolver().getResource(userPath);
