symfonycorp / connect

The SymfonyConnect official API SDK
https://connect.symfony.com/
MIT License
90 stars 40 forks source link

Unable to retrieve auth token #23

Closed lucasaba closed 11 years ago

lucasaba commented 11 years ago

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 commented 11 years ago

Hello.

Do you use https://github.com/sensiolabs/Silex-Connect ?

lucasaba commented 11 years ago

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

lyrixx commented 11 years ago

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

lucasaba commented 11 years ago

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 commented 11 years ago

You should register the SecurityExtension in your app.php

lucasaba commented 11 years ago

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 commented 11 years ago

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 ?

stof commented 11 years ago

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 commented 11 years ago

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

stof commented 11 years ago

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

lyrixx commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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

lucasaba commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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

lyrixx commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

lucasaba commented 11 years ago

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

lyrixx commented 11 years ago

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