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

Simplify MVC-based authentication #12985

Open
3 tasks
jzheaux opened this issue Apr 10, 2023 · 7 comments
Open
3 tasks

Simplify MVC-based authentication #12985

jzheaux opened this issue Apr 10, 2023 · 7 comments
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Apr 10, 2023

It's common in an application to use Spring MVC to publish a custom login page, for example like so:

@GetMapping("/login")
String startLogin() {
    return "login";
}

And it's quite reasonable that the same application may want to do the same with customizing the subsequent POST:

@PostMapping("/login")
String finishLogin(@RequestParam("username") String username, @RequestParam("password") String password) {
    // ...
}

In this case, the application needs to remember to do several things, like store the context in SecurityContextHolderStrategy and SecurityContextRepository and invoke the set of SessionAuthenticationStrategys.

Often, the easiest thing is for the application to configure a custom filter that extends AbstractAuthenticationProcessingFilter instead. This can lead to some disjointedness in the application since, in this brief example, the GET is in Spring MVC, but the POST is in the SecurityFilterChain. It also forces the application down a path of using Spring-Security-specific components for things that Spring MVC was built for.

It would be nice if those using Spring MVC for processing logins didn't have to remember so much boilerplate. In addition to making it simpler, this will make it more secure as well for circumstances like session fixation protection.

One way to do this would be to introduce addtional method argument resolvers, like so:

@PostMapping("/login")
@FormLogin
String finishLogin(Authentication authentication, AuthenticationResultProcessor processor) {
    Authentication result = performLogin(authentication);
    processor.process(result);
    return "redirect:/home";
}

The @FormLogin annotation would exercise Spring Security components to read the request and formulate the correct Authentication instance. The AuthenticationResultProcessor would be prepared to perform the needed Spring Security actions to complete authentication, like storing the SecurityContextRepository.

This approach lends itself nicely to allowing the application to use Spring MVC primitives instead of needing to learn and understand filters, authentication success/failure handlers, and the like.

The pattern could be repeated for other authentication mechanisms, as in @OAuth2Login and @Saml2Login. The AuthenticationResultProcessor instances that are prepared could be different for each one. Consider the following:

@Autowired
OpenSaml4AuthenticationProvider provider;

@PostMapping("/saml2/login")
@Saml2Login
String finishLogin(Authentication authentication, AuthenticationResultProcessor processor) {
    Saml2Authentication result = this.provider.authenticate(authentication);
    Authentication custom = doCustomWork(result);
    processor.process(custom);
    return "redirect:/home";
}

In this case, the argument resolver uses the correct AuthenticationConverter to prepare a Saml2AuthenticationToken that includes the <saml2:Response>, the RelyingPartyRegistration, etc. And the response updates the security context repository, holder, and runs any session authentication strategies.

If the controller method throws an exception, then the ExceptionTranslationFilter will take effect. Or, if that needs to be customized as well, then the application could use a Spring MVC @ExceptionHandler instead. As this is already simple, no work is proposed here to simplify that.

This ticket proposes the following work:

  • Introduce the AuthenticationResultProcessor interface with a #process(Authentication) method
  • Introduce argument resolvers that coordinate with AuthenticationConverters to produce an Authentication instance corresponding to the appropriate annotation
  • Introduce argument resolvers that produce an AuthenticationResultProcessor instance corresponding to the appropriate annotation. It would mitigate session fixation, update the security context repository, and the other Spring Security boilerplate. Since it relies on Spring MVC for the rest, it would not perform any redirection or forwarding

Note that this proposal does not reuse AuthenticationSuccessHandler as that has at least two drawbacks. The first is that it makes the @Controller method signature more complex by requiring the HttpServletRequest and HttpServletResponse objects. The second is that implementations would fundamentally differ in their relationship to existing implementations to the point of mutual exclusion. For example, existing AuthenticationSuccessHandlers primarily redirect and forward, but these new ones would do everything but that. Existing AuthenticationSuccessHandlers do not manage the SecurityContextRepository, SecurityContextHolderStrategy, or SessionAuthenticationStrategy set, but these new ones would.

Future optimizations could possibly include having @Controller methods return an Authentication instance for processing. In that case, the result handler would invoke the AuthenticationResultProcessor as well as any globally configured AuthenticationSuccess/FailureHandler, for example:

@PostMapping("/login")
@FormLogin
Authentication finishLogin(Authentication authentication) {
    return performLogin(authentication);
}
@jzheaux jzheaux added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Apr 10, 2023
@rwinch
Copy link
Member

rwinch commented Apr 10, 2023

NOTE: It could also return an Authentication that is already authenticated and have an external result handler process the AuthenticationSuccessHandler.

@marcusdacoregio
Copy link
Contributor

I love this idea.

I'm curious about the performLogin(Authentication) method, would it usually invoke an AuthenticationManager? If so, we should consider #11926.

Also, if I am using @FormLogin then I wouldn't need the formLogin() DSL, right?

What would be the proposal if someone wants to receive the credentials as JSON (which is pretty common)? Does @FormLogin(converter = JsonAuthenticationConverter.class) sounds reasonable or could we have a base annotation that could be customized, like the AuthenticationFilter?

In the case of @Saml2Login does the user need to customize the saml2Login() DSL in order to make it use that endpoint?

@jzheaux
Copy link
Contributor Author

jzheaux commented Apr 17, 2023

I'm curious about the performLogin(Authentication) method, would it usually invoke an AuthenticationManager?

I don't think that it would necessarily invoke an AuthenticationManager. In fact, I think quite often it would not. IMO, the primary audience for this feature is not using Spring Security's processing components, just the domain objects.

Also, if I am using @FormLogin then I wouldn't need the formLogin() DSL, right?

Correct, since it would replace the UsernamePasswordAuthenticationFilter. That said, I think this will require some care since there are multiple filters created by some DSL calls. For example, oauth2Login stands up an authentication endpoint as well as an authentication request endpoint.

What would be the proposal if someone wants to receive the credentials as JSON (which is pretty common)? Does @FormLogin(converter = JsonAuthenticationConverter.class) sounds reasonable or could we have a base annotation that could be customized, like the AuthenticationFilter?

I'm not sure just yet; my initial impression is I'd hope this would be addressed upstream by Spring MVC. That is, I'd rather lean on Spring MVC primitives for content negotiation.

In the case of @Saml2Login does the user need to customize the saml2Login() DSL in order to make it use that endpoint?

I think it would be one or the other. We'll need to take some care though as saml2Login() is another example of a DSL method that creates more than one filter. It may be worth considering how simple it is to create OAuth 2.0 and SAML 2.0 authentication request endpoints in Spring MVC.

@BingChunMoLi
Copy link

hi Have you considered providing simplified solutions for emailCode, capticatCode, and phoneCode?
That would be too cool.

@SentretC
Copy link

I'm excited to see this! It's depressing having to relearn things the Spring Security way that I can do easily in Spring MVC.

However I think extracting username and password from the request is also among those "things that Spring MVC was built for" (I don't know about other authentication mechanisms). Does the proposal also allow the following? (I'm developing an SPA)

@Data
public static class LoginRequest {
    private String username;
    private String password;
    private boolean rememberMe;
    private String captcha;
}

@PostMapping("/login")
public ResponseEntity<LoginResult> login(@RequestBody LoginRequest request, AuthenticationResultProcessor processor) {
    ...

However I'm probably not "the primary audience for this feature". I want to use the DaoAuthenticationProvider for processing because it does things like timing attack protection, and I can customize easily by implementing UserDetailsService myself. (Although it bothered me a little whether my UserDetails should implement CredentialsContainer) Seems that the existing Servlet 3 integration HttpServletRequest.login is all I need?

Things I want to do in my login method:

  1. Extract username, password and other fields like I do in other handler methods. (Spring MVC)
  2. Do some pre-authentication checks, like checking the captcha or whether the client IP is temporarily blocked due to too many failed logins. (Custom code - I hope Spring Security can provide some guidance on this in the documentation)
  3. Check the password, with Spring Security helping me avoid common pitfalls. (Spring Security + Custom code)
  4. Store the authenticated principal, and possibly perform remember me. (Spring Security)
  5. Construct the response, including how long the user still have to wait if they're temporarily blocked. (Custom code + Spring MVC)

@jzheaux
Copy link
Contributor Author

jzheaux commented May 31, 2023

@SentretC, thanks for the feedback.

However I think extracting username and password from the request

  1. Extract username, password and other fields like I do in other handler methods. (Spring MVC)

Correct, you would not be required to use the Authentication instance as a method argument, it's just a nice default. Since it would be one of many argument resolvers, Spring MVC would still bind your custom object as you expect.

  1. Do some pre-authentication checks, like checking the captcha or whether the client IP is temporarily blocked due to too many failed logins. (Custom code - I hope Spring Security can provide some guidance on this in the documentation)

Spring Security has several authentication providers and I think it is worth adding more. It does not have one that does Captcha, for example, but I think that would be great to add.

  1. Check the password, with Spring Security helping me avoid common pitfalls.

Yes, you could still call AuthenticationManager and get all its security support

  1. Store the authenticated principal, and possibly perform remember me. (Spring Security)

This is the idea of the AuthenticationResultProcessor. It would handle the Spring Security-specific response handling.

  1. Construct the response, including how long the user still have to wait if they're temporarily blocked. (Custom code + Spring MVC)

Yes, if I'm understanding you correctly, this would just be standard Spring MVC, Spring Security wouldn't need to get in the way.

@jzheaux
Copy link
Contributor Author

jzheaux commented May 31, 2023

Thanks for the ideas, @BingChunMoLi. I'm not sure I've got your idea just yet, but at first sight, they sound more like authentication providers. Please consider providing more detail in a separate ticket.

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) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants