Oauth2 support beginnings #57

Merged
merged 9 commits into from Aug 31, 2011

2 participants

@andycutright

Hi,

We're migrating to oauth2 per the FB road map. This set of commits along with the commits for mogli start down that path. The changes are incremental, and reflect what we need, basically the ability to use the oauth2 cookie to get an access token.

Please let me know if you have questions or want changes.

Cheers,
Andy

@mmangino
Owner

This looks good, but I'm a little worried about a couple of things. First, I don't like the conditional and the configuration bits. Can we not try oauth2 and fall back to oauth if the oauth2 stuff isn't there? Also, it looks like code coverage is really light. Are all of the methods well covered?

@andycutright

Hi, thanks for reviewing the pull request. I see I've pushed code with some of the javascript tests commented. Sorry about that. I'd like to change those test to use JSlint rather than comparing with the output with a hardcoded heredoc. I just pushed that change. You want me to issue a new pull request or do you want to take a look at that first?

andycutright@1fea1bf

Using jslint ensures the generated javascript is well formed, and makes the spec easier to maintain imo. The critical js syntax testing is separated from the configuration testing. It does introduce a dependency on Java for the spec.

This should bring the coverage up significantly. I feel the new code is tested well. My original pull requests includes tests for all the files I've modified, the configuration code, the javascript helper and the controller. I can add more, but I don't think they'll add a lot of value. The new and modified controller methods are composed from other bits of code that are tested well. The new base64 URL decoding method has corresponding tests. Also, we should discuss what changes you want before I add more tests.

I used the configuration approach to reduce the impact on folks that aren't ready to migrate yet. Falling back to oauth1 from oauth2 at runtime will lead to more complicated code imo. The FB JS SDK requires the specific property 'oauth : true' and this setting changes the cookie name and cookie contents. The configuration makes it clear you're using oauth, and drives control down only that path. Also, you can simply enable or disable oauth2 by changing one configuration variable. That makes life simple for the gem users.

I'm trying to minimize the scope of change. This code doesn't represent a fully oauth2 compliant library since we currently don't need that (yet). I'm presuming folks that need more oauth2 functionality will build on what your code and my changes. If we had a full implementation it might be possible to more easily fall back.

Cheers,
Andy

@mmangino
Owner

Thanks for the detail. That all makes sense to me. Requiring java for tests is a non-starter for me, so the current stuff is fine. I can cherry pick in this commit. Thanks!

@mmangino mmangino merged commit f44b4ab into mmangino:master Aug 31, 2011
@andycutright

You see the Mogli pull request as well right? That's necessary for this to run properly. That change is minimal and includes testing.

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