-
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 30 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 |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| 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) { | ||
| 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", | ||
| ); | ||
|
|
||
| // 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: { | ||
| email: user.email, | ||
| }, | ||
| update: { | ||
| emailVerified: true, | ||
| oAuthProfiles: { | ||
| create: { | ||
| provider: { | ||
| connect: { | ||
| name: "github", | ||
| }, | ||
| }, | ||
| providerUserId: user.githubId, | ||
| providerUsername: user.username, | ||
| }, | ||
| }, | ||
| }, | ||
| create: { | ||
| email: user.email, | ||
| 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; | ||
| } | ||
| } | ||
| 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 |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import { Inject, Injectable } from "@nestjs/common"; | ||
| import { PassportStrategy } from "@nestjs/passport"; | ||
| import { Strategy, Profile } from "passport-github2"; | ||
| import { IAuthProvider } from "../../global/interfaces/oauth.interface"; | ||
| import { OAuthConfig } from "../../config/Oauth/oauthConfig.interface"; | ||
| import { InternalServerErrorException } from "@nestjs/common"; | ||
|
|
||
| @Injectable() | ||
| export class GithubStrategy extends PassportStrategy(Strategy, "github") { | ||
| constructor( | ||
| @Inject("GITHUB_OAUTH") | ||
| private readonly githubAuthService: IAuthProvider, | ||
| @Inject("OAuth-Config") private oAuthConfig: OAuthConfig, | ||
| ) { | ||
| const { clientId, clientSecret, callbackUrl } = oAuthConfig.github; | ||
| super({ | ||
| clientID: clientId, | ||
| clientSecret: clientSecret, | ||
| callbackURL: callbackUrl, | ||
| scope: ["identify", "user:email"], | ||
| }); | ||
| } | ||
|
|
||
| async validate( | ||
| accessToken: string, | ||
| refreshToken: string, | ||
|
Comment on lines
+26
to
+27
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. It doesn't look like we're using these variables? |
||
| profile: Profile, | ||
| ): Promise<any> { | ||
| const { username, id, photos, emails } = profile; | ||
|
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. I'm getting an error saying that "'username, id, photos' properties does not exist on type
Contributor
Author
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 the types package got lost resolving a merge conflict, oops! Definitely had that installed while developing.
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. ok, I'll look at again once you install the types package. |
||
|
|
||
| const avatar = photos && photos.length > 0 ? photos[0].value : 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. Might be related to the comment above about the types packages - for readability I think we should explicitly declare what the type of avatar is |
||
| const email = emails && emails.length > 0 ? emails[0].value : null; | ||
|
||
|
|
||
| if (!email) { | ||
| throw new InternalServerErrorException( | ||
| "[github-auth.service]: Cannot get email from GitHub.", | ||
| ); | ||
| } | ||
|
|
||
| return this.githubAuthService.validateUser({ | ||
| githubId: id, | ||
| username: username ? username : "", | ||
| avatar, | ||
| email: email ? email : "", | ||
| }); | ||
| } | ||
| } | ||
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.
Just a tiny suggestion to make the intelliSense clearer, since Typescript was inferring unnecessarily a union type of objects with the same properties
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 AuthUserResult something built in or something we have to define?
I think this might be same as the prisma User model type