KRYSTALM7 commented on code in PR #172: URL: https://github.com/apache/fineract-loan-origination/pull/172#discussion_r4293789747
########## src/test/java/org/apache/fineract/los/api/MethodSecurityAuthorizationTest.java: ########## Review Comment: Nice coverage of method-security authorization failures. One additional consideration would be to cover an authenticated user who lacks the required role on another representative protected endpoint, if that scenario is not already covered by the existing security tests. ########## src/main/java/org/apache/fineract/los/exception/LosErrorConstants.java: ########## Review Comment: Looks good. Using a shared constant for the access-denied response keeps the error message consistent across the exception handler and regression tests. The message is clear and avoids exposing internal authorization details. ########## src/main/java/org/apache/fineract/los/exception/GlobalExceptionHandler.java: ########## Review Comment: One thing to verify Spring Security can raise authorization exceptions inside the security filter chain, outside controller advice. This handler addresses exceptions that reach it; the tests should establish that the specific method-security failures in question do reach the handler. ########## src/test/java/org/apache/fineract/los/exception/GlobalExceptionHandlerTest.java: ########## Review Comment: Good regression coverage for both "AuthorizationDeniedException" and its parent "AccessDeniedException". Verifying the fixed response message ensures internal exception details are not leaked while preserving the standard error response structure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
