Skip to content

Commit

Permalink
merge: #2934
Browse files Browse the repository at this point in the history
2934: fix(auth-api): Ensure that signing up as an invited user doesn’t create new user r=stack72 a=stack72

When a non-SI user is invited to a workspace, we create a row in the Users table with the email of the user and no other details. Previously, when that user attempted to signup, we didn’t check for a partially created user and instead we allowed the user to create a new account. This meant that the invitation to the workspace was for a “dead” user - i.e. a user that didn’t actually exist.

In this PR, we check for an existing user being either auth0 based OR that has an email and no signup date (email + no signup date means that they are invited but were not previously a member). This means we can now find the correct user and thus update the existing user with the auth0 response rather than creating a new user

Co-authored-by: stack72 <[email protected]>
  • Loading branch information
si-bors-ng[bot] and stack72 authored Nov 6, 2023
2 parents 99e0f84 + 0cc2231 commit 07e72c4
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 4 deletions.
23 changes: 21 additions & 2 deletions bin/auth-api/src/services/users.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,23 @@ export async function createOrUpdateUserFromAuth0Details(auth0UserData: Auth0.Us
// auth0 docs showing user_id, but looks like "sub" contains the identifier
// TODO: check data when logging in with other providers
const auth0Id = auth0UserData.user_id || (auth0UserData as any).sub;

const existingUser = await getUserByAuth0Id(auth0Id);
const email = auth0UserData.email;

// We want to ensure we track that we need to create a workspace for a partially
// signed up (i.e. invited) user.
let requiresWorkspace = false;

let existingUser = await getUserByAuth0Id(auth0Id);
if (!existingUser) {
// We should check that we have a user that is potentially partially signed up
// a partially signed up user is a user that we have a record for but that record
// only has an email and not anything else
const potentialUser = await getUserByEmail(email!);
if (potentialUser && !potentialUser.signupAt) {
existingUser = potentialUser;
requiresWorkspace = true;
}
}

const userData = {
// TODO: figure out json fields...
Expand Down Expand Up @@ -91,6 +106,10 @@ export async function createOrUpdateUserFromAuth0Details(auth0UserData: Auth0.Us
});

tracker.identifyUser(existingUser);

if (requiresWorkspace) {
await createWorkspace(existingUser);
}
return existingUser;
} else {
const newUser = await prisma.user.create({
Expand Down
22 changes: 20 additions & 2 deletions bin/auth-api/test/auth-routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { request } from './helpers/supertest-agents';
import { testSuiteAfter, testSuiteBefore } from './helpers/test-suite-hooks';
import { mockAuth0TokenExchange } from './helpers/auth0-mocks';
import { decodeAuthToken, createSdfAuthToken } from '../src/services/auth.service';
import { createWorkspace } from "../src/services/workspaces.service";
import { createInvitedUser, getUserById } from "../src/services/users.service";

t.before(testSuiteBefore);
t.teardown(testSuiteAfter);
Expand Down Expand Up @@ -83,7 +85,12 @@ t.test('Auth routes', async () => {

t.test('verify sdf auth token succeeds for whoami', async () => {
const authData = await decodeAuthToken(validToken);
const sdfToken = createSdfAuthToken(authData.userId, 'foo');
const user = await getUserById(authData.userId);
if (!user) {
t.bailout("User Fetch has failed");
}
const workspace = await createWorkspace(user!);
const sdfToken = createSdfAuthToken(authData.userId, workspace.id);
await request.get('/whoami')
.set('cookie', `si-auth=${sdfToken}`)
.expectOk()
Expand Down Expand Up @@ -189,6 +196,17 @@ t.test('Auth routes', async () => {
expectUserData: { id: originalUserId, email: EMAIL_2 },
});
});

t.test('logging in with a partially signed up user will not create a new account, but will update other data', async () => {
const user = await createInvitedUser("[email protected]");
const { userId } = await runAuthTest({
mockOptions: {
profileOverrides: { sub: AUTH0_ID, email: user.email },
},
expectUserData: { id: originalUserId, email: "[email protected]" },
});
originalUserId = userId;
});
});

t.test('GET /auth/logout - begin logout flow', async (t) => {
Expand All @@ -213,7 +231,7 @@ t.test('Auth routes', async () => {
.expect(302)
.expect((res) => {
const redirectUrl = res.headers.location;
expect(redirectUrl).to.eq(`${process.env.AUTH_PORTAL_URL}/login`);
expect(redirectUrl).to.eq(`${process.env.AUTH_PORTAL_URL}/logout-success`);
});
});
});
Expand Down

0 comments on commit 07e72c4

Please sign in to comment.