[java-idp-oidc] branch main updated: JOIDC-116 - Permit token without redirect_uri when client has registered only one redirect_uri
Henri Mikkonen
henri.mikkonen at iki.fi
Wed Jul 6 15:01:43 UTC 2022
This is an automated email from the git hooks/post-receive script.
hjmikkon pushed a commit to branch main
in repository java-idp-oidc.
View the commit online:
http://git.shibboleth.net/view/?p=java-idp-oidc.git;a=commit;h=364037519026024c6371b4f744ca899b80c5fbd5
The following commit(s) were added to refs/heads/main by this push:
new 36403751 JOIDC-116 - Permit token without redirect_uri when client has registered only one redirect_uri
36403751 is described below
commit 364037519026024c6371b4f744ca899b80c5fbd5
Author: Henri Mikkonen <henri.mikkonen at iki.fi>
AuthorDate: Wed Jul 6 17:59:20 2022 +0300
JOIDC-116 - Permit token without redirect_uri when client has registered only one redirect_uri
https://shibboleth.atlassian.net/browse/JOIDC-116
Allow token endpoint to be requested without redirect_uri parameter when only single redirect URI
has been registered for the RP. It has to match with the value in the authorization code grant.
Also changed TokenRequestRedirectURILookupFunction logging related to missing redirect_uri to DEBUG.
---
.../TokenRequestRedirectURILookupFunction.java | 4 +-
.../oidc/op/profile/impl/ValidateRedirectURI.java | 88 ++++++++++++++++++++--
.../idp/flows/oidc/authorize/authorize-beans.xml | 2 +-
.../idp/flows/oidc/token/token-beans.xml | 3 +-
.../oidc/op/profile/flow/AuthorizeFlowTest.java | 24 ++++++
.../plugin/oidc/op/profile/flow/TokenFlowTest.java | 45 ++++++++++-
.../op/profile/impl/ValidateRedirectURITest.java | 62 ++++++++++++++-
7 files changed, 213 insertions(+), 15 deletions(-)
diff --git a/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/context/navigate/TokenRequestRedirectURILookupFunction.java b/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/context/navigate/TokenRequestRedirectURILookupFunction.java
index 1b5b1015..e1051025 100644
--- a/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/context/navigate/TokenRequestRedirectURILookupFunction.java
+++ b/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/context/navigate/TokenRequestRedirectURILookupFunction.java
@@ -45,12 +45,12 @@ public class TokenRequestRedirectURILookupFunction extends AbstractTokenRequestL
URI doLookup(final TokenRequest req) {
final List<String> redirectURIs = req.getAuthorizationGrant().toParameters().get("redirect_uri");
if (redirectURIs == null || redirectURIs.isEmpty()) {
- log.warn("No redirect_uri parameter");
+ log.debug("No redirect_uri parameter");
return null;
}
final String redirectURI = redirectURIs.get(0);
if (redirectURI == null) {
- log.warn("No redirect_uri parameter");
+ log.debug("No redirect_uri parameter");
return null;
}
URI uri = null;
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURI.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURI.java
index 36d9c1fc..efa29d55 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURI.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURI.java
@@ -31,6 +31,7 @@ import org.slf4j.LoggerFactory;
import net.shibboleth.idp.plugin.oidc.op.profile.context.navigate.DefaultRequestRedirectURILookupFunction;
import net.shibboleth.idp.plugin.oidc.op.profile.context.navigate.DefaultValidRedirectUrisLookupFunction;
import net.shibboleth.oidc.profile.core.OidcEventIds;
+import net.shibboleth.utilities.java.support.annotation.constraint.NotEmpty;
import net.shibboleth.utilities.java.support.component.ComponentSupport;
import net.shibboleth.utilities.java.support.logic.Constraint;
@@ -48,12 +49,19 @@ public class ValidateRedirectURI extends AbstractOIDCAuthenticationResponseActio
/** Strategy used to obtain the redirect uris to compare request value to. */
@Nonnull private Function<ProfileRequestContext, Set<URI>> validRedirectURIsLookupStrategy;
+ /** Strategy used to obtain registered redirect uris to compare if request had no redirect uri value. */
+ @Nonnull private Function<ProfileRequestContext, Set<URI>> registeredRedirectURIsLookupStrategy;
+
+ /** Whether to require redirect uri value in the request also when only single value is registered. */
+ private boolean requireRequestedValue = true;
+
/**
* Constructor.
*/
public ValidateRedirectURI() {
redirectURILookupStrategy = new DefaultRequestRedirectURILookupFunction();
validRedirectURIsLookupStrategy = new DefaultValidRedirectUrisLookupFunction();
+ registeredRedirectURIsLookupStrategy = new DefaultValidRedirectUrisLookupFunction();
}
/**
@@ -78,16 +86,33 @@ public class ValidateRedirectURI extends AbstractOIDCAuthenticationResponseActio
Constraint.isNotNull(strategy, "ValidRedirectURIsLookupStrategy lookup strategy cannot be null");
}
+ /**
+ * Set the strategy used to obtain registered redirect uris to compare if request had no redirect uri value.
+ *
+ * @param strategy lookup strategy
+ */
+ public void setRegisteredRedirectURIsLookupStrategy(@Nonnull final Function<ProfileRequestContext,
+ Set<URI>> strategy) {
+ ComponentSupport.ifInitializedThrowUnmodifiabledComponentException(this);
+ registeredRedirectURIsLookupStrategy =
+ Constraint.isNotNull(strategy, "RegisteredRedirectURIsLookupStrategy lookup strategy cannot be null");
+ }
+
+ /**
+ * Set whether to require redirect uri value in the request also when only single value is registered.
+ *
+ * @param flag flag to set
+ */
+ public void setRequireRequestedValue(final boolean flag) {
+ ComponentSupport.ifInitializedThrowUnmodifiabledComponentException(this);
+ requireRequestedValue = flag;
+ }
+
/** {@inheritDoc} */
@Override
protected void doExecute(@Nonnull final ProfileRequestContext profileRequestContext) {
final URI requestRedirectURI = redirectURILookupStrategy.apply(profileRequestContext);
- if (requestRedirectURI == null) {
- log.warn("{} Redirection URI of the request not located for verification", getLogPrefix());
- ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REDIRECT_URI);
- return;
- }
-
+
final Set<URI> redirectionURIs = validRedirectURIsLookupStrategy.apply(profileRequestContext);
if (redirectionURIs == null || redirectionURIs.isEmpty()) {
log.warn("{} Client has not registered Redirection URIs. Redirection URI cannot be validated.",
@@ -95,13 +120,18 @@ public class ValidateRedirectURI extends AbstractOIDCAuthenticationResponseActio
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REDIRECT_URI);
return;
}
-
+
+ if (requestRedirectURI == null) {
+ handleNullRequestedURI(profileRequestContext, redirectionURIs);
+ return;
+ }
+
if (redirectionURIs.contains(requestRedirectURI)) {
getOidcResponseContext().setRedirectURI(requestRedirectURI);
log.debug("{} Redirection URI validated {}", getLogPrefix(), requestRedirectURI);
return;
}
-
+
String registered = "";
for (final URI uri : redirectionURIs) {
registered += registered.isEmpty() ? uri.toString() : ", " + uri.toString();
@@ -110,4 +140,46 @@ public class ValidateRedirectURI extends AbstractOIDCAuthenticationResponseActio
requestRedirectURI.toString(), registered);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REDIRECT_URI);
}
+
+ /**
+ * Handles the missing requested redirect URI case: it may be missing if it's not required to exist and the
+ * registered and valid records contain only single matching value.
+ *
+ * @param profileRequestContext profile request context
+ * @param validRedirectionURIs set of valid redirection uris
+ */
+ protected void handleNullRequestedURI(final ProfileRequestContext profileRequestContext,
+ @Nonnull @NotEmpty final Set<URI> validRedirectionURIs) {
+ if (requireRequestedValue) {
+ log.warn("{} Redirection URI of the request not located for verification", getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REDIRECT_URI);
+ return;
+ }
+ final Set<URI> registeredURIs = registeredRedirectURIsLookupStrategy.apply(profileRequestContext);
+ if (registeredURIs == null || registeredURIs.isEmpty()) {
+ log.warn("{} Client has not registered Redirection URIs. Redirection URI cannot be validated.",
+ getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REDIRECT_URI);
+ return;
+ }
+ if (registeredURIs.size() == 1 && validRedirectionURIs.size() == 1) {
+ final URI singleRegisteredUri = registeredURIs.iterator().next();
+ final URI singleValidUri = validRedirectionURIs.iterator().next();
+ if (singleRegisteredUri.equals(singleValidUri)) {
+ log.debug("{} No requested redirect_uri found, but allowing it due to single trusted value {}",
+ getLogPrefix(), singleRegisteredUri);
+ getOidcResponseContext().setRedirectURI(singleRegisteredUri);
+ return;
+ } else {
+ log.warn("{} Registered URI '{}' did not match with the valid one '{}'", getLogPrefix(),
+ singleRegisteredUri, singleValidUri);
+ ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REDIRECT_URI);
+ return;
+ }
+ }
+ // more than one registered registered/valid URIs
+ log.warn("{} Redirection URI of the request missing even though multiple values registered/valid",
+ getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REDIRECT_URI);
+ }
}
\ No newline at end of file
diff --git a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-beans.xml b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-beans.xml
index b4f01e81..a686307d 100644
--- a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-beans.xml
+++ b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-beans.xml
@@ -168,7 +168,7 @@
</util:list>
<bean id="ValidateRedirectURI" class="net.shibboleth.idp.plugin.oidc.op.profile.impl.ValidateRedirectURI"
- scope="prototype" />
+ scope="prototype" p:requireRequestedValue="true" />
<bean id="ValidateResponseType" class="net.shibboleth.idp.plugin.oidc.op.profile.impl.ValidateResponseType"
scope="prototype" />
diff --git a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/token/token-beans.xml b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/token/token-beans.xml
index af045296..b6e1fd2b 100644
--- a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/token/token-beans.xml
+++ b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/token/token-beans.xml
@@ -70,7 +70,8 @@
scope="prototype"
p:activationCondition-ref="AuthorizationCodeGrantCondition"
p:redirectURILookupStrategy-ref="shibboleth.TokenRequestRedirectURILookupStrategy"
- p:validRedirectURIsLookupStrategy-ref="shibboleth.TokenRequestValidRequestUrisLookupStrategy" />
+ p:validRedirectURIsLookupStrategy-ref="shibboleth.TokenRequestValidRequestUrisLookupStrategy"
+ p:requireRequestedValue="false" />
<bean id="shibboleth.TokenRequestRedirectURILookupStrategy"
class="net.shibboleth.idp.plugin.oidc.op.profile.context.navigate.TokenRequestRedirectURILookupFunction" />
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AuthorizeFlowTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AuthorizeFlowTest.java
index fc3fe75e..32e41a43 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AuthorizeFlowTest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AuthorizeFlowTest.java
@@ -154,6 +154,18 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
Assert.assertEquals("ErrorView", result.getOutcome().getId());
}
+ @Test
+ public void testWithAuthorizationCodeFlowNoRedirectURI() throws IOException, SessionException {
+ request.setMethod("GET");
+ request.setQueryString("client_id=mockClientId&response_type=code&scope=openid%20profile");
+ storeMetadata(storageService, clientId, clientSecret, scope, redirectUri);
+
+ initializeThreadLocals();
+
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ }
+
@Test
public void testWithImplicitFlowNoNonce() throws IOException, SessionException {
request.setMethod("GET");
@@ -323,6 +335,18 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
Assert.assertEquals("ErrorView", result.getOutcome().getId());
}
+ @Test
+ public void testWithImplicitFlowNoRedirectURI() throws IOException, SessionException {
+ request.setMethod("GET");
+ request.setQueryString("client_id=mockClientId&response_type=id_token&scope=openid%20profile");
+ storeMetadata(storageService, clientId, clientSecret, scope, redirectUri);
+
+ initializeThreadLocals();
+
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ }
+
@Test
public void testWithHybridIdTokenFlow() throws IOException, SessionException {
request.setMethod("GET");
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java
index 42d73448..ec57697f 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java
@@ -175,7 +175,7 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
if (registeredMethod != null) {
storeMetadata(storageService, clientId, clientSecret, scope, null, registeredMethod);
} else {
- storeMetadata(storageService, clientId, clientSecret, scope);
+ storeMetadata(storageService, clientId, clientSecret, scope, redirectUri);
}
}
@@ -191,6 +191,49 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
Assert.assertNotNull(response.getOIDCTokens().getIDToken());
}
+ @Test
+ public void testValidGrantNonMatchingRedirectURI() throws Exception {
+ initializeGrantAndRequest(clientId, createRequestParameters(redirectUri + "wrong", "authorization_code",
+ buildAuthorizationCode(clientId), clientId));
+ storeConsent(storageService, "jdoe", clientId, "mail");
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ assertErrorCode(result, OAuth2Error.INVALID_REQUEST_CODE);
+ }
+
+ @Test
+ public void testValidGrantNoRedirectURISingleTrusted() throws Exception {
+ initializeGrantAndRequest(clientId, createRequestParameters(null, "authorization_code",
+ buildAuthorizationCode(clientId), clientId));
+ storeConsent(storageService, "jdoe", clientId, "mail");
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ final OIDCTokenResponse response = parseSuccessResponse(result, OIDCTokenResponse.class);
+ Assert.assertNotNull(response.getTokens().getAccessToken());
+ Assert.assertNotNull(response.getTokens().getRefreshToken());
+ Assert.assertNotNull(response.getOIDCTokens().getIDToken());
+ }
+
+ @Test
+ public void testValidGrantNoRedirectURISingleTrustedNotMatchingAuthzCode() throws Exception {
+ initializeGrantAndRequest(clientId, createRequestParameters(null, "authorization_code",
+ buildAuthorizationCode(clientId), clientId));
+ storeConsent(storageService, "jdoe", clientId, "mail");
+ removeMetadata(storageService, clientId);
+ storeMetadata(storageService, clientId, clientSecret, scope, redirectUri + "wrong");
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ assertErrorCode(result, OAuth2Error.INVALID_REQUEST_CODE);
+ }
+
+ @Test
+ public void testValidGrantNoRedirectURIMultipleTrusted() throws Exception {
+ initializeGrantAndRequest(clientId, createRequestParameters(null, "authorization_code",
+ buildAuthorizationCode(clientId), clientId));
+ storeConsent(storageService, "jdoe", clientId, "mail");
+ removeMetadata(storageService, clientId);
+ storeMetadata(storageService, clientId, clientSecret, scope, redirectUri, redirectUri + "another");
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ assertErrorCode(result, OAuth2Error.INVALID_REQUEST_CODE);
+ }
+
@Test
public void testValidGrantWithCustomTokens() throws Exception {
initializeGrantAndRequest(clientIdCustomTokens, createRequestParameters(redirectUri, "authorization_code",
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURITest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURITest.java
index 15998749..344774c2 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURITest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/ValidateRedirectURITest.java
@@ -19,6 +19,7 @@ package net.shibboleth.idp.plugin.oidc.op.profile.impl;
import java.net.URI;
import java.net.URISyntaxException;
+import java.util.Set;
import net.shibboleth.idp.profile.testing.ActionTestingSupport;
import net.shibboleth.oidc.metadata.context.OIDCMetadataContext;
@@ -36,9 +37,23 @@ import com.nimbusds.openid.connect.sdk.rp.OIDCClientMetadata;
public class ValidateRedirectURITest extends BaseOIDCResponseActionTest {
private ValidateRedirectURI action;
+
+ private String requestUri = "https://client.example.org/cb";
- private void init() throws ComponentInitializationException {
+ private void init() throws ComponentInitializationException, URISyntaxException {
+ init(true, new URI(requestUri), null, null);
+ }
+
+ private void init(final boolean requireRequestedValue, final URI requestedUri, final Set<URI> validUris, final Set<URI> registeredUris) throws ComponentInitializationException {
action = new ValidateRedirectURI();
+ action.setRequireRequestedValue(requireRequestedValue);
+ action.setRedirectURILookupStrategy(prc -> requestedUri);
+ if (validUris != null) {
+ action.setValidRedirectURIsLookupStrategy(prc -> validUris);
+ }
+ if (registeredUris != null) {
+ action.setRegisteredRedirectURIsLookupStrategy(prc -> registeredUris);
+ }
action.initialize();
}
@@ -46,9 +61,10 @@ public class ValidateRedirectURITest extends BaseOIDCResponseActionTest {
* Test that action copes with no OIDC metadata context.
*
* @throws ComponentInitializationException
+ * @throws URISyntaxException
*/
@Test
- public void testNoCtx() throws ComponentInitializationException {
+ public void testNoCtx() throws ComponentInitializationException, URISyntaxException {
init();
profileRequestCtx.getInboundMessageContext().removeSubcontext(OIDCMetadataContext.class);
final Event event = action.execute(requestCtx);
@@ -92,4 +108,46 @@ public class ValidateRedirectURITest extends BaseOIDCResponseActionTest {
ActionTestingSupport.assertProceedEvent(event);
Assert.assertNotNull(respCtx.getRedirectURI());
}
+
+ @Test
+ public void testNullRequesNotAllowed() throws ComponentInitializationException, URISyntaxException {
+ init(true, null, Set.of(new URI(requestUri)), Set.of(new URI(requestUri)));
+ final Event event = action.execute(requestCtx);
+ ActionTestingSupport.assertEvent(event, OidcEventIds.INVALID_REDIRECT_URI);
+ Assert.assertNull(respCtx.getRedirectURI());
+ }
+
+ @Test
+ public void testNullRequesAllowedNotSingleValidUri() throws ComponentInitializationException, URISyntaxException {
+ init(false, null, Set.of(new URI(requestUri), new URI("https://another.example.org")), Set.of(new URI(requestUri)));
+ final Event event = action.execute(requestCtx);
+ ActionTestingSupport.assertEvent(event, OidcEventIds.INVALID_REDIRECT_URI);
+ Assert.assertNull(respCtx.getRedirectURI());
+ }
+
+ @Test
+ public void testNullRequesAllowedNotSingleRegisteredUri() throws ComponentInitializationException, URISyntaxException {
+ init(false, null, Set.of(new URI(requestUri)), Set.of(new URI(requestUri), new URI("https://another.example.org")));
+ final Event event = action.execute(requestCtx);
+ ActionTestingSupport.assertEvent(event, OidcEventIds.INVALID_REDIRECT_URI);
+ Assert.assertNull(respCtx.getRedirectURI());
+ }
+
+ @Test
+ public void testNullRequesAllowedNoRegisteredUris() throws ComponentInitializationException, URISyntaxException {
+ init(false, null, Set.of(new URI(requestUri)), null);
+ final Event event = action.execute(requestCtx);
+ ActionTestingSupport.assertEvent(event, OidcEventIds.INVALID_REDIRECT_URI);
+ Assert.assertNull(respCtx.getRedirectURI());
+ }
+
+ @Test
+ public void testNullRequesAllowed() throws ComponentInitializationException, URISyntaxException {
+ init(false, null, Set.of(new URI(requestUri)), Set.of(new URI(requestUri)));
+ final Event event = action.execute(requestCtx);
+ ActionTestingSupport.assertProceedEvent(event);
+ Assert.assertNotNull(respCtx.getRedirectURI());
+ Assert.assertEquals(respCtx.getRedirectURI(), new URI(requestUri));
+ }
+
}
\ No newline at end of file
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list