Make `oauth_callback_confirmed` optional

[atrauzzi]

I'm working with an API (goodreads) that doesn't appear to be honouring it.

The check can be found here: https://github.com/thephpleague/oauth1-client/blob/4d4edd9b6014f882e319231a9b3351e3a1dfdc81/src/Client/Server/Server.php#L432

(Also worth noting is that the exception message being shown here could be a bit more descriptive.)

[stevenmaguire]

Thanks for the issue and the feedback! Two things:

If the good reads API is doing something a bit different, consider building or updating an existing provider to handle its use case. The core project is intended to cover the spec's implementation.

If you think the messaging could better for an error here, consider submitting a pull request to update. Prose updates are easy to update without fear of breaking functionality.

Also, it's worth noting that this project is in the midst of a refactor, so if you need the changes now feel free to build against or pull against current master, otherwise consider the changes that are currently in develop branch.

[atrauzzi]

I'm working against develop in anticipation of the rewrite. I require the newer guzzle support.

[atrauzzi]

Anyway, I think there's still an issue with passing the response you get from any API to createFromResponse as that then requires me to subclass TeporaryCredentials just to dodge the hard-requirement of oauth_callback_confirmed.

The only legitimate alternative to subclassing would be to somehow fake the presence of that value just to trick the check. Otherwise, the method createFromResponse really just ends up encapsulating nothing.

[atrauzzi]

@stevenmaguire

Sorry for the spam. I took the time based on your suggestion to produce a provider/server implementation for goodreads. The two quirks they have are:

• oauth_callback_confirmed is not provided
• oauth_verifier is a bit of a flying dutchman

Gotta love oauth1 signatures...

<?php namespace App\Social\Oauth1Server;

use League\OAuth1\Client\Credentials\Credentials;
use League\OAuth1\Client\Server\GenericServer;
//
use GuzzleHttp\Exception\BadResponseException;
use League\OAuth1\Client\Credentials\TemporaryCredentials;
use League\OAuth1\Client\Credentials\TokenCredentials;
use League\OAuth1\Client\Exceptions\CredentialsException;

class Goodreads extends GenericServer {

    /**
     * Gets temporary credentials by performing a request to
     * the server.
     *
     * @return TemporaryCredentials
     *
     * @throws CredentialsException If the request failed
     */
    public function getTemporaryCredentials() {

        $uri = $this->getBaseTemporaryCredentialsUrl();
        $header = $this->getTemporaryCredentialsProtocolHeader($uri);
        $authorizationHeader = ['Authorization' => $header];
        $headers = $this->buildHttpClientHeaders($authorizationHeader);

        try {
            $request = $this->getRequestFactory()->getRequest('POST', $uri, $headers);
            $response = $this->getHttpClient()->send($request);
        }
        catch (BadResponseException $e) {
            throw CredentialsException::temporaryCredentialsBadResponse($e);
        }

        // Skip the built-in factory so that we aren't forced to have `oauth_callback_confirmed` (even though it's nice)
        // https://github.com/thephpleague/oauth1-client/issues/63
        parse_str($response->getBody(), $data);
        return new TemporaryCredentials($data['oauth_token'], $data['oauth_token_secret']);

    }

    /**
     * Retrieves token credentials by passing in the temporary credentials,
     * the temporary credentials identifier as passed back by the server
     * and finally the verifier code.
     *
     * @param TemporaryCredentials $temporaryCredentials
     * @param string               $temporaryIdentifier
     * @param string               $verifier
     *
     * @return TokenCredentials
     *
     * @throws CredentialsException If the request failed
     */
    public function getTokenCredentials(TemporaryCredentials $temporaryCredentials, $temporaryIdentifier, $verifier) {

        $temporaryCredentials->checkIdentifier($temporaryIdentifier);
        $uri = $this->getBaseTokenCredentialsUrl();

        // Goodreads wants `oauth_verifier` to be one here. That's it.  Just here.  Nowhere else.
        // https://www.goodreads.com/topic/show/2043791-missing-oauth-verifier-parameter-on-user-auth-redirect#comment_149135628
        $bodyParameters = ['oauth_verifier' => 1];

        $headers = $this->getHeaders($temporaryCredentials, 'POST', $uri, $bodyParameters);
        $body = json_encode($bodyParameters);

        try {
            $request = $this->getRequestFactory()->getRequest('POST', $uri, $headers, $body);
            $response = $this->getHttpClient()->send($request);
        }
        catch (BadResponseException $e) {
            throw CredentialsException::tokenCredentialsBadResponse($e);
        }

        return TokenCredentials::createFromResponse($response);

    }

    /**
     * Generates the OAuth protocol header for requests other than temporary
     * credentials, based on the URI, method, given credentials & body query
     * string.
     *
     * @param string      $method
     * @param string      $uri
     * @param Credentials $credentials
     * @param array       $bodyParameters
     *
     * @return string
     */
    protected function getProtocolHeader($method, $uri, Credentials $credentials, array $bodyParameters = []) {

        $parameters = array_merge(
            $this->getBaseProtocolParameters(),
            $this->getAdditionalProtocolParameters(),
            [
                'oauth_token' => $credentials->getIdentifier(),
            ]
        );

        // Goodreads flying dutchman oauth_verifier
        // https://www.goodreads.com/topic/show/18053350-getting-invalid-oauth-request-with-signed-requests
        if($verifier = static::getValueByKey($bodyParameters, 'oauth_verifier'))
            $parameters['oauth_verifier'] = $verifier;

        $this->signature->setCredentials($credentials);

        $parameters['oauth_signature'] = $this->signature->sign(
            $uri,
            array_merge($parameters, $bodyParameters),
            $method
        );

        return $this->normalizeProtocolParameters($parameters);

    }

}

Although of course subclassing works (thank goodness!) -- there's probably a worthwhile discussion here about what can be done to reduce the amount of boilerplate that needs to be reproduced when making a custom provider/server implementation.

[bencorlett]

Heya, so I agree with Steven here - the main aim when I originally wrote this package was to be the first fully-compliant package out there, and I don't want to deviate from that focus.

However, I'm also open to the suggestion of refactoring the Server class to maybe inject a "transport" object (or something of the like) in order to simplify boilerplate.

What are the others' views on this?

[atrauzzi]

The more I think about this, the more I realise there could be any number of whacked oauth implementations out there. Things are probably fine the way they are.

[bencorlett]

Yeah that's the tricky thing!

If Steven is okay, I'm happy to close this off then

[stevenmaguire]

Close away.