Unable to retrieve auth token #23

Closed
lucasaba opened this Issue Mar 24, 2013 · 25 comments

Projects

None yet

3 participants

@lucasaba
Contributor

I'm trying to use sensiolabs/connect to retrieve a user's badges.
I've beeing trying to avoid the use of Silex but I couldn't complete the operation.
Then I've been trying with Silex and following the documentation.

When it comes to the connect/callback route, I have the following error:

ClientException:
file_get_contents(https://connect.sensiolabs.com/oauth/access_token):
failed to open stream: operation failed

An here's what it says:

[...]
at Browser->submit('https://connect.sensiolabs.com/oauth/access_token', array('client_id' => '9e...ac', 'client_secret' => '002...36', 'code' => 'b8...4b', 'grant_type' => 'authorization_code', 'redirect_uri' =>'http://badges.local/connect/callback', 'response_type' => 'code', 'scope' => 'SCOPE_PUBLIC')) in /var/www/badges/vendor/sensiolabs/connect/src/SensioLabs/Connect/OAuthConsumer.php line 93
at OAuthConsumer->requestAccessToken('http://badges.local/connect/callback', 'b8...4b') in /var/www/badges/web/index.php line 42
[...]

Is it possible that the problem is in: https://connect.sensiolabs.com/oauth/access_token ?

Without Silex the problem was the same: a 500 error from https://connect.sensiolabs.com/oauth/access_token

@lyrixx
Member
lyrixx commented Mar 24, 2013
@lucasaba
Contributor

Nope, I just followed the read-me of sensiolabs/connect

@lyrixx
Member
lyrixx commented Mar 24, 2013

IMHO, you should try it. Everything is setup-ed. It will be easier.

@lucasaba
Contributor

Finally I could try your suggestion.
Actually it gives me more problems than solutions. With Silex-Connect I have a

PHP Fatal error:  Class 'SensioLabs\\Connect\\Security\\Firewall' not found in /var/www/badges/vendor/silex/silex/src/Silex/Provider/SecurityServiceProvider.php on line 112

And that's the require section of my composer.json:

"require": {
        "sensiolabs/connect": "1.1.*",
        "silex/silex": "1.0.*@dev",
        "sensiolabs/silex-connect": "dev-master"
    }
@lyrixx
Member
lyrixx commented Mar 25, 2013

You should register the SecurityExtension in your app.php

@lucasaba
Contributor
lucasaba commented Apr 2, 2013

I have to study Silex...at least to understand why my code doesn't work. Unitll then, I'll close this issue and re-open it as soon as I will be able to say what to do to make sensiolabs/connect work as a standalone element.
Thanks for your help.

@lucasaba lucasaba closed this Apr 2, 2013
@lucasaba
Contributor

Finally I could find the time to inspect the code! And I've found the solution.
The problem is with sensio's certificate wich is not recognized by Buzz\Client\FileGetContens (wich has a default verify_peer set to false).
Possible solutions:
Number one:

$fg       = new \Buzz\Client\FileGetContents();
$fg->setVerifyPeer(false);
$browser  = new \Buzz\Browser($fg);
$consumer = new \SensioLabs\Connect\OAuthConsumer($connect_id, $connect_secret, $scope, 'https://connect.sensiolabs.com' ,$browser);
$api      = new \SensioLabs\Connect\Api\Api('https://connect.sensiolabs.com/api', $browser);

Number two: it would be slightly better if it could be possible to change the constructor's signatures moving the endpoint parameter after the browser parameter:

$fg = new \Buzz\Client\FileGetContents();
$fg->setVerifyPeer(false);
$browser = new \Buzz\Browser($fg);
$consumer = new \SensioLabs\Connect\OAuthConsumer($connect_id, $connect_secret, $scope, $browser);
$api = new \SensioLabs\Connect\Api\Api($browser);

Last one:
There is this class: SensioLabs\Connect\Buzz\Client\Curl wich extends Buzz\Client\Curl which sets to false CURLOPT_SSL_VERIFYPEER. It would be easyer to create a SensioLabs\Connect\Buzz\Client\FileGetContents class and to change the code in SensioLabs\Connect\OAuthConsumer and in SensioLabs\Connect\Api\Api to force their use....

What do you think about ?
And, by the way, how is it possible that the code in your example works ? Probably it depends on some certificate already installed and available in your computer ?

@lucasaba lucasaba reopened this May 23, 2013
@stof
stof commented May 23, 2013

All the solutions proposed by @lucasaba are bad as far as security is concerned (and an oauth consumer should not dismiss security). The certificate should be checked to be safe.

@lyrixx
Member
lyrixx commented May 23, 2013

At sensio, we all use this sdk, but we do not use file_get_content transport, we use curl.
Try to use curl.

@stof
stof commented May 23, 2013

@lyrixx But are you using the Buzz curl client (secure by default) or the insecure curl client included in the SDK ?

@lyrixx
Member
lyrixx commented May 23, 2013

Yes, I just discover I used to use the client in the SDK. :(
I put @smaftoul on this topic.

Yes, IMHO, we have an issue here.

@lucasaba
Contributor

@stof I agree with your concern with security. It's just that the README could be misleading. It was hard to get the error (at least for me). I had to use wireshark to understand what was wrong (the error is in the SSL layer). The best solution would be to offer a guide on how to install sensio's certificate to be used by curl or php.

We could just update the README...

@stof
stof commented May 23, 2013

@lyrixx this client should be removed IMO.

People really wanting to disable the security (for instance because they are on windows locally and PHP on windows is unable to use the system certificates) can still configure it by hand

@lucasaba
Contributor

Buzz\Browser uses FileGetContents as default. With my Ubuntu curl will connect without problems, I don't know about the win or mac.
That said, I'd suggest to update \SensioLabs\Connect\Api\Api and \SensioLabs\Connect\OAuthConsumer so that they use curl as ClientInterface (bypassing Buzz\Browser default). Than, update the README and say something like this: "if you have problems with HTTPS and certificates, have a look at this guide on how to configure a CA for curl or file_get_content".

@stof
stof commented May 23, 2013

Why would you use the adapter instead of the public interface of Buzz ?

@lucasaba
Contributor

To make the use of the library easier...I suppose (at least if I understand the meaning of 'adapter'...beg your pardon)...
If you update the adapter in the SDK there's no problem for the final user (aka the README.md is ok). Otherwise you have to use the previous solution one (with some modification):

$cc = new \Buzz\Client\Curl();
$browser  = new \Buzz\Browser($cc);
$consumer = new \SensioLabs\Connect\OAuthConsumer($connect_id, $connect_secret, $scope, 'https://connect.sensiolabs.com' ,$browser);
$api      = new \SensioLabs\Connect\Api\Api('https://connect.sensiolabs.com/api', $browser);

which, in my ioinion, is unpleasent.

@stof
stof commented May 23, 2013

@lucasaba the public API of Buzz is the Browser class, not the Client implementations (which are implementing an adapter pattern). So IMO, it makes more sense to keep injecting the browser.

Thus, if you use the client directly, you will duplicate the code available in the Browser class to be able to do it.
However, the way to pass the endpoint should be changed IMO to avoid having to hardcode it in your code when passing the browser. I see 2 solutions: accepting null and replace it by the default endpoint or define constants for the default endpoint so that you can use them without hardcoding the url.

@lucasaba
Contributor

@stof got it. As for the endpoint I prefer the first option (in this way I don't have to remeber anything...not even the constant) maybe mixed with the second like in

$this->endpoint  = ($endpoint == null) ? self::CONNECT_ENDPOINT : $endpoint;

But, I know it is bad for backward compatibility, Api and OAuthConsumer's constructor signature should be changed. I don't know why but I don't like the idea to pass a null as first parameter.

@stof
stof commented May 23, 2013

Well, another solution (not BC though) would be to move the endpoint at the end of the signature. People will probably never change this endpoint except Sensio devs running a local version of connect.

@lucasaba
Contributor

I think that would the the best solution but...you have to cross your finger :)

@lyrixx
Member
lyrixx commented May 23, 2013

Why not doing something like:

use Buzz\Client\Curl; // Added this line
class Api
{
    // ...
    public function __construct($endpoint = 'https://connect.sensiolabs.com/api', Browser $browser = null, ParserInterface $parser = null, LoggerInterface $logger = null)
    {
        $this->browser = $browser ?: new Browser(new Curl()); // Added new Curl
@lucasaba
Contributor

Well... I think that's what @stof said...

Why would you use the adapter instead of the public interface of Buzz ?

And, anyway, if a programmer wants to use a different Buzz\Client has to hardcode the endpoint.

$browser = new \Buzz\Client\FileGetContents();
$api = new \SensioLabs\Connect\Api\Api('https://connect.sensiolabs.com/api', $browser);
@lyrixx
Member
lyrixx commented May 23, 2013

Yes. But who want to change ?
More over I want to replace buzz by guzzle ;)

@lucasaba lucasaba added a commit to lucasaba/connect that referenced this issue May 23, 2013
@lucasaba lucasaba Ease the use of a custom Browser with Api and OAuthConsumer
With this patch it is easier to pass a customized \Buzz\Browser to
\SensioLabs\Connect\Api\Api and \SensioLabs\Connect\OAuthConsumer
without the nedd to provide the endpoint to the constructor.
It also removes \SensioLabs\Connect\Buzz\Client\Curl which is
unsecure and not needed anymore.

References issue #23
4f4860e
@lucasaba lucasaba added a commit to lucasaba/connect that referenced this issue May 23, 2013
@lucasaba lucasaba Ease the use of a custom Browser with Api and OAuthConsumer
With this patch it is easier to pass a customized \Buzz\Browser to
\SensioLabs\Connect\Api\Api and \SensioLabs\Connect\OAuthConsumer
without the nedd to provide the endpoint to the constructor.
It also removes \SensioLabs\Connect\Buzz\Client\Curl which is
unsecure and not needed anymore.

References issue #23
a80c5a9
@lucasaba
Contributor

Well, guzzle uses curl so...there would not be any problem... at least with non Windows user!

@lyrixx lyrixx added a commit that referenced this issue Jun 3, 2013
@lyrixx lyrixx merged branch lucasaba/23-ease-use-of-custom-browser (PR #25)
This PR was merged into the master branch.

Discussion
----------

Ease the use of a custom Browser with Api and OAuthConsumer

With this patch it is easier to pass a customized \Buzz\Browser to
\SensioLabs\Connect\Api\Api and \SensioLabs\Connect\OAuthConsumer
without the nedd to provide the endpoint to the constructor.
It also removes \SensioLabs\Connect\Buzz\Client\Curl which is
unsecure and not needed anymore.

References issue #23

Commits
-------

d1b3e33 Fix CS and update README
a80c5a9 Ease the use of a custom Browser with Api and OAuthConsumer
9748ebb
@lyrixx
Member
lyrixx commented Jun 3, 2013

I merged your PR and I changed the default transport in c6ec1e4

@lyrixx lyrixx closed this Jun 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment