Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Google OAuth provider #32

Merged
merged 3 commits into from Jun 25, 2015

Conversation

Projects
None yet
5 participants
@ssaavedra
Copy link
Contributor

ssaavedra commented Jun 7, 2015

No description provided.

@jferris

This comment has been minimized.

Copy link
Member

jferris commented Jun 8, 2015

Hey @ssaavedra,

Thanks for the pull request. Does this provide different functionality than the GoogleEmail2 module?

@ssaavedra

This comment has been minimized.

Copy link
Contributor Author

ssaavedra commented Jun 8, 2015

Well, to be honest, I didn't find GoogleEmail2 yesterday, and that was why I made this.

However, there are a couple differences:

  • GoogleEmail2 uses the user's mail address as identifier, whilst this PR currently uses the some Google UID.
  • GoogleEmail2 provides many more answers and metadata (such as gender or whether the user has Google Plus activated)
  • This PR is much less in size and I think it can be easier to understand to novel eyes how it works (of course, we already have other examples like Yesod.Auth.OAuth2.GitHub for this)

If you believe this is not worth merging, I can understand that, but let me know what you think about it.

@pbrisbin

This comment has been minimized.

Copy link
Collaborator

pbrisbin commented Jun 8, 2015

I too didn't know about GoogleEmail2 for a long time, and would've expected to find that functionality here, not there (see comments in #18).

ping @snoyberg -- what would you think of deprecating GoogleEmail2 and pointing users at this project instead?

If that's a Good Idea, we should merge this and ensure feature parity. If not, then I would suggest not merging this, adding a note to the README pointing users at GoogleEmail2 and (optionally) opening your "more answers and metadata" feature as a PR on that module. I think using the guid-vs-email is not a big enough difference to warrant duplicating the functionality in two projects.

@snoyberg

This comment has been minimized.

Copy link
Contributor

snoyberg commented Jun 9, 2015

I'm in favor of your recommendation (deprecating GoogleEmail2), though it seems to do it the differences mentioned above will need to be worked out. For the ID issue, perhaps a configure option would be sufficient.

@pbrisbin

This comment has been minimized.

Copy link
Collaborator

pbrisbin commented Jun 9, 2015

Agreed about considering the API differences before moving forward.

  • email vs guid

IMO, this would cause the biggest problem for users trying to migrate. I don't see a clear win in storing the guid vs the email as the ident -- sure it makes more conceptual sense, but email should be just as unique and consistent for a given user. @ssaavedra what do you think about changing this PR to use the email?

  • more answers and metadata

As long as this is a strict increase in functionality and not a change to any existing behavior in GoogleEmail2, I don't think we should worry about it.

In general, I think as long as a user could easily migrate from one to the other (where I define "easily" as without migrating stored data), differences in the API are OK.

@ssaavedra

This comment has been minimized.

Copy link
Contributor Author

ssaavedra commented Jun 9, 2015

I think we could leave both choices.
By exporting the toCreds with a meaningful name and providing an alternative "frontend" function receiving a (GoogleUser -> Creds m) we can let the user pick their winner.

I think you understood me wrong in some point. The existing GoogleEmail2 API has more metadata available, so an existing application can query many more things about a user's profile than mine can (currently).

However, these characteristics for querying Google+ data should be restricted to apps with the plus.me scope, which my implementation does not need to request (as that exceeds OAuth2).

Maybe the Google+ information about a profile is not so related to authorization & authentication as it is about user's profiling, but I would love your input about that.

I will rework my PR so that the credsIdent function can be selected (defaulting to google UID so that the abbreviated functions are consistent with those of other providers in this project).

And please, if I can do anything to improve the styling, docs or anything let me know; I'm new to contributing Haskell code :)

@pbrisbin

This comment has been minimized.

Copy link
Collaborator

pbrisbin commented Jun 9, 2015

I think you understood me wrong in some point. The existing GoogleEmail2 API has more metadata available, so an existing application can query many more things about a user's profile than mine can (currently).

Ah, sorry about that.

However, these characteristics for querying Google+ data should be restricted to apps with the plus.me scope, which my implementation does not need to request (as that exceeds OAuth2).

Maybe the Google+ information about a profile is not so related to authorization & authentication as it is about user's profiling, but I would love your input about that.

That's the approach we've been taking. As long as there's something in credsExtra that the end-user can use to get profile data (e.g. an access token) and there's a mechanism for them to pass in the scopes they'll eventually need, the plugin itself should limit its responsibilities to authorization only.

However, if we're looking to replace GoogleEmail2 we might have to re-think that a bit.

I will rework my PR so that the credsIdent function can be selected (defaulting to google UID so that the abbreviated functions are consistent with those of other providers in this project).

Awesome, thanks!

And please, if I can do anything to improve the styling, docs or anything let me know; I'm new to contributing Haskell code :)

I haven't looked very closely yet, but definitely plan to once this discussion settles on a path forward and the churn decreases.

Thanks again for the contribution!

@ssaavedra

This comment has been minimized.

Copy link
Contributor Author

ssaavedra commented Jun 23, 2015

I am not sure if you are awaiting any further input from me. I made those improvements 14 days back (just before your answer :) and I haven't heard back.

I am not sure on what to say about the Authentication/Profiling issue. Maybe this is getting discussed somewhere else (mailing list?)?

, googleUid
, emailUid
, module Yesod.Auth.OAuth2
) where

This comment has been minimized.

@pbrisbin

pbrisbin Jun 23, 2015

Collaborator

Can you dedent this so it's 4 spaces, not 8?

, googleUserGivenName :: Text
, googleUserFamilyName :: Text
, googleUserHostedDomain :: Maybe Text
}

This comment has been minimized.

@pbrisbin

pbrisbin Jun 23, 2015

Collaborator

Can you dedent this to 4 spaces?

<*> o .: "picture"
<*> o .: "given_name"
<*> o .: "family_name"
<*> o .:? "hd"

This comment has been minimized.

@pbrisbin

pbrisbin Jun 23, 2015

Collaborator

Can you dedent this to 4 spaces past parseJSON?

emailUid = uidBuilder googleUserEmail

uidBuilder :: (GoogleUser -> Text) -> GoogleUser -> AccessToken -> Creds m
uidBuilder f user token = Creds { credsPlugin = "google"

This comment has been minimized.

@pbrisbin

pbrisbin Jun 23, 2015

Collaborator

Can you line-break before { and indent it and the following ,'s 4 spaces?

, ("access_token", decodeUtf8 $ accessToken token)
] ++ maybeHostedDomain
}
where maybeHostedDomain = maybeToList $ ((,) "hosted_domain") `fmap` googleUserHostedDomain user

This comment has been minimized.

@pbrisbin

pbrisbin Jun 23, 2015

Collaborator

Can you line-break before maybeHostedDomain and indent it 2 spaces beyone the where?

Can you use (<$>) in place of infix fmap?

@pbrisbin

This comment has been minimized.

Copy link
Collaborator

pbrisbin commented Jun 23, 2015

Added some style notes (sorry I didn't realize this was ready for review).

Other than that this LGTM and I plan on merging soon.

@ssaavedra ssaavedra force-pushed the ssaavedra:master branch from 0176d9c to 2f2c14c Jun 23, 2015

@ssaavedra ssaavedra force-pushed the ssaavedra:master branch from 2f2c14c to 8fa938d Jun 23, 2015

@ssaavedra

This comment has been minimized.

Copy link
Contributor Author

ssaavedra commented Jun 23, 2015

All right. Hlint does no longer show any suggestion on the file. And sorry about the indentation, I used emacs with default haskell-indentation-- lengths, and it didn't even respect the lines from the copy & paste. I hope it is ok now, but let me know! :)

@pbrisbin

This comment has been minimized.

Copy link
Collaborator

pbrisbin commented Jun 23, 2015

This LGTM! I'll merge/release today or tomorrow for sure. We can pursue the deprecation of GoogleEmail2 at a later time (and I'll add a comment to make the situation clear in the meantime).

@pbrisbin

This comment has been minimized.

Copy link
Collaborator

pbrisbin commented Jun 25, 2015

Ping @jferris it looks like I lost the ability to merge.

@jferris

This comment has been minimized.

Copy link
Member

jferris commented Jun 25, 2015

@pbrisbin fixed.

pbrisbin added a commit that referenced this pull request Jun 25, 2015

Merge pull request #32 from ssaavedra/master
Add Google OAuth provider

@pbrisbin pbrisbin merged commit b2e698d into thoughtbot:master Jun 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@scan

This comment has been minimized.

Copy link
Collaborator

scan commented Jun 25, 2015

Funnily enough I started this project back then with Google OAuth2 because GoogleEmail2 didn't exist and I needed OAuth2 for it. Somewhere along the way it's been patched out and now it's getting patched in.

I like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.