diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java index f7104ebc..36bccdc4 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java @@ -7,7 +7,6 @@ import com.iflytek.skillhub.auth.merge.AccountMergeException; import com.iflytek.skillhub.auth.merge.AccountMergeFailureCode; import com.iflytek.skillhub.auth.identity.IdentityLinkException; import com.iflytek.skillhub.auth.identity.IdentityLinkFailureCode; -import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; import com.iflytek.skillhub.dto.AccountMergeErrorResponse; @@ -244,11 +243,11 @@ public class GlobalExceptionHandler { public ResponseEntity> handleStorageAccess(StorageAccessException ex, HttpServletRequest request) { metrics.incrementStorageAccessFailure(ex.getOperation()); logger.warn( - "Object storage unavailable [requestId={}, method={}, path={}, userId={}, operation={}, key={}]", + "Object storage unavailable [requestId={}, method={}, path={}, authentication={}, operation={}, key={}]", requestIdAccessor.current(), request.getMethod(), sensitiveLogSanitizer.sanitizeRequestTarget(request), - resolveUserId(request), + resolveAuthenticationState(request), ex.getOperation(), ex.getKey(), ex @@ -273,11 +272,11 @@ public class GlobalExceptionHandler { @ExceptionHandler(Exception.class) public ResponseEntity> handleGlobalException(Exception ex, HttpServletRequest request) { logger.error( - "Unhandled API exception [requestId={}, method={}, path={}, userId={}]", + "Unhandled API exception [requestId={}, method={}, path={}, authentication={}]", requestIdAccessor.current(), request.getMethod(), sensitiveLogSanitizer.sanitizeRequestTarget(request), - resolveUserId(request), + resolveAuthenticationState(request), ex ); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body( @@ -286,12 +285,12 @@ public class GlobalExceptionHandler { private void logHandledException(HttpStatus status, String messageCode, HttpServletRequest request) { logger.info( - "API request failed [requestId={}, status={}, method={}, path={}, userId={}, code={}]", + "API request failed [requestId={}, status={}, method={}, path={}, authentication={}, code={}]", requestIdAccessor.current(), status.value(), request.getMethod(), sensitiveLogSanitizer.sanitizeRequestTarget(request), - resolveUserId(request), + resolveAuthenticationState(request), messageCode ); } @@ -304,13 +303,11 @@ public class GlobalExceptionHandler { apiResponseFactory.error(status.value(), error.messageCode(), error.messageArgs())); } - private String resolveUserId(HttpServletRequest request) { - if (!(request.getUserPrincipal() instanceof Authentication authentication)) { - return "anonymous"; + private String resolveAuthenticationState(HttpServletRequest request) { + if (request.getUserPrincipal() instanceof Authentication authentication + && authentication.isAuthenticated()) { + return "authenticated"; } - if (authentication.getPrincipal() instanceof PlatformPrincipal principal) { - return principal.userId(); - } - return authentication.getName(); + return "anonymous"; } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/exception/GlobalExceptionHandlerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/exception/GlobalExceptionHandlerTest.java index 9f5bbc09..24012968 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/exception/GlobalExceptionHandlerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/exception/GlobalExceptionHandlerTest.java @@ -4,30 +4,47 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.when; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import com.iflytek.skillhub.auth.exception.AuthFlowException; +import com.iflytek.skillhub.auth.merge.AccountMergeException; +import com.iflytek.skillhub.auth.merge.AccountMergeFailureCode; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; import com.iflytek.skillhub.dto.IdentityLinkErrorResponse; -import com.iflytek.skillhub.auth.exception.AuthFlowException; import com.iflytek.skillhub.metrics.SkillHubMetrics; import com.iflytek.skillhub.observability.RequestIdAccessor; import com.iflytek.skillhub.security.SensitiveLogSanitizer; +import com.iflytek.skillhub.storage.StorageAccessException; import jakarta.servlet.http.HttpServletRequest; import java.time.Clock; import java.time.Instant; import java.time.ZoneOffset; +import java.util.List; +import java.util.Set; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.slf4j.LoggerFactory; import org.springframework.context.support.StaticMessageSource; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.web.context.request.async.AsyncRequestTimeoutException; @ExtendWith(MockitoExtension.class) class GlobalExceptionHandlerTest { + private static final String STABLE_USER_ID = "stable-user-123"; + @Mock private SensitiveLogSanitizer sensitiveLogSanitizer; @@ -37,7 +54,11 @@ class GlobalExceptionHandlerTest { @Mock private HttpServletRequest request; + private final Logger logger = + (Logger) LoggerFactory.getLogger(GlobalExceptionHandler.class); + private ListAppender appender; private GlobalExceptionHandler handler; + private RequestIdAccessor requestIdAccessor; @BeforeEach void setUp() { @@ -47,7 +68,7 @@ class GlobalExceptionHandlerTest { "error.auth.local.invalidCredentials", java.util.Locale.getDefault(), "Invalid username or password"); - RequestIdAccessor requestIdAccessor = new RequestIdAccessor(); + requestIdAccessor = new RequestIdAccessor(); ApiResponseFactory responseFactory = new ApiResponseFactory( messageSource, Clock.fixed(Instant.parse("2026-03-20T00:00:00Z"), ZoneOffset.UTC), @@ -61,6 +82,14 @@ class GlobalExceptionHandlerTest { ); } + @AfterEach + void tearDown() { + if (appender != null) { + logger.detachAppender(appender); + appender.stop(); + } + } + @Test void handleAsyncRequestTimeout_shouldReturnNoContentForSseRequests() { when(request.getRequestURI()).thenReturn("/api/v1/notifications/sse"); @@ -135,4 +164,127 @@ class GlobalExceptionHandlerTest { assertThat(body.reasonCode()) .isEqualTo("REAUTHENTICATION_REQUIRED"); } + + @ParameterizedTest + @EnumSource(value = AccountMergeFailureCode.class, names = { + "MERGE_REAUTH_REQUIRED", "MERGE_CONFLICT", "ACCOUNT_MERGE_UNAVAILABLE" + }) + void handleAccountMergeException_shouldLogAuthenticationWithoutStableUserId( + AccountMergeFailureCode failureCode) { + authenticateRequest(); + attachAppender(); + when(request.getMethod()).thenReturn("POST"); + when(sensitiveLogSanitizer.sanitizeRequestTarget(request)) + .thenReturn("/api/v1/auth/account-merge/intents/test/confirm"); + + try (RequestIdAccessor.Scope ignored = requestIdAccessor.open("request-123")) { + ResponseEntity response = handler.handleAccountMergeException( + new AccountMergeException(failureCode), request); + assertThat(response.getStatusCode()).isEqualTo(failureCode.status()); + } + + assertThat(loggedMessages()).anySatisfy(message -> assertThat(message) + .contains("requestId=request-123") + .contains("status=" + failureCode.status().value()) + .contains("method=POST") + .contains("path=/api/v1/auth/account-merge/intents/test/confirm") + .contains("authentication=authenticated") + .contains("code=" + failureCode.messageCode()) + .doesNotContain(STABLE_USER_ID) + .doesNotContain("userId=")); + } + + @Test + void handleGlobalException_shouldLogAuthenticationWithoutStableUserId() { + authenticateRequest(); + attachAppender(); + when(request.getMethod()).thenReturn("GET"); + when(sensitiveLogSanitizer.sanitizeRequestTarget(request)) + .thenReturn("/api/v1/skills/sensitive"); + + try (RequestIdAccessor.Scope ignored = requestIdAccessor.open("request-123")) { + ResponseEntity> response = handler.handleGlobalException( + new RuntimeException("boom"), request); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + } + + assertThat(loggedMessages()).anySatisfy(message -> assertThat(message) + .contains("Unhandled API exception") + .contains("requestId=request-123") + .contains("method=GET") + .contains("path=/api/v1/skills/sensitive") + .contains("authentication=authenticated") + .doesNotContain(STABLE_USER_ID) + .doesNotContain("userId=")); + } + + @Test + void handleStorageAccess_shouldLogAuthenticationWithoutStableUserId() { + authenticateRequest(); + attachAppender(); + when(request.getMethod()).thenReturn("GET"); + when(sensitiveLogSanitizer.sanitizeRequestTarget(request)) + .thenReturn("/api/v1/skills/test/download"); + + StorageAccessException exception = new StorageAccessException( + "download", "skills/test.zip", new RuntimeException("unavailable")); + try (RequestIdAccessor.Scope ignored = requestIdAccessor.open("request-123")) { + ResponseEntity> response = handler.handleStorageAccess(exception, request); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); + } + + assertThat(loggedMessages()).anySatisfy(message -> assertThat(message) + .contains("Object storage unavailable") + .contains("requestId=request-123") + .contains("method=GET") + .contains("path=/api/v1/skills/test/download") + .contains("authentication=authenticated") + .contains("operation=download") + .contains("key=skills/test.zip") + .doesNotContain(STABLE_USER_ID) + .doesNotContain("userId=")); + } + + @Test + void handleAsyncRequestTimeout_shouldLogAnonymousAuthenticationState() { + attachAppender(); + when(request.getRequestURI()).thenReturn("/api/v1/publish"); + when(request.getMethod()).thenReturn("POST"); + when(sensitiveLogSanitizer.sanitizeRequestTarget(request)) + .thenReturn("/api/v1/publish"); + + try (RequestIdAccessor.Scope ignored = requestIdAccessor.open("request-123")) { + handler.handleAsyncRequestTimeout(new AsyncRequestTimeoutException(), request); + } + + assertThat(loggedMessages()).anySatisfy(message -> assertThat(message) + .contains("API request failed") + .contains("requestId=request-123") + .contains("status=408") + .contains("method=POST") + .contains("path=/api/v1/publish") + .contains("authentication=anonymous") + .contains("code=error.request.timeout") + .doesNotContain("userId=")); + } + + private void authenticateRequest() { + PlatformPrincipal principal = new PlatformPrincipal( + STABLE_USER_ID, "User", "user@example.com", null, "local", Set.of("USER")); + when(request.getUserPrincipal()).thenReturn( + new UsernamePasswordAuthenticationToken(principal, null, List.of())); + } + + private void attachAppender() { + logger.setLevel(Level.INFO); + appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + } + + private List loggedMessages() { + return appender.list.stream() + .map(ILoggingEvent::getFormattedMessage) + .toList(); + } }