spring-attic / spring-security-oauth

Support for adding OAuth1(a) and OAuth2 features (consumer and provider) for Spring web applications.
http://github.com/spring-projects/spring-security-oauth
Apache License 2.0
4.7k stars 4.04k forks source link

Aspects of some Spring Security exceptions should be visible from client #487

Open bcecchinato opened 9 years ago

bcecchinato commented 9 years ago

In the class _ResourceOwnerPasswordTokenGranter_, when spring security throws an exception, it is shaded by a new exception thrown by the class.

try {
    userAuth = authenticationManager.authenticate(userAuth);
}
catch (AccountStatusException ase) {
    //covers expired, locked, disabled cases (mentioned in section 5.2, draft 31)
    throw new InvalidGrantException(ase.getMessage());
}
catch (BadCredentialsException e) {
    // If the username/password are wrong the spec says we should send 400/invlid grant
    throw new InvalidGrantException(e.getMessage());
}

The result is that it is impossible to get the reason of the refusal. By that, I mean that without comparing the string error code, I can't make the difference between a locked account and a disables account.

Shouldn't it be more logical to rethrow the exception and treat it in the _WebResponseExceptionTranslator_ ?

Regards,

dsyer commented 9 years ago

I think that makes sense, but the OAuth spec is rather specific (as indicated in the comments). Where did you hope to catch the exception?

Most people would rather have a predictable experience than a rich one when it comes to implementing specs. I believe many OAuth2 providers are even more gnomic about error responses and don't even send the extra information in the error description.

bcecchinato commented 9 years ago

Hi @dsyer !

As the exception translator is customizable, this is where i would have put my rule (so that means that i would have to implement the _WebResponseExceptionTranslator_ class).

When you create the _InvalidGrantException_, it might be interessing to give the thrown exception so that you can use it the translator. This would give :

try {
    userAuth = authenticationManager.authenticate(userAuth);
}
catch (AccountStatusException ase) {
    //covers expired, locked, disabled cases (mentioned in section 5.2, draft 31)
    throw new InvalidGrantException(ase.getMessage(), ase);
}
catch (BadCredentialsException e) {
    // If the username/password are wrong the spec says we should send 400/invlid grant
    throw new InvalidGrantException(e.getMessage(), e);
}
frankskywalker commented 7 years ago

Hi @dsyer, I would like to have this feature too! Since many websites now have their error message more user friendly by telling them why the authentication is failed: password wrong, user not registered, or account expired. So that user could act accordingly to the error message.

Since we don't have this New Feature now, is it customize the ResourceOwnerPasswordTokenGranter is the only way?

Regards,