Add support for signing in with a Google Account. #2040
Conversation
There was a problem hiding this comment.
High level feedback: I think the implementation here is good, and I'm generally supportive of the change. As you called out, it does let anybody create a JR account, and I think that that runs into a couple other specific open issues.
In particular, I think that both #631 and #698 are potentially cases where we surface sensitive (I mean, sure, "sensitive") information to users that shouldn't be able to see it, which has mostly been OK historically. I also think that having a one-click account creation button is potentially going to exacerbate #296.
I would love if we could make progress on those issues, but I don't want to put a bunch of roadblocks in front of you. As a compromise, could we hide the Google account login behind a feature flag? That way it can be off by default to avoid those interactions, but you can turn it on for your deployment. And if #296, #631, and #698 ever get fixed, I'd be very happy removing the feature flag and making this behavior the default.
| @@ -246,6 +256,51 @@ const AccountForm = (props: AccountFormProps) => { | |||
| return <div>loading...</div>; | |||
| } | |||
|
|
|||
| const SignInWithGoogleBlock = () => { | |||
There was a problem hiding this comment.
Is this closing over anything inside of AccountForm or can it just live at top-level?
There was a problem hiding this comment.
It uses a couple of things from the form: the submit state (to disable widgets while the popup is open) and the error message (to show an error if sign-in fails).
There was a problem hiding this comment.
Ah sure. I think I'd still prefer to see this live at top-level - defining a component inside of another component strikes me as kind of weird. (I know that it'll have slightly weird behavior on render, since each render will re-create the function and require swapping the component out)
It makes sense that we're not ready to let arbitrary accounts be created. But I think there may still be at least a little bit of utility in having the Google sign-in button for existing users who have associated their Google Accounts so you don't have to worry about the username/password entry. So I kept the login button, but now we throw an error if the Google Account doesn't match that of an existing user. Does this seem worth keeping for you, or would you rather I just go ahead and put it behind a flag instead? (FWIW, my current plan for the follow-up is to allow configuration of an optional Drive folder that will be used for ACL checks, and to only create new users in JR if they have access to that folder. Then I'll need to decide whether that should suffice for access to all hunts, or if we want to try to keep per-hunt ACLs working in this world). |
imports/server/accounts.ts
Outdated
| // Attempt to match existing Google users by their account ID. | ||
| // We can't use the async method since Meteor's API only takes a sync one. | ||
| // eslint-disable-next-line jolly-roger/no-disallowed-sync-methods | ||
| return MeteorUsers.findOne({ googleAccountId: info.serviceData.id }); |
There was a problem hiding this comment.
Oh we probably need to make sure there's only one user with that Google ID, because we don't enforce uniqueness. So, like, find with limit=2 and make sure you get exactly 1 back
There was a problem hiding this comment.
On digging further, this might be a bit more involved to get right.
Meteor is tracking the Google association in its own field, services.google.id (ref). While the current PR does a one-time link between these two fields, looking for JR users by existing googleAccountId, and (in the commented-out code) populating the googleAccountId from the service ID on new account creation, after this point, nothing is keeping these in sync. So you can still run into weirdness by e.g. making an association with a unique account at the time, and then associating more accounts with that Google account via the JR profile page.
Do you have any opinions on how we should handle this? I feel conceptually like having two different fields for this will lead to pain; maybe we should just migrate our existing fields to the standard Meteor ones, or will that cause other problems?
There was a problem hiding this comment.
Ugh yes that is annoying. I agree that if we're going to support logging in with Google account, then we should also migrate our existing fields onto the Meteor standard ones.
Here are the reasons I can think of that it's going to suck:
- We now need to care about uniqueness. Unfortunately we do actually need to explicitly handle this somehow, at least for the Death and Mayhem deployment. I just checked and we have about 25 Google accounts that are associated with more than one JR account. (Realistically, I'm pretty sure all of these should be merged into a single account, but that's a more complicated problem)
- We probably need to migrate incrementally (as people log in or re-link their accounts), rather than being able to do it all at once. The
googleAccountandgoogleAccountIdfields aren't enough to populate Meteor's service record. I guess it's possible that we can fudge it (and just not have an accessToken or any of the other fields), but I'm not sure if that will break things down the line. My personal preference would be to lazily migrate the data, either when someone logs in using an existing Google account or when they re-link their Google account, and then at some point in the future we can deprecate that and write a database migration to remove any users that still have the old fields set.
There was a problem hiding this comment.
After thinking about it more, I'm less convinced that it's worth using the standard mechanism at this time. Now that we're moving towards invitation links rather than other Google-based methods, we don't have a particular use case for it right now, and a custom login handler that uses our existing fields nets out to be simpler than the standard one, given that most of the complexity is already in the google-oauth module anyway, and that we don't have to deal with any migration questions at the moment.
So I've pivoted this PR to using a custom login handler instead, and we could punt migrating to the standard handler unless/until there's a more compelling reason for it.
I did find a bug along the way - unlinking a Google Account only cleared the linked email but not the ID. I've fixed that here, but if this PR doesn't get merged for any reason we should make sure not to lose track of that.
Let me know what you think!
|
Yeah I'm fine with "throw an error if the account doesn't exist already" Just for the record, I know that your Drive folder thing is coming, and I haven't decided how I feel about it yet 🙂. I guess I need to get on that. |
Show a "Sign in with Google" button on the login page. This sign in flow first attempts to match against existing users who have linked that same Google Account. If there is no such user, or multiple such users, the request is rejected. We use a custom login handler rather than the standard loginWithGoogle module for simplicity since we already have existing linked users that would be difficult to migrate to the standard system due to not storing the necessary credentials. Since we have enough to validate identity in our current schema, this is fine for now. Icon taken from: https://developers.google.com/identity/branding-guidelines Notes / issues: - This fixes a bug in the existing "unlink Google Account" flow where the linked email is cleared but not the linked account ID. - At least on local instances, a console warning appears upon showing the Google Account popup: "Cross-Origin-Opener-Policy policy would block the window.closed call". This also occurs for the existing Google Account integration so isn't exclusive to this flow. - Sometimes, when you click Sign Out, the login page renders with a blank "Sign in with Google" button, though the button appears to work as expected. See deathandmayhem#2021
|
Sorry again for not getting a chance to look at this sooner. This seems like a good compromise approach for now, and the implementation seems good to me. I did have one question around your specific requirements. If I'm reading this correctly, it's not possible to create a new JR account via Google sign-in, right? Only log into an existing account? And looking at #2056, it looks like the invite links currently require you to already have an account to apply an invite. Do you have a flow in mind for how your users create their accounts initially? |
|
Ah. I just read your comment in the description of #2056 as reserving that for future work. In that case, I'm happy with this |
Show a "Sign in with Google" button on the login page. This sign in flow first attempts to match against existing users who have linked that same Google Account. If there is no such user, a new account is created.
Icon taken from:
https://developers.google.com/identity/branding-guidelines
Notes / issues:
This allows anyone to create a Jolly Roger account as long as they have a Jolly Roger account; previously, access was invite only. This seems reasonable in that hunts have their own ACLs, but if this is a concern, we could reject new account creation until a follow-up change which optionally configures access based on Drive folder access.
A user can unlink their Google Account; if the user was originally created through this flow, they will be left with an account with no password or method of entry. However, they can follow the forgotten password flow to create a new password (if an email server is configured).
At least on local instances, a console warning appears upon showing the Google Account popup: "Cross-Origin-Opener-Policy policy would block the window.closed call". This also occurs for the existing Google Account integration so isn't exclusive to this flow.
If the popup is closed without completing sign-in, the button remains disabled because we don't appear to get a failure callback. I'm unsure why this is happening, though it's easy enough to refresh the page.
Sometimes, when you click Sign Out, the login page renders with a blank "Sign in with Google" button, though the button appears to work as expected.
See #2021