bigfork / silverstripe-oauth

SilverStripe OAuth2 authentication, based on the PHP League's OAuth2 client
BSD 3-Clause "New" or "Revised" License
9 stars 11 forks source link

Better handling of cancellations/errors #12

Closed kinglozzer closed 5 years ago

kinglozzer commented 5 years ago

E.g. with Facebook login, clicking “Cancel” redirects the user to:

mysite.com/oauth/callback?error=access_denied&error_code=200&error_description=Permissions%20error&error_reason=user_denied&state=...

madmatt commented 5 years ago

At the moment this just results in an HTTP 400 response, which is the same response as if the OAuth response was not returned at all (even though one is a failure case and the other is a recoverable error).

I'll probably try and work on this sometime soon as we'll need it for a project we're working on, @kinglozzer do you have any thoughts on how you'd ideally want this solved?

FWIW: I'm working on this as part of an upgrade from SS3, where the previous project used the now-deprecated BetterBrief/silverstripe-opauth module. That handled it by having two methods - getSuccessBackURL() and getCantLoginBackURL() which are called if the OAuth response was successful or not. I've built those into a custom TokenHandler for now, but this logic is part of the Controller class rather than the TokenHandler, so is a few steps too early in the controller context...

kinglozzer commented 5 years ago

As is tradition, I opened this issue, forgot about it for a while, and now can’t remember how half of this works...

When I looked at it before, there was no real consistency between providers and how they provide error messages/codes, so I think ideally it should be possible to add relevant error handling for each provider & context. Perhaps we could use a similar approach the TokenHandler classes? Pseudo-code:

Bigfork\SilverStripeOAuth\Client\Control\Controller:
  error_handlers:
    genericerrorhandler:
      priority: 1
      context: '*'
      class: 'OAuthGenericErrorHandler'
catch (Exception $e) {
    $message = $e->getMessage();
    $handlers = $this->getErrorHandlersForContext($session->get('oauth2.context'));
    // Run handlers to process the error message
    $results = [];
    foreach ($handlers as $handlerConfig) {
        $handler = Injector::inst()->create($handlerConfig['class']);
        $results[] = $handler->handleError($message, $provider, $request);
    }

    foreach ($results as $result) {
        if ($result instanceof HTTPResponse) {
            return $result;
        }
    }

    // Redirect to login form and show $message
}
use Bigfork\SilverStripeOAuth\Client\Handler\ErrorHandler;

class OAuthGenericErrorHandler implements ErrorHandler
{
    public function handleError(&$message, $provider, $request)
    {
        if ($provider instanceof FacebookProvider) {
            switch ($request->getVar('error_code') {
                case '200':
                    $message = 'You clicked no, plz click yes.';
                    break;
                // ...
            }
        }
    }
}

What do you think?

kinglozzer commented 5 years ago

Resolved by #13 and https://github.com/bigfork/silverstripe-oauth-login/commit/7c0b6c49cf0f046a414a264153ff86217239f9a0