Tech - Corrections techniques SSO, erreurs APIs & sentry

backend/src/main/kotlin/fr/gouv/cnsp/monitorfish/config/SecurityConfig.kt

@@ -1,6 +1,7 @@
 package fr.gouv.cnsp.monitorfish.config
 
 import fr.gouv.cnsp.monitorfish.infrastructure.api.log.CustomAuthenticationEntryPoint
+import fr.gouv.cnsp.monitorfish.infrastructure.api.public_api.SpaController.Companion.FRONTEND_APP_ROUTES
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
 import org.springframework.context.annotation.Bean
@@ -45,32 +46,25 @@ class SecurityConfig(
                     authorize
                         .requestMatchers(
                             "/",
+                            *FRONTEND_APP_ROUTES.toTypedArray(),
                             "/*.jpg",
                             "/*.js",
                             "/*.png",
                             "/*.svg",
                             "/api/**",
                             "/asset-manifest.json",
                             "/assets/**",
-                            "/backoffice",
-                            "/backoffice/**",
                             // Used to redirect to the frontend SPA, see SpaController.kt
                             "/error",
-                            "/ext",
                             "/favicon-32.ico",
                             "/favicon.ico",
                             "/flags/**",
                             "/index.html",
-                            "/light",
-                            "/load_light",
-                            "/login",
                             "/map-icons/**",
                             "/proxy/**",
                             "/realms/**",
-                            "/register",
                             "/resources/**",
                             "/robots.txt",
-                            "/side_window",
                             "/static/**",
                             "/swagger-ui/**",
                             "/version",

backend/src/main/kotlin/fr/gouv/cnsp/monitorfish/infrastructure/api/ControllersExceptionHandler.kt

@@ -1,13 +1,11 @@
 package fr.gouv.cnsp.monitorfish.infrastructure.api
 
-import fr.gouv.cnsp.monitorfish.config.SentryConfig
 import fr.gouv.cnsp.monitorfish.domain.exceptions.*
 import fr.gouv.cnsp.monitorfish.infrastructure.api.outputs.*
 import fr.gouv.cnsp.monitorfish.infrastructure.exceptions.BackendRequestException
-import io.sentry.Sentry
+import jakarta.annotation.Priority
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
-import org.springframework.core.Ordered.LOWEST_PRECEDENCE
 import org.springframework.core.annotation.Order
 import org.springframework.http.HttpStatus
 import org.springframework.http.ResponseEntity
@@ -17,10 +15,9 @@ import org.springframework.web.bind.annotation.ResponseStatus
 import org.springframework.web.bind.annotation.RestControllerAdvice
 
 @RestControllerAdvice
-@Order(LOWEST_PRECEDENCE)
-class ControllersExceptionHandler(
-    val sentryConfig: SentryConfig,
-) {
+@Priority(1)
+@Order(1)
+class ControllersExceptionHandler {
     private val logger: Logger = LoggerFactory.getLogger(ControllersExceptionHandler::class.java)
 
     @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
@@ -58,10 +55,6 @@ class ControllersExceptionHandler(
     fun handleNAFMessageParsingException(e: Exception): ApiError {
         logger.error(e.message, e.cause)
 
-        if (sentryConfig.enabled == true) {
-            Sentry.captureException(e)
-        }
-
         return ApiError(e)
     }
 
@@ -75,10 +68,6 @@ class ControllersExceptionHandler(
     fun handleIllegalArgumentException(e: Exception): ApiError {
         logger.error(e.message, e.cause)
 
-        if (sentryConfig.enabled == true) {
-            Sentry.captureException(e)
-        }
-
         return ApiError(IllegalArgumentException(e.message.toString(), e))
     }
 
@@ -87,10 +76,6 @@ class ControllersExceptionHandler(
     fun handleNoParameter(e: MissingServletRequestParameterException): MissingParameterApiError {
         logger.error(e.message, e.cause)
 
-        if (sentryConfig.enabled == true) {
-            Sentry.captureException(e)
-        }
-
         return MissingParameterApiError("Parameter \"${e.parameterName}\" is missing.")
     }
 }

backend/src/main/kotlin/fr/gouv/cnsp/monitorfish/infrastructure/api/public_api/SentryController.kt

@@ -1,6 +1,7 @@
 package fr.gouv.cnsp.monitorfish.infrastructure.api.public_api
 
 import fr.gouv.cnsp.monitorfish.config.SentryConfig
+import io.sentry.Sentry
 import org.slf4j.LoggerFactory
 import org.springframework.web.bind.annotation.GetMapping
 import org.springframework.web.bind.annotation.RequestMapping
@@ -21,6 +22,7 @@ class SentryController(
             throw IllegalArgumentException("Sentry test error triggered from get request.")
         } catch (e: Exception) {
             logger.error(e.message, e)
+            Sentry.captureException(e)
         }
 
         return mapOf(

backend/src/main/kotlin/fr/gouv/cnsp/monitorfish/infrastructure/api/public_api/SpaController.kt

@@ -1,10 +1,12 @@
 package fr.gouv.cnsp.monitorfish.infrastructure.api.public_api
 
+import jakarta.servlet.RequestDispatcher
 import jakarta.servlet.http.HttpServletRequest
 import jakarta.servlet.http.HttpServletResponse
 import org.slf4j.LoggerFactory
 import org.springframework.boot.web.servlet.error.ErrorController
 import org.springframework.http.HttpStatus
+import org.springframework.http.ResponseEntity
 import org.springframework.stereotype.Controller
 import org.springframework.web.bind.annotation.RequestMapping
 
@@ -15,18 +17,67 @@ import org.springframework.web.bind.annotation.RequestMapping
 class SpaController : ErrorController {
     private val logger = LoggerFactory.getLogger(SpaController::class.java)
 
+    companion object {
+        val FRONTEND_APP_ROUTES =
+            listOf(
+                "/backoffice",
+                "/backoffice/**",
+                "/ext",
+                "/light",
+                "/load_light",
+                "/login",
+                "/register",
+                "/side_window",
+            )
+    }
+
+    private fun isStaticResource(path: String): Boolean =
+        path.matches(
+            """.*\.(js|mjs|css|json|map|png|jpg|jpeg|gif|webp|svg|woff2?|ttf|txt|eot|otf|ico|xml)$""".toRegex(),
+        )
+
     @RequestMapping("/error")
     fun error(
         request: HttpServletRequest,
         response: HttpServletResponse,
     ): Any {
-        // TODO Do not return the `index.html` when an assets GET fail
-        if (response.status != HttpStatus.UNAUTHORIZED.value()) {
-            response.status = HttpStatus.OK.value()
-            return "forward:/index.html"
+        val path = request.getAttribute(RequestDispatcher.ERROR_REQUEST_URI) as? String ?: "unknown"
+        val errorMessage = request.getAttribute(RequestDispatcher.ERROR_MESSAGE) as? String ?: "Unknown error"
+        val status = response.status
+
+        // Prevent returning index.html for failed asset requests
+        if (isStaticResource(path)) {
+            logger.warn("API error or asset not found: $path (status: $status)")
+            response.status = HttpStatus.NOT_FOUND.value()
+            return ResponseEntity
+                .status(HttpStatus.NOT_FOUND)
+                .body(
+                    mapOf(
+                        "error" to errorMessage,
+                        "path" to path,
+                        "status" to status,
+                    ),
+                )
+        }
+
+        // Prevent returning index.html for failed API requests
+        // "/light/v1" is used to avoid clashes with the /light frontend route
+        if (path.startsWith("/bff") || path.startsWith("/api") || path.startsWith("/light/v1")) {
+            logger.warn("API error: $path (status: $status)")
+            return ResponseEntity
+                .status(status)
+                .body(
+                    mapOf(
+                        "error" to errorMessage,
+                        "path" to path,
+                        "status" to status,
+                    ),
+                )
         }
 
-        return response
+        // Else, it might be a frontend path
+        response.status = HttpStatus.OK.value()
+        return "forward:/index.html"
     }
 
     val errorPath: String

backend/src/test/kotlin/fr/gouv/cnsp/monitorfish/infrastructure/api/public_api/SpaControllerUTests.kt

@@ -0,0 +1,82 @@
+package fr.gouv.cnsp.monitorfish.infrastructure.api.public_api
+
+import jakarta.servlet.RequestDispatcher.ERROR_MESSAGE
+import jakarta.servlet.RequestDispatcher.ERROR_REQUEST_URI
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.extension.ExtendWith
+import org.springframework.http.HttpStatus
+import org.springframework.mock.web.MockHttpServletRequest
+import org.springframework.mock.web.MockHttpServletResponse
+import org.springframework.test.context.junit.jupiter.SpringExtension
+
+@ExtendWith(SpringExtension::class)
+class SpaControllerUTests {
+    @Test
+    fun `should return JSON error response for API BFF errors`() {
+        // Given
+        val request = MockHttpServletRequest()
+        request.setAttribute(ERROR_REQUEST_URI, "/bff/dummy")
+        request.setAttribute(ERROR_MESSAGE, "Custom error message")
+
+        val response = MockHttpServletResponse()
+        response.status = 401
+        val controller = SpaController()
+
+        // When
+        val result = controller.error(request, response)
+
+        // Then
+        assertThat(response.status).isEqualTo(HttpStatus.UNAUTHORIZED.value())
+        val textResponse = result.toString()
+        assertThat(textResponse).contains(
+            """
+            {error=Custom error message, path=/bff/dummy, status=401}
+            """.trimIndent(),
+        )
+    }
+
+    @Test
+    fun `should return index for frontend routes`() {
+        // Given
+        val request = MockHttpServletRequest()
+        request.setAttribute(ERROR_REQUEST_URI, "/backoffice")
+        request.setAttribute(ERROR_MESSAGE, "Custom error message")
+
+        val response = MockHttpServletResponse()
+        response.status = 404
+        val controller = SpaController()
+
+        // When
+        val result = controller.error(request, response)
+
+        // Then
+        assertThat(response.status).isEqualTo(HttpStatus.OK.value())
+        val textResponse = result.toString()
+        assertThat(textResponse).contains("forward:/index.html")
+    }
+
+    @Test
+    fun `should return 404 for static assets`() {
+        // Given
+        val request = MockHttpServletRequest()
+        request.setAttribute(ERROR_REQUEST_URI, "/missing.js")
+        request.setAttribute(ERROR_MESSAGE, "Custom error message")
+
+        val response = MockHttpServletResponse()
+        response.status = 404
+        val controller = SpaController()
+
+        // When
+        val result = controller.error(request, response)
+
+        // Then
+        assertThat(response.status).isEqualTo(HttpStatus.NOT_FOUND.value())
+        val textResponse = result.toString()
+        assertThat(textResponse).contains(
+            """
+            {error=Custom error message, path=/missing.js, status=404}
+            """.trimIndent(),
+        )
+    }
+}

frontend/src/api/api.ts

@@ -16,6 +16,7 @@ import { redirectToLoginIfUnauthorized } from '../auth/utils'
 
 import type { BackendApi } from './BackendApi.types'
 import type { CustomResponseError, RTKBaseQueryArgs } from './types'
+import type { BaseQueryError } from '@reduxjs/toolkit/query'
 
 // Using local MonitorEnv stubs:
 export const MONITORENV_API_URL = import.meta.env.FRONTEND_MONITORENV_URL
@@ -92,7 +93,15 @@ const monitorfishBaseQuery = retry(
     baseUrl: `/bff/v1`,
     prepareHeaders: setAuthorizationHeader
   }),
-  { maxRetries: RTK_MAX_RETRIES }
+  {
+    retryCondition: (error: BaseQueryError<BaseQueryError<any>>, _, { attempt }) => {
+      if (attempt > RTK_MAX_RETRIES) {
+        return false
+      }
+
+      return !isUnauthorizedOrForbidden(error.status)
+    }
+  }
 )
 
 export const monitorfishApi = createApi({

frontend/src/auth/utils.ts

@@ -19,6 +19,8 @@ export function redirectToLoginIfUnauthorized(error: CustomResponseError) {
     name: 'Après une erreur API 401'
   })
 
-  // We don't use `router.navigate()` to avoid circular dependency issues
-  window.location.href = ROUTER_PATHS.login
+  if (!window.location.pathname.includes(ROUTER_PATHS.login)) {
+    // We don't use `router.navigate()` to avoid circular dependency issues
+    window.location.href = ROUTER_PATHS.login
+  }
 }