-
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 15 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 |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import { | ||
| Injectable, | ||
| Inject, | ||
| InternalServerErrorException, | ||
| } from "@nestjs/common"; | ||
| import { 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) { | ||
| // 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 | ||
| return 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, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { 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> { | ||
| const activate = (await super.canActivate(context)) as boolean; | ||
| 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; | ||
|
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 wonder if we should match/check all the (github verified) emails with existing users in the database, instead of just the first email.
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. Does discord need this functionality too?
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. probably, but I don't think discord lets you have multiple emails |
||
|
|
||
| if (!email) { | ||
| throw new InternalServerErrorException( | ||
| "[github-auth.service]: Cannot get email from GitHub.", | ||
| ); | ||
| } | ||
|
|
||
| return this.githubAuthService.validateUser({ | ||
| githubId: id, | ||
| username: username ? username : "", | ||
| avatar, | ||
| email, | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,12 @@ | ||
| import { DiscordUser } from "../types/auth.types"; | ||
| import { GithubUser } from "../types/auth.types"; | ||
|
|
||
| interface AuthUserResult { | ||
| id: string; | ||
| email: string | undefined; | ||
| } | ||
|
|
||
| export interface IAuthProvider { | ||
| // TODO: Maybe change it to OAuthUser: DiscordUser | GithubUser etc | ||
| // Or change it to a more general type name | ||
| validateUser(user: DiscordUser): void; | ||
| createUser(user: DiscordUser): void; | ||
| validateUser(user: DiscordUser | GithubUser): Promise<AuthUserResult>; | ||
| createUser(user: DiscordUser | GithubUser): Promise<AuthUserResult>; | ||
| } |
|
curtwl marked this conversation as 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.
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