Added optional appsecret_proof configuration parameter. #323

nchelluri · 2013-08-14T07:35:57Z

No description provided.

nchelluri · 2013-08-14T08:51:55Z

I wouldn't merge this in just yet. I am thinking, it probably makes more sense to pass in the appsecret and have Koala use OpenSSL to add the appsecret_proof param if there is an access token present. But, it should only do this if using the Graph API, since that is the only one that supports it.

What do you think?

nchelluri · 2013-08-14T12:06:18Z

Better...

niels · 2013-09-20T14:13:52Z

lib/koala/api.rb

@@ -45,6 +46,8 @@ def api(path, args = {}, verb = "get", options = {}, &error_checking_block)
        # This is explicitly needed in batch requests so GraphCollection
        # results preserve any specific access tokens provided
        args["access_token"] ||= @access_token || @app_access_token if @access_token || @app_access_token
+        args['appsecret_proof'] = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha256'), Koala.config.appsecret, args['access_token']) if options[:appsecret_proof] && args['access_token'] && Koala.config.appsecret


Perhaps also use double quotes here for consistency?

I would also break this line up -- it's really long. Putting the if on the line above will be much more readable.

I concur with @nielsomat that we probably don't need to offer overriding on a per-request basis. If someone wants it, they can submit a PR, but for now less code is better code.

I've changed the quotes -- but the Ruby Style Guide suggests we use single quotes :)

https://github.com/bbatsov/ruby-style-guide#strings

niels · 2013-09-20T14:18:58Z

+1 from me in principle!

I would however set the default config in Koala itself, just like http_service is currently configured (https://github.com/arsduo/koala/blob/master/lib/koala.rb#L26). Further, I would rename the option to use_appsecret_proof for clarity.

Not sure if the option needs to remain overridable per-request as in your current implementation. Once you have your app secret configured, what's the benefit of disabling appsecret_proof for individual requests? The only one I see is to save the overhead incurred by generating a hash for every request. IMO using appsecret_proof at all only makes sense though when forbidding non-proofed calls via the app settings. When doing so, you can't send any non-proofed calls anyway, so… should we get rid of the override in order to promote safety? :)

arsduo · 2013-09-28T01:31:24Z

readme.md

+Koala.configure do |config|
+  config.appsecret = app_secret
+  # see lib/koala/api/graph_api.rb
+end


I would actually suggest triggering the app secret slightly differently -- there are applications out there that have to work with multiple Facebook applications within the context of the same Ruby app. (A good example is social marketing/brand management, the type of platform I was working on when I started Koala.) A global configuration wouldn't work for that situation.

Instead, what do you think of making the secret be the second parameter for the API initializer? That way, you could easily specify the secret to be used for proofing at the time you create the API. This has two additional bonuses -- it's consistent with the way we pass the secret into other Koala classes, and additionally, many applications don't use the Koala setup block (they just instantiate an API object) so this avoids the need to configure Koala somewhere else on application load for those apps.

# if secret is passed in, proof all the requests that API object makes api = Koala::Facebook::API.new(token, secret)

arsduo · 2013-09-28T01:47:13Z

I really like this -- thanks for a great pull request. Having documentation and tests is a huge help as a maintainer.

I've posted some comments -- in particular, I've suggested a different mechanism for passing in the app's secret and enabling the proofing, since the current global approach (while a good solution for most use cases) won't work for Ruby applications that need to manage more than one Facebook app.

Let me know what you think about that and my other comments -- I'd love to get this into 1.8.

Best,

Alex

nchelluri · 2013-10-11T17:55:13Z

Hi, sure, I'd be happy to make the changes.

What is your timeline for releasing 1.8? I'm a bit busy atm, but can probably fit it in if the release is not too soon. Could make end of October for sure.

If I get tired of my day job it might end up happening sooner :)

- updated readme accordingly - updated tests accordingly - cleaned up some code per code review

nchelluri · 2013-10-11T19:09:24Z

Made all the changes I could think of; please let me know how you guys feel about this. Thanks for the feedback!

nchelluri · 2013-10-11T23:46:03Z

Looks like the tests are failing because bundle install is failing to complete. I am not too sure what to do about this.

arsduo · 2013-10-26T16:28:38Z

I'll take a look at the Travis tests and get this merged in today; sorry about the delay. Looks great!

arsduo · 2013-10-26T17:40:17Z

The Travis tests were failing for various Ruby 1.8-compatible installations due to underlying gems requiring 1.9+. I think it's time to just cut off support for 1.8. Merging this in now!

nchelluri · 2013-11-01T23:23:16Z

Hooray!

nlsrchtr · 2013-11-15T16:21:24Z

Great commit! Looking forward to this merge and the increase in security. Especially since tools like Buffer were hacked.

Thanks for your work, @nchelluri!

arsduo · 2013-11-15T16:52:24Z

The code is working, I'll merge it by this weekend and release 1.8rc2.

arsduo · 2013-12-17T18:32:44Z

Huh, I thought I merged this in. I'll take care of this soon, sorry about the delay.

nlsrchtr · 2013-12-17T21:06:22Z

👍

adrienkohlbecker · 2014-01-28T13:20:49Z

It seems this PR has not been merged before the 1.8.0 release, it's in the changelog but not in the code

arsduo · 2014-02-01T17:36:16Z

You're right. Not sure how that happened, I thought I merged that in. Fixing now.

arsduo · 2014-02-01T19:11:31Z

The new merged version is in integration testing on Travis -- I'll be releasing it and a few other internal changes as 1.9.0rc1 shortly, and will post here so you can test it.

Here's the final behavior for appsecrets:

If you provide an appsecret argument when initializing an API instance, it'll be included by default in all your requests: API.new(token, secret).get_object("/foo")
You can disable this on a per-request basis (if you really wanted to): API.new(token, secret).get_object("/foo", {}, appsecret_proof: false)
If you don't provide an appsecret (or you're making public API requests without a token), no appsecret_proof will be included.

The readme will be updated as well when this gets merged in.

Thanks and sorry about the mistake!

arsduo · 2014-02-01T19:36:35Z

1.90rc1 is released -- let me know if you enounter any issues or have any questions around this! Otherwise I'll release the full gem soon.

adrienkohlbecker · 2014-02-01T22:16:12Z

Great! I'll try this out and report back if I find any issues.
Thanks

adrienkohlbecker · 2014-02-05T14:21:13Z

No issues so far

arsduo · 2014-02-05T14:29:18Z

Great, I'll release it this afternoon. Given the changes I really don't
expect any problems.
Am 05.02.2014 08:21 schrieb "Adrien Kohlbecker" notifications@github.com:

No issues so far

Reply to this email directly or view it on GitHubhttps://github.com//pull/323#issuecomment-34175958
.

nchelluri added 2 commits August 14, 2013 00:32

Added optional appsecret_proof configuration parameter.

948fe43

Clean up full config after each testcase

0ab275e

Handle Graph argument appsecret_proof given an appsecret

c7451cb

nchelluri added 3 commits August 14, 2013 05:14

Attempting to fix tests for Ruby 1.8.7

38769b2

Attempting to fix tests for Ruby 1.8.7

0c9bbff

Updated readme for appsecret configuration option

2577786

niels reviewed Sep 20, 2013
View reviewed changes

arsduo reviewed Sep 28, 2013
View reviewed changes

nchelluri added 2 commits October 11, 2013 12:03

Changed mechanism for passing appsecret in from config to constructor.

bca2bc9

- updated readme accordingly - updated tests accordingly - cleaned up some code per code review

Added Koala.reset_config method for testing

21694dc

nlsrchtr pushed a commit to nlsrchtr/koala that referenced this pull request Nov 15, 2013

Added pull-request arsduo#323 manually

227f718

nlsrchtr mentioned this pull request Nov 16, 2013

Enable secured Graph API requests by default simi/omniauth-facebook#137

Closed

arsduo merged commit 21694dc into arsduo:master Feb 1, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added optional appsecret_proof configuration parameter. #323

Added optional appsecret_proof configuration parameter. #323

nchelluri commented Aug 14, 2013

nchelluri commented Aug 14, 2013

nchelluri commented Aug 14, 2013

niels Sep 20, 2013

arsduo Sep 28, 2013

nchelluri Oct 11, 2013

niels commented Sep 20, 2013

arsduo Sep 28, 2013

arsduo commented Sep 28, 2013

nchelluri commented Oct 11, 2013

nchelluri commented Oct 11, 2013

nchelluri commented Oct 11, 2013

arsduo commented Oct 26, 2013

arsduo commented Oct 26, 2013

nchelluri commented Nov 1, 2013

nlsrchtr commented Nov 15, 2013

arsduo commented Nov 15, 2013

arsduo commented Dec 17, 2013

nlsrchtr commented Dec 17, 2013

adrienkohlbecker commented Jan 28, 2014

arsduo commented Feb 1, 2014

arsduo commented Feb 1, 2014

arsduo commented Feb 1, 2014

adrienkohlbecker commented Feb 1, 2014

adrienkohlbecker commented Feb 5, 2014

arsduo commented Feb 5, 2014

Added optional appsecret_proof configuration parameter. #323

Added optional appsecret_proof configuration parameter. #323

Conversation

nchelluri commented Aug 14, 2013

nchelluri commented Aug 14, 2013

nchelluri commented Aug 14, 2013

niels Sep 20, 2013

Choose a reason for hiding this comment

arsduo Sep 28, 2013

Choose a reason for hiding this comment

nchelluri Oct 11, 2013

Choose a reason for hiding this comment

niels commented Sep 20, 2013

arsduo Sep 28, 2013

Choose a reason for hiding this comment

arsduo commented Sep 28, 2013

nchelluri commented Oct 11, 2013

nchelluri commented Oct 11, 2013

nchelluri commented Oct 11, 2013

arsduo commented Oct 26, 2013

arsduo commented Oct 26, 2013

nchelluri commented Nov 1, 2013

nlsrchtr commented Nov 15, 2013

arsduo commented Nov 15, 2013

arsduo commented Dec 17, 2013

nlsrchtr commented Dec 17, 2013

adrienkohlbecker commented Jan 28, 2014

arsduo commented Feb 1, 2014

arsduo commented Feb 1, 2014

arsduo commented Feb 1, 2014

adrienkohlbecker commented Feb 1, 2014

adrienkohlbecker commented Feb 5, 2014

arsduo commented Feb 5, 2014