[NAN-578] Implement Google Social Login/Signup #1872

khaliqgant · 2024-03-18T18:55:15Z

Describe your changes

Adds in google auth using workos

Issue ticket number and link

NAN-578

Checklist before requesting a review (skip if just adding/editing APIs & templates)

I added tests, otherwise the reason is:
I added observability, otherwise the reason is:
I added analytics, otherwise the reason is:

linear · 2024-03-18T18:55:17Z

NAN-578 Implement Social Auth (Google)

packages/server/lib/server.ts

bodinsamuel

Tested in staging, works well 💪🏻

packages/server/lib/controllers/auth.controller.ts

packages/webapp/src/pages/InviteSignup.tsx

bodinsamuel · 2024-03-19T15:13:46Z

packages/webapp/src/components/ui/button/Auth/Google.tsx

@@ -0,0 +1,82 @@
+interface Props {


I was the impression that /ui folder was for Generic reusable component, this one is really a one of. (my pov is that everything is related to ui anyway so it's either an unnecessary hierarchy level or it's specific for one scenario)

packages/server/lib/controllers/auth.controller.ts

packages/webapp/src/pages/ResetPassword.tsx

khaliqgant · 2024-03-19T19:37:18Z

@bodinsamuel Thanks for the review. I updated the PR based on some of your feedback. The other items I didn't see as blocking but let me know if you feel strongly about any items I didn't address and I can add more color on why I didn't adjust it based on your feedback.

packages/server/lib/controllers/auth.controller.ts

TBonnin · 2024-03-19T20:38:18Z

packages/server/lib/controllers/auth.controller.ts

+}
+
+const createAccountIfNotInvited = async (state: string, name: string): Promise<number | null> => {
+    const parsedAccount = (state ? JSON.parse(Buffer.from(state, 'base64').toString('ascii')) : null) as InviteAccountState | null;


Cannot JSON.parse throw an exception? What should happen in this case?

TBonnin · 2024-03-19T20:47:16Z

packages/server/lib/controllers/auth.controller.ts

+        }
+
+        return accountId;
+    }


can this be?

const res: Result<InviteAccountState> = parseState(state) // returns a valid InviteAccountState or a ResultErr if (isOk(res)) { const validToken = await userService.getInvitedUserByToken(token); if (validToken) { await userService.markAcceptedInvite(parsedAccount.token); } return parsedAccount.accountId ; }

sure, updated

TBonnin · 2024-03-19T20:48:32Z

packages/server/lib/controllers/auth.controller.ts

+            if (!workos) {
+                errorManager.errRes(res, 'workos_not_configured');
+                return;
+            }


see my comment above. This is not a concern for the user and I think it would be better to crash the server at startup

WorkOS is only required for Cloud so don't think it should crash here.

what I mean is this is a configuration issue that would better detected as soon as possible (aka: when the app starts) rather than deferred to whenever a request is being received.
For sure for non-cloud setup this configuration can be ignored.

I prefer never to deployed a misconfigured app than getting a bug report by an end-user

Yup, I gotcha, but who are we protecting here? Developers running the application locally? Cloud will have the necessary env variables.

Yup, I gotcha, but who are we protecting here?

We protecting our future self

Cloud will have the necessary env variables.

They will, until one day the infra changes and configuration is somehow forgotten or messed up.
In general trying to detect error as soon as possible is a good principle to follow.

Not a blocker though.

packages/server/lib/controllers/auth.controller.ts

TBonnin · 2024-03-19T20:53:18Z

packages/server/lib/controllers/auth.controller.ts

+                        ? `${authorizedUser.firstName || ''} ${authorizedUser.lastName || ''}`
+                        : authorizedUser.email.split('@')[0];
+
+                const accountId = await createAccountIfNotInvited(state as string, name as string);


should state and name be validated prior to make sure they are acceptable string. That would also avoid having to cast them

TBonnin · 2024-03-19T20:54:28Z

packages/server/lib/controllers/auth.controller.ts

@@ -260,6 +292,79 @@ class AuthController {
            next(error);
        }
    }
+
+    getHostedLogin(req: Request, res: Response, next: NextFunction) {


not sure what Hosted referred to here?

Hosted = WorkOs

I find our use of Hosted rather confusing, everything is hosted by default.
Is it enterprise, cloud or community? if hosted = enterprise + cloud maybe we can come up with a naming more representative like "managed".
Plus we use hosted for self-hosted in the Docker images right now, which is the opposite

bodinsamuel

Minor comments 👌🏻

packages/webapp/src/components/ui/button/Auth/Google.tsx

bodinsamuel · 2024-03-20T10:42:22Z

packages/server/lib/controllers/auth.controller.ts

@@ -260,6 +292,79 @@ class AuthController {
            next(error);
        }
    }
+
+    getHostedLogin(req: Request, res: Response, next: NextFunction) {


I find our use of Hosted rather confusing, everything is hosted by default.
Is it enterprise, cloud or community? if hosted = enterprise + cloud maybe we can come up with a naming more representative like "managed".
Plus we use hosted for self-hosted in the Docker images right now, which is the opposite

packages/server/lib/controllers/auth.controller.ts

packages/server/lib/server.ts

khaliqgant · 2024-03-20T11:08:31Z

I add the impression that user could only be part of one org, so if they only have one, they get suspended and they login what are they seeing?

If a user is suspended they're essentially locked out of Nango. Handling this seems out of scope of this as it is a known limitation of the invite flow. Let me know if you disagree

TBonnin

I have only one small feedback regarding configuration management but other than that looks good.

bodinsamuel

Minor comments still opened but overall ✅

bodinsamuel · 2024-03-20T15:08:11Z

Tested one last time on staging, all good 👍🏻

khaliqgant added 3 commits March 18, 2024 13:26

[nan-578] workos implementtion

cacdccd

[nan-578] workos implementation

8642139

[nan-578] refactor

68fec0b

github-advanced-security bot found potential problems Mar 18, 2024

View reviewed changes

packages/server/lib/server.ts Fixed Show fixed Hide fixed

khaliqgant added 4 commits March 19, 2024 09:39

[nan-578] route to home page

9bd4f07

[nan-578] env varible for redirect

51538b2

[nan-578] remove env var

818204e

[nan-578] cross purpose hosted login routes

386b6d0

github-advanced-security bot found potential problems Mar 19, 2024

View reviewed changes

packages/server/lib/server.ts Dismissed Show dismissed Hide dismissed

[nan-578] fix base url

fbd3c44

khaliqgant changed the title ~~Khaliq/nan 578 implement social auth google~~ [NAN-578] Implement Google Social Login/Signup Mar 19, 2024

[nan-578] add social buttons

93ac937

khaliqgant marked this pull request as ready for review March 19, 2024 12:57

khaliqgant requested review from TBonnin and bodinsamuel March 19, 2024 12:59

bodinsamuel reviewed Mar 19, 2024

View reviewed changes

[nan-578] feedback updates

c010b0b

TBonnin reviewed Mar 19, 2024

View reviewed changes

khaliqgant added 5 commits March 20, 2024 08:47

[nan-578] only show for cloud

6d7cf1a

[nan-578] handle organization being returned

a460604

[nan-578] feedback and organization handling

0c5f6d5

bring in latest

95692ca

[nan-578] fix type separation and service usage

f155434

khaliqgant requested a review from bodinsamuel March 20, 2024 09:56

bodinsamuel reviewed Mar 20, 2024

View reviewed changes

[nan-578] naming update

9659dc4

khaliqgant requested a review from bodinsamuel March 20, 2024 11:09

TBonnin approved these changes Mar 20, 2024

View reviewed changes

bodinsamuel approved these changes Mar 20, 2024

View reviewed changes

[nan-578] throw error if workos not configured and is cloud

3dfd105

khaliqgant merged commit 59d64f2 into master Mar 20, 2024
18 checks passed

khaliqgant deleted the khaliq/nan-578-implement-social-auth-google branch March 20, 2024 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NAN-578] Implement Google Social Login/Signup #1872

[NAN-578] Implement Google Social Login/Signup #1872

khaliqgant commented Mar 18, 2024 •

edited

linear bot commented Mar 18, 2024

bodinsamuel left a comment

bodinsamuel Mar 19, 2024

khaliqgant commented Mar 19, 2024

TBonnin Mar 19, 2024

TBonnin Mar 19, 2024

khaliqgant Mar 20, 2024

TBonnin Mar 19, 2024

khaliqgant Mar 20, 2024

TBonnin Mar 20, 2024 •

edited

khaliqgant Mar 20, 2024

TBonnin Mar 20, 2024 •

edited

TBonnin Mar 19, 2024

TBonnin Mar 19, 2024

khaliqgant Mar 20, 2024

bodinsamuel Mar 20, 2024

bodinsamuel left a comment

bodinsamuel Mar 20, 2024

khaliqgant commented Mar 20, 2024

TBonnin left a comment

bodinsamuel left a comment

bodinsamuel commented Mar 20, 2024

[NAN-578] Implement Google Social Login/Signup #1872

[NAN-578] Implement Google Social Login/Signup #1872

Conversation

khaliqgant commented Mar 18, 2024 • edited

Describe your changes

Issue ticket number and link

Checklist before requesting a review (skip if just adding/editing APIs & templates)

linear bot commented Mar 18, 2024

bodinsamuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khaliqgant commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBonnin Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBonnin Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodinsamuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khaliqgant commented Mar 20, 2024

TBonnin left a comment

Choose a reason for hiding this comment

bodinsamuel left a comment

Choose a reason for hiding this comment

bodinsamuel commented Mar 20, 2024

khaliqgant commented Mar 18, 2024 •

edited

TBonnin Mar 20, 2024 •

edited

TBonnin Mar 20, 2024 •

edited