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 3751f5531ce2ce47e85b22a8690795487b2794ba Author: Robert Munteanu <[email protected]> AuthorDate: Thu Jul 6 19:17:39 2023 +0300 oidc-rp: Rename the OidcConnectionFinder to OidcTokenStore and clarify some APIs/docs --- org.apache.sling.servlets.oidc-rp/README.md | 2 +- .../apache/sling/servlets/oidc_rp/OidcToken.java | 12 ++++++++++ ...dcConnectionFinder.java => OidcTokenStore.java} | 20 ++++++++-------- ...derImpl.java => JcrUserHomeOidcTokenStore.java} | 7 +++--- .../servlets/oidc_rp/impl/OidcCallbackServlet.java | 8 +++---- .../oidc_rp/impl/OidcConnectionPersister.java | 27 ---------------------- .../oidc_rp/impl/OidcConnectionFinderImplTest.java | 18 +++++++-------- 7 files changed, 39 insertions(+), 55 deletions(-) diff --git a/org.apache.sling.servlets.oidc-rp/README.md b/org.apache.sling.servlets.oidc-rp/README.md index 8b4cf0f7..2664b06d 100644 --- a/org.apache.sling.servlets.oidc-rp/README.md +++ b/org.apache.sling.servlets.oidc-rp/README.md @@ -12,8 +12,8 @@ objective is to simplify access to user and access tokens in a secure manner. ## Whiteboard graduation TODO - bundle/package should probably be org.apache.sling.extensions.oidc, as the primary entry point is the Java API -- clarify Java API and allow extracting both id and access tokens - allow use of refresh tokens +- extract the token exchange code from the OidcCallbackServlet and move it to the OauthClientImpl - document usage for the supported OIDC providers; make sure to explain this is _not_ an authentication handler - provide a sample content package and instructions how to use - review security best practices diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcToken.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcToken.java index f67361fb..203ed185 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcToken.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcToken.java @@ -16,6 +16,12 @@ */ package org.apache.sling.servlets.oidc_rp; +/** + * Information about an OIDC token + * + * <p>This class encapsulated the known information about and OIDC token. It allows the client to + * make decisions based on the possible states.</p> + */ public class OidcToken { private final OidcTokenState state; @@ -30,6 +36,12 @@ public class OidcToken { return state; } + /** + * Returns the token value + * + * @return the value, in case the {@link #getState() state} is {@code OidcTokenState#VALID}. + * @throws IllegalStateException in case the {@link #getState() state} is not {@code OidcTokenState#VALID}. + */ public String getValue() { if ( state != OidcTokenState.VALID ) throw new IllegalStateException("Can't retrieve a token value when the token state is " + state); diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcConnectionFinder.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcTokenStore.java similarity index 74% rename from org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcConnectionFinder.java rename to org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcTokenStore.java index 3c211b03..992e93b7 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcConnectionFinder.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/OidcTokenStore.java @@ -18,15 +18,6 @@ package org.apache.sling.servlets.oidc_rp; import org.apache.sling.api.resource.ResourceResolver; -// TODO - bad name -// thoughts on the API -// we have two main parts -// I. repository-based storage for tokens ( get access token / refresh token / user token ) -// II. interactions with the authorization and token endpoints ( build redirect URL for auth, use refresh token to get new access/refresh tokens , exchanging access codes for tokens ) -// These should be split into two client-facing interfaces, and the code from the Servlets pushed into II. -// It is definitely an unpleasant smell that the OidcConnnectionFinderImpl needs to reach out to the OidcClient. -// -// Further question - do we need a way of introspecting the OidcConnections? // // In terms of what typed objects we expose, there are a number of ways // @@ -37,7 +28,16 @@ import org.apache.sling.api.resource.ResourceResolver; // and ask consumers to parse them, while internally using the Nimbus SDK. This is the most // flexible, but also a bit wasteful // - (?) can we do without exposing the actual tokens? -public interface OidcConnectionFinder { + +/** + * Storage for OIDC Tokens + * + * <p>This service allows access to storing and retrieving OIDC tokens. It is the responsibility of the caller + * to ensure that the tokens are valid.</p> + * + * <p>For methods that return {@link OidcToken}, the state must be inspected before attempting to read the value.</p> + */ +public interface OidcTokenStore { OidcToken getAccessToken(OidcConnection connection, ResourceResolver resolver) throws OidcException; diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionFinderImpl.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/JcrUserHomeOidcTokenStore.java similarity index 97% rename from org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionFinderImpl.java rename to org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/JcrUserHomeOidcTokenStore.java index 12476652..15aff7aa 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionFinderImpl.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/JcrUserHomeOidcTokenStore.java @@ -31,10 +31,10 @@ import javax.jcr.Value; import org.apache.jackrabbit.api.security.user.User; import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.servlets.oidc_rp.OidcConnection; -import org.apache.sling.servlets.oidc_rp.OidcConnectionFinder; import org.apache.sling.servlets.oidc_rp.OidcException; import org.apache.sling.servlets.oidc_rp.OidcToken; import org.apache.sling.servlets.oidc_rp.OidcTokenState; +import org.apache.sling.servlets.oidc_rp.OidcTokenStore; import org.apache.sling.servlets.oidc_rp.OidcTokens; import org.osgi.service.component.annotations.Component; import org.slf4j.Logger; @@ -45,8 +45,8 @@ import com.nimbusds.oauth2.sdk.token.RefreshToken; import com.nimbusds.oauth2.sdk.token.Tokens; import com.nimbusds.openid.connect.sdk.token.OIDCTokens; -@Component -public class OidcConnectionFinderImpl implements OidcConnectionFinder, OidcConnectionPersister { +@Component(service = { OidcTokenStore.class, JcrUserHomeOidcTokenStore.class } ) +public class JcrUserHomeOidcTokenStore implements OidcTokenStore { // TODO - expires_at private static final String PROPERTY_NAME_EXPIRES_AT = "expiresAt"; @@ -112,7 +112,6 @@ public class OidcConnectionFinderImpl implements OidcConnectionFinder, OidcConne } } - @Override public void persistTokens(OidcConnection connection, ResourceResolver resolver, OIDCTokens tokens) { persistTokens0(connection, resolver, tokens); } diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcCallbackServlet.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcCallbackServlet.java index b6fb06e8..c186bc31 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcCallbackServlet.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcCallbackServlet.java @@ -72,7 +72,7 @@ public class OidcCallbackServlet extends SlingAllMethodsServlet { private final Logger logger = LoggerFactory.getLogger(getClass()); private final Map<String, OidcConnection> connections; - private final OidcConnectionPersister persister; + private final JcrUserHomeOidcTokenStore tokenStore; private final OidcProviderMetadataRegistry metadataRegistry; static String getCallbackUri(HttpServletRequest request) { @@ -80,12 +80,12 @@ public class OidcCallbackServlet extends SlingAllMethodsServlet { } @Activate - public OidcCallbackServlet(@Reference(policyOption = GREEDY) List<OidcConnection> connections, @Reference OidcConnectionPersister persister, + public OidcCallbackServlet(@Reference(policyOption = GREEDY) List<OidcConnection> connections, @Reference JcrUserHomeOidcTokenStore tokenStore, @Reference OidcProviderMetadataRegistry metadataRegistry) { this.connections = connections.stream() .collect(Collectors.toMap( OidcConnection::name, Function.identity())); this.metadataRegistry = metadataRegistry; - this.persister = persister; + this.tokenStore = tokenStore; } @Override @@ -154,7 +154,7 @@ public class OidcCallbackServlet extends SlingAllMethodsServlet { // - nonce validation (?) // - iat/exp validation (?) - persister.persistTokens(connection, request.getResourceResolver(), tokenResponse.getOIDCTokens()); + tokenStore.persistTokens(connection, request.getResourceResolver(), tokenResponse.getOIDCTokens()); if ( redirect.isEmpty() ) { response.setStatus(HttpServletResponse.SC_OK); diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionPersister.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionPersister.java deleted file mode 100644 index a3427ab6..00000000 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/servlets/oidc_rp/impl/OidcConnectionPersister.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.sling.servlets.oidc_rp.impl; - -import org.apache.sling.api.resource.ResourceResolver; -import org.apache.sling.servlets.oidc_rp.OidcConnection; - -import com.nimbusds.openid.connect.sdk.token.OIDCTokens; - -public interface OidcConnectionPersister { - - void persistTokens(OidcConnection connection, ResourceResolver resourceResolver, OIDCTokens tokens); -} 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 e3f93361..ecba6dce 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 @@ -59,7 +59,7 @@ class OidcConnectionFinderImplTest { OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12), null); - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); Resource connectionResource = getConnectionResource(connection); @@ -76,7 +76,7 @@ class OidcConnectionFinderImplTest { OIDCTokens tokens = new OIDCTokens(new PlainJWT(new JWTClaimsSet.Builder().issuer("example.com").build()), new BearerAccessToken(12), null); - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); Resource connectionResource = getConnectionResource(connection); @@ -92,7 +92,7 @@ class OidcConnectionFinderImplTest { @Test void getAccessToken_missing() { - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); OidcToken accessToken = connectionFinder.getAccessToken(connection, context.resourceResolver()); @@ -107,7 +107,7 @@ class OidcConnectionFinderImplTest { OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12), null); - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); OidcToken accessToken = connectionFinder.getAccessToken(connection, context.resourceResolver()); @@ -123,7 +123,7 @@ class OidcConnectionFinderImplTest { int lifetimeSeconds = 1; OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12, lifetimeSeconds, null), null); - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); // wait for the token to expire @@ -140,7 +140,7 @@ class OidcConnectionFinderImplTest { void getRefreshToken_valid() { OIDCTokens tokens = new OIDCTokens(new BearerAccessToken(12), new RefreshToken(12)); - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); OidcToken refreshToken = connectionFinder.getRefreshToken(connection, context.resourceResolver()); @@ -153,7 +153,7 @@ class OidcConnectionFinderImplTest { @Test void getRefreshToken_missing() { - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); OidcToken refreshToken = connectionFinder.getRefreshToken(connection, context.resourceResolver()); assertThat(refreshToken).as("refresh token") @@ -164,7 +164,7 @@ class OidcConnectionFinderImplTest { @Test void getIdToken_missing() { - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); OidcToken refreshToken = connectionFinder.getIdToken(connection, context.resourceResolver()); assertThat(refreshToken).as("id token") @@ -176,7 +176,7 @@ class OidcConnectionFinderImplTest { @Test void getIdToken_valid() { OIDCTokens tokens = new OIDCTokens(new PlainJWT(new JWTClaimsSet.Builder().issuer("example.com").build()), new BearerAccessToken(12), null); - OidcConnectionFinderImpl connectionFinder = new OidcConnectionFinderImpl(); + JcrUserHomeOidcTokenStore connectionFinder = new JcrUserHomeOidcTokenStore(); connectionFinder.persistTokens(connection, context.resourceResolver(), tokens); OidcToken refreshToken = connectionFinder.getIdToken(connection, context.resourceResolver());
