-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adds Google as an Authentication method #669
Conversation
waspc/data/Generator/templates/react-app/src/auth/buttons/Google.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, excited about this :)!
One note, that I dealt with on a previous project so I immediately remembered it: it is dangerous to allow both email/password and any other auth method for a user if email verification is not used, since it can be used to hijack an account. So for example, if [email protected] logged in with google account, and then somebody else decided to sign up with the [email protected] via email and password and as a result got connected to that same user account, but we didn't verify they own the email, we just gave them access to the user account that is not theirs. You don't have that issue though, first of all because they can't do it at all right now since there is no password resetting :D, but once we have it, we are also ok because password resetting forces them to prove it is their email. So we are ok in that scenario.
What about the other direction hm -> let's say an attacker A created an account with email that is not theirs, and then somebody B who owns that email logs in via Google later. Now the problem is that both A and B have access to the account, while B might not be aware of it! We might be affected by this right now I think?
The solution is to introduce email verification, then we are safe.
What until then? Maybe we can just reset password in any case -> so if account already exists, we also reset password. This disables password login until they reset the password. Since we don't even have reset currently, it means it disables auth via password completely, but I think that is ok in this situation where we are still missing features.
Generally, this is looking awesome :)!!! Reviewed it all, looks great, I did leave a bunch of comments but nothing major, so we can go through those but generally all good from my side!
I just realized now we don't have password reset or email verification steps -> what about that, when should we implement that? Hmmmm. Tricky thing is that smth to send emails is needed, some kind of emailer. I am not sure if any server can just send them? Ok, this is beyond scope of this PR.
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for all the great questions, comments, and suggestions @Martinsos! I appreciate it. I replied to them all, and will start working on them shortly.
Per your two other notes:
- Good point on still allowing the first user who signed up for the email to log in. I think we could either:
- change the password on every Google login, as suggested, or
- we could add a field to the User entity that tracks what login method can be used. it would start as
usernameAndPassword
but the minute they used any other auth method, it would change and they could only use that moving forward. however, that does introduce a bit of complexity and requires us to either change the model for them or ask them to update it themselves. additionally, most of this goes away once we do email validation and password reset in the future.
So for now, I think the best way is just to update the password on login by Google.
-
I just realized now we don't have password reset or email verification steps -> what about that, when should we implement that? Hmmmm. Tricky thing is that smth to send emails is needed, some kind of emailer. I am not sure if any server can just send them? Ok, this is beyond scope of this PR.
Since this is already getting pretty big and complex, I agree we should punt on that. I think this will give users some real value/buzz, and we can later add those incrementally over time.
Thanks again!
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Show resolved
Hide resolved
soudns good let's just reset the password on google login! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded to everything!
8d24790
to
562ef0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @Martinsos! I believe I addressed it all, but will do another quick scan through tomorrow just to make sure. I will also start doing some permutation testing to make sure I didn't break anything and it works without emailAndPassword
, etc. Appreciate it!
waspc/data/Generator/templates/server/src/routes/auth/passport/google/google.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/server/src/core/auth/prismaMiddleware.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/server/src/routes/auth/passport/google/google.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!! I left a couple of small comments, but otherwise all good from my side! Auth is coming :D!!!
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/server/src/routes/auth/passport/google/google.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,26 @@ | |||
import { upsertUserWithRandomPassword } from '../../../../core/auth.js' | |||
|
|||
// Default implementation if there is no `auth.methods.google.configFn`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha reference to Wasp-lang! We avoided that in the rest of the code so far. But we said we will be less careful about that. So ok, if you think it is useful, let's leave it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha Yeah, I was kinda hesitant to do so, but this file is pretty unique in that I actually link to it from the documentation to show users what the default implementation is, so I think in that context it may be ok. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe I should have put it here then? That's also a default implementation used only if the users don't define their own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, but maybe only if you link to it from the docs? The comment itself may not be a great idiom, I'm not sold on it entirely, but figured it would help users who want to see the default but are unsure where they can override in the wasp file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep lower, it doesn't hurt and it communicates intent / documents a bit.
// This token was used to get the Google profile information supplied as a parameter. | ||
// We add the Google profile to the request for downstream use. | ||
async function addGoogleProfileToRequest(req, _accessToken, _refreshToken, googleProfile, done) { | ||
req.wasp = { ...req.wasp, googleProfile } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to really simplify this function to just give us back the google profile (by adding it to the request). This makes it easier to follow the downstream usage and flow I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it take these extra ignored params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just the shape of the callback for Google's Passport implementation. The library invokes this.
waspc/data/Generator/templates/server/src/routes/auth/passport/google/google.js
Show resolved
Hide resolved
waspc/data/Generator/templates/server/src/routes/auth/passport/google/google.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/server/src/routes/auth/passport/google/googleDefaults.js
Outdated
Show resolved
Hide resolved
waspc/examples/todoApp/todoApp.wasp
Outdated
@@ -8,7 +8,14 @@ app todoApp { | |||
], | |||
auth: { | |||
userEntity: User, | |||
methods: [ UsernameAndPassword ], | |||
// externalAuthAssociationEntity: SocialLogin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: I am disabling Google auth by default in the todo app, just so server won't crash if you don't have the Google project envars.
Ok @Martinsos and @sodic, I think this is ready for a final review. The main update here is we no longer intertwine Google authn with any Please let me know if you have any questions about this update, and thanks for sticking with me through basically three reviews :) (cookies, JWT with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work once again!
I left a lot of comments, but none that would require mandatory changes. Take a look, see which make sense and merge when ready!
waspc/data/Generator/templates/server/src/routes/auth/passport/google/google.js
Outdated
Show resolved
Hide resolved
export async function getUserFields(_context, args) { | ||
const username = await generateAvailableUsername(args.profile.displayName.split(' '), { separator: '.' }) | ||
return { username } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea allowing them to define their own username-derivation function!
One question, if we support Google and know how its args
look like, maybe we could provide this as a default implementation instead of Heroku animals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good question! I know some sites use available info (like name), and others just do dummy stuff like Heroku-style... I personally, as a user, don't really like it when my actual name/email is by default my username, from a privacy perspective. But I may be in the minority here. The other benefit to a standard style across different providers is if they enable two kinds of external auth in the future, then they will have a uniform username format out of the box. But it is a good thing to think about.
@Martinsos any preferences on animal vs real name for the Google default username?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good for me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that, whatever we prefer? :D or a specific option lol you were supposed to tie break haha jk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah sorry, but I really think any choice is good here so I am sure you can figure it out :D. Or ok to tie break: if it is not clear that change is better, best to leave it as it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shayneczyzewski awesome, a lot of very nice work, and it is a big step forward!
Looks good to me! I will let @sodic finalize the review, but you have a green light from my side.
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
username: { in: potentialUsernames }, | ||
} | ||
}) | ||
const takenUsernames = users.map(user => user.username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can do this filtering at Prisma level, with select
or smth like that, so we maybe save some data transfer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean just select
ing the username col, instead of returning the whole object? Yeah, we could. At max we will have just 10 results though, so I am not sure it would be a huge gain.
return $ | ||
createTemplateFileDraft | ||
([relfile|Dockerfile|] :: Path' (Rel ProjectRootDir) File') | ||
([relfile|Dockerfile|] :: Path' (Rel TemplatesDir) File') | ||
( Just $ | ||
object | ||
[ "usingPrisma" .= not (null $ AS.getDecls @AS.Entity.Entity spec) | ||
[ "usingPrisma" .= not (null $ AS.getDecls @AS.Entity.Entity spec), | ||
"nodeMajorVersion" .= show (SV.major latestMajorNodeVersion), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did this become part of this PR, because you learned about this on the way? NO problem with it, just checking that you did not commit this by accident!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was discovered during the development of this feature and I just decided to fix it during since I was already in this file.
@@ -686,9 +688,13 @@ app MyApp { | |||
#### `userEntity: entity` (required) | |||
Entity which represents the user (sometimes also referred to as *Principal*). | |||
|
|||
#### `methods: [AuthMethod]` (required) | |||
#### `externalAuthAssociationEntity: entity` (optional) | |||
Entity which associates a user with some external authentication provider. We currently offer support for [Google](#google). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, so they pick an entity they want to use? What would it typically be, can it be User again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it follows the same convention we use for User. It cannot be User itself, as it needs a many-to-one relationship to User. I don't think we would want to do it under the covers since if we add/remove an entity as they enable/disable it, they will potentially lose the table data when they disabled it (possibly by accident). I think keeping it explicit is the way to go, and they can decorate it with additional fields/info.
Thanks so much for the review and comments @Martinsos and @sodic They were helpful. We are super close. I got most of the tiny ones included already (and resolved some as I went). The last major thing is to rename |
@shayneczyzewski great, all good for me, feel free to resolve all of my comments any way you see fit. |
waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.js
Outdated
Show resolved
Hide resolved
import { configFn, getUserFieldsFn } from './googleConfig.js' | ||
|
||
// Validates the provided config function returns all required data. | ||
const config = ((config) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this function throws an exception. Who can catch it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. In this case, it will die. But we really don't have a way to recover here, since we need that config. So we could either:
- leave as is, let it blow up, and hope the stack trace points them to the problem in their code, or
- catch it, make an error message, and they would still need to find their problem and fix it
I slightly lead toward the first (current) only because it should help them find the issue more easily. But this is a good thing to think about in general - if we have user-defined code errors, and we cannot recover, do we just let it blow up or catch and provide a "better" error message in some way?
Description
This change adds Google as a new AuthN method. It does so by creating a new Entity,
externalAuthAssociationEntity
that associates an external user with an internal user.Demo
Here is an example if you want to play around with it: https://wasp-csrf-demo.netlify.app/ (Note: I'm using the old Cookies project but it is using this branch and JWTs). I added everyone's email to the Google project, which you have to do if unpublished, but let me know if you get some error about access when signing in with Google.
Syntax
The syntax for
auth
is updated, to look like this when fully specified:OAuth Flow
The OAuth flow used here is as follows:
GET
request to the API server happens: https://wasp-csrf-demo.herokuapp.com/auth/external/google/login5a) The React page on the frontend takes this URL, and issues an AJAX
GET
request to the API server with the same query string: https://wasp-csrf-demo.herokuapp.com/auth/external/google/validateCode?code=4%2F0AdQt8qjMwXABj01-i3MOwy9o8OkxWg4z6dxwcxaNPf6WfgEsmb1v4XNmU_EskVypkI7jHQ&scope=email+profile+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.profile+openid+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.email&authuser=0&prompt=none5b) The API server takes the code, and Passport talks directly to Google in a server-to-server communication. After that succeeds, it returns the JWT for the authenticated user back to the frontend.
Notes
A couple of things to note:
node-oauth
) of our Passport Google library, but it wasn't being updated. Therefore, I had to introduce: https://www.npmjs.com/package/patch-packagegoogle: {}
to theauth.methods
, aexternalAuthEntity
, one line touserEntity
and add 2 environment variables 🪄usernameAndPassword
.googleConfig.js
sogoogle.js
andgoogleDefaults.js
could be pure JS.Closes #667
Closes #575
Type of change
auth.methods
syntax, now add a new required env var when deploying (WASP_WEB_CLIENT_URL
for CORS protection, as well as knowing where to redirect from on the server), and updated the Docker base image.Please select the option(s) that is more relevant.