feat: Add google auth #53
Conversation
gabrielmachin
requested review from
mlvieras,
lauta2912,
MBerguer and
chaba11
as code owners
There was a problem hiding this comment.
Can you please add instructions on the Readme for how to setup firebase? What does a person that wants to setup the project need to do to get Firebase running? Where are credentials stored? What does the frontend need to do to bridge the gap? (can be a link to a tutorial or documentation, I just want it referenced somewhere)
| @@ -0,0 +1,10 @@ | |||
| import Firebase from 'firebase-admin'; | |||
| import serviceAccount from './ServiceAccount.json'; | |||
There was a problem hiding this comment.
I'm wondering if we want this. This will always crash on startup if the developer does not have this file. What if they don't want to support Google auth? Maybe we should conditionally import this file.
There was a problem hiding this comment.
Yes, I'm trying to make that work with dynamic import.
| @@ -67,6 +75,82 @@ export class AuthService { | |||
| }; | |||
| }; | |||
|
|
|||
| static firebaseAuth = async (idToken: SocialAuth): Promise<ReturnAuth> => { | |||
| const decodeValue = await getAuth().verifyIdToken(idToken.idToken); | |||
There was a problem hiding this comment.
Should we try-catch this?
There was a problem hiding this comment.
Wouldn't it be useful to just throw the error given by firebase in case that something goes wrong? Or do we want to log it and throw a more generic error.
There was a problem hiding this comment.
Good point. I'm not sure, depends on what errors might be thrown. Some we might not want to throw. Can you please check what that method throws? We might not need to do anything, I just want to be sure
There was a problem hiding this comment.
Sure, will look into it.
| if (!email) throw new ApiError(errors.INVALID_CREDENTIALS); | ||
|
|
||
| const cryptPassword = await bcrypt.hash( | ||
| faker.internet.password({ length: 30 }), |
There was a problem hiding this comment.
I don't know if we should use faker in production. I'd prefer randomly generating a string of characters.
| let userAlreadyExist = false; | ||
|
|
||
| try { | ||
| user = await prisma.user.create({ data }); |
There was a problem hiding this comment.
Shouldn't we call the UserService here?
There was a problem hiding this comment.
If you mean calling the create method of the UserService, I didn't use it since there are some differences between the behavior of the sign up with email/password and the one with firebase, and didn't want to make it more complex. But maybe it's not as complex as I imagine.
If you mean just creating a new method in the UserService for creating the user with firebase auth then I think it's a good idea to move it to the UserService.
There was a problem hiding this comment.
It was not that complex, already fix this.
| name, | ||
| email, | ||
| password: cryptPassword, | ||
| signUpMethod: SignUpMethod.GOOGLE, |
There was a problem hiding this comment.
Shouldn't we also do this with the PASSWORD method on the register method above?
There was a problem hiding this comment.
That's done in the UserService.create() method.
| } | ||
|
|
||
| if (!userAlreadyExist) { | ||
| addToMailQueue('Sign up Email', { |
There was a problem hiding this comment.
I think we should have a dictionary of valid email identifiers and use it here. This can be prone to bugs since a small typo could prevent sending an email. What do you think? We could create a ticket for this.
NOTION Ticket
Type of change
Description of the change
Add google auth using
firebase-adminlibrary.