-
Notifications
You must be signed in to change notification settings - Fork 7
Feature/oauth GitHub #222
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
base: dev
Are you sure you want to change the base?
Feature/oauth GitHub #222
Changes from all commits
dd31b38
db2d22c
9135941
f033dac
1cb4252
7e4dfc3
0559506
e394036
a9266f1
2949359
044717d
8b4461b
e24869b
b12e197
c91a3ef
2bc151d
5d109bd
13dc3d3
ce21985
b209387
268dd76
fbca3fc
889aa95
0b9dca2
d98cd9a
8a414ff
aafd1ef
c88cc69
ee0f8fa
f9f8b4b
4ff3e4c
36d5945
2977e16
dafdd20
13a4dc8
6f25ab0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,7 @@ export default [ | |
| { | ||
| name: "discord", | ||
| }, | ||
| { | ||
| name: "github", | ||
| }, | ||
| ]; | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,8 +7,8 @@ import { IAuthProvider } from "@/global/interfaces/oauth.interface"; | |||||
| import { PrismaService } from "@/prisma/prisma.service"; | ||||||
| import { DiscordUser } from "@/global/types/auth.types"; | ||||||
| import { generatePasswordHash } from "@/global/auth/utils"; | ||||||
|
|
||||||
| import { OAuthConfig } from "@/config/Oauth/oauthConfig.interface"; | ||||||
|
|
||||||
| @Injectable() | ||||||
| export class DiscordAuthService implements IAuthProvider { | ||||||
| constructor( | ||||||
|
|
@@ -40,10 +40,12 @@ export class DiscordAuthService implements IAuthProvider { | |||||
| "[discord-auth.service]: Cannot get email from discord to create a new Chingu account", | ||||||
| ); | ||||||
|
|
||||||
| const existingUser = await this.findUserByEmails([user.email]); | ||||||
|
|
||||||
| // check if email is in the database, add oauth profile to existing account, otherwise, create a new user account | ||||||
| return this.prisma.user.upsert({ | ||||||
| where: { | ||||||
| email: user.email, | ||||||
| id: existingUser.id, | ||||||
| }, | ||||||
| update: { | ||||||
| emailVerified: true, | ||||||
|
|
@@ -78,4 +80,19 @@ export class DiscordAuthService implements IAuthProvider { | |||||
| }, | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| async findUserByEmails(emails): Promise<any | null> { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typescript is complaining.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we can get a better return type for this method? Can we actually return a properly typed user? If we're going to use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, this is a prisma User Model type, I think there's one we could use, for reference: https://www.prisma.io/docs/orm/prisma-client/type-safety. The current implicit type is this |
||||||
| // collect emails as strings | ||||||
| const emailStrings = emails.map((email) => | ||||||
| typeof email === "string" ? email : email.value, | ||||||
| ); | ||||||
|
|
||||||
| return this.prisma.user.findFirst({ | ||||||
| where: { | ||||||
| email: { | ||||||
| in: emailStrings, | ||||||
| }, | ||||||
| }, | ||||||
| }); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,121 @@ | ||||||
| import { | ||||||
| Injectable, | ||||||
| Inject, | ||||||
| InternalServerErrorException, | ||||||
| NotFoundException, | ||||||
| } from "@nestjs/common"; | ||||||
| import { | ||||||
| AuthUserResult, | ||||||
| IAuthProvider, | ||||||
| } from "../global/interfaces/oauth.interface"; | ||||||
| import { PrismaService } from "../prisma/prisma.service"; | ||||||
| import { GithubUser } from "../global/types/auth.types"; | ||||||
| import { generatePasswordHash } from "../global/auth/utils"; | ||||||
| import { OAuthConfig } from "@/config/Oauth/oauthConfig.interface"; | ||||||
|
|
||||||
| @Injectable() | ||||||
| export class GithubAuthService implements IAuthProvider { | ||||||
| constructor( | ||||||
| private prisma: PrismaService, | ||||||
| @Inject("OAuth-Config") private oAuthConfig: OAuthConfig, | ||||||
| ) {} | ||||||
| async validateUser(user: GithubUser) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a tiny suggestion to make the intelliSense clearer, since Typescript was inferring unnecessarily a union type of objects with the same properties
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is AuthUserResult something built in or something we have to define? |
||||||
| const userInDb = await this.prisma.findUserByOAuthId( | ||||||
| "github", | ||||||
| user.githubId, | ||||||
| ); | ||||||
|
|
||||||
| if (userInDb) | ||||||
| return { | ||||||
| id: userInDb.userId, | ||||||
| email: user.email, | ||||||
| }; | ||||||
| return this.createUser(user); | ||||||
| } | ||||||
|
|
||||||
| async createUser(user: GithubUser): Promise<AuthUserResult> { | ||||||
| // generate a random password and not tell them so they can't login, but they will be able to reset password, | ||||||
| // and will be able to login with this in future, | ||||||
| // or maybe the app will prompt user the input the password, exact oauth flow is to be determined | ||||||
|
|
||||||
| // this should not happen when "user:email" is in the scope | ||||||
| if (!user.email) | ||||||
| throw new InternalServerErrorException( | ||||||
| "[github-auth.service]: Cannot get email from github to create a new Chingu account", | ||||||
| ); | ||||||
|
|
||||||
| const existingUser = await this.findUserByEmails([ | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should check for null to be safe
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes according to the Prisma API https://www.prisma.io/docs/orm/reference/prisma-client-reference#findfirst |
||||||
| user.email, | ||||||
| ...(user.verifiedEmails || []), | ||||||
| ]); | ||||||
|
|
||||||
| // check if email is in the database, add oauth profile to existing account, otherwise, create a new user account | ||||||
| let upsertResult; | ||||||
| try { | ||||||
| upsertResult = await this.prisma.user.upsert({ | ||||||
| where: { | ||||||
| id: existingUser.id, | ||||||
| }, | ||||||
| update: { | ||||||
| emailVerified: true, | ||||||
| oAuthProfiles: { | ||||||
| create: { | ||||||
| provider: { | ||||||
| connect: { | ||||||
| name: "github", | ||||||
| }, | ||||||
| }, | ||||||
| providerUserId: user.githubId, | ||||||
| providerUsername: user.username, | ||||||
| }, | ||||||
| }, | ||||||
| }, | ||||||
| create: { | ||||||
| email: user.email.value, | ||||||
| password: await generatePasswordHash(), | ||||||
| emailVerified: true, | ||||||
| avatar: user.avatar, | ||||||
| oAuthProfiles: { | ||||||
| create: { | ||||||
| provider: { | ||||||
| connect: { | ||||||
| name: "github", | ||||||
| }, | ||||||
| }, | ||||||
| providerUserId: user.githubId, | ||||||
| providerUsername: user.username, | ||||||
| }, | ||||||
| }, | ||||||
| }, | ||||||
| }); | ||||||
| } catch (e) { | ||||||
| if (e.code === "P2025") { | ||||||
curtwl marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| if (e.message.includes("OAuthProvider")) { | ||||||
| throw new NotFoundException( | ||||||
| "OAuth provider not found in the database", | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
| console.error("Unexpected error during upsert:", e); | ||||||
| throw e; | ||||||
| } | ||||||
| return upsertResult; | ||||||
| } | ||||||
|
|
||||||
| async findUserByEmails( | ||||||
| emails: (string | { value: string })[], | ||||||
| ): Promise<any | null> { | ||||||
| // collect emails as strings | ||||||
| const emailStrings = emails.map((email) => | ||||||
| typeof email === "string" ? email : email.value, | ||||||
| ); | ||||||
|
|
||||||
| return this.prisma.user.findFirst({ | ||||||
| where: { | ||||||
| email: { | ||||||
| in: emailStrings, | ||||||
| }, | ||||||
| }, | ||||||
| }); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,8 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { ExecutionContext, Injectable } from "@nestjs/common"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BadRequestException, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ExecutionContext, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Injectable, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from "@nestjs/common"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { AuthGuard } from "@nestjs/passport"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Injectable() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -9,9 +13,20 @@ export class DiscordAuthGuard extends AuthGuard("discord") { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async canActivate(context: ExecutionContext): Promise<boolean> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const activate = (await super.canActivate(context)) as boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let activate; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| activate = (await super.canActivate(context)) as boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (e.code == "invalid_grant") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new BadRequestException( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `Invalid code in redirect query param.`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw e; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
15
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For readability, could we move line 27 and 28 into the try block?
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const request = context.switchToHttp().getRequest(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await super.logIn(request); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return activate; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { | ||
| BadRequestException, | ||
| ExecutionContext, | ||
| Injectable, | ||
| } from "@nestjs/common"; | ||
| import { AuthGuard } from "@nestjs/passport"; | ||
|
|
||
| @Injectable() | ||
| export class GithubAuthGuard extends AuthGuard("github") { | ||
| constructor() { | ||
| super({ | ||
| session: false, | ||
| }); | ||
| } | ||
| async canActivate(context: ExecutionContext): Promise<boolean> { | ||
| let activate; | ||
| try { | ||
| activate = (await super.canActivate(context)) as boolean; | ||
| } catch (e) { | ||
| if (e.message.includes("Failed to obtain access token")) { | ||
| throw new BadRequestException( | ||
| `Failed to obtain access token. Possibly because of invalid redirect code.`, | ||
| ); | ||
| } | ||
| throw e; | ||
| } | ||
| const request = context.switchToHttp().getRequest(); | ||
| await super.logIn(request); | ||
|
|
||
| return activate; | ||
| } | ||
|
Comment on lines
+16
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment here that I made in the file |
||
| } | ||

Uh oh!
There was an error while loading. Please reload this page.
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 it possible that this could return null?To be safe we could make sure that this value is not null.
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.
Yes it can https://www.prisma.io/docs/orm/reference/prisma-client-reference#findunique