search

spring-projects / spring-authorization-server

Spring Authorization Server
https://spring.io/projects/spring-authorization-server
Apache License 2.0
4.78k stars 1.25k forks source link

Return more appropriate http status codes in OAuth2ErrorAuthenticationFailureHandler #1636

Closed MatthiasWinzeler closed 2 weeks ago

MatthiasWinzeler commented 1 month ago

Hi folks

Many thanks for this great project, it helps tremendously implementing a modern identity solution!

I noticed that the OAuth2ErrorAuthenticationFailureHandler always return an error 400 (bad request) to clients.

While this might be a sensible default, there are a lot of OAuth2ErrorCodes that would have more appropriate HTTP status codes, for example:

ACCESS_DENIED could be a 403
SERVER_ERROR could be a 500
TEMPORARILY_UNAVAILABLE could be a 503
INVALID_CLIENT could be a 401
...

I could not find anything related to http status codes in the OAuth2 specs, but it seems to me that the Ory Fosite project has a good mapping of those OAuth2 error codes to http status codes.

It sounds like cosmetic, but we encountered real world issues where some clients need to distinguish between those errors (for example when silent refreshing in a iframe they handle 401/403s differently). Would it make sense to return the more appropriate error codes?

jgrandja commented 4 weeks ago

@MatthiasWinzeler

Many thanks for this great project, it helps tremendously implementing a modern identity solution!

We are so happy to hear this !!!

Regarding customizing the HTTP status, please take a look at this comment for more context. Does this help?

MatthiasWinzeler commented 4 weeks ago

@jgrandja many thanks - the approach in the comment looks good, we can then use this as a workaround.

However, do you think it would still make sense to return different return codes for some errors by default? i.e. the ones that are clearly not 400s, like internal server error or the 401/403s?

jgrandja commented 3 weeks ago

@MatthiasWinzeler Sure, we can enhance OAuth2ErrorAuthenticationFailureHandler to return a more appropriate status code depending on the OAuth2Error.errorCode. Would you be willing to submit a PR for this?

MatthiasWinzeler commented 3 weeks ago

@jgrandja see https://github.com/spring-projects/spring-authorization-server/pull/1643

jgrandja commented 2 weeks ago

@MatthiasWinzeler

Sure, we can enhance OAuth2ErrorAuthenticationFailureHandler to return a more appropriate status code depending on the OAuth2Error.errorCode

Apologies, but after reviewing the spec, I don't think this makes sense any longer. From a logical standpoint, it does make sense. However, the spec specifies 400 status only.

At the moment, OAuth2ErrorAuthenticationFailureHandler is used by OAuth2TokenEndpointFilter, OAuth2TokenIntrospectionEndpointFilter, OAuth2TokenRevocationEndpointFilter and OAuth2DeviceAuthorizationEndpointFilter and each of the associated specs specify to use the same behaviour as the Token Endpoint, as follows:

3.2.3.1. Error Response

The authorization server responds with a HTTP 400 (Bad Request) status code (unless specified otherwise) ...

Furthermore, invalid_client states:

The authorization server MAY return an HTTP 401 (Unauthorized) status code to indicate which HTTP authentication schemes are supported. (NOTE: MAY indicates it's optional)

None of the other error codes specify a different status code to use.

Based on this, I don't think any changes are necessary.

Apologies for the confusion here. Any thoughts?

MatthiasWinzeler commented 2 weeks ago

@jgrandja oh, that makes sense. I did not know that the spec only mentions 400. I agree with your conclusion to leave it out then.

jgrandja commented 2 weeks ago

Sounds good @MatthiasWinzeler. I'll close the PR as well.