Merge pull request #211 from devondragon/issue-210-Audit-logging-throws-NPE-on-first-time-OAuth-registration-when-user-ID-is-null

Fix NPE in audit logging when user ID is null

Author: devondragon

README.md

@@ -96,14 +96,14 @@ Check out the [Spring User Framework Demo Application](https://github.com/devond
 <dependency>
     <groupId>com.digitalsanctuary</groupId>
     <artifactId>ds-spring-user-framework</artifactId>
-    <version>3.4.0</version>
+    <version>3.4.1</version>
 </dependency>
 ```
 
 ### Gradle
 
 ```groovy
-implementation 'com.digitalsanctuary:ds-spring-user-framework:3.4.0'
+implementation 'com.digitalsanctuary:ds-spring-user-framework:3.4.1'
 ```
 
 ## Quick Start

src/main/java/com/digitalsanctuary/spring/user/audit/AuditEventListener.java

@@ -31,10 +31,15 @@ public class AuditEventListener {
 	 */
 	@EventListener
 	public void onApplicationEvent(AuditEvent event) {
-		log.debug("AuditEventListener.onApplicationEvent: called with event: {}", event);
-		if (auditConfig.isLogEvents() && event != null) {
-			log.debug("AuditEventListener.onApplicationEvent: logging event...");
-			auditLogWriter.writeLog(event);
+		try {
+			log.debug("AuditEventListener.onApplicationEvent: called with event: {}", event);
+			if (auditConfig.isLogEvents() && event != null) {
+				log.debug("AuditEventListener.onApplicationEvent: logging event...");
+				auditLogWriter.writeLog(event);
+			}
+		} catch (Exception e) {
+			// Never let audit failures impact application flow
+			log.error("AuditEventListener.onApplicationEvent: Failed to process audit event (suppressed): {}", e.getMessage(), e);
 		}
 	}
 }

src/main/java/com/digitalsanctuary/spring/user/audit/FileAuditLogWriter.java

@@ -65,8 +65,25 @@ public synchronized void writeLog(AuditEvent event) {
             return;
         }
         try {
-            String userId = event.getUser() != null ? event.getUser().getId().toString() : null;
-            String userEmail = event.getUser() != null ? event.getUser().getEmail() : null;
+            // Null-safe extraction of user ID and email
+            String userId = null;
+            String userEmail = null;
+            
+            if (event.getUser() != null) {
+                Long id = event.getUser().getId();
+                userId = (id != null) ? id.toString() : null;
+                userEmail = event.getUser().getEmail();
+            }
+            
+            // If no user ID available, use email or "unknown" for the userId field
+            if (userId == null) {
+                if (userEmail != null) {
+                    userId = userEmail;  // Use email as identifier when ID is null
+                } else {
+                    userId = "unknown";  // Fallback when both are null
+                }
+            }
+            
             String output = MessageFormat.format("{0}|{1}|{2}|{3}|{4}|{5}|{6}|{7}|{8}|{9}", event.getDate(), event.getAction(),
                     event.getActionStatus(), userId, userEmail, event.getIpAddress(), event.getSessionId(), event.getMessage(), event.getUserAgent(),
                     event.getExtraData());
@@ -77,6 +94,9 @@ public synchronized void writeLog(AuditEvent event) {
             }
         } catch (IOException e) {
             log.error("FileAuditLogWriter.writeLog: IOException writing to log file: {}", auditConfig.getLogFilePath(), e);
+        } catch (Exception e) {
+            // Never let audit failures impact app flow
+            log.error("FileAuditLogWriter.writeLog: Failed to write audit log (suppressed): {}", e.getMessage(), e);
         }
     }
 

src/test/java/com/digitalsanctuary/spring/user/audit/AuditEventListenerTest.java

@@ -1,6 +1,7 @@
 package com.digitalsanctuary.spring.user.audit;
 
 import static org.mockito.Mockito.*;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 
 import com.digitalsanctuary.spring.user.persistence.model.User;
 import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder;
@@ -168,6 +169,32 @@ void logs_failedLoginEvent() {
             verify(auditLogWriter).writeLog(auditEvent);
         }
 
+        @Test
+        @DisplayName("Handles event with user having null ID gracefully")
+        void handles_eventWithUserHavingNullId() {
+            // Given
+            User userWithNullId = UserTestDataBuilder.aUser()
+                    .withId(null)  // User exists but ID is null (e.g., during registration)
+                    .withEmail("[email protected]")
+                    .withFirstName("New")
+                    .withLastName("User")
+                    .build();
+
+            auditEvent = AuditEvent.builder()
+                    .source(this)
+                    .user(userWithNullId)
+                    .action("Registration")
+                    .actionStatus("In Progress")
+                    .message("User registration started")
+                    .build();
+
+            // When
+            auditEventListener.onApplicationEvent(auditEvent);
+
+            // Then
+            verify(auditLogWriter).writeLog(auditEvent);
+        }
+
         @Test
         @DisplayName("Logs account deletion event")
         void logs_accountDeletionEvent() {
@@ -305,4 +332,55 @@ void logs_eventWithCompleteInfo() {
             verify(auditLogWriter).writeLog(auditEvent);
         }
     }
+
+    @Nested
+    @DisplayName("Exception Handling Tests")
+    class ExceptionHandlingTests {
+
+        @BeforeEach
+        void setUp() {
+            when(auditConfig.isLogEvents()).thenReturn(true);
+        }
+
+        @Test
+        @DisplayName("Does not propagate exceptions from writer")
+        void doesNotPropagateExceptionsFromWriter() {
+            // Given
+            auditEvent = AuditEvent.builder()
+                    .source(this)
+                    .user(testUser)
+                    .action("Test Action")
+                    .actionStatus("Success")
+                    .build();
+
+            // Simulate an exception in the writer
+            doThrow(new RuntimeException("Writer failed")).when(auditLogWriter).writeLog(any());
+
+            // When & Then - should not throw
+            assertDoesNotThrow(() -> auditEventListener.onApplicationEvent(auditEvent));
+            
+            // Verify the writer was still called
+            verify(auditLogWriter).writeLog(auditEvent);
+        }
+
+        @Test
+        @DisplayName("Handles exceptions when checking config")
+        void handlesExceptionsWhenCheckingConfig() {
+            // Given
+            auditEvent = AuditEvent.builder()
+                    .source(this)
+                    .action("Test")
+                    .actionStatus("Success")
+                    .build();
+
+            // Simulate an exception when checking config
+            when(auditConfig.isLogEvents()).thenThrow(new RuntimeException("Config error"));
+
+            // When & Then - should not throw
+            assertDoesNotThrow(() -> auditEventListener.onApplicationEvent(auditEvent));
+            
+            // Verify writer was never called since config check failed
+            verify(auditLogWriter, never()).writeLog(any());
+        }
+    }
 }