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

Closed
yelm-212 opened this issue Feb 18, 2025 · 1 comment · May be fixed by #16616
Closed

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

yelm-212 opened this issue Feb 18, 2025 · 1 comment · May be fixed by #16616
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue 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?
@jzheaux
Copy link
Contributor

jzheaux commented Feb 24, 2025

Thanks for the suggestion, @yelm-212 and the thorough write-up. I'll close this in favor of #16616

@jzheaux jzheaux closed this as completed Feb 24, 2025
@jzheaux jzheaux self-assigned this Feb 24, 2025
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 24, 2025
yelm-212 added a commit to yelm-212/spring-security that referenced this issue Feb 25, 2025
yelm-212 added a commit to yelm-212/spring-security that referenced this issue Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants