General: Record a deactivation once and clear the keys beside it

One deactivation through the admin edit form wrote two DEACTIVATE_USER entries.
The develop merge is where that came from: develop moved the audit call to after
the credential revocation, this branch still had it before, and the merge kept
both. It is the same defect that was fixed once already for the activation
direction.

The audit call now happens in one place, after the revocation, with the
recovery-key clearing beside it.

Both deactivation tests asserted with anySatisfy, which two entries satisfy just
as well as one, so neither could see this. They count now.

Author: krusche

src/main/java/de/tum/cit/aet/artemis/account/service/user/UserCreationService.java

@@ -302,16 +302,6 @@ else if (!wasInternal || user.getPassword() == null) {
         log.debug("Changed Information for User: {}", user);
 
         User savedUser = saveUser(user);
-        // Only the deactivation is audited here. The admin edit form reaches that transition without going through
-        // deactivateUser, so it would otherwise go unrecorded. The opposite direction needs no entry here: the only caller,
-        // AdminUserResource.updateUser, follows an activating update with userService.activateUser, which audits it - doing
-        // it in both places recorded a single activation twice.
-        if (isBeingDeactivated) {
-            auditAccountStateChange(savedUser, Constants.DEACTIVATE_USER);
-            // Same reason as in deactivateUser. Deliberately not done for a plain administrative password change, which
-            // also revokes credentials but must leave an administrator-created account's invitation keys intact.
-            userRecoveryKeyService.clearAll(savedUser.getId());
-        }
         boolean passwordChangedByAdministrator = user.isInternal() && updatedUserDTO.getPassword() != null;
         boolean credentialsRevoked = isBeingDeactivated || revokeCredentialsAfterPasswordChange;
         if (credentialsRevoked) {
@@ -324,6 +314,10 @@ else if (!wasInternal || user.getPassword() == null) {
         // AdminUserResource.updateUser, follows an activating update with userService.activateUser, which audits it - doing
         // it in both places recorded a single activation twice.
         if (isBeingDeactivated) {
+            // Same reason as in deactivateUser: an outstanding activation or reset key would be a way back into an account
+            // whose control has just been taken away. Deliberately not done for a plain administrative password change,
+            // which revokes credentials too but must leave an administrator-created account's invitation keys intact.
+            userRecoveryKeyService.clearAll(savedUser.getId());
             auditAccountStateChange(savedUser, Constants.DEACTIVATE_USER);
         }
         if (passwordChangedByAdministrator) {

src/test/java/de/tum/cit/aet/artemis/account/service/AccountCredentialRevocationIntegrationTest.java

@@ -4,10 +4,12 @@
 
 import java.time.Instant;
 import java.time.ZonedDateTime;
+import java.util.List;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.actuate.audit.AuditEvent;
 import org.springframework.context.annotation.Conditional;
 import org.springframework.data.domain.Pageable;
 import org.springframework.http.HttpStatus;
@@ -471,11 +473,17 @@ void deactivatingAUserIsRecordedInTheAuditLog() {
         userCreationService.deactivateUser(user);
 
         assertAccountStateAudited(Constants.DEACTIVATE_USER);
+        assertThat(deactivationEvents()).as("one deactivation produces one audit entry").hasSize(1);
     }
 
+    /**
+     * Once, not once per thing the deactivation does. {@code updateUser} both revokes the credentials and audits, and an
+     * entry written on either side of the revocation reads the same in the log - so only counting catches the case where
+     * both happen.
+     */
     @Test
     @WithMockUser(username = "admin", roles = "ADMIN")
-    void deactivatingAUserThroughTheAdminUpdateIsRecordedInTheAuditLog() {
+    void deactivatingAUserThroughTheAdminUpdateIsRecordedExactlyOnce() {
         securityAuditEventRepository.deleteAll();
 
         User userWithAuthorities = userRepository.findOneWithAuthoritiesByLogin(user.getLogin()).orElseThrow();
@@ -485,6 +493,7 @@ void deactivatingAUserThroughTheAdminUpdateIsRecordedInTheAuditLog() {
         userCreationService.updateUser(userWithAuthorities, update);
 
         assertAccountStateAudited(Constants.DEACTIVATE_USER);
+        assertThat(deactivationEvents()).as("one deactivation produces one audit entry").hasSize(1);
     }
 
     @Test
@@ -520,6 +529,10 @@ void activatingThroughTheAdminUpdateIsRecordedExactlyOnce() {
                 .as("one activation produces one audit entry").hasSize(1);
     }
 
+    private List<AuditEvent> deactivationEvents() {
+        return auditEventService.findAll(AuditLogType.SECURITY, Pageable.unpaged()).stream().filter(event -> Constants.DEACTIVATE_USER.equals(event.getType())).toList();
+    }
+
     /**
      * Read through AuditEventService rather than the repository, because it loads {@code data} through an entity graph
      * while the repository's findAll() leaves that collection lazy and unreadable outside a session. The log to read from