Simplifies the exception and status code handling

[akkie]

This pull request simplifies the handling of the exceptions and the fallback methods to show the appropriate status codes. Previously it was confusing that the notAuthenticated method handles the 401 Unauthorized status code and the notAuthorized method handles the 403 Forbidden result. Now this is completely unified:

There are now three types of exceptions:
UnauthorizedException -> handles the 401 Unauthorized status code
ForbiddenException -> handles the 403 Forbidden status code
AuthenticationException -> handles errors in the authentication flow

The notAuthenticated fallback handler was renamed to onUnauthorized and the notAuthorized fallback handler was renamed to handleForbidden.

The credential based providers will now throw an UnauthorizedException if the password doesn't match or if the user couldn't be found.

The exceptions do not log the errors anymore. Instead the exception handler will log an info for the UnauthorizedException and the ForbiddenException. The AuthenticationException will not be handled by the exception handler. This should be handled by the user or by the global onError handler.

Any suggestion would be highly appreciated. Especially from @rfranco or @igorbernstein.

[igorbernstein]

Thanks for pinging me on this, but I won't have a chance to look at it until Friday morning. If you can wait that long I'll gladly review this.

[akkie]

@igorbernstein Sure, I can wait!

[rfranco]

you could use default constructor param

class ForbiddenException(msg:String, cause:Throwable = null)

[akkie]

Sure! Thanks!

[rfranco]

@akkie looks very good.

[igorbernstein]

I'm looking through this now, a couple of early thoughts (I'll try to solidify tomorrow):

• Naming difference between of ForbiddenException and AccessDeniedEvent is a bit weird.
• Logging.rst still references AccessDeniedException.
• Unless I'm mistaken, AuthenticationException is used as a facade for any errors associated with misconfiguration or external service outages. Unfortunately I don't think there's any sane way to distinguish between those 2 causes, so they have stay lumped together. So, if the scope has to be so broad, why not make SilhouetteException a class and use it in place of AuthenticationException to make it clear how generic the errors are.
• I still think that the exceptions that result from unauthenticated or unauthorized users trying to access resources behind a SecuredAction, should be of a different type then those resulting from a user trying to register, login or connect to a 3rd party. A forbidden exception caused by a user canceling an oauth flow feels very different from a logged in user who doesn't have permissions to access a resource.

[akkie]

I'm not really sure if the terms Unauthorized and Forbidden are the correct terms in conjunction with Silhouette. Sure that are the right terms for the HTTP response codes, but in the sense of an authentication and authorization framework I think they have the wrong meaning.

In Silhouette a user is not-authenticated and not-authorized. It is bad that the terms not authorized and Unauthorized conflicts here for two different meanings. But I think we should still use the terms not-authenticated and not-authorized. It's also not a must to return the Unauthorized and Forbidden response codes in the fallback handlers. It's also possible to return 3** status codes to redirect to another page.

So my proposal would be to rename the methods to onNotAuthenticated and onNotAuthorized. The exceptions should be also renamed to NotAuthenticatedException and NotAuthorizedException.

Any thoughts?

[akkie]

@igorbernstein Thanks for your comment. I will not update the doc anymore, because I've moved the documentation to http://silhouette.readme.io. I'll remove the current doc and the current page with the next release.

The problem with the SilhouetteException is that it's a marker trait to catch all Silhouette related exceptions. So if we remove AuthenticationException and make SilhouetteException a class then we cannot only catch the mentioned misconfiguration or external service outage errors, because we get the not-authenticated and not-authorized exceptions too.

To your last point. We could introduce different types of exceptions for the different cases. But at the end all the different types will handle the two fallback methods. So I'm not sure how this could improve the lib. Could you please provide an example? Anyway the register and login actions are not part of the core. So a user could implement custom exceptions if needed.

[igorbernstein]

re: NotAuthenticated/NotAuthorized
I completely agree, NotAuthenticated & NotAuthorized make way more sense for SecuredAction. And it would be nice if the events & default handlers followed in suit:

NotAuthenticatedException => NotAuthenticatedEvent => handleNotAuthenticated
NotAuthorizedException => NotAuthorizedEvent => handleNotAuthorized

But I still think the providers should have a separate exception for when the user types in a wrong password or cancels an oauth flow. Maybe AuthenticationFailedException? I don't think this exception would be related to the two fallback methods. This exception should to be handled locally in the developer's implementation of the CredentialsAuthController & SocialAuthController. It makes sense to have global handleNotAuthenticated/handleNotAuthorized handlers since SecuredActions will be all over the codebase. But the developer will generally have one CredentialsAuthController & one SocialAuthController. Using the same exception type for both usecase feels a bit misleading.

re: SilhouetteException as a class
I'm not sure when you would want to catch configuration errors. I can see wanting to catch service outage errors, but as I mentioned earlier, I don't think its possible to distinguish between configuration & service outage problems. Furthermore, the service outage problems would be extremely rare...the service would have to die after the oauth redirect but before fetching the access token.

[igorbernstein]

It might even make sense to have the credential provider throw an InvalidPasswordException and the oauth providers throw a OAuthCanceledException

[akkie]

Thanks for the input. I work on an update to this pull request and I'll see if I can address this issues.

[akkie]

I've updated the pull request and the documentation. Every feedback is highly appreciated.

[igorbernstein]

👍👍
I haven't taken a deep dive into this iteration of the PR but from what I've seen, I like it

• it makes it easier to debug stackstraces generated from futures which tend to be hard to read: https://groups.google.com/forum/#!msg/scala-user/BjgQpWeqA0w/6b4Tth6RAisJ
• it makes it fairly clear that global error handlers shouldn't be used to handle failed login attempts

You might want to reorder the exceptions in the docs and pull NotAuthenticatedException & NotAuthorizedException towards the top because they'll be the most common.

I'll have some time on tuesday to read through the full commit and give more feedback then.

[akkie]

@igorbernstein Thanks. I had the same thought regarding the order of exceptions. I'll change this. Looking forward for the full review.

[rfranco]

@akkie What do you think about use I18N for exception message? it give flexibility to customize the message
https://www.playframework.com/documentation/2.3.x/ScalaI18N

[igorbernstein]

I'm not sure I would ever want to bubble exception messages to the user...it has the potential to leak too much info. Also, there is some value in keeping the exception messages consistent so that developers have an easier time googling for similar issues that other developers might've solved & blogged about.

What messages were you thinking needed translations?

edit: fix grammar & add explanation why I wouldn't want to bubble exception messages (sorry brain is still asleep)

[rfranco]

@igorbernstein the ideia is not to buble or translatie the exception message to the user otherwise i'd like to be possible to customize the message. One way to do that is use Play Message.

Likely we use log to do some bigdata stuff so it's nice if we could customize the exception message because it goes to the log.

That is only one idea to remove hardcode message.

[akkie]

Play message is made for front-end and not for back-end translation. The problem is that if you would translate the exception messages, you must pass two language objects around. The one for the front-end and one for the back-end. Otherwise you log the messages in the language of the user.

[akkie]

I would like to merge the commit. Any objections?

[igorbernstein]

I'm looking it over now...a couple of notes:

• I think the outdated docs should be dropped
• AccessDeniedException is easy to confuse with NotAuthorizedException, maybe AuthenticationAborted might be a better name?

Other than that, it looks good to me

[akkie]

I have choosen the name AccessDeniedException because the providers return an access denied error, so I think we should also use this name. It's also a ProviderException and it's documented in this way. I'll remove the old doc with the next pull request before the next release.

[igorbernstein]

Sounds good

On Tue, Mar 10, 2015 at 5:24 PM, Christian Kaps notifications@github.com
wrote:

I have choosen the name AccessDeniedException because the providers return an access denied error, so I think we should also use this name. It's also a ProviderException and it's documented in this way. I'll remove the old doc with the next pull request before the next release.

Reply to this email directly or view it on GitHub:
#302 (comment)

[rfranco]

LGTG 👍