From 1e808fc52c84d308622b555808adc44e0d1fbf6b Mon Sep 17 00:00:00 2001 From: Nan Yu Date: Fri, 8 Jul 2022 00:24:46 -0700 Subject: [PATCH] Feat: add auth provider to users on sign in (#3739) * feat: merge a new authentication method onto existing user records when emails match * adds test for invite acceptance and auth provider creation * addresses comments - test existing user and invites in different test cases - update lastActiveAt syncronously when an invite is accepted * sort arrays in test to prevent nondeterministic test behaivior when doing array compare --- server/commands/userCreator.test.ts | 72 ++++++++++++++++++++++++ server/commands/userCreator.ts | 85 +++++++++++++++++------------ server/routes/api/team.test.ts | 4 +- server/test/factories.ts | 1 + 4 files changed, 125 insertions(+), 37 deletions(-) diff --git a/server/commands/userCreator.test.ts b/server/commands/userCreator.test.ts index d22d7bf43..6969ba7f6 100644 --- a/server/commands/userCreator.test.ts +++ b/server/commands/userCreator.test.ts @@ -1,3 +1,4 @@ +import { v4 as uuidv4 } from "uuid"; import { TeamDomain } from "@server/models"; import { buildUser, buildTeam, buildInvite } from "@server/test/factories"; import { flushdb, seed } from "@server/test/support"; @@ -37,6 +38,77 @@ describe("userCreator", () => { expect(isNewUser).toEqual(false); }); + it("should add authentication provider to existing users", async () => { + const team = await buildTeam({ inviteRequired: true }); + const teamAuthProviders = await team.$get("authenticationProviders"); + const authenticationProvider = teamAuthProviders[0]; + + const email = "mynam@email.com"; + const existing = await buildUser({ + email, + teamId: team.id, + authentications: [], + }); + + const result = await userCreator({ + name: existing.name, + email, + username: "new-username", + avatarUrl: existing.avatarUrl, + teamId: existing.teamId, + ip, + authentication: { + authenticationProviderId: authenticationProvider.id, + providerId: uuidv4(), + accessToken: "123", + scopes: ["read"], + }, + }); + const { user, authentication, isNewUser } = result; + expect(authentication.accessToken).toEqual("123"); + expect(authentication.scopes.length).toEqual(1); + expect(authentication.scopes[0]).toEqual("read"); + + const authentications = await user.$get("authentications"); + expect(authentications.length).toEqual(1); + expect(isNewUser).toEqual(false); + }); + + it("should add authentication provider to invited users", async () => { + const team = await buildTeam({ inviteRequired: true }); + const teamAuthProviders = await team.$get("authenticationProviders"); + const authenticationProvider = teamAuthProviders[0]; + + const email = "mynam@email.com"; + const existing = await buildInvite({ + email, + teamId: team.id, + }); + + const result = await userCreator({ + name: existing.name, + email, + username: "new-username", + avatarUrl: existing.avatarUrl, + teamId: existing.teamId, + ip, + authentication: { + authenticationProviderId: authenticationProvider.id, + providerId: uuidv4(), + accessToken: "123", + scopes: ["read"], + }, + }); + const { user, authentication, isNewUser } = result; + expect(authentication.accessToken).toEqual("123"); + expect(authentication.scopes.length).toEqual(1); + expect(authentication.scopes[0]).toEqual("read"); + + const authentications = await user.$get("authentications"); + expect(authentications.length).toEqual(1); + expect(isNewUser).toEqual(true); + }); + it("should create user with deleted user matching providerId", async () => { const existing = await buildUser(); const authentications = await existing.$get("authentications"); diff --git a/server/commands/userCreator.ts b/server/commands/userCreator.ts index ac485cdf2..59448f67b 100644 --- a/server/commands/userCreator.ts +++ b/server/commands/userCreator.ts @@ -1,4 +1,3 @@ -import { Op } from "sequelize"; import { sequelize } from "@server/database/sequelize"; import InviteAcceptedEmail from "@server/emails/templates/InviteAcceptedEmail"; import { DomainNotAllowedError, InviteRequiredError } from "@server/errors"; @@ -89,48 +88,62 @@ export default async function userCreator({ // A `user` record might exist in the form of an invite even if there is no // existing authentication record that matches. In Outline an invite is a // shell user record. - const invite = await User.scope(["withAuthentications", "withTeam"]).findOne({ + const existingUser = await User.scope([ + "withAuthentications", + "withTeam", + ]).findOne({ where: { // Email from auth providers may be capitalized and we should respect that // however any existing invites will always be lowercased. email: email.toLowerCase(), teamId, - lastActiveAt: { - [Op.is]: null, - }, }, }); // We have an existing invite for his user, so we need to update it with our // new details, link up the authentication method, and count this as a new // user creation. - if (invite && !invite.authentications.length) { + if (existingUser) { + // A `user` record might exist in the form of an invite. + // In Outline an invite is a shell user record with no authentication method + // that's never been active before. + const isInvite = existingUser.isInvited; + const auth = await sequelize.transaction(async (transaction) => { - await invite.update( + if (isInvite) { + await Event.create( + { + name: "users.create", + actorId: existingUser.id, + userId: existingUser.id, + teamId: existingUser.teamId, + data: { + name, + }, + ip, + }, + { + transaction, + } + ); + } + + // Regardless, create a new authentication record + // against the existing user (user can auth with multiple SSO providers) + // Update user's name and avatar based on the most recently added provider + await existingUser.update( { name, avatarUrl, + lastActiveAt: new Date(), + lastActiveIp: ip, }, { transaction, } ); - await Event.create( - { - name: "users.create", - actorId: invite.id, - userId: invite.id, - teamId: invite.teamId, - data: { - name, - }, - ip, - }, - { - transaction, - } - ); - return await invite.$create( + + return await existingUser.$create( "authentication", authentication, { @@ -139,20 +152,22 @@ export default async function userCreator({ ); }); - const inviter = await invite.$get("invitedBy"); - if (inviter) { - await InviteAcceptedEmail.schedule({ - to: inviter.email, - inviterId: inviter.id, - invitedName: invite.name, - teamUrl: invite.team.url, - }); + if (isInvite) { + const inviter = await existingUser.$get("invitedBy"); + if (inviter) { + await InviteAcceptedEmail.schedule({ + to: inviter.email, + inviterId: inviter.id, + invitedName: existingUser.name, + teamUrl: existingUser.team.url, + }); + } } return { - user: invite, + user: existingUser, authentication: auth, - isNewUser: true, + isNewUser: isInvite, }; } @@ -165,9 +180,9 @@ export default async function userCreator({ transaction, }); - // If the team settings are set to require invites, and the user is not already invited, + // If the team settings are set to require invites, and there's no existing user record, // throw an error and fail user creation. - if (team?.inviteRequired && !invite) { + if (team?.inviteRequired) { throw InviteRequiredError(); } diff --git a/server/routes/api/team.test.ts b/server/routes/api/team.test.ts index 44db902dc..b613cf689 100644 --- a/server/routes/api/team.test.ts +++ b/server/routes/api/team.test.ts @@ -39,7 +39,7 @@ describe("#team.update", () => { }); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.allowedDomains).toEqual([ + expect(body.data.allowedDomains.sort()).toEqual([ "example-company.com", "example-company.org", ]); @@ -47,7 +47,7 @@ describe("#team.update", () => { const teamDomains: TeamDomain[] = await TeamDomain.findAll({ where: { teamId: team.id }, }); - expect(teamDomains.map((d) => d.name)).toEqual([ + expect(teamDomains.map((d) => d.name).sort()).toEqual([ "example-company.com", "example-company.org", ]); diff --git a/server/test/factories.ts b/server/test/factories.ts index d9ff3cc3c..086ed4d74 100644 --- a/server/test/factories.ts +++ b/server/test/factories.ts @@ -179,6 +179,7 @@ export async function buildInvite(overrides: Partial = {}) { name: `User ${count}`, createdAt: new Date("2018-01-01T00:00:00.000Z"), invitedById: actor.id, + authentications: [], ...overrides, lastActiveAt: null, });