[java-idp-plugin-oidc-rp] branch main updated: Revert and improve redirect uri handling
Phil Smart
philip.smart at jisc.ac.uk
Wed Oct 12 14:43:31 UTC 2022
This is an automated email from the git hooks/post-receive script.
philsmart pushed a commit to branch main
in repository java-idp-plugin-oidc-rp.
View the commit online:
http://git.shibboleth.net/view/?p=java-idp-plugin-oidc-rp.git;a=commit;h=5c6b992909423d2499a40e9758f054b30f90324e
The following commit(s) were added to refs/heads/main by this push:
new 5c6b992 Revert and improve redirect uri handling
5c6b992 is described below
commit 5c6b992909423d2499a40e9758f054b30f90324e
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Wed Oct 12 15:43:25 2022 +0100
Revert and improve redirect uri handling
---
.../impl/DefaultRedirectUriCreationFunction.java | 62 ++++++++++++-------
.../rp/messaging/impl/AddRedirectURIHandler.java | 38 +++---------
.../oidc-relying-party-authn-beans.xml | 4 +-
.../DefaultRedirectUriCreationFunctionTest.java | 72 +++++++++++++++++++---
.../messaging/impl/AddRedirectURIHandlerTest.java | 34 ++--------
5 files changed, 118 insertions(+), 92 deletions(-)
diff --git a/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunction.java b/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunction.java
index 36244bf..15cb1bc 100644
--- a/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunction.java
+++ b/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunction.java
@@ -26,8 +26,6 @@ import java.util.function.Function;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
-import javax.annotation.concurrent.Immutable;
-import javax.annotation.concurrent.ThreadSafe;
import javax.servlet.http.HttpServletRequest;
import org.apache.http.client.utils.URIBuilder;
@@ -39,9 +37,14 @@ import org.slf4j.LoggerFactory;
import net.shibboleth.idp.plugin.authn.oidc.rp.context.OAuth2ClientContext;
import net.shibboleth.idp.plugin.authn.oidc.rp.context.OIDCPeerEntityContext;
-import net.shibboleth.utilities.java.support.annotation.ParameterName;
+import net.shibboleth.utilities.java.support.annotation.constraint.NonnullAfterInit;
import net.shibboleth.utilities.java.support.annotation.constraint.NotEmpty;
+import net.shibboleth.utilities.java.support.annotation.constraint.ThreadSafeAfterInit;
+import net.shibboleth.utilities.java.support.component.AbstractIdentifiableInitializableComponent;
+import net.shibboleth.utilities.java.support.component.ComponentInitializationException;
+import net.shibboleth.utilities.java.support.component.ComponentSupport;
import net.shibboleth.utilities.java.support.logic.Constraint;
+import net.shibboleth.utilities.java.support.primitive.StringSupport;
/**
* Constructive, pure, function that returns a redirect_uri from one of (ordered):
@@ -56,10 +59,9 @@ import net.shibboleth.utilities.java.support.logic.Constraint;
*
* <p>Is thread-safe and immutable</p>
*/
- at ThreadSafe
- at Immutable
+ at ThreadSafeAfterInit
//TODO similar to that used in the Duo plugin.
-public class DefaultRedirectUriCreationFunction
+public class DefaultRedirectUriCreationFunction extends AbstractIdentifiableInitializableComponent
implements BiFunction<HttpServletRequest, ProfileRequestContext, URI> {
/** Class logger. */
@@ -69,43 +71,57 @@ public class DefaultRedirectUriCreationFunction
@Nonnull private Function<ProfileRequestContext, OAuth2ClientContext> oauth2ClientContextLookupStrategy;
/** The path, excluding the context and servlet paths, to the RP callback handler.*/
- @Nonnull @NotEmpty private final String callbackServletPath;
+ @NonnullAfterInit @NotEmpty private String callbackServletPath;
/**
* A set of 'allowed' origins that can be used as the scheme, host, and port portion of the redirectURI.
* Can be null, if so a redirect_uri must be specified in the context tree.
*/
- @Nullable private Set<String> allowedOrigins;
+ @NonnullAfterInit private Set<String> allowedOrigins;
/**
* Constructor.
- *
- * @param callbackPath the path segment relative to the servlet path of the callback endpoint.
- * @param origins the allowed origins to use if a redirect_uri is computed
*/
- public DefaultRedirectUriCreationFunction(
- @Nonnull @NotEmpty @ParameterName(name="callbackPath") final String callbackPath,
- @Nonnull @NotEmpty @ParameterName(name="allowedOrigins") @Nullable final Set<String> origins) {
-
- callbackServletPath = Constraint.isNotNull(callbackPath,"RP Proxy Call back path can not be null");
+ public DefaultRedirectUriCreationFunction() {
// Default under OIDCPeerEntityContext in the outbound context (create true) under the nested PRC.
oauth2ClientContextLookupStrategy = new ChildContextLookup<>(OAuth2ClientContext.class).compose(
new ChildContextLookup<>(OIDCPeerEntityContext.class).compose(
- new OutboundMessageContextLookup()));
+ new OutboundMessageContextLookup()));
+ }
+
+ @Override
+ protected void doInitialize() throws ComponentInitializationException {
+ super.doInitialize();
- if (origins == null) {
+ if (StringSupport.trimOrNull(callbackServletPath) == null) {
+ throw new ComponentInitializationException("Callback servlet path can not be null");
+ }
+ if (allowedOrigins == null) {
allowedOrigins = Collections.emptySet();
- } else {
- allowedOrigins = Collections.unmodifiableSet(origins);
}
}
/**
- * Set the allowed origins.
+ * Set the path segment relative to the servlet path of the callback endpoint.
+ *
+ * @param path the callback servlet path
+ */
+ public void setCallbackServletPath(@Nonnull @NotEmpty final String path) {
+ ComponentSupport.ifInitializedThrowUnmodifiabledComponentException(this);
+ ComponentSupport.ifDestroyedThrowDestroyedComponentException(this);
+
+ callbackServletPath = Constraint.isNotEmpty(path, "callbackServletPath can not be null");
+ }
+
+ /**
+ * Set the allowed origins to use if a redirect_uri is computed.
*
* @param origins the origins
*/
public void setAllowedOrigins(@Nullable final Set<String> origins) {
+ ComponentSupport.ifInitializedThrowUnmodifiabledComponentException(this);
+ ComponentSupport.ifDestroyedThrowDestroyedComponentException(this);
+
if (origins == null) {
allowedOrigins = Collections.emptySet();
}
@@ -120,7 +136,9 @@ public class DefaultRedirectUriCreationFunction
* @param strgy the strategy.
*/
public void setOAuth2ClientContextLookupStrategy(
- @Nonnull final Function<ProfileRequestContext, OAuth2ClientContext> strgy) {
+ @Nonnull final Function<ProfileRequestContext, OAuth2ClientContext> strgy) {
+ ComponentSupport.ifInitializedThrowUnmodifiabledComponentException(this);
+ ComponentSupport.ifDestroyedThrowDestroyedComponentException(this);
oauth2ClientContextLookupStrategy = Constraint.isNotNull(strgy,
"OAuth2 client context lookup strategy cannot be null");
diff --git a/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandler.java b/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandler.java
index 1e3a4c5..a58e927 100644
--- a/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandler.java
+++ b/idp-oidc-rp-impl/src/main/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandler.java
@@ -18,7 +18,6 @@
package net.shibboleth.idp.plugin.authn.oidc.rp.messaging.impl;
import java.net.URI;
-import java.net.URISyntaxException;
import java.util.function.BiFunction;
import javax.annotation.Nonnull;
@@ -35,12 +34,9 @@ import net.shibboleth.utilities.java.support.annotation.constraint.NonnullAfterI
import net.shibboleth.utilities.java.support.component.ComponentInitializationException;
import net.shibboleth.utilities.java.support.component.ComponentSupport;
import net.shibboleth.utilities.java.support.logic.Constraint;
-import net.shibboleth.utilities.java.support.primitive.StringSupport;
/**
- * A message handler that adds a redirect_uri to the authentication request. The redirect_uri is either
- * taken directly from the override set on the RP profile configuration, or if none exist, is computed using
- * the creation strategy.
+ * A message handler that adds a redirect_uri to the authentication request.
*/
public class AddRedirectURIHandler extends AbstractOIDCAuthenticationRequestActionMessageHandler {
@@ -77,30 +73,14 @@ public class AddRedirectURIHandler extends AbstractOIDCAuthenticationRequestActi
@Override protected void doInvoke(@Nonnull final MessageContext messageContext)
throws MessageHandlerException {
-
- final String redirectOverride =
- getProfileConfiguration().getRedirectUriOverride(lookupProfileRequestContext(messageContext));
-
-
- if (StringSupport.trimOrNull(redirectOverride) != null) {
- log.debug("{} Redirect_uri override in profile configuration is: '{}'", getLogPrefix(),
- redirectOverride);
- try {
- getAuthenticationRequest().setRedirectURI(new URI(redirectOverride));
- } catch (final URISyntaxException e) {
- throw new MessageHandlerException("Redirect URI override was not a valid URI", e);
- }
- } else {
- final URI redirectUri =
- redirectUriCreationStrategy.apply(
- getHttpServletRequest(), lookupProfileRequestContext(messageContext));
- if (redirectUri == null) {
- throw new MessageHandlerException("Redirect URI could not be located or created using the strategy");
- }
- getAuthenticationRequest().setRedirectURI(redirectUri);
- }
-
- log.trace("{} Created redirect_uri '{}'", getLogPrefix(), getAuthenticationRequest().getRedirectURI());
+
+ final URI redirectUri =
+ redirectUriCreationStrategy.apply(getHttpServletRequest(), lookupProfileRequestContext(messageContext));
+ if (redirectUri == null) {
+ throw new MessageHandlerException("Redirect URI could not be located or created using the strategy");
+ }
+ log.trace("{} Created redirect_uri '{}'", getLogPrefix(), redirectUri);
+ getAuthenticationRequest().setRedirectURI(redirectUri);
}
diff --git a/idp-oidc-rp-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/OIDCRelyingParty/oidc-relying-party-authn-beans.xml b/idp-oidc-rp-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/OIDCRelyingParty/oidc-relying-party-authn-beans.xml
index e21343a..9444cf7 100644
--- a/idp-oidc-rp-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/OIDCRelyingParty/oidc-relying-party-authn-beans.xml
+++ b/idp-oidc-rp-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/authn/OIDCRelyingParty/oidc-relying-party-authn-beans.xml
@@ -301,8 +301,8 @@
</bean>
<bean id="shibboleth.authn.oidc.rp.DefaultRedirectUriCreationStrategy"
- c:callbackPath="#{getObject('shibboleth.authn.OIDC.externalServletPath')}/callback"
- c:allowedOrigins="%{idp.authn.oidc.rp.client.redirecturl.allowedOrigins:}"
+ p:callbackServletPath="#{getObject('shibboleth.authn.OIDC.externalServletPath')}/callback"
+ p:allowedOrigins="%{idp.authn.oidc.rp.client.redirecturl.allowedOrigins:}"
class="net.shibboleth.idp.plugin.authn.oidc.rp.impl.DefaultRedirectUriCreationFunction" />
<!-- Message Decoding -->
diff --git a/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunctionTest.java b/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunctionTest.java
index bc481ac..8a555ce 100644
--- a/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunctionTest.java
+++ b/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/impl/DefaultRedirectUriCreationFunctionTest.java
@@ -1,3 +1,20 @@
+/*
+ * Licensed to the University Corporation for Advanced Internet Development,
+ * Inc. (UCAID) under one or more contributor license agreements. See the
+ * NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The UCAID 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 net.shibboleth.idp.plugin.authn.oidc.rp.impl;
import static org.testng.Assert.assertEquals;
@@ -16,6 +33,7 @@ import org.testng.annotations.Test;
import net.shibboleth.idp.plugin.authn.oidc.rp.context.OAuth2ClientContext;
import net.shibboleth.idp.plugin.authn.oidc.rp.context.OIDCPeerEntityContext;
+/** Tests for {@link DefaultRedirectUriCreationFunction}.*/
public class DefaultRedirectUriCreationFunctionTest {
/** Static callback path from servlet request.*/
@@ -49,7 +67,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectAllowed() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("https://allowed.com"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Set.of("https://allowed.com"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "allowed.com");
request.setServerPort(443);
@@ -63,7 +85,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectDisallowed() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("https://allowed.com"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Set.of("https://allowed.com"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "not-allowed.com");
request.setServerPort(443);
@@ -76,7 +102,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectDisallowedNotOrigins() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Collections.emptySet());
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Collections.emptySet());
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "not-allowed.com");
request.setServerPort(443);
@@ -89,7 +119,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectAllowedHTTPSCustomPort() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("https://allowed.com:8443"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins( Set.of("https://allowed.com:8443"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "allowed.com");
request.setServerPort(8443);
@@ -102,7 +136,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectAllowedHTTPCustomPort() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("http://allowed.com:8080"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Set.of("http://allowed.com:8080"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "allowed.com");
request.setServerPort(8080);
@@ -115,7 +153,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectAllowedNullPort() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("http://allowed.com"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Set.of("http://allowed.com"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "allowed.com");
request.setServerPort(-1);
@@ -128,7 +170,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectDisallowedOnPort() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("http://allowed.com"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Set.of("http://allowed.com"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "allowed.com");
request.setServerPort(443);
@@ -141,7 +187,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testPreregisteredURL() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("http://allowed.com"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Set.of("http://allowed.com"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
//request is irrelevant
request.addHeader("Host", "notused.com");
@@ -156,7 +206,11 @@ public class DefaultRedirectUriCreationFunctionTest {
@Test
public final void testComputedRedirectBadHostname() throws Exception {
- strategy = new DefaultRedirectUriCreationFunction(CALLBACK_PATH, Set.of("http://allowed.com"));
+ strategy = new DefaultRedirectUriCreationFunction();
+ strategy.setAllowedOrigins(Set.of("http://allowed.com"));
+ strategy.setCallbackServletPath(CALLBACK_PATH);
+ strategy.setId("mock-redirect-strategy");
+ strategy.initialize();
request.addHeader("Host", "<script>inject</script>");
request.setServerPort(80);
diff --git a/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandlerTest.java b/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandlerTest.java
index 4e22420..85ea2ea 100644
--- a/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandlerTest.java
+++ b/idp-oidc-rp-impl/src/test/java/net/shibboleth/idp/plugin/authn/oidc/rp/messaging/impl/AddRedirectURIHandlerTest.java
@@ -25,9 +25,11 @@ import org.opensaml.messaging.handler.MessageHandlerException;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
+import net.shibboleth.idp.plugin.authn.oidc.rp.context.OAuth2ClientContext;
import net.shibboleth.idp.plugin.authn.oidc.rp.impl.AbstractOIDCTest;
import net.shibboleth.idp.profile.context.RelyingPartyContext;
import net.shibboleth.idp.relyingparty.RelyingPartyConfiguration;
+import net.shibboleth.oidc.authn.context.OAuth2ClientAuthenticationContext;
import net.shibboleth.oidc.profile.config.OIDCAuthorizationConfiguration;
/** Tests for {@link AddRedirectURI}.*/
@@ -77,36 +79,8 @@ public class AddRedirectURIHandlerTest extends AbstractOIDCTest {
handler.initialize();
handler.invoke(prc.getOutboundMessageContext());
}
-
- @Test
- public void testRedirectURIOverride() throws Exception {
-
- final String redirectUriOverride = "https://rp.example.com/callback";
- oidcAuthzConfig.setRedirectUriOverride(redirectUriOverride);
-
- //Should take the override and not this
- final URI redirectUriFromFunction = new URI("https://rp.example.com/not-this-callback");
- handler.setRedirectUriCreationStrategy((http, prc) -> redirectUriFromFunction);
-
- handler.initialize();
- handler.invoke(prc.getOutboundMessageContext());
- assertEquals(authnRequest.getRedirectURI().toString(),redirectUriOverride);
- }
-
- @Test(expectedExceptions = MessageHandlerException.class)
- public void testRedirectURIOverrideInvalidURI() throws Exception {
-
- final String redirectUriOverride = "http://rp.example.com/fail?id={1}";
- oidcAuthzConfig.setRedirectUriOverride(redirectUriOverride);
-
- //Should fail on the override and not this
- final URI redirectUriFromFunction = new URI("https://rp.example.com/not-this-callback");
- handler.setRedirectUriCreationStrategy((http, prc) -> redirectUriFromFunction);
-
- handler.initialize();
- handler.invoke(prc.getOutboundMessageContext());
-
- }
+
+
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list