Fix webauthn multifactor authentication

config/src/integration-test/java/org/springframework/security/config/annotation/configurers/WebAuthnWebDriverTests.java

@@ -31,6 +31,7 @@
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.openqa.selenium.By;
 import org.openqa.selenium.WebDriverException;
@@ -55,6 +56,7 @@
 import org.springframework.security.provisioning.InMemoryUserDetailsManager;
 import org.springframework.security.web.FilterChainProxy;
 import org.springframework.security.web.SecurityFilterChain;
+import org.springframework.util.StringUtils;
 import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
 import org.springframework.web.filter.DelegatingFilterProxy;
 import org.springframework.web.servlet.config.annotation.EnableWebMvc;
@@ -67,7 +69,7 @@
  *
  * @author Daniel Garnier-Moiroux
  */
-@org.junit.jupiter.api.Disabled
+@Disabled
 class WebAuthnWebDriverTests {
 
 	private String baseUrl;
@@ -82,6 +84,8 @@ class WebAuthnWebDriverTests {
 
 	private static final String PASSWORD = "password";
 
+	private String authenticatorId = null;
+
 	@BeforeAll
 	static void startChromeDriverService() throws Exception {
 		driverService = new ChromeDriverService.Builder().usingAnyFreePort().build();
@@ -144,7 +148,7 @@ void cleanupDriver() {
 	@Test
 	void loginWhenNoValidAuthenticatorCredentialsThenRejects() {
 		createVirtualAuthenticator(true);
-		this.driver.get(this.baseUrl);
+		this.getAndWait("/", "/login");
 		this.driver.findElement(signinWithPasskeyButton()).click();
 		await(() -> assertThat(this.driver.getCurrentUrl()).endsWith("/login?error"));
 	}
@@ -153,7 +157,7 @@ void loginWhenNoValidAuthenticatorCredentialsThenRejects() {
 	void registerWhenNoLabelThenRejects() {
 		login();
 
-		this.driver.get(this.baseUrl + "/webauthn/register");
+		this.getAndWait("/webauthn/register");
 
 		this.driver.findElement(registerPasskeyButton()).click();
 		assertHasAlertStartingWith("error", "Error: Passkey Label is required");
@@ -163,7 +167,7 @@ void registerWhenNoLabelThenRejects() {
 	void registerWhenAuthenticatorNoUserVerificationThenRejects() {
 		createVirtualAuthenticator(false);
 		login();
-		this.driver.get(this.baseUrl + "/webauthn/register");
+		this.getAndWait("/webauthn/register");
 		this.driver.findElement(passkeyLabel()).sendKeys("Virtual authenticator");
 		this.driver.findElement(registerPasskeyButton()).click();
 
@@ -178,7 +182,8 @@ void registerWhenAuthenticatorNoUserVerificationThenRejects() {
 	 * <li>Step 1: Log in with username / password</li>
 	 * <li>Step 2: Register a credential from the virtual authenticator</li>
 	 * <li>Step 3: Log out</li>
-	 * <li>Step 4: Log in with the authenticator</li>
+	 * <li>Step 4: Log in with the authenticator (no allowCredentials)</li>
+	 * <li>Step 5: Log in again with the same authenticator (with allowCredentials)</li>
 	 * </ul>
 	 */
 	@Test
@@ -190,7 +195,7 @@ void loginWhenAuthenticatorRegisteredThenSuccess() {
 		login();
 
 		// Step 2: register a credential from the virtual authenticator
-		this.driver.get(this.baseUrl + "/webauthn/register");
+		this.getAndWait("/webauthn/register");
 		this.driver.findElement(passkeyLabel()).sendKeys("Virtual authenticator");
 		this.driver.findElement(registerPasskeyButton()).click();
 
@@ -212,9 +217,58 @@ void loginWhenAuthenticatorRegisteredThenSuccess() {
 		logout();
 
 		// Step 4: log in with the virtual authenticator
-		this.driver.get(this.baseUrl + "/webauthn/register");
+		this.getAndWait("/webauthn/register", "/login");
 		this.driver.findElement(signinWithPasskeyButton()).click();
 		await(() -> assertThat(this.driver.getCurrentUrl()).endsWith("/webauthn/register?continue"));
+
+		// Step 5: authenticate while being already logged in
+		// This simulates some use-cases with MFA. Since the user is already logged in,
+		// the "allowCredentials" property is populated
+		this.getAndWait("/login");
+		this.driver.findElement(signinWithPasskeyButton()).click();
+		await(() -> assertThat(this.driver.getCurrentUrl()).endsWith("/"));
+	}
+
+	@Test
+	void registerWhenAuthenticatorAlreadyRegisteredThenRejects() {
+		createVirtualAuthenticator(true);
+		login();
+		registerAuthenticator("Virtual authenticator");
+
+		// Cannot re-register the same authenticator because excludeCredentials
+		// is not empty and contains the given authenticator
+		this.driver.findElement(passkeyLabel()).sendKeys("Same authenticator");
+		this.driver.findElement(registerPasskeyButton()).click();
+
+		await(() -> assertHasAlertStartingWith("error", "Registration failed"));
+	}
+
+	@Test
+	void registerSecondAuthenticatorThenSucceeds() {
+		createVirtualAuthenticator(true);
+		login();
+
+		registerAuthenticator("Virtual authenticator");
+		this.getAndWait("/webauthn/register");
+		List<WebElement> passkeyRows = this.driver.findElements(passkeyTableRows());
+		assertThat(passkeyRows).hasSize(1)
+			.first()
+			.extracting((row) -> row.findElement(firstCell()))
+			.extracting(WebElement::getText)
+			.isEqualTo("Virtual authenticator");
+
+		// Create second authenticator and register
+		removeAuthenticator();
+		createVirtualAuthenticator(true);
+		registerAuthenticator("Second virtual authenticator");
+
+		this.getAndWait("/webauthn/register");
+
+		passkeyRows = this.driver.findElements(passkeyTableRows());
+		assertThat(passkeyRows).hasSize(2)
+			.extracting((row) -> row.findElement(firstCell()))
+			.extracting(WebElement::getText)
+			.contains("Second virtual authenticator");
 	}
 
 	/**
@@ -231,11 +285,14 @@ void loginWhenAuthenticatorRegisteredThenSuccess() {
 	 * "https://chromedevtools.github.io/devtools-protocol/tot/WebAuthn/">https://chromedevtools.github.io/devtools-protocol/tot/WebAuthn/</a>
 	 */
 	private void createVirtualAuthenticator(boolean userIsVerified) {
+		if (StringUtils.hasText(this.authenticatorId)) {
+			throw new IllegalStateException("Authenticator already exists, please remove it before re-creating one");
+		}
 		HasCdp cdpDriver = (HasCdp) this.driver;
 		cdpDriver.executeCdpCommand("WebAuthn.enable", Map.of("enableUI", false));
 		// this.driver.addVirtualAuthenticator(createVirtualAuthenticatorOptions());
 		//@formatter:off
-		cdpDriver.executeCdpCommand("WebAuthn.addVirtualAuthenticator",
+		Map<String, Object> cmdResponse = cdpDriver.executeCdpCommand("WebAuthn.addVirtualAuthenticator",
 				Map.of(
 						"options",
 						Map.of(
@@ -248,21 +305,38 @@ private void createVirtualAuthenticator(boolean userIsVerified) {
 						)
 				));
 		//@formatter:on
+		this.authenticatorId = cmdResponse.get("authenticatorId").toString();
+	}
+
+	private void removeAuthenticator() {
+		HasCdp cdpDriver = (HasCdp) this.driver;
+		cdpDriver.executeCdpCommand("WebAuthn.removeVirtualAuthenticator",
+				Map.of("authenticatorId", this.authenticatorId));
+		this.authenticatorId = null;
 	}
 
 	private void login() {
-		this.driver.get(this.baseUrl);
+		this.getAndWait("/", "/login");
 		this.driver.findElement(usernameField()).sendKeys(USERNAME);
 		this.driver.findElement(passwordField()).sendKeys(PASSWORD);
 		this.driver.findElement(signinWithUsernamePasswordButton()).click();
+		// Ensure login has completed
+		await(() -> assertThat(this.driver.getCurrentUrl()).doesNotContain("/login"));
 	}
 
 	private void logout() {
-		this.driver.get(this.baseUrl + "/logout");
+		this.getAndWait("/logout");
 		this.driver.findElement(logoutButton()).click();
 		await(() -> assertThat(this.driver.getCurrentUrl()).endsWith("/login?logout"));
 	}
 
+	private void registerAuthenticator(String passkeyName) {
+		this.getAndWait("/webauthn/register");
+		this.driver.findElement(passkeyLabel()).sendKeys(passkeyName);
+		this.driver.findElement(registerPasskeyButton()).click();
+		await(() -> assertThat(this.driver.getCurrentUrl()).endsWith("/webauthn/register?success"));
+	}
+
 	private AbstractStringAssert<?> assertHasAlertStartingWith(String alertType, String alertMessage) {
 		WebElement alert = this.driver.findElement(new By.ById(alertType));
 		assertThat(alert.isDisplayed())
@@ -289,6 +363,15 @@ private void await(Supplier<AbstractAssert<?, ?>> assertion) {
 			});
 	}
 
+	private void getAndWait(String endpoint) {
+		this.getAndWait(endpoint, endpoint);
+	}
+
+	private void getAndWait(String endpoint, String redirectUrl) {
+		this.driver.get(this.baseUrl + endpoint);
+		this.await(() -> assertThat(this.driver.getCurrentUrl()).endsWith(redirectUrl));
+	}
+
 	private static By.ById passkeyLabel() {
 		return new By.ById("label");
 	}
@@ -325,6 +408,10 @@ private static By.ByCssSelector logoutButton() {
 		return new By.ByCssSelector("button");
 	}
 
+	private static By.ByCssSelector deletePasskeyButton() {
+		return new By.ByCssSelector("table > tbody > tr > button");
+	}
+
 	/**
 	 * The configuration for WebAuthN tests. It accesses the Server's current port, so we
 	 * can configurer WebAuthnConfigurer#allowedOrigin

webauthn/src/main/java/org/springframework/security/web/webauthn/management/Webauthn4JRelyingPartyOperations.java

@@ -95,7 +95,7 @@ public class Webauthn4JRelyingPartyOperations implements WebAuthnRelyingPartyOpe
 
 	private final PublicKeyCredentialRpEntity rp;
 
-	private final ObjectConverter objectConverter = new ObjectConverter();
+	private ObjectConverter objectConverter = new ObjectConverter();
 
 	private final AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl();
 
@@ -137,6 +137,15 @@ public void setWebAuthnManager(WebAuthnManager webAuthnManager) {
 		this.webAuthnManager = webAuthnManager;
 	}
 
+	/**
+	 * Sets the {@link ObjectConverter} to use.
+	 * @param objectConverter the {@link ObjectConverter} to use. Cannot be null.
+	 */
+	void setObjectConverter(ObjectConverter objectConverter) {
+		Assert.notNull(objectConverter, "objectConverter cannot be null");
+		this.objectConverter = objectConverter;
+	}
+
 	/**
 	 * Sets a {@link Consumer} used to customize the
 	 * {@link PublicKeyCredentialCreationOptionsBuilder} for
@@ -390,7 +399,7 @@ public PublicKeyCredentialUserEntity authenticate(RelyingPartyAuthenticationRequ
 			.getUserVerification() == UserVerificationRequirement.REQUIRED;
 
 		com.webauthn4j.data.AuthenticationRequest authenticationRequest = new com.webauthn4j.data.AuthenticationRequest(
-				request.getPublicKey().getId().getBytes(), assertionResponse.getAuthenticatorData().getBytes(),
+				request.getPublicKey().getRawId().getBytes(), assertionResponse.getAuthenticatorData().getBytes(),
 				assertionResponse.getClientDataJSON().getBytes(), assertionResponse.getSignature().getBytes());
 
 		// CollectedClientData and ExtensionsClientOutputs is registration data, and can

webauthn/src/test/java/org/springframework/security/web/webauthn/management/Webauthn4jRelyingPartyOperationsTests.java

@@ -27,15 +27,20 @@
 import com.fasterxml.jackson.databind.node.JsonNodeFactory;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.fasterxml.jackson.dataformat.cbor.CBORFactory;
+import com.webauthn4j.WebAuthnManager;
 import com.webauthn4j.converter.AttestationObjectConverter;
 import com.webauthn4j.converter.util.ObjectConverter;
+import com.webauthn4j.data.AuthenticationData;
+import com.webauthn4j.data.AuthenticationRequest;
 import com.webauthn4j.data.attestation.AttestationObject;
+import com.webauthn4j.data.attestation.authenticator.AttestedCredentialData;
 import com.webauthn4j.data.attestation.authenticator.AuthenticatorData;
 import com.webauthn4j.data.extension.authenticator.RegistrationExtensionAuthenticatorOutput;
 import org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
@@ -44,30 +49,36 @@
 import org.springframework.security.core.authority.AuthorityUtils;
 import org.springframework.security.core.userdetails.PasswordEncodedUser;
 import org.springframework.security.core.userdetails.UserDetails;
+import org.springframework.security.web.webauthn.api.AuthenticatorAssertionResponse;
 import org.springframework.security.web.webauthn.api.AuthenticatorAttestationResponse;
 import org.springframework.security.web.webauthn.api.AuthenticatorAttestationResponse.AuthenticatorAttestationResponseBuilder;
 import org.springframework.security.web.webauthn.api.AuthenticatorSelectionCriteria;
 import org.springframework.security.web.webauthn.api.AuthenticatorTransport;
 import org.springframework.security.web.webauthn.api.Bytes;
 import org.springframework.security.web.webauthn.api.CredentialRecord;
+import org.springframework.security.web.webauthn.api.ImmutableCredentialRecord;
 import org.springframework.security.web.webauthn.api.PublicKeyCredential;
 import org.springframework.security.web.webauthn.api.PublicKeyCredentialCreationOptions;
 import org.springframework.security.web.webauthn.api.PublicKeyCredentialDescriptor;
 import org.springframework.security.web.webauthn.api.PublicKeyCredentialParameters;
 import org.springframework.security.web.webauthn.api.PublicKeyCredentialRequestOptions;
 import org.springframework.security.web.webauthn.api.PublicKeyCredentialRpEntity;
 import org.springframework.security.web.webauthn.api.PublicKeyCredentialUserEntity;
+import org.springframework.security.web.webauthn.api.TestAuthenticationAssertionResponses;
 import org.springframework.security.web.webauthn.api.TestAuthenticatorAttestationResponses;
 import org.springframework.security.web.webauthn.api.TestCredentialRecords;
 import org.springframework.security.web.webauthn.api.TestPublicKeyCredentialCreationOptions;
+import org.springframework.security.web.webauthn.api.TestPublicKeyCredentialRequestOptions;
 import org.springframework.security.web.webauthn.api.TestPublicKeyCredentialUserEntities;
 import org.springframework.security.web.webauthn.api.TestPublicKeyCredentials;
 import org.springframework.security.web.webauthn.api.UserVerificationRequirement;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.assertj.core.api.Assertions.assertThatRuntimeException;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verifyNoInteractions;
 
 @ExtendWith(MockitoExtension.class)
@@ -587,6 +598,50 @@ void createCredentialRequestOptionsWhenAuthenticated() {
 			.containsExactly(credentialRecord.getCredentialId());
 	}
 
+	// gh-18158
+	@Test
+	void authenticateThenWa4jRequestCredentialIdIsRawIdBytes() throws Exception {
+		PublicKeyCredentialRequestOptions options = TestPublicKeyCredentialRequestOptions.create().build();
+		AuthenticatorAssertionResponse response = TestAuthenticationAssertionResponses
+			.createAuthenticatorAssertionResponse()
+			.build();
+		PublicKeyCredential<AuthenticatorAssertionResponse> credentials = TestPublicKeyCredentials
+			.createPublicKeyCredential(response)
+			.build();
+		RelyingPartyAuthenticationRequest request = new RelyingPartyAuthenticationRequest(options, credentials);
+		PublicKeyCredential<AuthenticatorAssertionResponse> publicKey = request.getPublicKey();
+
+		ImmutableCredentialRecord credentialRecord = TestCredentialRecords.fullUserCredential().build();
+		given(this.userCredentials.findByCredentialId(publicKey.getRawId())).willReturn(credentialRecord);
+		ObjectMapper json = mock(ObjectMapper.class);
+		ObjectMapper cbor = mock(ObjectMapper.class);
+		given(cbor.getFactory()).willReturn(mock(CBORFactory.class));
+		AttestationObject attestationObject = mock(AttestationObject.class);
+		AuthenticatorData wa4jAuthData = mock(AuthenticatorData.class);
+		given(attestationObject.getAuthenticatorData()).willReturn(wa4jAuthData);
+		given(wa4jAuthData.getAttestedCredentialData()).willReturn(mock(AttestedCredentialData.class));
+		given(cbor.readValue(credentialRecord.getAttestationObject().getBytes(), AttestationObject.class))
+			.willReturn(attestationObject);
+		this.rpOperations.setObjectConverter(new ObjectConverter(json, cbor));
+
+		WebAuthnManager manager = mock(WebAuthnManager.class);
+		ArgumentCaptor<AuthenticationRequest> wa4jRequest = ArgumentCaptor.forClass(AuthenticationRequest.class);
+		AuthenticationData wa4jData = mock(AuthenticationData.class);
+		given(wa4jData.getAuthenticatorData()).willReturn(mock(AuthenticatorData.class));
+		given(manager.verify(wa4jRequest.capture(), any())).willReturn(wa4jData);
+		given(this.userEntities.findById(any())).willReturn(TestPublicKeyCredentialUserEntities.userEntity().build());
+		this.rpOperations.setWebAuthnManager(manager);
+
+		this.rpOperations.authenticate(request);
+
+		// this ensures that our next assertion is valid (we want the rawId bytes, not the
+		// id bytes to be used)
+		assertThat(publicKey.getRawId().getBytes()).isNotEqualTo(publicKey.getId().getBytes());
+		// ensure that the raw id bytes are passed into webauthn4j (not the id bytes which
+		// are base64 encoded)
+		assertThat(wa4jRequest.getValue().getCredentialId()).isEqualTo(publicKey.getRawId().getBytes());
+	}
+
 	private static AuthenticatorAttestationResponse setFlag(byte... flags) throws Exception {
 		AuthenticatorAttestationResponseBuilder authAttResponseBldr = TestAuthenticatorAttestationResponses
 			.createAuthenticatorAttestationResponse();