Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Hardcoded 403 in Http403ForbiddenEntryPoint with HttpStatus.FORBIDDEN.value() #16615

Open
yelm-212 opened this issue Feb 18, 2025 · 0 comments · May be fixed by #16616
Open

Replace Hardcoded 403 in Http403ForbiddenEntryPoint with HttpStatus.FORBIDDEN.value() #16615

yelm-212 opened this issue Feb 18, 2025 · 0 comments · May be fixed by #16616
Labels
status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement

Comments

@yelm-212
Copy link

yelm-212 commented Feb 18, 2025

Summary

In BasicAuthenticationEntryPoint and DelegatingAuthenticationEntryPoint, HTTP status codes are returned using HttpStatus.UNAUTHORIZED.value().
However, in Http403ForbiddenEntryPoint, the status code 403 is hardcoded.

For consistency and maintainability, should we update Http403ForbiddenEntryPoint to also use HttpStatus.FORBIDDEN.value()?

Suggested Improvement

To maintain consistency across different authentication entry points,
Http403ForbiddenEntryPoint could be modified as follows:

public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException)
        throws IOException {
    logger.debug("Pre-authenticated entry point called. Rejecting access");
    response.sendError(HttpStatus.FORBIDDEN.value(), HttpStatus.FORBIDDEN.getReasonPhrase());
}

Current Implementation

  • BasicAuthenticationEntryPoint (Uses HttpStatus.UNAUTHORIZED.value())
    public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException)
            throws IOException {
        response.addHeader("WWW-Authenticate", "Basic realm=\"" + this.realmName + "\"");
        response.sendError(HttpStatus.UNAUTHORIZED.value(), HttpStatus.UNAUTHORIZED.getReasonPhrase());
    }
  • DelegatingAuthenticationEntryPoint (Uses HttpStatus.UNAUTHORIZED.value())
    public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException)
            throws IOException {
        response.addHeader("WWW-Authenticate", authenticateHeader);
        response.sendError(HttpStatus.UNAUTHORIZED.value(), HttpStatus.UNAUTHORIZED.getReasonPhrase());
    }
  • Http403ForbiddenEntryPoint (Hardcoded 403)
    public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException)
            throws IOException {
        logger.debug("Pre-authenticated entry point called. Rejecting access");
        response.sendError(403, "Access Denied");
    }

Questions

  • Is there any specific reason why Http403ForbiddenEntryPoint does not follow the same pattern as BasicAuthenticationEntryPoint and DelegatingAuthenticationEntryPoint?
  • Would it make sense to standardize the use of HttpStatus.FORBIDDEN.value() for better readability and maintainability?
@yelm-212 yelm-212 added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant