OAuth2Login should process authenticated requests

[wangzw]

Use case:

We have a system A use OAuth2Login to implement user login(Code flow). And another system B works as OAuth2 provider.

Step 1: A supervisor login system A. Everything works well.
Step 2: Supervisor close browser without logout. System A session is still available.
Step 3: A non-privilege user login system B to do something, Everything works well. Session of system B is reset to non-privilege user.
Step 4: The non-privilege user login system A from login page, but found that the login user is actually supervisor. (Reuse the previous supervisor session)

Expected: At step 4, since non-privilege user start a new login process from login page, we expected non-privilege user login finally.

After debug, we found that at step 4, the login process of system A have got the oauth2 code (Code flow) and redirected to the redirect url, but the redirect url is not processed correctly and skipped since the following code.

spring-security/config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java

Lines 851 to 855 in b1195e7

	PathPatternParserServerWebExchangeMatcher loginPathMatcher = new PathPatternParserServerWebExchangeMatcher("/login/oauth2/code/{registrationId}");
	ServerWebExchangeMatcher notAuthenticatedMatcher = e -> ReactiveSecurityContextHolder.getContext()
	.flatMap(p -> ServerWebExchangeMatcher.MatchResult.notMatch())
	.switchIfEmpty(ServerWebExchangeMatcher.MatchResult.match());
	return new AndServerWebExchangeMatcher(loginPathMatcher, notAuthenticatedMatcher);

Since the existence of the previous session, the process of redirect url is skipped. And the previous session is reused incorrectly.

AFAIK the process of redirect url is used to validate oauth2 code and retrieve user/token info and cannot be skipped. So it looks a security issue.

And since the process of redirect url is skipped, the saved authentication request cannot be removed from (in memory) session and potential run out server memory.

[rwinch]

Thanks @wangzw! @jgrandja will take a look soon

[jgrandja]

@wangzw The one thing that I would like to point out (in case you are not aware) is that when a user authenticates via oauth2Login(), 2 sessions are created. The first session is with the provider and the second session is with the application.

Given your use case, if supervisor logs out of the application (but not with the provider) and a non-privilege user attempts to authenticate via the application than authentication will automatically happen given that there is an existing session cookie (in the browser) with the provider. This session cookie (created for supervisor) is used to authenticate the non-privilege user which than follows up with the auth-code flow to complete OpenID Connect authentication.

NOTE: This only happens when using the same browser session/window. I believe in your scenario, you are also testing this using the same browser/window session? Can you please test your scenario using a different browser (or new session window) and let me know your results?

FYI, we also introduced support (in 5.2.0.M2) for RP (Client) initiated logout via #5350, which would allow you to configure the application to logout at the provider whenever the user logs out of the application.

[wangzw]

@jgrandja

Thanks for your reply. I'm aware that two different sessions exist for application and provider.

At Step 3, a non-privilege user login system B (Provider), so the session of provider is for non-privilege user.

a non-privilege user attempts to authenticate via the application than authentication will automatically happen given that there is an existing session cookie (in the browser) with the provider.

Yes, it is expected.

But finally application shows that the login user is supervisor.

The reason is that since the session of system A (application) for supervisor still available during login process, the login process is aborted due to following code.

spring-security/config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java

Lines 851 to 855 in b1195e7

	PathPatternParserServerWebExchangeMatcher loginPathMatcher = new PathPatternParserServerWebExchangeMatcher("/login/oauth2/code/{registrationId}");
	ServerWebExchangeMatcher notAuthenticatedMatcher = e -> ReactiveSecurityContextHolder.getContext()
	.flatMap(p -> ServerWebExchangeMatcher.MatchResult.notMatch())
	.switchIfEmpty(ServerWebExchangeMatcher.MatchResult.match());
	return new AndServerWebExchangeMatcher(loginPathMatcher, notAuthenticatedMatcher);

Since at step 4, the non-privilege user start a new login process to application, the login process should not be aborted no matter a previous session exists or not.

[wangzw]

Let me make the issue clear.

OAuth2 login has 3 steps (code flow):

1. Authenticate and get code from provider.
2. validate code and get access token.
3. validate token and get token/user information.

Current issue is that since there is a previous session, step 2 and 3 is skipped. And the existence session is reused.

I do not think that step 2 can be skipped in any case.

Two consequences:

1. If pervious session is for user A and user B start login process in same browser/window. Login process finish successfully but the session is still for user A. The action of reset session happen in step 2/3, but skipped.

2. At step 1, an authorization request is stored in (in-memory) session and expected be removed at step 2. But since step 2 is skipped, the authorization request is not removed from session. So it can be used to blow up server's memory.

[jgrandja]

@wangzw

The reason is that since the session of system A (application) for supervisor still available during login process, the login process is aborted due to following code.

In this scenario the login process is never started (or aborted) given that there is an established session from the supervisor on the previous step. Please keep in mind that the authentication process will not initiate if there is already an authenticated session.

Since at step 4, the non-privilege user start a new login process to application, the login process should not be aborted no matter a previous session exists or not.

Can you please provide details on exactly how you start a new login process with the non-privilege user? The fact that there is already an established session from supervisor and you do not log out, how do you start a new login process with non-privilege user without logging out supervisor first?

Also, this really isn't a valid real world use-case IMO. If a supervisor logs into an application and performs some tasks but than wants to log into the application with lesser privileges than they would typically logout first and than log back in using the non-privilege user.

At this point, I do not see any issues with the current implementation. This will likely get closed as Works as Designed but I'll wait for your reply.

[wangzw]

In this scenario the login process is never started (or aborted) given that there is an established session from the supervisor on the previous step. Please keep in mind that the authentication process will not initiate if there is already an authenticated session.

That is not true.

From the view of user at step 4, non-privilege user start a new login process of application by open the login URL, redirected to authentication page and type in non-privilege user name and password (since provider's session is cleaned by logout of provider), non-privilege user will think that I have login. But not, supervisor's session of application is actually reused.

From the view of OAuth2 protocol at step 4, authorization code is granted by provider for non-privilege user, that means the oauth2 login process has started. But OAuth2Login skip validate authorization code and reuse existing session. It violate the OAuth2 protocol. Application must validate authorization code unconditionally.

The fact that there is already an established session from supervisor and you do not log out, how do you start a new login process with non-privilege user without logging out supervisor first?

It is a common use case that user can start a new login process by open login URL no matter a previous session exists or not. It is spring-security default form login behavior, previous session will be reset after a new successful login.

Currently issue is that OAuth2Login skip validating authorization code if a previous session exists.

[wangzw]

Compare FormLogin and OAuth2Login

FormLogin: A new user login with a pervious session exists, new user login successfully and session is reset for new user. Expected.

OAuth2Login: A new user login with a pervious application session exists (provider session not exist), new user type in new user name and password and no error throws but previous session is reused. Not Expected, new user feels login successfully but actually previous session is reused.

[wangzw]

how do you start a new login process with non-privilege user without logging out supervisor first?

To start a new oauth2 login from application, open browser with http://application.example.com/oauth2/authorization/demo

If provider's session has expired, or supervisor has logout from provider(not application). non-privilege user is required to authenticated first.

If any provider session exists, authentication will be skipped.

I found that webflux and servlet have different behavior at this case.

when start a new oauth2 login with previous application session exist. (code flow)

1. Servlet: when the browser call the redirect url, 404 is returned.
2. WebFlux: when the browser call the redirect url, 2xx/3xx is returned (means new user login successfully), but previous session is reused.

Both behaviors are not expected.
Expected: Application session is reset for new user, just like form login.

[wangzw]

I have filed a pull request to fix this issue. With this fix, OAuth2Login behave the same as FormLogin when a new user login with a previous session exist. New user login will success and session will reset for new user.

[wangzw]

I have setup a repo to reproduce the issue. https://github.com/wangzw/oauth2-login-6890

[jgrandja]

@wangzw Thanks for providing a sample that reproduces the issue. After inspecting the code in more detail, I have noticed some inconsistencies between servlet and reactive.

I'm currently working on another task but I will get to this later this week. I'll update you when I get back to this. Thanks for your patience.