[java-idp-oidc] branch main updated: JOIDC-223 - IDP Session (cookies) not created after OIDC Login in IDP Version 5.1.3
Henri Mikkonen
henri.mikkonen at iki.fi
Fri Sep 6 06:57:11 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=e755a0be44b9595430a2c23251c0ad624e77841a
The following commit(s) were added to refs/heads/main by this push:
new e755a0be JOIDC-223 - IDP Session (cookies) not created after OIDC Login in IDP Version 5.1.3
e755a0be is described below
commit e755a0be44b9595430a2c23251c0ad624e77841a
Author: Henri Mikkonen <henri.mikkonen at iki.fi>
AuthorDate: Fri Sep 6 09:56:50 2024 +0300
JOIDC-223 - IDP Session (cookies) not created after OIDC Login in IDP Version 5.1.3
https://shibboleth.atlassian.net/browse/JOIDC-223
Refactored NimbusResponseEncoder not to use Nimbus JakartaServletUtils.applyHTTPResponse(..)
for applying response contents into the servlet response. Now all the other headers except
'Location' are added to the servlet response like before, but the first value of Location
-header is used for the sendRedirect(..) method.
Also harmonised writing/closing the reseponse content.
Improved flow tests: some of them with multiple calls to authorize endpoint needed to have
extra initializations of servlet request/response objects in order to prevent them for
inheriting the state from previous call.
---
.../op/encoding/impl/NimbusResponseEncoder.java | 77 +++++++++++++++++++---
.../flow/AbstractIssuedJWTSecurityTest.java | 9 ++-
.../op/profile/flow/PushedAuthorizeFlowTest.java | 14 ++--
.../oidc/op/profile/flow/RequestObjectJWETest.java | 6 +-
4 files changed, 83 insertions(+), 23 deletions(-)
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/encoding/impl/NimbusResponseEncoder.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/encoding/impl/NimbusResponseEncoder.java
index 07f65a13..c9c5954c 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/encoding/impl/NimbusResponseEncoder.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/encoding/impl/NimbusResponseEncoder.java
@@ -16,9 +16,11 @@ package net.shibboleth.idp.plugin.oidc.op.encoding.impl;
import java.io.IOException;
import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
import java.io.StringWriter;
import java.io.Writer;
import java.util.List;
+import java.util.Map;
import java.util.Map.Entry;
import javax.annotation.Nonnull;
@@ -27,13 +29,13 @@ import org.apache.velocity.VelocityContext;
import org.apache.velocity.app.VelocityEngine;
import org.opensaml.messaging.encoder.MessageEncodingException;
import org.opensaml.messaging.encoder.servlet.AbstractHttpServletResponseMessageEncoder;
+import org.slf4j.Logger;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.nimbusds.oauth2.sdk.AuthorizationResponse;
import com.nimbusds.oauth2.sdk.Response;
import com.nimbusds.oauth2.sdk.ResponseMode;
import com.nimbusds.oauth2.sdk.http.HTTPResponse;
-import com.nimbusds.oauth2.sdk.http.JakartaServletUtils;
import jakarta.servlet.http.HttpServletResponse;
import net.shibboleth.shared.annotation.constraint.NonnullAfterInit;
@@ -41,15 +43,22 @@ import net.shibboleth.shared.annotation.constraint.NotEmpty;
import net.shibboleth.shared.codec.HTMLEncoder;
import net.shibboleth.shared.codec.StringDigester;
import net.shibboleth.shared.logic.Constraint;
+import net.shibboleth.shared.primitive.LoggerFactory;
import net.shibboleth.shared.security.IdentifierGenerationStrategy;
import net.shibboleth.shared.servlet.HttpServletSupport;
/**
* A message encodes that encodes the Nimbus {@link Response} in the message context inside the attached
- * {@link HttpServletResponse}.
+ * {@link HttpServletResponse}. All the other headers found from the {@link Response} except <pre>Location</pre> are
+ * included to the {@link HttpServletResponse}. The Location header is ignored, but its value is given as the parameter
+ * to {@link HttpServletResponse#sendRedirect(String)}. The response is assumed not to committed before this encoder is
+ * used.
*/
public class NimbusResponseEncoder extends AbstractHttpServletResponseMessageEncoder {
+ /** Class logger. */
+ @Nonnull private final Logger log = LoggerFactory.getLogger(NimbusResponseEncoder.class);
+
/** Default template ID for using FORM POST response mode. */
@Nonnull @NotEmpty public static final String DEFAULT_TEMPLATE_ID = "/templates/oidc-form-post.vm";
@@ -188,26 +197,74 @@ public class NimbusResponseEncoder extends AbstractHttpServletResponseMessageEnc
HttpServletSupport.addNoCacheHeaders(response);
HttpServletSupport.setUTF8Encoding(response);
HttpServletSupport.setContentType(response, "text/html");
- final Writer out = new OutputStreamWriter(response.getOutputStream(), "UTF-8");
- engine.mergeTemplate(velocityTemplateId, "UTF-8", context, out);
- out.flush();
- out.close();
+ try (final Writer out = new OutputStreamWriter(response.getOutputStream(), "UTF-8")) {
+ engine.mergeTemplate(velocityTemplateId, "UTF-8", context, out);
+ out.flush();
+ }
// Write it also to log
- final StringWriter writer = new StringWriter();
- engine.mergeTemplate(velocityTemplateId, "UTF-8", context, writer);
- getProtocolMessageLogger().trace("Outbound response {}", ResponseUtil.toString(response, writer.toString()));
+ try (final StringWriter writer = new StringWriter()) {
+ engine.mergeTemplate(velocityTemplateId, "UTF-8", context, writer);
+ getProtocolMessageLogger().trace("Outbound response {}",
+ ResponseUtil.toString(response, writer.toString()));
+ }
return;
}
final HTTPResponse resp = ((Response) message).toHTTPResponse();
- JakartaServletUtils.applyHTTPResponse(resp, response);
+
+ response.setStatus(resp.getStatusCode());
+ final String redirect = processHeaders(resp.getHeaderMap(), response);
+ if (redirect != null) {
+ log.debug("Sending redirect to {}", redirect);
+ response.sendRedirect(redirect);
+ } else if (resp.getContent() != null) {
+ if (resp.getEntityContentType() != null) {
+ response.setContentType(resp.getEntityContentType().toString());
+ }
+ try (final PrintWriter out = response.getWriter()) {
+ out.print(resp.getContent());
+ out.flush();
+ }
+ }
+
for (final String header : response.getHeaderNames()) {
resp.setHeader(header, response.getHeader(header));
}
+
getProtocolMessageLogger().trace("Outbound response {}", ResponseUtil.toString(resp, objectMapper));
} catch (final IOException e) {
throw new MessageEncodingException("Problem encoding response", e);
}
}
+
+ /**
+ * Adds all the other given headers to the given {@link HttpServletResponse} except the location header, whose
+ * first value is returned by this method if any value exists.
+ *
+ * @param headers the headers to be added to the servlet response
+ * @param response the servlet response for which the headers are populated
+ * @return the first value of Location header, if any value existed in the given map of headers
+ */
+ @Nullable protected String processHeaders(@Nullable final Map<String, List<String>> headers,
+ @Nonnull final HttpServletResponse response) {
+ if (headers != null) {
+ for (final String header : headers.keySet()) {
+ if (!"Location".equalsIgnoreCase(header)) {
+ for (final String value : headers.get(header)) {
+ response.addHeader(header, value);
+ }
+ }
+ }
+ final List<String> location = headers.get("Location");
+ if (location != null && !location.isEmpty()) {
+ if (location.size() > 1) {
+ log.warn("More than one ({}) values found for the Location-header, using first {}",
+ location.size(), location.get(0));
+ }
+ return location.get(0);
+ }
+ }
+ return null;
+ }
/** {@inheritDoc} */
@Override
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AbstractIssuedJWTSecurityTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AbstractIssuedJWTSecurityTest.java
index 9767b4f8..23d570ba 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AbstractIssuedJWTSecurityTest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/AbstractIssuedJWTSecurityTest.java
@@ -309,7 +309,6 @@ public abstract class AbstractIssuedJWTSecurityTest extends AbstractOidcFlowTest
Assert.assertTrue(SignedJWT.class.isInstance(jwt), "Expected SignedJWT, obtained " + jwt);
} else {
Assert.assertNull(jwt);
-
}
}
}
@@ -469,6 +468,8 @@ public abstract class AbstractIssuedJWTSecurityTest extends AbstractOidcFlowTest
protected JWT obtainIdTokenFromAuthorizeEndpoint(final String clientId, final String clientSecret,
final PublicKey publicKey,final JWSAlgorithm storedJwsAlgorithm, final JWEAlgorithm storedJweAlgorithm,
final EncryptionMethod storedJweMethod) {
+ initializeMocks();
+ initializeThreadLocals();
setBasicAuth("jdoe", "changeit");
request.setMethod("GET");
final String redirectUri = "https://example.org/cb";
@@ -492,8 +493,6 @@ public abstract class AbstractIssuedJWTSecurityTest extends AbstractOidcFlowTest
Assert.fail(e.getMessage(), e);
}
- initializeThreadLocals();
-
final FlowExecutionResult result = flowExecutor.launchExecution(flowId, null, externalContext);
try {
super.removeMetadata(storageService, clientId);
@@ -513,6 +512,8 @@ public abstract class AbstractIssuedJWTSecurityTest extends AbstractOidcFlowTest
protected JWT obtainAccessTokenFromAuthorizeEndpoint(final String clientId, final String clientSecret,
final PublicKey publicKey,final JWSAlgorithm storedJwsAlgorithm, final JWEAlgorithm storedJweAlgorithm,
final EncryptionMethod storedJweMethod) {
+ initializeMocks();
+ initializeThreadLocals();
setBasicAuth("jdoe", "changeit");
request.setMethod("GET");
final String redirectUri = "https://example.org/cb";
@@ -549,8 +550,6 @@ public abstract class AbstractIssuedJWTSecurityTest extends AbstractOidcFlowTest
Assert.fail(e.getMessage(), e);
}
- initializeThreadLocals();
-
final FlowExecutionResult result = flowExecutor.launchExecution(flowId, null, externalContext);
try {
super.removeMetadata(storageService, clientId);
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/PushedAuthorizeFlowTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/PushedAuthorizeFlowTest.java
index 3880c86d..0258494d 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/PushedAuthorizeFlowTest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/PushedAuthorizeFlowTest.java
@@ -408,9 +408,10 @@ public class PushedAuthorizeFlowTest extends AbstractOidcClientAuthenticationFlo
}
protected void verifyAuthorizeEndpoint(final String clientId, final String requestUri) {
+ initializeMocks();
+ initializeThreadLocals();
setBasicAuth("jdoe", "changeit");
request.setMethod("GET");
- request.removeAllParameters();
final String redirectUri = "https://example.org/cb";
AuthorizeFlowTest.setRequestParameters(request, List.of(new Pair<>("client_id", clientId),
@@ -418,17 +419,18 @@ public class PushedAuthorizeFlowTest extends AbstractOidcClientAuthenticationFlo
new Pair<>("redirect_uri", redirectUri),
new Pair<>("request_uri", requestUri)));
- initializeThreadLocals();
-
final FlowExecutionResult result = flowExecutor.launchExecution("oidc/authorize", null, externalContext);
Assert.assertEquals(result.getOutcome().getId(), END_STATE_ID);
+ initializeMocks();
+ initializeThreadLocals();
+
final FlowExecutionResult replayResult = flowExecutor.launchExecution("oidc/authorize", null, externalContext);
Assert.assertEquals(replayResult.getOutcome().getId(), "ErrorView");
- request.removeAllParameters();
- request.removeHeader("Authorization");
-
+ // re-initialize request/response as this method is called from loops that assume clear state for them
+ initializeMocks();
+ initializeThreadLocals();
}
protected static Map<String,String> createRequestParameters(final String id) {
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 00f14a7f..61b8e375 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
@@ -240,6 +240,8 @@ public class RequestObjectJWETest extends IssuedEncryptedJWTTest {
protected void assertErrorRequestObjectResponse(final String requestObject,
final JWSAlgorithm requestObjectSigAlg, final JWEAlgorithm requestObjectEncAlg,
final EncryptionMethod requestObjectEncMethod, final String clientSecret, final PublicKey publicKey) {
+ initializeMocks();
+ initializeThreadLocals();
request.setMethod("GET");
final String clientId = encryptionOptional ? defaultClientId : defaultClientIdEncryptionEnforced;
final String redirectUri = "https://example.org/cb";
@@ -248,7 +250,6 @@ public class RequestObjectJWETest extends IssuedEncryptedJWTTest {
new Pair<>("scope", "openid profile"),
new Pair<>("redirect_uri", redirectUri),
new Pair<>("request", requestObject)));
- initializeThreadLocals();
final OIDCClientMetadata metadata = buildMetadataSkeleton();
metadata.setScope(new Scope("openid"));
@@ -276,6 +277,8 @@ public class RequestObjectJWETest extends IssuedEncryptedJWTTest {
protected void assertSuccessRequestObjectResponse(final String requestObject,
final JWSAlgorithm requestObjectSigAlg, final JWEAlgorithm requestObjectEncAlg,
final EncryptionMethod requestObjectEncMethod, final String clientSecret, final PublicKey publicKey) {
+ initializeMocks();
+ initializeThreadLocals();
request.setMethod("GET");
final String clientId = encryptionOptional ? defaultClientId : defaultClientIdEncryptionEnforced;
final String redirectUri = "https://example.org/cb";
@@ -284,7 +287,6 @@ public class RequestObjectJWETest extends IssuedEncryptedJWTTest {
new Pair<>("scope", "openid profile"),
new Pair<>("redirect_uri", redirectUri),
new Pair<>("request", requestObject)));
- initializeThreadLocals();
final OIDCClientMetadata metadata = buildMetadataSkeleton();
metadata.setScope(new Scope("openid"));
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list