[java-idp-plugin-webauthn] branch main updated: Improve credential deletion action for the admin flow
Phil Smart
philip.smart at jisc.ac.uk
Fri Apr 26 16:17:54 UTC 2024
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=fb3d72098f0c1567f41216e922d3312056dbfdcc
The following commit(s) were added to refs/heads/main by this push:
new fb3d720 Improve credential deletion action for the admin flow
fb3d720 is described below
commit fb3d72098f0c1567f41216e922d3312056dbfdcc
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Fri Apr 26 17:17:51 2024 +0100
Improve credential deletion action for the admin flow
- Delete for a given username and credentialId, not just a credential
Id. Will isolate the removal if somehow different users had the same
credential Id (should never happen).
---
.../StorageServiceCredentialRepository.java | 10 ++
.../admin/impl/AdminDeletePublicKeyCredential.java | 13 ++-
.../IdPStorageServiceCredentialRespository.java | 41 +++++++++
.../authn/webauthn/views/webauthn-management.vm | 4 +-
...IdPStorageServiceCredentialRespositoryTest.java | 101 ++++++++++++++++++++-
.../storage/impl/InMemoryRegistrationStorage.java | 7 ++
6 files changed, 166 insertions(+), 10 deletions(-)
diff --git a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java
index dc6a754..6b653d7 100644
--- a/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java
+++ b/webauthn-api/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/StorageServiceCredentialRepository.java
@@ -107,4 +107,14 @@ public interface StorageServiceCredentialRepository extends CredentialRepository
* @return all credential registrations know to the system.
*/
@Nonnull @NotLive @Unmodifiable Set<CredentialRegistration> getAllRegistrations();
+
+ /**
+ * Remove the credential with the given credential Id for the given user.
+ *
+ * @param username the user to remove the credential from
+ * @param credentialId the Id of the credential to remove
+ *
+ * @return true iff the credential was remove, false otherwise.
+ */
+ boolean removeRegistrationByUsernameAndCredentialId(@Nonnull String username, @Nonnull ByteArray credentialId);
}
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/AdminDeletePublicKeyCredential.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/AdminDeletePublicKeyCredential.java
index 0cbe6a7..50607a2 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/AdminDeletePublicKeyCredential.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/admin/impl/AdminDeletePublicKeyCredential.java
@@ -52,17 +52,24 @@ public class AdminDeletePublicKeyCredential extends AbstractWebAuthnManagementAc
final byte[] credentialId = context.getCredentialIdToRemove();
if (credentialId == null) {
- log.error("{} Unable to find credentialId in registration context", getLogPrefix());
+ log.error("{} Unable to find credentialId in management context", getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, WebAuthnRegistrationEventIds.INVALID_MANAGEMENT_CTX);
+ return;
+ }
+ final String credentialUsername = context.getSearchUsername();
+ if (credentialUsername == null) {
+ log.error("{} Unable to find username to remove credential from in management context", getLogPrefix());
ActionSupport.buildEvent(profileRequestContext, WebAuthnRegistrationEventIds.INVALID_MANAGEMENT_CTX);
return;
}
- final int removed = getCredentialRepository().removeRegistrationByCredentialId(new ByteArray(credentialId));
+ final boolean removed = getCredentialRepository().removeRegistrationByUsernameAndCredentialId(
+ credentialUsername, new ByteArray(credentialId));
if (log.isDebugEnabled()) {
try {
log.debug("{} Credential '{}' {} removed", getLogPrefix(), Base64Support.encodeURLSafe(credentialId),
- removed > 0 ? "was" : "was not");
+ removed ? "was" : "was not");
} catch (final EncodingException e) {
// Do nothing if the encoding fails.
}
diff --git a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java
index 9dbcb68..b62a7be 100644
--- a/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java
+++ b/webauthn-impl/src/main/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/IdPStorageServiceCredentialRespository.java
@@ -419,6 +419,47 @@ public class IdPStorageServiceCredentialRespository extends AbstractIdentifiable
writeLock.unlock();
}
}
+
+ /** {@inheritDoc} */
+ @Override
+ public boolean removeRegistrationByUsernameAndCredentialId(final String username, final ByteArray credentialId) {
+ checkComponentActive();
+ final Lock writeLock = lock.writeLock();
+ try {
+ writeLock.lock();
+ final Set<CredentialRegistration> existingRegistrations = getRegistrationsByUsername(username);
+ if (!existingRegistrations.isEmpty()) {
+ final Set<CredentialRegistration> updateSet = new LinkedHashSet<>(existingRegistrations);
+
+ // Find an matching credential from the existing registration
+ final List<CredentialRegistration> matchingRegistrations = existingRegistrations.stream()
+ .filter(reg -> reg.getCredential().getCredentialId().equals(credentialId))
+ .collect(CollectionSupport.nonnullCollector(Collectors.toList())).get();
+
+ if (matchingRegistrations.size() != 1) {
+ // If no match, there is nothing to remove
+ return false;
+ }
+
+ updateSet.remove(matchingRegistrations.iterator().next());
+ if (updateSet.isEmpty()) {
+ //remove the entire storage record
+ return storageService.delete(STORAGE_CONTEXT, username);
+ } else {
+ //else, add back what remains
+ assert serializer != null;
+ return storageService.update(STORAGE_CONTEXT, username, updateSet, serializer, null);
+ }
+ }
+ // Nothing to do if the registration does not exist
+ return false;
+
+ } catch (final IOException e) {
+ throw new CredentialRepositoryException(e);
+ } finally {
+ writeLock.unlock();
+ }
+ }
/** {@inheritDoc} */
@Override
diff --git a/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/views/webauthn-management.vm b/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/views/webauthn-management.vm
index c4ed5d2..90d6b5f 100644
--- a/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/views/webauthn-management.vm
+++ b/webauthn-impl/src/main/resources/net/shibboleth/idp/plugin/authn/webauthn/views/webauthn-management.vm
@@ -39,10 +39,10 @@
<div id="supportedDiv">
<div class="centre">
#if ($managementOutcomes)
- <p id="reg-success-outcome" class="output-message output--success">$registrationOutcomes</p>
+ <p id="reg-success-outcome" class="output-message output--success">$managementOutcomes</p>
#end
#if ($managementErrorOutcomes)
- <p id="reg-error-outcome" class="output-message output--error">$registrationErrorOutcomes</p>
+ <p id="reg-error-outcome" class="output-message output--error">$managementErrorOutcomes</p>
#end
<div class="hidden output-message output--error" id="error_div">
<p id="error_message"></p>
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 50624af..b185b5c 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
@@ -230,13 +230,14 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
final var iterator = registrations.iterator();
var credReg = iterator.next();
assertEquals(credReg.getUsername(),"jdoe");
- assertEquals(credReg.getCredential().getCredentialId(),
- registration.getCredential().getCredentialId());
+ // Iterator is not ordered, so check it is either regisration in the first result
+ assertTrue(credReg.getCredential().getCredentialId().equals(registration.getCredential().getCredentialId())
+ || credReg.getCredential().getCredentialId().equals(registrationTwo.getCredential().getCredentialId()));
credReg = iterator.next();
assertEquals(credReg.getUsername(),"jdoe");
- assertEquals(credReg.getCredential().getCredentialId(),
- registrationTwo.getCredential().getCredentialId());
-
+ // Iterator is not ordered, so check it is either regisration in the first result
+ assertTrue(credReg.getCredential().getCredentialId().equals(registration.getCredential().getCredentialId())
+ || credReg.getCredential().getCredentialId().equals(registrationTwo.getCredential().getCredentialId()));
final int removalCount =
repo.removeRegistrationByCredentialId(registrationTwo.getCredential().getCredentialId());
@@ -334,6 +335,96 @@ public class IdPStorageServiceCredentialRespositoryTest extends AbstractWebAuthn
assertEquals(registrations.size(), 0);
}
+ @Test
+ public void testRemoveRegistrationByUsernameAndCredentialId() throws Exception {
+
+ final CredentialRegistration registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+
+ var registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ final var iterator = registrations.iterator();
+ final var credReg = iterator.next();
+ assertEquals(credReg.getUsername(),"jdoe");
+ assertEquals(credReg.getCredential().getCredentialId(),
+ registration.getCredential().getCredentialId());
+
+ repo.removeRegistrationByUsernameAndCredentialId("jdoe", registration.getCredential().getCredentialId());
+
+ registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 0);
+ }
+
+ @Test
+ public void testRemoveRegistrationByUsernameAndCredentialId_TwoRegistrations() throws Exception {
+
+ final CredentialRegistration registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ final CredentialRegistration registrationTwo =
+ createRegistration("jdoe", "John Doe", "user-handle-2".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+ repo.addRegistrationByUsername("jdoe", registrationTwo);
+
+ var registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 2);
+ final var iterator = registrations.iterator();
+ final var credReg = iterator.next();
+ assertEquals(credReg.getUsername(),"jdoe");
+ // Iterator is not ordered, so check it is either regisration in the first result
+ assertTrue(credReg.getCredential().getCredentialId().equals(registration.getCredential().getCredentialId())
+ || credReg.getCredential().getCredentialId().equals(registrationTwo.getCredential().getCredentialId()));
+
+ repo.removeRegistrationByUsernameAndCredentialId("jdoe", registrationTwo.getCredential().getCredentialId());
+
+ registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ }
+
+ @Test
+ public void testRemoveRegistrationByUsernameAndCredentialId_TwoUsers() throws Exception {
+
+ final CredentialRegistration registration = createRegistration("jdoe", "John Doe", "user-handle".getBytes());
+ repo.addRegistrationByUsername("jdoe", registration);
+ final CredentialRegistration registrationTwo = createRegistration("pdoe", "Poe Doe", "user-handle-2".getBytes());
+ repo.addRegistrationByUsername("pdoe", registrationTwo);
+
+ var registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ var iterator = registrations.iterator();
+ var credReg = iterator.next();
+ assertEquals(credReg.getUsername(),"jdoe");
+ assertEquals(credReg.getCredential().getCredentialId(),
+ registration.getCredential().getCredentialId());
+
+ registrations = repo.getRegistrationsByUsername("pdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ iterator = registrations.iterator();
+ credReg = iterator.next();
+ assertEquals(credReg.getUsername(),"pdoe");
+ assertEquals(credReg.getCredential().getCredentialId(),
+ registrationTwo.getCredential().getCredentialId());
+
+ repo.removeRegistrationByUsernameAndCredentialId("jdoe", registration.getCredential().getCredentialId());
+
+ registrations = repo.getRegistrationsByUsername("jdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 0);
+
+ registrations = repo.getRegistrationsByUsername("pdoe");
+ assertNotNull(registrations);
+ assertEquals(registrations.size(), 1);
+ iterator = registrations.iterator();
+ credReg = iterator.next();
+ assertEquals(credReg.getUsername(),"pdoe");
+ assertEquals(credReg.getCredential().getCredentialId(),
+ registrationTwo.getCredential().getCredentialId());
+ }
+
@SuppressWarnings("null")
@Test
public void testUpdateSignatureCounter() throws Exception {
diff --git a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/InMemoryRegistrationStorage.java b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/InMemoryRegistrationStorage.java
index 15a208b..64d6771 100644
--- a/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/InMemoryRegistrationStorage.java
+++ b/webauthn-impl/src/test/java/net/shibboleth/idp/plugin/authn/webauthn/storage/impl/InMemoryRegistrationStorage.java
@@ -191,4 +191,11 @@ public class InMemoryRegistrationStorage implements StorageServiceCredentialRepo
// TODO Fill this in if needed.
return CollectionSupport.emptySet();
}
+
+ /** {@inheritDoc} */
+ @Override
+ public boolean removeRegistrationByUsernameAndCredentialId(final String username, final ByteArray credentialId) {
+ // TODO Auto-generated method stub
+ return false;
+ }
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list