From f6f90ff4068cac3db106ab460b842f3a248d95d2 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 29 Oct 2022 16:46:29 -0400 Subject: [PATCH] fix: Unable to login with matching email from another auth provider (#4356) * fix: Unable to login with matching email from another auth provider * refactor --- server/commands/accountProvisioner.ts | 17 +++---- server/commands/userProvisioner.test.ts | 49 +++++++++++++++----- server/commands/userProvisioner.ts | 59 ++++++++++++------------- server/models/Team.ts | 14 +++++- 4 files changed, 88 insertions(+), 51 deletions(-) diff --git a/server/commands/accountProvisioner.ts b/server/commands/accountProvisioner.ts index e2f0c0aad..a833bcb63 100644 --- a/server/commands/accountProvisioner.ts +++ b/server/commands/accountProvisioner.ts @@ -133,15 +133,16 @@ async function accountProvisioner({ isAdmin: isNewTeam || undefined, avatarUrl: userParams.avatarUrl, teamId: team.id, - emailMatchOnly, ip, - authentication: { - authenticationProviderId: authenticationProvider.id, - ...authenticationParams, - expiresAt: authenticationParams.expiresIn - ? new Date(Date.now() + authenticationParams.expiresIn * 1000) - : undefined, - }, + authentication: emailMatchOnly + ? undefined + : { + authenticationProviderId: authenticationProvider.id, + ...authenticationParams, + expiresAt: authenticationParams.expiresIn + ? new Date(Date.now() + authenticationParams.expiresIn * 1000) + : undefined, + }, }); const { isNewUser, user } = result; diff --git a/server/commands/userProvisioner.test.ts b/server/commands/userProvisioner.test.ts index 3b8cdcfa2..f7434e7b5 100644 --- a/server/commands/userProvisioner.test.ts +++ b/server/commands/userProvisioner.test.ts @@ -301,20 +301,11 @@ describe("userProvisioner", () => { email: externalUser.email, }); - const authenticationProviders = await team.$get("authenticationProviders"); - const authenticationProvider = authenticationProviders[0]; const result = await userProvisioner({ name: invite.name, email: "external@ExamPle.com", // ensure that email is case insensistive teamId: invite.teamId, - emailMatchOnly: true, ip, - authentication: { - authenticationProviderId: authenticationProvider.id, - providerId: "whatever", - accessToken: "123", - scopes: ["read"], - }, }); const { user, authentication, isNewUser } = result; expect(authentication).toEqual(null); @@ -351,7 +342,7 @@ describe("userProvisioner", () => { ); }); - it("should create a user from allowed Domain", async () => { + it("should create a user from allowed domain", async () => { const { admin, team } = await seed(); await TeamDomain.create({ teamId: team.id, @@ -382,6 +373,44 @@ describe("userProvisioner", () => { expect(isNewUser).toEqual(true); }); + it("should create a user from allowed domain with emailMatchOnly", async () => { + const { admin, team } = await seed(); + await TeamDomain.create({ + teamId: team.id, + name: "example-company.com", + createdById: admin.id, + }); + + const result = await userProvisioner({ + name: "Test Name", + email: "user@example-company.com", + teamId: team.id, + ip, + }); + const { user, authentication, isNewUser } = result; + expect(authentication).toBeUndefined(); + expect(user.email).toEqual("user@example-company.com"); + expect(isNewUser).toEqual(true); + }); + + it("should not create a user with emailMatchOnly when no allowed domains are set", async () => { + const { team } = await seed(); + let error; + + try { + await userProvisioner({ + name: "Test Name", + email: "user@example-company.com", + teamId: team.id, + ip, + }); + } catch (err) { + error = err; + } + + expect(error && error.toString()).toContain("UnauthorizedError"); + }); + it("should reject an user when the domain is not allowed", async () => { const { admin, team } = await seed(); await TeamDomain.create({ diff --git a/server/commands/userProvisioner.ts b/server/commands/userProvisioner.ts index 280a8a4af..a4bd3c4e7 100644 --- a/server/commands/userProvisioner.ts +++ b/server/commands/userProvisioner.ts @@ -29,11 +29,10 @@ type Props = { * subdomain that the request came from, if any. */ teamId: string; - /** Only match against existing user accounts using email, do not create a new account */ - emailMatchOnly?: boolean; /** The IP address of the incoming request */ ip: string; - authentication: { + /** Bundle of props related to the current external provider authentication */ + authentication?: { authenticationProviderId: string; /** External identifier of the user in the authentication provider */ providerId: string; @@ -53,31 +52,31 @@ export default async function userProvisioner({ email, username, isAdmin, - emailMatchOnly, avatarUrl, teamId, authentication, ip, }: Props): Promise { - const { providerId, authenticationProviderId, ...rest } = authentication; - - const auth = await UserAuthentication.findOne({ - where: { - providerId, - }, - include: [ - { - model: User, - as: "user", - where: { teamId }, - required: true, - }, - ], - }); + const auth = authentication + ? await UserAuthentication.findOne({ + where: { + providerId: authentication.providerId, + }, + include: [ + { + model: User, + as: "user", + where: { teamId }, + required: true, + }, + ], + }) + : undefined; // Someone has signed in with this authentication before, we just // want to update the details instead of creating a new record - if (auth) { + if (auth && authentication) { + const { providerId, authenticationProviderId, ...rest } = authentication; const { user } = auth; // We found an authentication record that matches the user id, but it's @@ -123,6 +122,10 @@ export default async function userProvisioner({ }, }); + const team = await Team.scope("withDomains").findByPk(teamId, { + attributes: ["defaultUserRole", "inviteRequired", "id"], + }); + // We have an existing user, so we need to update it with our // new details and count this as a new user creation. if (existingUser) { @@ -165,9 +168,8 @@ export default async function userProvisioner({ } ); - // We don't want to associate a user auth with the auth provider - // if we're doing a simple email match, so early return here - if (emailMatchOnly) { + // Only need to associate the authentication with the user if there is one. + if (!authentication) { return null; } @@ -197,9 +199,9 @@ export default async function userProvisioner({ authentication: auth, isNewUser: isInvite, }; - } else if (emailMatchOnly) { + } else if (!authentication && !team?.allowedDomains.length) { // There's no existing invite or user that matches the external auth email - // This is simply unauthorized + // and there is no possibility of matching an allowed domain. throw InvalidAuthenticationError(); } @@ -210,11 +212,6 @@ export default async function userProvisioner({ const transaction = await User.sequelize!.transaction(); try { - const team = await Team.findByPk(teamId, { - attributes: ["defaultUserRole", "inviteRequired", "id"], - transaction, - }); - // 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) { @@ -240,7 +237,7 @@ export default async function userProvisioner({ teamId, avatarUrl, service: null, - authentications: [authentication], + authentications: authentication ? [authentication] : [], }, { include: "authentications", diff --git a/server/models/Team.ts b/server/models/Team.ts index 1e7fd700c..3bbd86ed0 100644 --- a/server/models/Team.ts +++ b/server/models/Team.ts @@ -253,7 +253,7 @@ class Team extends ParanoidModel { }); }; - collectionIds = async function (paranoid = true) { + public collectionIds = async function (this: Team, paranoid = true) { const models = await Collection.findAll({ attributes: ["id"], where: { @@ -267,7 +267,17 @@ class Team extends ParanoidModel { return models.map((c) => c.id); }; - isDomainAllowed = async function (domain: string) { + /** + * Find whether the passed domain can be used to sign-in to this team. Note + * that this method always returns true if no domain restrictions are set. + * + * @param domain The domain to check + * @returns True if the domain is allowed to sign-in to this team + */ + public isDomainAllowed = async function ( + this: Team, + domain: string + ): Promise { const allowedDomains = (await this.$get("allowedDomains")) || []; return (