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

WebSessionServerSecurityContextRepository provides Mono.cache option #8422

Closed
simonpai opened this issue Apr 21, 2020 · 5 comments
Closed

WebSessionServerSecurityContextRepository provides Mono.cache option #8422

simonpai opened this issue Apr 21, 2020 · 5 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Milestone

Comments

@simonpai
Copy link

simonpai commented Apr 21, 2020

Describe the bug
Authenticate an unauthorized Authentication using ServerSecurityContextRepository cause the Mono<SecurityContext> produced by its load method subscribed twice.

To Reproduce
See the following config snippet:

@EnableWebFluxSecurity
public class TestSecurityConfig {

    @Bean
    public SecurityWebFilterChain securityWebFilterChain(ServerHttpSecurity http,
                                                         ServerSecurityContextRepository serverSecurityContextRepository) {
        return http.csrf().disable()
                   .formLogin().disable()
                   .httpBasic().disable()
                   .securityContextRepository(serverSecurityContextRepository)
                   .authorizeExchange()
                   .anyExchange().authenticated()
                   .and()
                   .build();
    }

    @Bean
    public ServerSecurityContextRepository serverSecurityContextRepository() {
        return new ServerSecurityContextRepository() {
            @Override
            public Mono<Void> save(ServerWebExchange exchange,
                                   SecurityContext context) {
                return Mono.empty();
            }

            @Override
            public Mono<SecurityContext> load(ServerWebExchange exchange) {
                // no authorities -> not authenticated
                final Authentication authentication = new UsernamePasswordAuthenticationToken("test", "test");
                return Mono.defer(() -> {
                    System.out.println("TestSecurityConfig: load"); // invoked twice
                    return Mono.just(new SecurityContextImpl(authentication));
                });
            }
        };
    }
}

The root cause is:

  • ReactorContextWebFilter save the Mono of SecurityContext into the reactor context, rather than the actual value. The Mono is subscribed the first time in regular authentication process.
  • The Mono in ExceptionTranslationWebFilter on AccessDeniedException resumes to retrieve the Principal, which leads to getting the Mono of SecurityContext (and subscribe it again) in SecurityContextServerWebExchange.

Expected behavior
I would expect the load method is only called once in this scenario. Not sure if the current behavior is intended, but it's quite confusing to me.
Perhaps saving Mono<SecurityContext> (opposed to saving the SecurityContext itself) is the root of the pain. I think keeping the Mono could lead to a more ambiguous lifecycle and might eventually harm the way to make Authentication immutable as well.

Sample
See the snippet above. An empty project with such a security config and any request could trigger the issue.

@simonpai simonpai added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Apr 21, 2020
@simonpai
Copy link
Author

simonpai commented Apr 21, 2020

@rwinch
Copy link
Member

rwinch commented Apr 21, 2020

Thanks for the report.

Perhaps saving Mono (opposed to saving the SecurityContext itself) is the root of the pain. I think keeping the Mono could lead to a more ambiguous lifecycle and might eventually harm the way to make Authentication immutable as well.

This is intentional to avoid calling load if no one needs the SecurityContext. We could potentially add an option to use Mono.cache() to have it only invoked once. Of course looking it up once has the drawback of potentially getting a stale value

@mafei-dev
Copy link

after enabling @EnableReactiveMethodSecurity it is called 3 times.

@simonpai
Copy link
Author

simonpai commented Mar 8, 2022

@mafei-dev FYI, I worked around by passing in a cached Mono myself.

@rwinch rwinch added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels May 3, 2022
@rwinch rwinch added this to the 5.7.0 milestone May 3, 2022
@rwinch rwinch changed the title Authenticate an unauthorized Authentication using ServerSecurityContextRepository cause the Mono<SecurityContext> produced by its load method subscribed twice WebSessionServerSecurityContextRepository provides Mono.cache option May 3, 2022
@rwinch
Copy link
Member

rwinch commented May 3, 2022

There is now an option to cache the result of WebSessionServerSecurityContextRepository which ensures that the logic inside the WebSessionServerSecurityContextRepository is only invoked once.

@rwinch rwinch added type: enhancement A general enhancement and removed type: bug A general bug labels May 3, 2022
@rwinch rwinch closed this as completed in 3c259b4 May 4, 2022
rwinch added a commit that referenced this issue May 4, 2022
Fix the checkstyle for this feature

Closes gh-8422
rwinch added a commit that referenced this issue May 4, 2022
Fix the checkstyle for this feature

Closes gh-8422
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

3 participants