symfonycorp / connect

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

Ease the use of a custom Browser with Api and OAuthConsumer #25

Closed lucasaba closed 11 years ago

lucasaba commented 11 years ago

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

lyrixx commented 11 years ago

@marcw: What do you thing ?

lucasaba commented 11 years ago

I'm sorry but I couldn't find any code calling SensioLabs\Connect\Buzz\Client\Curl. It's only purpouse is to not verify https connections. It is somehow hidden inside the code. It is not reported in the example.... it seems useles. Am I wrong ? I think @marcw put that file in his initial commit (ee95cb5958a7a656e80143046ee64b469641c155) for testing purpose. Maybe he had some problem with https too :)

lyrixx commented 11 years ago

I just checked in some sensiolabs project. It seems it is not used. I will try to check on all project next week.

stof commented 11 years ago

@lucasaba it would be used if some people configure the client with it directly.

but I agree it should be removed. If someone really want to be insecure in their project, they can still do it with the buzz client.

marcw commented 11 years ago

Yes it should be removed. It was added because on some systems you may not have the necessary certificates to validate that the one on connect is valid. The correct thing we can do to deal with this issue is to add the certificate chain to this repository. Facebook is doing this for their php sdk: See https://github.com/facebook/facebook-php-sdk/blob/master/src/fb_ca_chain_bundle.crt and https://github.com/facebook/facebook-php-sdk/blob/master/src/base_facebook.php#L966-L972

lucasaba commented 11 years ago

Do you think something like this https://gist.github.com/lucasaba/5661223 could be a solution ?

lyrixx commented 11 years ago

I do not like this hack. I prefer @marcw solution.

lucasaba commented 11 years ago

Maybe I didn't get it. I tought that the gist was like @marcw solution. It overloads Buzz\Browser's send method and catches the CA error like in Facebook's API. You mean you'd prefer to completely remove the dependecy from Buzz\Browser ?

marcw commented 11 years ago

I think it should be documented rather than implemented in the SDK.

lucasaba commented 11 years ago

Then, I think, this PR is ok :) We just need a new one wich updates the documentation and adds the ca_chain.crt

lyrixx commented 11 years ago

Thanks you.

I made some tweaks in c6ec1e4

lucasaba commented 11 years ago

Thank you @lyrixx. With your tweaks is much clearer and elegant! :+1: