[4.1] Simple request response refactor #169

Merged
merged 1 commit into from Aug 21, 2014

Projects

None yet

4 participants

@SammyK
Collaborator
SammyK commented Jul 25, 2014

Phew! OK, let's see everyone can agree on this setup. :) This isn't 100% 4.1 feature packed but we can use it as a good starting point if everything is OK.

Setup

To make a request to Graph, you need to instantiate a new FacebookApp to hold your app config. You'll also need a FacebookClient to send request entities to Graph.

use Facebook\Entities\FacebookApp;
use Facebook\FacebookClient;

$facebookApp = new FacebookApp('{app-id}', '{app-secret}');
$facebookClient = new FacebookClient(/* $httpClientHandler, $enableBeta */);

Simple GET Request

use Facebook\Entities\FacebookRequest;

$request = new FacebookRequest($facebookApp, '{access-token}', 'GET', '/me'/*, $params, $eTag, $graphVersion */);
$facebookResponse = $facebookClient->sendRequest($request);

$userProfile = $facebookResponse->getGraphObject();

Simple POST Request

use Facebook\Entities\FacebookRequest;

$request = new FacebookRequest($facebookApp, '{access-token}', 'POST', '/me/feed', ['message'=>'Foo message!']/*, $eTag, $graphVersion */);
$facebookResponse = $facebookClient->sendRequest($request);

$data = $facebookResponse->getDecodedBody();

$id = $data['id'];

Simple DELETE Request

use Facebook\Entities\FacebookRequest;

$request = new FacebookRequest($facebookApp, '{access-token}', 'DELETE', '/{some-id}'/*, $params, $eTag, $graphVersion */);
$facebookResponse = $facebookClient->sendRequest($request);

$data = $facebookResponse->getDecodedBody();

$wasSuccessful = $data['was_successful'];

Batch Request Setup

use Facebook\Entities\FacebookBatchRequest;

$batchRequest = new FacebookBatchRequest(/* $facebookApp, '{access-token}', [$requests], $graphVersion */);

Three ways to add requests to batch request

Via constructor

use Facebook\Entities\FacebookBatchRequest;
use Facebook\Entities\FacebookRequest;

// If $facebookApp and $accessToken on a `FacebookRequest` are null, it will fallback to the parent `FacebookBatchRequest` app & token.
$requests = [
  new FacebookRequest(null, null, 'GET', '/me'),
  new FacebookRequest($facebookApp, '{access-token}', 'POST', '/me/feed', ['foo'=>'bar']),
  ];
$batchRequest = new FacebookBatchRequest($facebookApp, '{access-token}', $requests);

Via add() as an array

use Facebook\Entities\FacebookRequest;

// Numeric array:
$requests = [
  new FacebookRequest(null, null, 'GET', '/me'),
  new FacebookRequest($facebookApp, '{access-token}', 'POST', '/me/feed', ['foo'=>'bar']),
  ];
$batchRequest->add($requests);

// ... OR ...

// Associative array:
// The key of the array will be used to assign a name to the request.
$requests = [
  'named-request-one' => new FacebookRequest(null, null, 'GET', '/me'),
  'named-request-two' => new FacebookRequest($facebookApp, '{access-token}', 'POST', '/me/feed', ['foo'=>'bar']),
  ];
$batchRequest->add($requests);

Via add() as a FacebookRequest

$batchRequest->add(new FacebookRequest(null, null, 'GET', '/me'));
$batchRequest->add(new FacebookRequest($facebookApp, {access-token}, 'GET', '/me'), 'my-reqest-name');

Getting a response from batch request

$facebookBatchResponse = $facebookClient->sendBatchRequest($batchRequest);

foreach ($facebookBatchResponse as $response) {
  if (!$response->isError()) {
    // $response is a `FacebookResponse` entity
  } else {
    $e = $response->getThrownException();
    echo $e->getMessage();
  }
}

Helpers

You also pass along the FacebookApp and FacebookClient into the helpers like so:

$app = new FacebookApp('123', 'foo_app_secret');
$helper = new FacebookRedirectLoginHelper($app);

AccessToken

The AccessToken entity requires FacebookApp and FacebookClient for requests that call Graph. For example:

$accessTokenFromCode = AccessToken::getAccessTokenFromCode('foo_code', $app, $client);

I think this could be done better for the future. I'd like to simplify the AccessToken entity to only hold data about the access token. And any calls to graph could be done with a FacebookAccessTokenHelper. What do you guys think?

Simplified FacebookHttpClientInterface

I also simplified the FacebookHttpClientInterface so that the request headers could be sent as an argument in the send() method.

Integration Tests Labled

The tests that actually touch Graph are labeled "integration". So you can run the test suite with:

$ phpunit --exclude-group integration

And it won't touch the live version of Graph. So now we can incorporate Travis CI without any issues! :)

TODO

For the next few PR's:

  • Factories to make FacebookRequest's and FacebookBatchRequest's
  • Collections on GrahObject
  • Adding GraphList with pagination
@gfosco
Contributor
gfosco commented Jul 25, 2014

I would think the basic use-case involves a single FacebookApp. I'd like to see passing the app everywhere as totally optional, or a ->setApp($app) method on the request?..

@SammyK
Collaborator
SammyK commented Jul 25, 2014

Fo sho! Instead of using statics to achieve this, I think we can turn FacebookApp into a factory to do the injection for you:

$facebookApp = new FacebookApp('{app-id}', '{app-secret}'/*, '{default-access-token}' */);

$getRequest = $facebookApp->newRequest(/* '{access-token}' */)->get('/me');
$postRequest= $facebookApp->newRequest()->post('/me/feed', ['foo'=>'bar']);

$bastchRequest = $facebookApp->newBatchRequest(/* '{access-token}' */);
@gfosco
Contributor
gfosco commented Jul 25, 2014

In that example, is the FacebookApp also creating/managing the FacebookClient? How does that change the set-up, etc.. If you can update the docs above I'll give it another pass.

@yguedidi
Collaborator

I'm still not a big fan of how you did it @SammyK, sorry. :/
But yours is tested, not mine... I "bow down" (Google Translate lol)

@SammyK
Collaborator
SammyK commented Jul 25, 2014

You'd pass it into sendRequest() or sendBatchRequest() just like before:

$facebookApp = new FacebookApp('{app-id}', '{app-secret}'/*, '{default-access-token}' */);
$facebookClient = new FacebookClient();

$getRequest = $facebookApp->newRequest()->get('/me');
$facebookResponse = $facebookClient->sendRequest($request);

That keeps a nice separation on concerns. If we really wanted to, we could have a factory for the client in FacebookApp as well:

$facebookApp = new FacebookApp('{app-id}', '{app-secret}'/*, '{default-access-token}' */);
$facebookClient = $facebookApp->newClient(/* $httpClientHandler, $enableBeta */);

Or maybe a way to set the default $httpClientHandler and $enableBeta in the FacebookApp and then the FacebookRequest object could new up a FacebookClient automatically:

// Single Request
$facebookApp = new FacebookApp('{app-id}', '{app-secret}', '{default-access-token}', $httpClientHandler, $enableBeta);
$facebookResponse = $facebookApp->newRequest()->get('/me')->send();

// Batch Request
$batchRequest = $facebookApp->newBatchRequest();

$requests = [
  $facebookApp->newRequest()->get('/me'),
  $facebookApp->newRequest()->post('/me/feed', ['foo' => 'bar']),
  $facebookApp->newRequest('access-token')->get('/me'),
];

$batchRequest->add($requests);

$facebookBatchResponse = $batchRequest->send();
@SammyK
Collaborator
SammyK commented Jul 25, 2014

@yguedidi - what do you not like? I'm open to suggestions. :)

@yguedidi
Collaborator

I don't like how you decouple the code and how you link classes.
My thought is that the key data is the access token, and end users will always use a helper to get an access token, at least at the first time. So helpers is the main interface for them.
That said, I think internal classes have to be strictly typed, and all "magic" things (like converting a string access token to an object, building a request with or without a fluent interface, any thing that make lif easier) should be made in helpers, not in internal classes.

@SammyK
Collaborator
SammyK commented Jul 25, 2014

@yguedidi Do you have specific suggestions on how to decouple or link the classes better? Or if @gfosco were to merge this in, would you be able to use it as a base to tweak it the way you think it should be handled?

I agree with you on the helpers, at least if I understand you correctly. For example, my suggestion on the AccessToken entity and splitting out the calls to Graph in an access token helper.

@gfosco
Contributor
gfosco commented Jul 25, 2014

I'd like @yguedidi to put together his example usages as @SammyK did in the first comment here.

@yguedidi
Collaborator

I already show them in #145 starting from this comment but here is an updated quick summary about the easier way using request builders:

Setup

$helper = FacebookRedirectLoginHelper::create($appId, $appSecret/* , $httpClient, $useSecretProof, $useBeta */);
// or an other helper

Simple requests

$response = $helper->request($accessToken)->{get|post|delete}($endpoint)
  //->withParams($params)
  //->withETag($eTag)
  ->send();
// send() can also directly returns a GraphObject
$property = $response['property']; // use ArrayAccess

Batch requests

$batchResponse = $helper->batch(/*$fallbackAccessToken*/)
  ->add(new FacebookRequest(...))
  ->{get|post|delete}($endpoint)
  //->withAccessToken($requestAccessToken)
  //->withParams($params)
  //->withETag($eTag)
  ->{get|post|delete}($endpoint) // add a new request
  // ...
  ->send();
foreach ($batchResponse as $response) {
  if (null === $response) {
    continue; // Timeouted response
  }
  if ($response->isError()) {
    $e = FacebookResponseException::create($response);
    echo $e->getMessage();
  } else {
    // $response is a `FacebookResponse` entity
  }
}

But all this things above are the end user public API, it can be subject to debate, change and discuted later, I more talk about internals, and my main idea is in #145 commits.

@yguedidi
Collaborator

Internals

This is verbose, without helpers or builders, but it's just to show you how I think internals should be.

$facebookApp = new FacebookApp($appId, $appSecret);
$facebookClient = new FacebookClient(/*$httpClient, $graphVersion, $useSecretProof, $useBeta*/)

// Get an access token through a helper or:
$accessToken = new AccessToken($facebookApp, $accessTokenValue);

$request = new FacebookRequest($endpoint/*, $method, $params, $accessToken, $eTag*/);
$requests = [$request, $request];
$batchRequest = new FacebookBatchRequest($requests/*, $fallbackAccessToken*/);

$response = $facebookClient->handle($request);
$batchResponse = $facebookClient->handle($batchRequest);
@SammyK
Collaborator
SammyK commented Jul 26, 2014

So you're saying add factories to the helpers, yes? :) If so then 👍 We can always modify the helpers to work as you outlined in another PR while keeping the same internal structure.

Basically the internals for handling request and responses between the PR's are very similar:

  1. You new up a FacebookApp.
  2. You new up a FacebookClient.
  3. You make a FacebookRequest.
  4. You send it with your instantiation of FacebookClient.

That's also why I'm thinking front-end API factory stuff can be added after this PR is merged. Either like I outlined or like @yguedidi outlined. Whichever one you prefer. :)

A few more thoughts:

  1. FacebookClient doesn't need to concern itself with $graphVersion since the version needs to be tracked at the FacebookRequest level. This is because in a batch call you can send requests to multiple versions at once.
  2. I've made app_secretproof enabled by default in my PR with no way to turn it off. I think this is one of those security features Facebook should require of all their apps without a way to turn them off. Otherwise people could session hijack the hell out of access tokens. Yikes! :-0
  3. The reason why I send FacebookApp to FacebookRequest is because the app_secretproof needs to be calculated at the request level. In a batch request, you can make requests across multiple apps and access tokens. So each request needs to depend on the FacebookApp entity. Likewise the FacebookResponse's need to know which app and access token were used so that they can make subsequent pagination calls. This makes the SDK hella useful for the power users.
  4. FacebookBatchRequest doesn't need to concern itself with the eTag since those are per-request specific and Graph does not return an eTag for the root batch request.

So aside from the order of the arguments on some of the methods and what I pointed out above, I think the internals are very similar. :)

@yguedidi
Collaborator

Yes, as I already said here.

  1. 👍
  2. I think it can be disable as the doc say it's optional. Maybe some people don't want to use it (but they should)
  3. You can get FacebookApp from the AccessToken in my thought
  4. You're right.

I'm not really against merging this, even if I still don't like the way it's coded (this is not a personal attack @SammyK lol). It will be harder for me to add my changes on top of this, but things have to go forward!

@gfosco if you want to compare with my "coding style", look at #167 or the outdated but more complete #145.

@SammyK
Collaborator
SammyK commented Jul 27, 2014

I never feel like your code reviews are a personal attack @yguedidi! :) I always really appreciate your ideas and thoughts. Thanks for that!

@SammyK SammyK referenced this pull request Jul 31, 2014
Closed

[4.1] Refactor #175

@gfosco
Contributor
gfosco commented Jul 31, 2014

I'm not liking this: $request = $app->newRequest()->get('/me');... get/post/delete seem like actions, and these methods are just configurators. How about $request = $app->newGetRequest('/me');

This whole refactoring effort is so massive, I wish we could all sit in the same room and hash it out.

Let's put the brakes on for just a few days. I should have more bandwidth later next week.

@SammyK
Collaborator
SammyK commented Jul 31, 2014

Sounds good! I'm down for scheduling a 3-way Skype session if you guys want to.

This was referenced Aug 2, 2014
@dcelasun

I'd like to make a tiny suggestion, based on the discussion in #193:

Please let the SDK have a way of making a request without an application or session context. The existing propasals in this PR all seem to require either an app object like:

$facebookApp = new FacebookApp('{app-id}', '{app-secret}', '{default-access-token}', $httpClientHandler, $enableBeta);
$facebookResponse = $facebookApp->newRequest()->get('/me')->send();

or helper objects like:

$helper = FacebookRedirectLoginHelper::create($appId, $appSecret/* , $httpClient, $useSecretProof, $useBeta */);
$response = $helper->request($accessToken)->{get|post|delete}($endpoint)

However, both suggestions require the presence of an app id / secret. I think the very basic form of FacebookRequest should be completely decoupled from them.

For example, if I have an app-scoped user id and an associated access token, I should simply be able get, say the friends list, with just that information:

$request = new FacebookRequest();
$request->setAccessToken("CAx7867xc8v6x8c7v");
$request->setUserId("4");
$response = $request->get("friends"); // returns Zuck's friend list as a GraphObject

This is a quick & dirty example and the actual implementation could be a lot cleaner, but I hope it's enough to convey the idea.

Thoughts @SammyK @yguedidi @gfosco?

@SammyK
Collaborator
SammyK commented Aug 13, 2014

I hear yeah. The thing to keep in mind is that the app secret proof feature is enabled by default and the hash it generates needs the app secret in order to be calculated.

So for your example to work you'd need to either provide at minimum the app secret...

$request = new FacebookRequest('{app-secret}');
$request->setAccessToken("{access-token}");
$response = $request->get("/me");

...Or disable the app secret proof which is strongly discouraged as it exposes all your access tokens to much higher risk of a session hijack attack.

$request = new FacebookRequest();
$request->disableAppSecretProof();
$request->setAccessToken("{access-token}");
$response = $request->get("/me");

It would be in the best interest of everyone to enforce higher security out-of-the-box. Which is why I have pushed for removing the feature that allows you to disable the app secret proof altogether.

@dcelasun

app secret proof feature is enabled by default

It is enabled by default in the SDK, but newly created apps have it off by default. I understand (and agree with) the security implications of turning it off, but sometimes it simply is necessary. For example, we have Flash applications directly making calls to the Graph API and they can't use app secret proofs as it would expose the proof hash to end users.

Which is why I have pushed for removing the feature that allows you to disable the app secret proof altogether.

I fully agree with keeping it on by default (and discourage disabling it in the docs), but it should be possible to disable it as long as the API supports not having it.

@SammyK
Collaborator
SammyK commented Aug 13, 2014

It shouldn't matter if calls are being made to Graph on the client side and the client can see the app secret proof hash. The app secret proof is a one-way encrypted hash of the app secret and access token. So the app secret proof should still be used in the context you described.

That being said, all calls to Graph on a client need to be using a short-lived access token so that in the unfortunate event that the token does get hijacked, it won't last long.

enabled by default in the SDK, but newly created apps have it off by default

Indeed! This should be changed. The recent iOS/Android SDK "access token leak" is a prefect example of why app secret proofs should be enabled at the app-level by default.

@dcelasun

So the app secret proof should still be used in the context you described.

If the client has the proof, what's the point of having it? Some malicious third party can just grab it along with the access token and make requests. It doesn't matter that the hash is one-way, simply having it is enough to spoof requests.

all calls to Graph on a client need to be using a short-lived access token so that in the unfortunate event that the token does get hijacked, it won't last long.

True, but in my experience, using only the extended access token is wide spread across the industry. Which is not really surprising since the official docs have a big graphic that recommends passing the extended token to clients :)

@SammyK
Collaborator
SammyK commented Aug 13, 2014

what's the point of having it? Some malicious third party can just grab it along with the access token and make requests.

Yes, with that one instance for a short amount of time (assuming you're using a short-lived token). But say an attacker gets into your database and suddenly has access to all your access tokens. If you have app secret proof enabled on your app, the attacker won't be able to make any requests to Graph. So it's still more secure.

using only the extended access token is wide spread across the industry

And it shouldn't be. That's not a valid excuse not to implement better security practices for projects. We have a responsibility as developers to protect our user's data as best we can.

@dcelasun

Those are all great points @SammyK and I agree with them. Still, I think we are getting a bit off-topic here :)

@SammyK
Collaborator
SammyK commented Aug 14, 2014

You're inspiring me to write my next blog post about securing your Facebook app. :)

@dcelasun

Send me a link if you write it :)

@SammyK
Collaborator
SammyK commented Aug 14, 2014

Will do!

@SammyK
Collaborator
SammyK commented Aug 14, 2014
@dcelasun

Looking good 👍 it's a nice summary of what the app proof is and how to use it.

One minor nitpick though: In the "going further" part, you say

If you ever expose the access token in the URL on the client side, make sure it's always a short-lived access token.

It doesn't have to be exposed in the URL. Any client-side exposure (think flash games that get the token via flashvars) should warrant a short-lived token :)

@yguedidi
Collaborator

Great blog post @SammyK

@SammyK
Collaborator
SammyK commented Aug 18, 2014

Thanks guys. :)

@SammyK
Collaborator
SammyK commented Aug 19, 2014

Rebased! Per our Hangout discussion we can add factories, builders, collections, Graph 2.1 support, etc on top of this to start seeing a light at the end of the 4.1 stable tunnel! :)

@gfosco gfosco merged commit 9a55bfd into facebook:master Aug 21, 2014
@SammyK
Collaborator
SammyK commented Aug 21, 2014

High-five for 4.1 progress!

@SammyK SammyK deleted the SammyK:simple-request-response-refactor branch Aug 24, 2014
@SammyK SammyK referenced this pull request in SammyK/LaravelFacebookSdk Aug 28, 2014
Closed

Batch API calls #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment