OAuth2AuthorizationCodeGrantWebFilter should handle OAuth2AuthorizationException

[wjlc]

@jgrandja

The code (authorization_code) is a temporary credential that can be used one-time only, hence, the AUTHORIZATION_REQUEST_NOT_FOUND error.

The problem is not in authorization_code. AuthenticationWebFilter has authenticationFailureHandler to handle invalid authentication(authorization_code). OidcAuthorizationCodeReactiveAuthenticationManager throws OAuth2AuthenticationException which is AuthenticationException and browser is redirected to /login?error

The problem is in

1. ServerOAuth2AuthorizationCodeAuthenticationTokenConverter it throws OAuth2AuthorizationException and
2. AuthenticationWebFilter doesn't handle any errors from ServerAuthenticationConverter

Only one ServerAuthenticationConverter implementation will redirect browser to login page
ServerHttpBasicAuthenticationConverter cause it return Mono.empty() in case of any authentication problem

Originally posted by @iilkevych in #7884 (comment)

[wjlc]

#7884 (comment)

[iilkevych]

Some ReactiveAuthenticationManager implementations also can throw OAuth2AuthorizationException It seems to be right to check all them in AuthenticationWebFilter using one base exception AccessDeniedException
Which is actually handled already

[jgrandja]

@wjlc @iilkevych Thank you for this report.

The issue is in OAuth2AuthorizationCodeGrantWebFilter as it does not handle OAuth2AuthorizationException, which can be thrown by ServerOAuth2AuthorizationCodeAuthenticationTokenConverter.

The fix should be in OAuth2AuthorizationCodeGrantWebFilter to handle OAuth2AuthorizationException and delegate to authenticationFailureHandler on errors.

Would either of you be interested in submitting a PR for this?

[wjlc]

@shazin This is not the correct place for the fix. plz check the issue and replicate it. https://github.com/spring-projects/spring-security/issues/7884#issue-558408713

[wjlc]

@jgrandja
OAuth2AuthorizationCodeGrantWebFilter is just used in ServerHttpSecurity. OAuth2Client, it's just a case.

but my WebFluxSecurityConfig like this:
......
.and()
.oauth2Login()
.authenticationSuccessHandler(new CustomAuthenticationSuccessHandler())
......

oauth2Login() configured OAuth2LoginAuthenticationWebFilter that extends AuthenticationWebFilter.

AuthenticationWebFilter only handle AuthenticationException now, that should also handle OAuth2AuthorizationException.
The bad thing is they're in different packages and no inheritance relationship between OAuth2AuthorizationException and AuthenticationException.

[jgrandja]

@wjlc Thanks for the feedback.

It looks like ServerOAuth2AuthorizationCodeAuthenticationTokenConverter should throw OAuth2AuthenticationException instead of OAuth2AuthorizationException. Let me think this one through but I think that is what needs to change.

[iilkevych]

@jgrandja

It looks like ServerOAuth2AuthorizationCodeAuthenticationTokenConverter should throw OAuth2AuthenticationException instead of OAuth2AuthorizationException. Let me think this one through but I think that is what needs to change.

This is not only issue for ServerOAuth2AuthorizationCodeAuthenticationTokenConverter

As I mentioned above:

Some ReactiveAuthenticationManager implementations also can throw OAuth2AuthorizationException

.flatMap(authenticationManager -> authenticationManager.authenticate(token))

and AuthenticationWebFilter will not catch them.

[jgrandja]

@iilkevych

Some ReactiveAuthenticationManager implementations also can throw OAuth2AuthorizationException

Which specific implementations are you referring to?

[iilkevych]

OAuth2AuthorizationCodeReactiveAuthenticationManager -> OAuth2AuthorizationExchangeValidator

* @author Joe Grandja
:)

[jgrandja]

Ah yes. Ok let me think about what changes are required here.

[jgrandja]

Thanks for all the feedback @wjlc @iilkevych.

I just pushed the fix in these commits 4c902bb da4b626. This has been backported to 5.1.x.

[iilkevych]

@jgrandja just in case, will this redirect user to /login?

[jgrandja]

@iilkevych OAuth2LoginAuthenticationWebFilter will redirect to /login but OAuth2AuthorizationCodeGrantWebFilter will not

[iilkevych]

@jgrandja
Then what changed with this fix?
Main main concern was that application redirects user to identity provider when session expires.
If user did not finish login after session expired and his new session expired then application should redirect user to login again and as soon as user have now session with identity provider hi will login implicitly.
From your previous comment I guess it will simply crash. Same we have today. no?

[wjlc]

@jgrandja
Then what changed with this fix?
Main main concern was that application redirects user to identity provider when session expires.
If user did not finish login after session expired and his new session expired then application should redirect user to login again and as soon as user have now session with identity provider hi will login implicitly.
From your previous comment I guess it will simply crash. Same we have today. no?

you can add custom authenticationFailureHandler that returns errorCode, your frontend receive code and redirect /oauth2/authorization/xxx, then it will be login because you had session of the identity provider.

[jgrandja]

@iilkevych Regarding your comment

...application should redirect user to login again...

I think this is where the confusion is? OAuth2AuthorizationCodeGrantWebFilter processes the 4.1.2. Authorization Response for the OAuth 2.0 Authorization Code Grant. NOTE: The OAuth 2.0 Authorization Framework deals specifically with Authorization, it does not address Authentication. So it doesn't make sense for OAuth2AuthorizationCodeGrantWebFilter to redirect to /login for a failed authorization.

OpenID Connect 1.0 extends the OAuth 2.0 Authorization Framework and addresses authentication. The OAuth2LoginAuthenticationWebFilter implements Authentication using the Authorization Code Flow, and will redirect to /login for failed authentication.

[iilkevych]

@jgrandja

Who is authorizing what in OAuth 2.0 Authorization Framework?

I believe resource owner authorizes access to resource by client. There is nothing about authorizing anything to anybody in client app.
It's authorization server and resource owner. They are authorizing access to resource by client app.
Authorization code is authentication from client app's user perspective.
That is why OAuth2AuthorizationCodeGrantWebFilter:

• This {@code Filter} will then create an {@link OAuth2AuthorizationCodeAuthenticationToken} with
• the {@link OAuth2ParameterNames#CODE code} received and
• delegate it to the {@link ReactiveAuthenticationManager} to authenticate.

Am I wrong?

[jgrandja]

@iilkevych I would recommend reading the specs to understand the difference between OAuth 2.0 Authorization Framework (Authorization) and OpenID Connect 1.0 (Authentication). The links are provided in the previous comment.

Also, it's not clear to me if you are still seeing an issue (bug) or missing functionality? I feel like this conversation has gone down a different path from the original issue. If you feel the current functionality is missing something or there is a bug then I would propose that you put together a minimal sample that reproduces the issue so I can better understand what you are expecting.

[iilkevych]

@jgrandja
I’m afraid I may not find time to create and explain example. Although I believe if state expired before user authenticated then user should not authenticate again. This should happen without any users action.
This was initial issue and it is not resolved yet.