Merge branch 'ciba-fixes' into 'master'

FAPI-CIBA: Fix for 3 bugs

Closes #898, #897, and #894

See merge request openid/conformance-suite!999

src/main/java/net/openid/conformance/condition/client/AddMTLSEndpointAliasesToEnvironment.java

@@ -7,6 +7,13 @@
 import net.openid.conformance.testmodule.Environment;
 import net.openid.conformance.testmodule.OIDFJSON;
 
+/**
+ * This copies any server endpoints into the root of the environment, overriding it with any entry found in
+ * mtls_endpoint_aliases.
+ *
+ * It is assumed that, when a test needs to use mtls, it will use the value in the root of the
+ * environment.
+ */
 public class AddMTLSEndpointAliasesToEnvironment extends AbstractCondition {
 
 	@Override

src/main/java/net/openid/conformance/condition/client/CallBackchannelAuthenticationEndpoint.java

@@ -42,7 +42,8 @@ public class CallBackchannelAuthenticationEndpoint extends AbstractCondition {
 	@PostEnvironment(required = "backchannel_authentication_endpoint_response")
 	public Environment evaluate(Environment env) {
 
-		final String bcAuthEndpoint = env.getString("server", "backchannel_authentication_endpoint");
+		final String endpointKey = "backchannel_authentication_endpoint";
+		final String bcAuthEndpoint = env.getString(endpointKey) != null ? env.getString(endpointKey) : env.getString("server", endpointKey);
 		if (bcAuthEndpoint == null) {
 			throw error("Couldn't find backchannel authentication endpoint");
 		}

src/main/java/net/openid/conformance/condition/client/WaitFor5Seconds.java

@@ -0,0 +1,12 @@
+package net.openid.conformance.condition.client;
+
+import net.openid.conformance.condition.common.AbstractWaitForSpecifiedSeconds;
+import net.openid.conformance.testmodule.Environment;
+
+public class WaitFor5Seconds extends AbstractWaitForSpecifiedSeconds {
+
+	@Override
+	protected long getExpectedWaitSeconds(Environment env) {
+		return 5;
+	}
+}

src/main/java/net/openid/conformance/fapiciba/AbstractFAPICIBAID1.java

@@ -309,6 +309,12 @@ public void configure(JsonObject config, String baseUrl, String externalUrlOverr
 
 		if (supportMTLSEndpointAliases != null) {
 			call(sequence(supportMTLSEndpointAliases));
+			if (getVariant(ClientAuthType.class) != ClientAuthType.MTLS) {
+				// we only need to call the mtls aliased backchannel authentication endpoint when using mtls client auth
+				// (but need to use the mtls alias for the token endpoint whenever we're using certificate bound
+				// access tokens)
+				env.removeNativeValue("backchannel_authentication_endpoint");
+			}
 		}
 
 		// make sure the server configuration passes some basic sanity checks

src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1AuthReqIdExpired.java

@@ -5,6 +5,7 @@
 import net.openid.conformance.condition.client.CheckTokenEndpointHttpStatusNot200;
 import net.openid.conformance.condition.client.SleepUntilAuthReqExpires;
 import net.openid.conformance.condition.client.TellUserToIgnoreCIBAAuthentication;
+import net.openid.conformance.condition.client.WaitFor5Seconds;
 import net.openid.conformance.testmodule.PublishTestModule;
 import net.openid.conformance.variant.CIBAMode;
 
@@ -50,11 +51,12 @@ protected void waitForAuthenticationToComplete(long delaySeconds) {
 		callAndStopOnFailure(TellUserToIgnoreCIBAAuthentication.class);
 
 		setStatus(Status.WAITING);
+		callAndStopOnFailure(SleepUntilAuthReqExpires.class);
 		if (testType == CIBAMode.PING) {
-			// test resumes when notification endpoint called
-			return;
+			// a ping notification may or may not be issued; allow an extra 5 seconds to make sure any ping arrives
+			// before we continue
+			callAndStopOnFailure(WaitFor5Seconds.class);
 		}
-		callAndStopOnFailure(SleepUntilAuthReqExpires.class);
 		setStatus(Status.RUNNING);
 
 		callTokenEndpointAndFinishTest();
@@ -64,7 +66,8 @@ protected void waitForAuthenticationToComplete(long delaySeconds) {
 	protected void processNotificationCallback(JsonObject requestParts) {
 		if (testType == CIBAMode.PING) {
 			verifyNotificationCallback(requestParts);
-			callTokenEndpointAndFinishTest();
+			setStatus(Status.WAITING);
+			// test continues when the above sleep/wait completes
 		} else {
 			super.processNotificationCallback(requestParts);
 		}

src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1EnsureWrongAuthenticationRequestIdInTokenEndpointRequest.java

@@ -14,7 +14,7 @@
 @PublishTestModule(
 	testName = "fapi-ciba-id1-ensure-wrong-auth-req-id-in-token-endpoint-request",
 	displayName = "FAPI-CIBA-ID1: Ensure wrong auth_req_id in token endpoint request",
-	summary = "This test passes the clinent_2's information (e.g client_id, client_jwks, client_mutual_tls_authentication) and the client_1's auth_req_id in the token endpoint parameters to the one inside the request. The token endpoint server returned an error message that the grant permission is invalid.",
+	summary = "This test uses an auth_req_id issued to client 1 at the token endpoint, but authenticates as client 2. The token endpoint server must return an 'invalid_grant' error.",
 	profile = "FAPI-CIBA-ID1",
 	configurationFields = {
 		"server.discoveryUrl",

src/main/java/net/openid/conformance/testmodule/AbstractTestModule.java

@@ -8,6 +8,7 @@
 import net.openid.conformance.condition.Condition;
 import net.openid.conformance.condition.ConditionError;
 import net.openid.conformance.condition.client.SleepUntilAuthReqExpires;
+import net.openid.conformance.condition.client.WaitFor5Seconds;
 import net.openid.conformance.frontChannel.BrowserControl;
 import net.openid.conformance.info.ImageService;
 import net.openid.conformance.info.TestInfoService;
@@ -248,7 +249,12 @@ protected void call(ConditionCallBuilder builder) {
 		// We skip these checks for a condition that we deliberately call without the lock held so as not to block
 		// other threads; I suspect it means that this condition should have the ability to call setStatus or that
 		// their functionality should be in a method in AbstractTestModule instead
-		if (builder.getConditionClass() != SleepUntilAuthReqExpires.class) {
+		// Not all the 'WaitFor' functions are listed here; that means some of them we are calling with the lock held
+		// which may be problematic (e.g. it prevents any incoming connections being process and I suspect may prevent
+		// the test being aborted until the sleep expires). It would probably be preferable to always release the
+		// lock whilst sleeping (which is probably best achieve by one of the ways outlined in the previous paragraph.)
+		if (builder.getConditionClass() != SleepUntilAuthReqExpires.class &&
+			builder.getConditionClass() != WaitFor5Seconds.class) {
 			if (getStatus() != Status.CREATED) {
 				// We don't run this check for 'CREATED' as the lock is currently not held during 'configure'; see
 				// https://gitlab.com/openid/conformance-suite/issues/688