spring-projects / spring-projects/spring-security

Use only one instance of `successHandler` in `AbstractAuthenticationFilterConfigurer`

Open
#19,123 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

status: waiting-for-triage type: enhancement
Dominant language
Java
Stars
9.6k
Forks
6.3k
Avg merge
2d 11h
Merged PRs (30d)
52

Description

(Flipped current and expected behavior because it's easier to read)

Current Behavior

AbstractAuthenticationFilterConfigurer currently has 2 instances of AuthenticationSuccessHandler:
https://github.com/spring-projects/spring-security/blob/b932571913d21c1f145578764135203607344245/config/src/main/java/org/springframework/security/config/annotation/web/configurers/AbstractAuthenticationFilterConfigurer.java#L69-L71

This is a problem when supplying your own AuthenticationSuccessHandler because only the defaultSuccessHandler will receive the requestCache:
https://github.com/spring-projects/spring-security/blob/b932571913d21c1f145578764135203607344245/config/src/main/java/org/springframework/security/config/annotation/web/configurers/AbstractAuthenticationFilterConfigurer.java#L277-L279

So you have to manually set the RequestCache on your own AuthenticationSuccessHandler which requires some extra work.

Expected Behavior

AbstractAuthenticationFilterConfigurer should contain one instance of AuthenticationSuccessHandler. If this instance implements a interface that allows setting the RequestCache it will be set during configure.
Additional it would probably be a good idea to also have a flag in the configurer which allows enabling or disabling this behavior.

Context

I'm currently trying to add a custom AuthenticationSuccessHandler and this handler requires the correct RequestCache being set.
There seems to be no way to do this by default so for now I required to add a custom Configurer.

Example pseudocode:

http.oauth2Login(c -> {
    final var handler = new MySavedRequestAwareAuthSuccessHandler();
    handler.setDefaultTargetUrl("/");
    c.successHandler(handler);
    // We need to set RequestCache later during build phase once it exists!
    http.setSharedObject(MySavedRequestAwareAuthSuccessHandler.class, handler);
  })
  .with(new CustomConfigurer(), Customizer.withDefaults())
// ...

static class CustomConfigurer extends AbstractHttpConfigurer<CustomConfigurer, HttpSecurity>
{
  @Override
  public void configure(final HttpSecurity http)
  {
    super.configure(http);
    http.getSharedObject(MySavedRequestAwareAuthSuccessHandler.class)
      .setsetRequestCache(http.getSharedObject(RequestCache.class));
  }

  // ...
}

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with config/src/main/java/org/springframework/security/config/annotation/web/configurers/AbstractAuthenticationFilterConfigurer.java, especially the success-handler fields and configure method around the linked lines. Trace how successHandler and RequestCache are supplied, then verify that a custom AuthenticationSuccessHandler receives the configured RequestCache without a separate custom configurer; the payload does not name a test file.

Written by the indexing model from the issue text.

Assessment

Tech stack
java, spring
Domain
authentication, security
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.