[java-idp-plugin-webauthn] branch main updated: Add more tests, minor code improvements
Phil Smart
philip.smart at jisc.ac.uk
Mon Jul 21 20:09:44 UTC 2025
This is an automated email from the git hooks/post-receive script.
philsmart pushed a commit to branch main
in repository java-idp-plugin-webauthn.
View the commit online:
http://git.shibboleth.net/view/?p=java-idp-plugin-webauthn.git;a=commit;h=08f44a1a9df3135fb9ddabd62af9913615ce2d83
The following commit(s) were added to refs/heads/main by this push:
new 08f44a1 Add more tests, minor code improvements
08f44a1 is described below
commit 08f44a1a9df3135fb9ddabd62af9913615ce2d83
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Mon Jul 21 21:09:42 2025 +0100
Add more tests, minor code improvements
---
.../IdPStorageServiceCredentialRepository.java | 10 +-
...BasedIdPStorageServiceCredentialRepository.java | 8 +-
...IdPStorageServiceCredentialRespositoryTest.java | 232 +++++++--------------
3 files changed, 83 insertions(+), 167 deletions(-)
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRepository.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRepository.java
index 34d469a..f77ef15 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRepository.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRepository.java
@@ -353,10 +353,8 @@ public class IdPStorageServiceCredentialRepository extends AbstractIdentifiableI
getId(), credentialId.getBase64Url(), usernameMapping);
credentialIdMappingCacheService.invalidate(credentialId);
} else {
- //TODO
- // Any of the credentials registered to that user can have the given credentialIdMappingCacheService,
- // although in practice they will likely all be the same.
- final boolean usernameUserHandleMappingIsValid =
+ // Test for valid credentials based on the credentialId given
+ final boolean usernameCredentialIdMappingIsValid =
credentials.stream().anyMatch(cr ->
cr.getCredential().getCredentialId().compareTo(credentialId)==0);
// All usernames contained in the credential set should match that in the map, otherwise the map is
@@ -370,7 +368,7 @@ public class IdPStorageServiceCredentialRepository extends AbstractIdentifiableI
getId(), credentialId.getBase64Url(), usernameMapping);
credentialIdMappingCacheService.invalidate(credentialId);
- } else if (!usernameUserHandleMappingIsValid){
+ } else if (!usernameCredentialIdMappingIsValid){
log.trace("{}: CredentialId '{}' to username '{}' mapping is inconsistent, "
+ "CredentialId is not present in any of '{}''s credentials, reloading",
getId(), credentialId.getBase64Url(), usernameMapping, usernameMapping);
@@ -1088,7 +1086,7 @@ public class IdPStorageServiceCredentialRepository extends AbstractIdentifiableI
return false;
}
- // Copy the credential into a new credential and update the last used time
+ // Copy the credential into a new credential and update the nickname
final CredentialRecord updatedRegistration = credential.get().toBuilder()
.withCredentialNickname(nickname)
.build();
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/StrategyBasedIdPStorageServiceCredentialRepository.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/StrategyBasedIdPStorageServiceCredentialRepository.java
index e73e81a..62ad320 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/StrategyBasedIdPStorageServiceCredentialRepository.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/StrategyBasedIdPStorageServiceCredentialRepository.java
@@ -138,9 +138,9 @@ public class StrategyBasedIdPStorageServiceCredentialRepository extends IdPStora
final Set<CredentialRecord> allCredentialRecords = new HashSet<>();
for (final StorageRecord<Set<CredentialRecord>> storageRecord : records) {
- final Set<CredentialRecord> individualRecords =
+ final Set<CredentialRecord> credentials =
storageRecord.getValue(getSerializer(), STORAGE_CONTEXT, userHandleB64);
- allCredentialRecords.addAll(individualRecords);
+ allCredentialRecords.addAll(credentials);
}
// Find the matching credentials, this checks the lookup function found only the correct credentials
final Set<CredentialRecord> matchingCredentials = allCredentialRecords.stream()
@@ -188,9 +188,9 @@ public class StrategyBasedIdPStorageServiceCredentialRepository extends IdPStora
final var serializer = getSerializer();
assert serializer != null;
- final Set<CredentialRecord> individualRecords =
+ final Set<CredentialRecord> credentials =
storageRecord.getValue(serializer, STORAGE_CONTEXT, credentialIdB64);
- allCredentialRecords.addAll(individualRecords);
+ allCredentialRecords.addAll(credentials);
}
// Find matching credentials, this checks the lookup function found only the correct credentials
final Set<CredentialRecord> matchingCredentials = allCredentialRecords.stream()
diff --git a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java
index 21ef17f..4aab08e 100644
--- a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java
+++ b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespositoryTest.java
@@ -24,6 +24,7 @@ import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
@@ -47,13 +48,13 @@ import net.shibboleth.idp.plugin.authn.webauthn.storage.impl.StorageServiceCrede
/**
* Tests for {@link IdPStorageServiceCredentialRepository}.
*/
+ at SuppressWarnings("null")
public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthnTest {
private StorageService storageService;
private IdPStorageServiceCredentialRepository repo;
- @SuppressWarnings("null")
@Override
@BeforeMethod
public void setup() throws Exception {
@@ -67,7 +68,8 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
repo = new IdPStorageServiceCredentialRepository();
repo.setId("test-repo");
repo.setSerializer(storageSerializer);
- repo.setStorageService(storageService);
+ repo.setStorageService(storageService);
+ // We are not mocking the cache
final var userHandleCacheService = CacheServiceImpl.builder().withCache(CacheBuilder.newBuilder()
.maximumSize(1000)
.build())
@@ -88,7 +90,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
mockAuthenticator = new MockAuthenticator(RPID);
}
- @SuppressWarnings("null")
@Test
public void testAddRegistrationByUsername() throws Exception {
@@ -209,7 +210,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
assertEquals(registrations.size(), 1);
}
- @SuppressWarnings("null")
@Test
public void testRemoveRegistrationByCredentialId_TwoRegistrationsSameAccount() throws Exception {
@@ -403,7 +403,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
registrationTwo.getCredential().getCredentialId());
}
- @SuppressWarnings("null")
@Test
public void testUpdateSignatureCounter() throws Exception {
@@ -434,7 +433,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
* Try an update with a signCount of 0, used by authenticators which do not implement the signature counter
* feature.
*/
- @SuppressWarnings("null")
@Test
public void testUpdateSignatureCounter_AuthenticatorDoesNotSupport() throws Exception {
@@ -460,8 +458,7 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
-
- @SuppressWarnings("null")
+
@Test
public void testUpdateSignatureCounter_TwoCredentials() throws Exception {
@@ -494,7 +491,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
- @SuppressWarnings("null")
@Test
public void testUpdateSignatureCounter_NoCredential() throws Exception {
@@ -580,6 +576,52 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
final String username = repo.getUsernameFromCredentialIdCache(registration.getCredential().getCredentialId());
assertEquals(username, "jdoe");
}
+
+ @Test
+ public void testGetUsernameFromCredentialIdCache_UsernameCredentialIdMappingInvalid() throws Exception {
+ final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ repo.getCredentialIdMappingCacheService().invalidate(registration.getCredential().getCredentialId());
+ // Add a new mapping with the same username but different credentialId
+ repo.getCredentialIdMappingCacheService().put(registration.toBuilder()
+ .withUsername("jdoe")
+ .withTransports(registration.getTransports())
+ .withRegistrationTime(registration.getRegistrationTime())
+ .withCredential(registration.getCredential().toBuilder()
+ .credentialId(new ByteArray("another-credentialId".getBytes())).build())
+ .build());
+
+ // Search for the other credentialId
+ final String username = repo.getUsernameFromCredentialIdCache(new ByteArray("another-credentialId".getBytes()));
+ assertEquals(username, "jdoe");
+ }
+
+ @Test(expectedExceptions = CredentialRepositoryException.class)
+ public void testGetUsernameFromCredentialIdCache_UsernameMappingInvalid() throws Exception {
+ final CredentialRecord registration = createRegistration("wrong-username", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ repo.getCredentialIdMappingCacheService().invalidate(registration.getCredential().getCredentialId());
+
+ // This registration maps to the key used for jdoe, but has a different credentialId, and the key does not
+ // map with 'wrong-username'
+ final CredentialRecord registrationTwo = registration.toBuilder()
+ .withUsername("jdoe")
+ .withTransports(registration.getTransports())
+ .withRegistrationTime(registration.getRegistrationTime())
+ .withCredential(registration.getCredential().toBuilder()
+ .credentialId(new ByteArray("another-credentialId".getBytes())).build())
+ .build();
+
+
+ repo.getCredentialIdMappingCacheService().put(registrationTwo);
+
+ // another-credentialId maps to jdoe, the key jdoe in the add method maps to a registration which has a username
+ // of wrong-username, so the username is inconsistent
+ repo.getUsernameFromCredentialIdCache(new ByteArray("another-credentialId".getBytes()));
+
+ }
@Test
public void testGetUsernameFromCredentialIdCache_NotInCache() throws Exception {
@@ -670,7 +712,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
-
@Test
public void testLookupAll() throws Exception {
@@ -703,8 +744,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
assertEquals(registrations.size(),0);
}
- //TODO lookup all with cache
-
@Test
public void testLookup() throws Exception {
@@ -856,7 +895,7 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
final CredentialRecord registration = createRegistration("jdoe-actual", "John Doe", userHandleBytes.getBytes());
repo.addRegistrationByUsername("jdoe", registration);
- final Optional<String> username = repo.getUsernameForUserHandle(userHandleBytes);
+ repo.getUsernameForUserHandle(userHandleBytes);
}
@@ -872,7 +911,7 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
repo.getUserHandleMappingCacheService().invalidateAll();
repo.getUserHandleMappingCacheService().put(createRegistration("jdoe", "John Doe", userHandleBytes.getBytes()));
- final Optional<String> username = repo.getUsernameForUserHandle(userHandleBytes);
+ repo.getUsernameForUserHandle(userHandleBytes);
}
@@ -961,7 +1000,7 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
repo.addRegistrationByUsername("jdoe", registration);
repo.addRegistrationByUsername("jdoe", registrationTwo);
- final Optional<String> username = repo.getUsernameForUserHandle(userHandleBytes);
+ repo.getUsernameForUserHandle(userHandleBytes);
}
@Test
@@ -1049,147 +1088,6 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
- /* Should result in two mappings in the index.*/
-// @SuppressWarnings("null")
-// @Test
-// public void testGetUsernameForUserHandle_DifferentUserHandleForSameUSer() throws Exception {
-//
-// final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
-// repo.addRegistrationByUsername("jdoe", registration);
-//
-// // Now add another for jdoe with different user handle
-// final CredentialRecord registrationTwo = createRegistration("jdoe", "John Doe", "different-user-handle".getBytes());
-// repo.addRegistrationByUsername("jdoe", registrationTwo);
-//
-// // And the index should have two mappings
-// final var mappingOne = repo.getStorageService().read(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// registration.getUserIdentity().getId().getBase64Url());
-// assertEquals(mappingOne.getValue(), "jdoe");
-//
-// final var mappingTwo = repo.getStorageService().read(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// registrationTwo.getUserIdentity().getId().getBase64Url());
-// assertEquals(mappingOne.getValue(), "jdoe");
-//
-//
-// final var usernameJdoe = repo.getUsernameForUserHandle(registration.getUserIdentity().getId());
-// assertTrue(usernameJdoe.isPresent());
-// assertEquals(usernameJdoe.get(), "jdoe");
-//
-//
-// final var usernameJdoeTwo = repo.getUsernameForUserHandle(registrationTwo.getUserIdentity().getId());
-// assertTrue(usernameJdoeTwo.isPresent());
-// assertEquals(usernameJdoeTwo.get(), "jdoe");
-//
-//
-// }
-
- /*
- * Tests for:
- * userHandle found (does not belong to that username)
- * <ol>
- * <li>choose username</li>
- * <li>lookup record</li>
- * <li>no credentials have the same userHandle</li>
- * <li>username FAIL</li>
- * <li>remove mapping</li>
- * <li>lookup ALL records</li>
- * <li>find username from ANY of the userHandle matches</li>
- * </ol>
- */
-// @SuppressWarnings("null")
-// @Test
-// public void testGetUsernameForUserHandle_MappingPointsToDifferetCredential() throws Exception {
-//
-// final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
-// repo.addRegistrationByUsername("jdoe", registration);
-//
-// // Now add another for bob
-// final CredentialRecord registrationTwo = createRegistration("bob", "Bob Doe", "bob-user-handle".getBytes());
-// repo.addRegistrationByUsername("bob", registrationTwo);
-//
-// // Remove any mappings that have been created
-// repo.getStorageService().delete(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// registration.getUserIdentity().getId().getBase64Url());
-// repo.getStorageService().delete(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// registrationTwo.getUserIdentity().getId().getBase64Url());
-//
-// // Now change the userHandle in the record so the mapping points to bob which is a different userHandle
-// repo.getStorageService().create(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// registration.getUserIdentity().getId().getBase64Url(), "bob", null);
-//
-// // This should find the mapping is not consistent and revert to the brute force behaviour
-// final var usernameJoe= repo.getUsernameForUserHandle(registration.getUserIdentity().getId());
-// assertTrue(usernameJoe.isPresent());
-// assertEquals(usernameJoe.get(), "jdoe");
-//
-// }
-
- /*
- * Tests: userHandle not found (SLOW)
- * <ol>
- * <li>lookup ALL records</li>
- * <li>find username from ANY of the userHandle matches</li>
- * <li>add mapping</li>
- * <li>username is OK</li>
- * </ol>
- */
-// @SuppressWarnings("null")
-// @Test
-// public void testGetUsernameForUserHandle_NoMapping() throws Exception {
-//
-// final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
-// repo.addRegistrationByUsername("jdoe", registration);
-//
-// // Now delete the mapping
-// repo.getStorageService().delete(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// registration.getUserIdentity().getId().getBase64Url());
-//
-// // Should find it from brute force
-// var usernameJdoe = repo.getUsernameForUserHandle(registration.getUserIdentity().getId());
-// assertTrue(usernameJdoe.isPresent());
-// assertEquals(usernameJdoe.get(), "jdoe");
-//
-// //And it should restore the mapping
-// final var mapping = repo.getStorageService().read(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// registration.getUserIdentity().getId().getBase64Url());
-// assertEquals(mapping.getValue(), "jdoe");
-//
-// //This one should come from the index (although we do not explicitly test for that)
-// usernameJdoe = repo.getUsernameForUserHandle(registration.getUserIdentity().getId());
-// assertTrue(usernameJdoe.isPresent());
-// assertEquals(usernameJdoe.get(), "jdoe");
-//
-// }
-
- /*
- * Tests for: userHandle found (userHandle no longer exists in credentials)
- * <ol>
- * <li>choose username</li>
- * <li>lookup record</li>
- * <li>no credentials have the same userHandle</li>
- * <li>username FAIL</li>
- * <li>remove mapping</li>
- * <li>lookup ALL records</li>
- * <li>do not find username in ANY registrations</li>
- * <li>fail</li>
- * </ol>
- */
-// @Test
-// public void testGetUsernameForUserHandle_NoMapping_NoCredentials() throws Exception {
-// final ByteArray userHandle = new ByteArray("no-credential-match".getBytes());
-// // Add a valid credential but one that does not match for the userHandle supplied
-// final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
-// repo.addRegistrationByUsername("jdoe", registration);
-//
-// // Create a mapping for the userHandle we will search for, that points jdoe which does not have the same handle
-// repo.getStorageService().create(IdPStorageServiceCredentialRepository.STORAGE_CONTEXT_USERHANDLE_INDEX,
-// userHandle.getBase64Url(), "jdoe", null);
-//
-// // Should not find it
-// final var usernameJdoe = repo.getUsernameForUserHandle(userHandle);
-// assertFalse(usernameJdoe.isPresent());
-// }
-
@Test(expectedExceptions = CredentialRepositoryException.class)
public void testGetUsernameByUserHandle_EnsureUsernameFromCredentialUsed() throws Exception {
final ByteArray userHandleBytes = new ByteArray("user-handle".getBytes());
@@ -1198,7 +1096,7 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
// should be authorative
repo.addRegistrationByUsername("not-jdoe", registration);
- final Optional<String> username = repo.getUsernameForUserHandle(userHandleBytes);
+ repo.getUsernameForUserHandle(userHandleBytes);
}
/* Failure here would be non-deterministic if it happened.*/
@@ -1312,5 +1210,25 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
}
+ @Test
+ public void testUpdateNickname() throws Exception {
+ final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ repo.updateNickname("jdoe", registration.getCredential().getCredentialId(), "new-nickname");
+ final Set<CredentialRecord> credentials = repo.getRegistrationsByUsername("jdoe");
+ assertEquals(credentials.size(),1);
+ assertEquals(credentials.iterator().next().getNickname(),"new-nickname");
+ }
+
+ @Test
+ public void testUpdateNickname_NoCredential() throws Exception {
+ final CredentialRecord registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+
+ repo.updateNickname("jdoe", registration.getCredential().getCredentialId(), "new-nickname");
+ final Set<CredentialRecord> credentials = repo.getRegistrationsByUsername("jdoe");
+ assertEquals(credentials.size(),0);
+ }
+
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list