You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tomee.apache.org by rz...@apache.org on 2026/04/23 13:51:01 UTC
(tomee) 23/23: security/openid: pick token-endpoint auth method by OP-advertised order
This is an automated email from the ASF dual-hosted git repository.
rzo1 pushed a commit to branch security-4.0
in repository https://gitbox.apache.org/repos/asf/tomee.git
commit 31f7bfa4ecdb80a2033987c9c523c524f55b5f27
Author: Richard Zowalla <rz...@apache.org>
AuthorDate: Thu Apr 23 15:47:58 2026 +0200
security/openid: pick token-endpoint auth method by OP-advertised order
preferBasicAuth() previously returned true as soon as client_secret_basic
appeared anywhere in token_endpoint_auth_methods_supported, ignoring the
OP's advertised preference order. Form-only OPs that legally list both
methods with client_secret_post first - including the Jakarta Security 4.0
TCK mock provider used by app-openid - therefore got an empty form body and
returned 500 invalid_client_id, propagating as HTTP 500 on /Callback.
Walk the array in order and pick the first method we know how to speak;
fall back to Basic only when no list is published or none of the listed
methods are implemented (RFC 6749 / OIDC Core §9 default).
Update the existing unit test to assert order-driven selection (post-first
=> Post wins, basic-first => Basic wins) so the contract is locked in.
With this change Security 4.0 TCK app-openid (OpenIdDefaultIT,
OpenIdWithELIT, InvalidRedirectURIIT) passes and the full security TCK
reports 26/26 modules green.
---
.../cdi/OpenIdAuthenticationMechanism.java | 16 +++--
.../cdi/OpenIdAuthenticationMechanismUnitTest.java | 79 ++++++++++++++++++++--
2 files changed, 86 insertions(+), 9 deletions(-)
diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java
index 504009575d..06bc23aeee 100644
--- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java
+++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java
@@ -442,9 +442,12 @@ public class OpenIdAuthenticationMechanism implements HttpAuthenticationMechanis
* default OP-mandated behaviour. The spec-level {@link jakarta.security.enterprise.authentication.mechanism.http.openid.OpenIdProviderMetadata
* OpenIdProviderMetadata} annotation does not expose {@code token_endpoint_auth_methods_supported},
* but when TomEE discovers the OP via {@code providerURI} the raw JSON document is available
- * on {@link CompositeOpenIdProviderMetadata}. If the discovered list advertises
- * {@code client_secret_post} without {@code client_secret_basic} we fall back to form parameters;
- * otherwise (list absent, empty, or contains Basic) we prefer Basic.</p>
+ * on {@link CompositeOpenIdProviderMetadata}. When that list is present we walk it in the
+ * order the OP advertised and pick the first method we know how to speak ({@code client_secret_basic}
+ * or {@code client_secret_post}); other listed methods (e.g. {@code client_secret_jwt},
+ * {@code private_key_jwt}, {@code tls_client_auth}) are skipped because this RP does not
+ * implement them. With no list (annotation-only config or empty discovery field) we fall
+ * back to Basic, the OIDC default.</p>
*/
protected boolean preferBasicAuth() {
final Object providerMetadata = getDefinition().providerMetadata();
@@ -462,10 +465,13 @@ public class OpenIdAuthenticationMechanism implements HttpAuthenticationMechanis
if ("client_secret_basic".equalsIgnoreCase(method)) {
return true;
}
+ if ("client_secret_post".equalsIgnoreCase(method)) {
+ return false;
+ }
}
- // Advertised list exists and does not include client_secret_basic => must use form.
- return false;
+ // List exists but advertises no method we implement; OIDC default is Basic.
+ return true;
}
protected String basicAuthHeader() {
diff --git a/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java b/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java
index 19cb6c2b44..ba2d912838 100644
--- a/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java
+++ b/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java
@@ -347,7 +347,7 @@ public class OpenIdAuthenticationMechanismUnitTest {
}
@Test
- public void tokenEndpointUsesClientSecretBasicWhenDiscoveryAdvertisesIt() throws Exception {
+ public void tokenEndpointHonoursOpAdvertisedOrder() throws Exception {
final AtomicReference<String> capturedAuthHeader = new AtomicReference<>();
final AtomicReference<String> capturedBody = new AtomicReference<>();
@@ -367,7 +367,9 @@ public class OpenIdAuthenticationMechanismUnitTest {
server.start();
try {
- // Discovery advertises both methods; Basic must win because OIDC Core §9 names it as the default.
+ // Discovery advertises both methods with client_secret_post listed FIRST. The RP must
+ // honour the OP's advertised order rather than always defaulting to Basic, otherwise
+ // form-only OPs (like the Jakarta Security 4.0 TCK mock) reject the token request.
final JsonObject discovery = Json.createObjectBuilder()
.add(OpenIdConstant.TOKEN_ENDPOINT,
"http://127.0.0.1:" + server.getAddress().getPort() + "/token")
@@ -399,11 +401,80 @@ public class OpenIdAuthenticationMechanismUnitTest {
AuthenticationStatus status = authenticationMechanism.performAuthentication(request, response, messageContext);
assertEquals(AuthenticationStatus.SUCCESS, status);
- assertNotNull(capturedAuthHeader.get());
+ assertNull("Authorization header must be absent when OP lists client_secret_post first",
+ capturedAuthHeader.get());
+
+ Map<String, String> formParams = parseFormBody(capturedBody.get());
+ assertTrue("client_id must be in form body when using client_secret_post",
+ formParams.containsKey(OpenIdConstant.CLIENT_ID));
+ assertTrue("client_secret must be in form body when using client_secret_post",
+ formParams.containsKey(OpenIdConstant.CLIENT_SECRET));
+ } finally {
+ server.stop(0);
+ }
+ }
+
+ @Test
+ public void tokenEndpointHonoursOpAdvertisedOrderBasicFirst() throws Exception {
+ // Mirror of tokenEndpointHonoursOpAdvertisedOrder with the order flipped; locks in
+ // that the array order, not just membership, drives the choice.
+ final AtomicReference<String> capturedAuthHeader = new AtomicReference<>();
+ final AtomicReference<String> capturedBody = new AtomicReference<>();
+
+ HttpServer server = HttpServer.create(new InetSocketAddress(0), 0);
+ server.createContext("/token", exchange -> {
+ capturedAuthHeader.set(exchange.getRequestHeaders().getFirst("Authorization"));
+ capturedBody.set(readRequestBody(exchange));
+
+ byte[] body = "{\"token_type\":\"Bearer\",\"access_token\":\"ACCESS\",\"id_token\":\"ID\",\"expires_in\":3600}"
+ .getBytes(StandardCharsets.UTF_8);
+
+ exchange.getResponseHeaders().set("Content-Type", "application/json");
+ exchange.sendResponseHeaders(200, body.length);
+ exchange.getResponseBody().write(body);
+ exchange.close();
+ });
+ server.start();
+
+ try {
+ final JsonObject discovery = Json.createObjectBuilder()
+ .add(OpenIdConstant.TOKEN_ENDPOINT,
+ "http://127.0.0.1:" + server.getAddress().getPort() + "/token")
+ .add(OpenIdConstant.TOKEN_ENDPOINT_AUTH_METHODS_SUPPORTED,
+ Json.createArrayBuilder().add("client_secret_basic").add("client_secret_post"))
+ .build();
+ final OpenIdProviderMetadata overrideDefaults =
+ DefaultOverrideHolder.class.getAnnotation(OpenIdProviderMetadata.class);
+ final CompositeOpenIdProviderMetadata composite =
+ new CompositeOpenIdProviderMetadata(discovery, overrideDefaults);
+
+ when(definition.providerMetadata()).thenReturn(composite);
+ when(identityStoreHandler.validate(any()))
+ .thenReturn(new CredentialValidationResult("caller", Set.of("users")));
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ HttpMessageContext messageContext = mock(HttpMessageContext.class, Answers.RETURNS_DEEP_STUBS);
+
+ when(request.getParameter(OpenIdConstant.STATE)).thenReturn("STATE");
+ when(request.getParameter(OpenIdConstant.CODE)).thenReturn("CODE");
+ when(request.getRequestURL()).thenReturn(new StringBuffer("https://example.com/redirect"));
+ when(request.getRequestURI()).thenReturn("/redirect");
+ when(messageContext.notifyContainerAboutLogin(any(CredentialValidationResult.class)))
+ .thenReturn(AuthenticationStatus.SUCCESS);
+
+ storageHandler.set(request, response, OpenIdStorageHandler.STATE_KEY, "STATE");
+
+ AuthenticationStatus status = authenticationMechanism.performAuthentication(request, response, messageContext);
+
+ assertEquals(AuthenticationStatus.SUCCESS, status);
+ assertNotNull("Authorization header must be present when OP lists client_secret_basic first",
+ capturedAuthHeader.get());
assertTrue(capturedAuthHeader.get().startsWith("Basic "));
Map<String, String> formParams = parseFormBody(capturedBody.get());
- assertFalse(formParams.containsKey(OpenIdConstant.CLIENT_SECRET));
+ assertFalse("client_secret must NOT be in form body when using client_secret_basic",
+ formParams.containsKey(OpenIdConstant.CLIENT_SECRET));
} finally {
server.stop(0);
}