Skip to content
This repository has been archived by the owner on Mar 29, 2019. It is now read-only.

Option to not provide appsecret_proof in API request #22

Merged
merged 2 commits into from
Oct 9, 2014

Conversation

MartinMystikJonas
Copy link

There are use cases when we do not want to provide appsecret_proof in request at all.

One example is validation of access_token generated by different application.

Validation is possible by checking access token owner:
https://graph.facebook.com/me?access_token=CAAHT1drGwlkBANZCnXNXTFzEvyVGKU89z0n0PMrnZCZCZB2VYL3YgTqhcwrSPGCCXDBZBKEfeDvrkJBgnkJhFo87IWFwTZAXU2SJ6GerYg4JnHgLt3NzqhFvNaFmBa7xzcf124UcZAFBJ0mEOef1ZAZAb7z8vJLLU2tyFL5DCOkciZCXLXWemU3awjJNZB8GaLq81MtpB50ilXJxIs9x4xlUQFQ

But when appsecret_proof is provided it ends by error "Invalid appsecret_proof provided in the API argument" if original token is generated by different app.

We use this to prove user is logged in mobile app when accessing our application API. User lg in in mobile app. Mobile app recevives own access token. When authentication with our API, mobile app just send us access token, which we validates.

Usage then looks like this:

$facebook->api('/me', NULL, array("appsecret_proof" => false));

@fprochazka
Copy link
Member

I think it would be better to come up with all the requests that require appsecret_proof and provide it only to them.

@@ -193,6 +193,10 @@ public function oauth($url, array $params)
$params['appsecret_proof'] = $this->fb->config->getAppSecretProof($params['access_token']);
}

if($params['appsecret_proof'] === false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work, because the appsecret_proof has already been set to some string, you have to work it in the previouse condition.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works. I explicitly provide false value in parameters of requests where I do not want appsecret_proof. Previous condition sets it to false and this just removes it from params if value is false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the cs typo, there should be a space after if

@MartinMystikJonas
Copy link
Author

It would be better but how do you know if request requires appsecret_proof or not? It depends on settings: https://developers.facebook.com/docs/graph-api/securing-requests#require-proof

@fprochazka
Copy link
Member

To not break the compatibility, best would be to add config value that turns off adding of appsecret_proof and explicitly add it to only few requests that require it.

@MartinMystikJonas
Copy link
Author

Facebook application could be configured that it requires appsecret_proof on all calls. If you don't use access_token provided for different application then appsecret_proof is not problem. It is problem only in cases like I described earlier.

Maybe it could be used as config value (by default true). And then have option to enable/disable it in code for single call by syntax I used. Therefore you can explicitly provide it in params or explicitly disable it by provide false value for it in params.

Draft in next commit.

@MartinMystikJonas MartinMystikJonas force-pushed the appsecret-proof-optional branch 2 times, most recently from c27b98b to 127ef2f Compare October 9, 2014 11:30
@fprochazka
Copy link
Member

Exactly! Now I like it :)

@fprochazka
Copy link
Member

I would preffer having a test case for this, but it is pretty straighforward, so if you fix that CS typo, we can merge it :)

@MartinMystikJonas
Copy link
Author

CS fixed

@fprochazka
Copy link
Member

Nice, thank you!

fprochazka added a commit that referenced this pull request Oct 9, 2014
Option to not provide appsecret_proof in API request
@fprochazka fprochazka merged commit cb8deb4 into Kdyby:master Oct 9, 2014
@fprochazka
Copy link
Member

@MartinMystikJonas Btw, I would suggest you to add the email you're using to commit to your github account, so your commits are bound to you :) It looks nicer, if you have the username and avatar next to your commits :)

@MartinMystikJonas
Copy link
Author

Thanks for noticing. I just added it.

@fprochazka
Copy link
Member

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants