[java-idp-oidc] branch main updated: JOIDC-218 - Error handling for missing and invalid request objects
Henri Mikkonen
henri.mikkonen at iki.fi
Fri Jun 21 11:21:09 UTC 2024
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=892fdf81dd21cd9267a1df438e443b92fb597267
The following commit(s) were added to refs/heads/main by this push:
new 892fdf81 JOIDC-218 - Error handling for missing and invalid request objects
892fdf81 is described below
commit 892fdf81dd21cd9267a1df438e443b92fb597267
Author: Henri Mikkonen <henri.mikkonen at iki.fi>
AuthorDate: Fri Jun 21 14:20:46 2024 +0300
JOIDC-218 - Error handling for missing and invalid request objects
https://shibboleth.atlassian.net/browse/JOIDC-218
- Add flag for request object validation failures in OIDCAuthenticationResponseContext
- If the flag is set, then authorize-flow calls ValidateRedirectURI before handling the error event
- Updated flow tests accordingly
---
.../context/OIDCAuthenticationResponseContext.java | 25 ++++++++++++++++++++++
.../impl/SetRequestObjectToResponseContext.java | 10 +++++++++
.../oauth2/profile/impl/ValidateRequestObject.java | 10 +++++++--
.../META-INF/net.shibboleth.idp/postconfig.xml | 2 ++
.../idp/flows/oidc/authorize/authorize-beans.xml | 11 ++++++++++
.../idp/flows/oidc/authorize/authorize-flow.xml | 1 +
.../oidc/op/profile/flow/AuthorizeFlowTest.java | 24 +++++++++++----------
.../oidc/op/profile/flow/RequestObjectJWETest.java | 4 +++-
.../oidc/op/profile/flow/RequestObjectJWSTest.java | 4 +++-
9 files changed, 76 insertions(+), 15 deletions(-)
diff --git a/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/messaging/context/OIDCAuthenticationResponseContext.java b/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/messaging/context/OIDCAuthenticationResponseContext.java
index b4065096..ce7d7bac 100644
--- a/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/messaging/context/OIDCAuthenticationResponseContext.java
+++ b/idp-oidc-extension-api/src/main/java/net/shibboleth/idp/plugin/oidc/op/messaging/context/OIDCAuthenticationResponseContext.java
@@ -132,6 +132,9 @@ public class OIDCAuthenticationResponseContext extends BaseContext {
/** Whether request object has already been validated. */
private boolean requestObjectValidated = false;
+ /** Whether request object valiadtion has failed. */
+ private boolean requestObjectFailure = false;
+
/** DPoP Proof JWK thumbprint. */
@Nullable private String dpopProofJwkThumbprint;
@@ -586,6 +589,28 @@ public class OIDCAuthenticationResponseContext extends BaseContext {
requestObjectValidated = flag;
}
+ /**
+ * Get whether request object validation has failed.
+ *
+ * @return true if failed, false if not
+ *
+ * @since 4.2.0
+ */
+ public boolean isRequestObjectFailure() {
+ return requestObjectFailure;
+ }
+
+ /**
+ * Set whether request object validation has failed.
+ *
+ * @param flag true if failed, false if not
+ *
+ * @since 4.2.0
+ */
+ public void setRequestObjectFailure(final boolean flag) {
+ requestObjectFailure = flag;
+ }
+
/**
* Get the DPoP Proof JWK thumbprint.
*
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/SetRequestObjectToResponseContext.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/SetRequestObjectToResponseContext.java
index f3fece49..5ca78f97 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/SetRequestObjectToResponseContext.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/SetRequestObjectToResponseContext.java
@@ -222,11 +222,15 @@ public class SetRequestObjectToResponseContext extends AbstractOAuthAuthorizatio
return false;
}
+ final OIDCAuthenticationResponseContext oidcResponseContext = getOidcResponseContext();
+ assert oidcResponseContext != null;
+
requirePushedAuthorization = pushedAuthorizationRequestEnforcedPredicate.test(profileRequestContext);
if (!authorizationRequest.specifiesRequestObject()) {
if (requestObjectEnforcedPredicate.test(profileRequestContext)) {
log.warn("{} No request_uri or request by value, even though it's enforced for {}", getLogPrefix(),
authorizationRequest.getClientID().getValue());
+ oidcResponseContext.setRequestObjectFailure(true);
if (requirePushedAuthorization) {
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.MISSING_MANDATORY_PAR_REQUEST_URI);
} else {
@@ -240,6 +244,7 @@ public class SetRequestObjectToResponseContext extends AbstractOAuthAuthorizatio
if (authorizationRequest.getRequestObject() != null
&& authorizationRequest.getRequestURI() != null) {
+ oidcResponseContext.setRequestObjectFailure(true);
log.error("{} request_uri and request object cannot be both set", getLogPrefix());
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.REQUEST_OBJECT_AND_URI);
return false;
@@ -300,12 +305,14 @@ public class SetRequestObjectToResponseContext extends AbstractOAuthAuthorizatio
}
log.error("{} Unregistered request URI blocked: {}", getLogPrefix(),
authorizationRequest.getRequestURI());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_URI);
return;
}
if (requirePushedAuthorization) {
log.warn("{} Pushed authorization request required but not used", getLogPrefix());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.MISSING_MANDATORY_PAR_REQUEST_URI);
return;
}
@@ -329,17 +336,20 @@ public class SetRequestObjectToResponseContext extends AbstractOAuthAuthorizatio
return;
} catch (final ParseException e) {
log.error("{} Unable to parse request object from request_uri, {}", getLogPrefix(), e.getMessage());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_URI);
return;
}
} else {
log.error("{} Unable to get request object from request_uri, HTTP status {}", getLogPrefix(),
response.getCode());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_URI);
return;
}
} catch (final IOException | org.apache.hc.core5.http.ParseException | URISyntaxException e) {
log.error("{} Unable to get request object from request_uri, {}", getLogPrefix(), e.getMessage());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_URI);
return;
}
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/ValidateRequestObject.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/ValidateRequestObject.java
index dac908de..b1c777e1 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/ValidateRequestObject.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/oauth2/profile/impl/ValidateRequestObject.java
@@ -126,11 +126,14 @@ public class ValidateRequestObject extends AbstractOAuthAuthorizationResponseAct
/** {@inheritDoc} */
@Override
protected void doExecute(@Nonnull final ProfileRequestContext profileRequestContext) {
+ final OIDCAuthenticationResponseContext oidcResponseContext = getOidcResponseContext();
+ assert oidcResponseContext != null;
// We let "none" to be used only if nothing else has been registered.
if (requestObject instanceof PlainJWT) {
if (getMetadataContext() == null) {
log.error("{} Request object unsigned, no client metadata", getLogPrefix());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_OBJECT);
return;
}
@@ -143,6 +146,7 @@ public class ValidateRequestObject extends AbstractOAuthAuthorizationResponseAct
if (requestObjectAlg != null && !"none".equals(requestObjectAlg.getName())) {
log.error("{} Request object is not signed, registered alg is {}", getLogPrefix(),
requestObjectAlg.getName());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_OBJECT);
return;
}
@@ -165,6 +169,7 @@ public class ValidateRequestObject extends AbstractOAuthAuthorizationResponseAct
&& !authorizationRequest.getClientID()
.equals(new ClientID((String) claimsSet.getClaim("client_id")))) {
log.error("{} client_id in request object not matching client_id request parameter", getLogPrefix());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_OBJECT);
return;
}
@@ -179,12 +184,14 @@ public class ValidateRequestObject extends AbstractOAuthAuthorizationResponseAct
&& !requestedType.equals(new ResponseType(claimsSet.getStringClaim("response_type").split(" ")))) {
log.error("{} response_type in request object not matching response_type request parameter",
getLogPrefix());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_OBJECT);
return;
}
}
} catch (final ParseException e) {
log.error("{} Unable to parse request object {}", getLogPrefix(), e.getMessage());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_OBJECT);
return;
}
@@ -198,12 +205,11 @@ public class ValidateRequestObject extends AbstractOAuthAuthorizationResponseAct
}
} catch (final JWTValidationException e) {
log.warn("{} JWT validation failed: {}", getLogPrefix(), e.getMessage());
+ oidcResponseContext.setRequestObjectFailure(true);
ActionSupport.buildEvent(profileRequestContext, OidcEventIds.INVALID_REQUEST_OBJECT);
return;
}
- final OIDCAuthenticationResponseContext oidcResponseContext = getOidcResponseContext();
- assert oidcResponseContext != null;
oidcResponseContext.setRequestObjectValidated(true);
}
diff --git a/idp-oidc-extension-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml b/idp-oidc-extension-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml
index 7196b647..1a456b6a 100644
--- a/idp-oidc-extension-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml
+++ b/idp-oidc-extension-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml
@@ -621,6 +621,8 @@
value="#{T(net.shibboleth.oidc.profile.core.OidcError).INVALID_PKCE_TRANSFORMATION_METHOD}" />
<entry key="#{T(net.shibboleth.oidc.profile.core.OidcEventIds).INVALID_SCOPE}"
value="#{T(com.nimbusds.oauth2.sdk.OAuth2Error).INVALID_SCOPE}" />
+ <entry key="#{T(net.shibboleth.oidc.profile.core.OidcEventIds).MISSING_MANDATORY_PAR_REQUEST_URI}"
+ value="#{T(com.nimbusds.oauth2.sdk.OAuth2Error).INVALID_REQUEST}" />
</map>
</property>
</bean>
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 2ce554a8..81e8fcce 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
@@ -1003,6 +1003,17 @@
class="org.opensaml.storage.impl.client.PopulateClientStorageSaveContext" scope="prototype"
p:storageServices="#{ getObject('shibboleth.ClientStorageServices') ?: getObject('shibboleth.DefaultClientStorageServices') }" />
+ <bean id="ValidateRedirectURIForErrorHandling"
+ class="net.shibboleth.idp.plugin.oidc.op.oauth2.profile.impl.ValidateRedirectURI"
+ scope="prototype"
+ p:requireRequestedValue="true"
+ p:unregisteredClientPolicyEnforcer="#{getObject('shibboleth.oidc.UnregisteredClientPolicyEnforcer') ?: getObject('shibboleth.oidc.DefaultUnregisteredClientPolicyEnforcer')}">
+ <property name="activationCondition">
+ <bean parent="shibboleth.Conditions.Expression"
+ c:expression="#input.ensureOutboundMessageContext().ensureSubcontext(T(net.shibboleth.idp.plugin.oidc.op.messaging.context.OIDCAuthenticationResponseContext)).isRequestObjectFailure()" />
+ </property>
+ </bean>
+
<bean id="BuildErrorResponseFromEvent"
class="net.shibboleth.idp.plugin.oidc.op.profile.impl.BuildAuthenticationErrorResponseFromEvent" scope="prototype"
p:httpServletResponseSupplier-ref="shibboleth.HttpServletResponseSupplier"
diff --git a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-flow.xml b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-flow.xml
index a103849b..ed3dfba6 100644
--- a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-flow.xml
+++ b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/flows/oidc/authorize/authorize-flow.xml
@@ -338,6 +338,7 @@
<!-- Third we see if we are able to form a error response, if that fails we revert to generic error display -->
<decision-state id="BuildErrorResponse">
<on-entry>
+ <evaluate expression="ValidateRedirectURIForErrorHandling" />
<evaluate expression="BuildErrorResponseFromEvent" />
</on-entry>
<if
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 608ec2ea..d738f0d8 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
@@ -183,7 +183,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -203,7 +203,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -276,7 +276,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -301,7 +301,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
protected URI createParGeneratedRequestUri(final String clientId) {
@@ -444,7 +444,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -700,7 +700,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -825,7 +825,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -1029,7 +1029,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -1180,7 +1180,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -1326,7 +1326,7 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
initializeThreadLocals();
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
- Assert.assertEquals("ErrorView", result.getOutcome().getId());
+ assertErrorCode(result, "invalid_request");
}
@Test
@@ -2309,7 +2309,9 @@ public class AuthorizeFlowTest extends AbstractOidcFlowTest {
final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
assertFlowExecutionResult(result, FLOW_ID);
- Assert.assertEquals(result.getOutcome().getId(), "ErrorView");
+ if (!result.getOutcome().getId().equals("ErrorView")) {
+ assertErrorCode(result, "invalid_request_object");
+ }
}
protected void assertErrorResponseWithNoIssuer(final FlowExecutionResult result) {
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWETest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWETest.java
index c3f70c8c..00f14a7f 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWETest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWETest.java
@@ -264,7 +264,9 @@ public class RequestObjectJWETest extends IssuedEncryptedJWTTest {
setBasicAuth("jdoe", "changeit");
final FlowExecutionResult result = flowExecutor.launchExecution(flowId, null, externalContext);
removeMetadata(storageService, clientId);
- Assert.assertEquals(result.getOutcome().getId(), "ErrorView");
+ if (!result.getOutcome().getId().equals("ErrorView")) {
+ assertErrorCode(result, "invalid_request");
+ }
} catch (final IOException | URISyntaxException e) {
Assert.fail();
}
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWSTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWSTest.java
index 77b96207..06218911 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWSTest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RequestObjectJWSTest.java
@@ -119,7 +119,9 @@ public class RequestObjectJWSTest extends IssuedSignedJWTTest {
setBasicAuth("jdoe", "changeit");
final FlowExecutionResult result = flowExecutor.launchExecution(flowId, null, externalContext);
removeMetadata(storageService, clientId);
- Assert.assertEquals(result.getOutcome().getId(), "ErrorView");
+ if (!result.getOutcome().getId().equals("ErrorView")) {
+ assertErrorCode(result, "invalid_request");
+ }
} catch (final IOException | URISyntaxException e) {
Assert.fail();
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list