Feature/oauth GitHub #222

curtwl · 2024-11-12T11:39:01Z

Description

To test this PR

obtain github oauth client id and client secret (on clickup, you can also make your own for dev), and put them in .env
npx prisma db push for both dev and test db
go to http://localhost:8000/api/v1/auth/github/login a github authorization table will pop up, and it should return the user's basic github profile

Issue link

Fixes # (issue)

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature updates / changes
Tests
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Manually and automated tests

Checklist:

[ x] My code follows the style guidelines of this project
[ x] I have performed a self-review of my code
[ x] I have commented my code, particularly in hard-to-understand areas
[ x] I have made corresponding changes to the documentation
[ x] My changes generate no new warnings
[ x] I have added tests that prove my fix is effective or that my feature works
[ x] New and existing tests pass locally with my changes
[ x] Any dependent changes have been merged and published in downstream modules
[ x] I have updated the change log

github-actions · 2024-11-13T17:01:24Z

@cherylli, @Ajen07
One business day has passed since the review started. Give priority to reviews as much as possible.

cherylli

I suggest adding a more desciptive error message for this error: upsert error because there's no github in the provider database

We need to update prisma/seed/data/oauth-providers.ts to include github. We also need to delete the file prisma/seed/oauth.ts as it's not used (mistake from a previous PR)

everything else work fine

cherylli · 2024-11-17T05:03:13Z

invalid code is not handled - I'm quite sure the discord one also has the same issue

If you can fix both it would be great, if not I can fix the discord one in another PR

curtwl · 2024-11-19T01:28:40Z

invalid code is not handled - I'm quite sure the discord one also has the same issue

If you can fix both it would be great, if not I can fix the discord one in another PR

Does {"message":"Failed to obtain access token. Possibly because of invalid redirect code.","error":"Bad Request","statusCode":400} work for the error message? I guess other situations could cause the same message since it's just a generic error for the super.canActivate(context) in the AuthGuards.

cherylli · 2024-11-27T10:16:14Z

src/auth/strategies/github.strategy.ts

+        const { username, id, photos, emails } = profile;
+
+        const avatar = photos && photos.length > 0 ? photos[0].value : null;
+        const email = emails && emails.length > 0 ? emails[0].value : null;


I wonder if we should match/check all the (github verified) emails with existing users in the database, instead of just the first email.
If none of them is found in the database we can create a new account with the primary email

Does discord need this functionality too?

probably, but I don't think discord lets you have multiple emails

cherylli · 2025-01-16T02:26:08Z

@curtwl do you feel like squashing the commits? This might be a good practice if you want to try it out. Otherwise we can probably just leave it, next release notes will probably be manually updated combining our changelog and release please change log

cherylli · 2025-01-16T10:53:44Z

One more question - do you know if oauth only returns verified emails? or is there a way to check if the emails are verified?

Otherwise if users add emails which do not belong to them we don't want to automatically link to an existing account

timDeHof · 2025-01-17T03:59:07Z

src/auth/strategies/github.strategy.ts

+        refreshToken: string,
+        profile: GithubProfile,
+    ): Promise<any> {
+        const { username, id, photos, emails } = profile;


I'm getting an error saying that "'username, id, photos' properties does not exist on type GitHubProfile". Also I found a types package for passport-github2 at @types/passport-github2 on yarn.

I think the types package got lost resolving a merge conflict, oops! Definitely had that installed while developing.

ok, I'll look at again once you install the types package.

curtwl · 2025-01-17T19:02:52Z

@curtwl do you feel like squashing the commits? This might be a good practice if you want to try it out. Otherwise we can probably just leave it, next release notes will probably be manually updated combining our changelog and release please change log

Sure, I've been reading through the docs and it doesn't seem like it'll take long after I understand the workflow.

davideastmond · 2025-02-19T21:50:15Z

src/auth/discord-auth.service.ts

        });
    }
+
+    async findUserByEmails(emails): Promise<any | null> {


Typescript is complaining.

Suggested change

async findUserByEmails(emails): Promise<any | null> {

async findUserByEmails(emails: string[]): Promise<any | null> {

davideastmond · 2025-02-19T21:53:25Z

src/auth/discord-auth.service.ts

        });
    }
+
+    async findUserByEmails(emails): Promise<any | null> {


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 any as a return type here, we shouldn't need the union with null, since any should cover the null type.

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

davideastmond · 2025-02-19T21:57:08Z

src/auth/discord-auth.service.ts

                "[discord-auth.service]: Cannot get email from discord to create a new Chingu account",
            );

+        const existingUser = await this.findUserByEmails([user.email]);


~~Is it possible that this could return null?~~
To be safe we could make sure that this value is not null.

Yes it can https://www.prisma.io/docs/orm/reference/prisma-client-reference#findunique

davideastmond · 2025-02-19T22:02:07Z

src/auth/github-auth.service.ts

+        private prisma: PrismaService,
+        @Inject("OAuth-Config") private oAuthConfig: OAuthConfig,
+    ) {}
+    async validateUser(user: GithubUser) {


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

async validateUser(user: GithubUser) {

async validateUser(user: GithubUser): Promise<AuthUserResult> {

Is AuthUserResult something built in or something we have to define?
I think this might be same as the prisma User model type

davideastmond · 2025-02-19T22:10:28Z

src/auth/github-auth.service.ts

+                "[github-auth.service]: Cannot get email from github to create a new Chingu account",
+            );
+
+        const existingUser = await this.findUserByEmails([


I think we should check for null to be safe

yes according to the Prisma API https://www.prisma.io/docs/orm/reference/prisma-client-reference#findfirst

davideastmond · 2025-02-19T22:46:30Z

src/auth/guards/discord-auth.guard.ts

    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;
+        }


For readability, could we move line 27 and 28 into the try block?

Suggested change

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;

}

try {

const activate = await super.canActivate(context) as boolean

const request = context.switchToHttp().getRequest();

await super.logIn(request);

return activate;

} catch (e) {

if (e.code == "invalid_grant") {

throw new BadRequestException(

`Invalid code in redirect query param.`,

);

}

throw e;

}

davideastmond · 2025-02-19T22:47:50Z

src/auth/guards/github-auth.guard.ts

+        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;
+    }


Similar comment here that I made in the file src/auth/guards/discord-auth.guard.ts

davideastmond · 2025-02-19T22:49:52Z

src/auth/strategies/github.strategy.ts

+        accessToken: string,
+        refreshToken: string,


It doesn't look like we're using these variables?

davideastmond · 2025-02-19T22:51:59Z

src/auth/strategies/github.strategy.ts

+        profile: GithubProfile,
+    ): Promise<any> {
+        const { username, id, photos, emails } = profile;
+        const avatar = photos && photos.length > 0 ? photos[0].value : null;


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

curtwl added 14 commits November 11, 2024 21:24

Add github env vars to readme

dd31b38

Add github oauth passport dependencies

db2d22c

Add GithubUser type

9135941

Add github-auth-service

f033dac

Add GithubUser to oauth interface

1cb4252

Add Github oauth strategy

7e4dfc3

Update auth module for github

0559506

Add github auth guard

e394036

Update auth controller for github

a9266f1

Update oauthConfig interface for github

2949359

Update oauthConfig module for github

044717d

Update oauthConfig schema for github

8b4461b

Add test for github oauth redirect

e24869b

Update changelog

b12e197

curtwl requested a review from cherylli November 12, 2024 11:39

Remove console.log

c91a3ef

cherylli requested a review from Ajen07 November 13, 2024 01:12

cherylli previously requested changes Nov 15, 2024

View reviewed changes

cherylli added the requested-changes label Nov 15, 2024

curtwl added 4 commits November 15, 2024 21:00

Add Github provider to seed

2bc151d

Add github provider to oauth interface

5d109bd

Make error handling more robust

13dc3d3

Remove console logs

ce21985

curtwl added review and removed requested-changes labels Nov 16, 2024

curtwl requested a review from cherylli November 16, 2024 23:37

Merge branch 'dev' into feature/oauth-github

b209387

cherylli reviewed Nov 27, 2024

View reviewed changes

cherylli added review and removed requested-changes labels Nov 27, 2024

cherylli removed the review label Dec 20, 2024

curtwl added 6 commits January 15, 2025 16:24

Search all user emails for oauth

4ff3e4c

Search all user emails for oauth

36d5945

Update some types and syntax for searching all emails

2977e16

Add GithubProfile and GithubEmail types

dafdd20

Update with new types

13a4dc8

Merge from dev

6f25ab0

curtwl dismissed andram11’s stale review via 6f25ab0 January 15, 2025 22:42

curtwl added the review label Jan 15, 2025

curtwl requested review from andram11 and cherylli January 15, 2025 22:42

timDeHof reviewed Jan 17, 2025

View reviewed changes

curtwl added requested-changes and removed review labels Jan 17, 2025

cherylli added the low priority label Feb 1, 2025

davideastmond reviewed Feb 19, 2025

View reviewed changes

	async findUserByEmails(emails): Promise<any \| null> {
	async findUserByEmails(emails: string[]): Promise<any \| null> {

	async validateUser(user: GithubUser) {
	async validateUser(user: GithubUser): Promise<AuthUserResult> {

Feature/oauth GitHub #222

Are you sure you want to change the base?

Feature/oauth GitHub #222

Uh oh!

Conversation

curtwl commented Nov 12, 2024

Description

To test this PR

Issue link

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

github-actions bot commented Nov 13, 2024

Uh oh!

cherylli left a comment

Choose a reason for hiding this comment

Uh oh!

cherylli commented Nov 17, 2024

Uh oh!

curtwl commented Nov 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cherylli commented Jan 16, 2025

Uh oh!

cherylli commented Jan 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curtwl commented Jan 17, 2025

Uh oh!

davideastmond Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davideastmond Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davideastmond Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davideastmond Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

davideastmond Feb 19, 2025 •

edited

Loading

davideastmond Feb 19, 2025 •

edited

Loading

davideastmond Feb 19, 2025 •

edited

Loading

davideastmond Feb 19, 2025 •

edited

Loading