diff --git a/server/commands/userCreator.js b/server/commands/userCreator.js index 531a4ab8c..9b6785ee0 100644 --- a/server/commands/userCreator.js +++ b/server/commands/userCreator.js @@ -37,7 +37,6 @@ export default async function userCreator({ const { authenticationProviderId, providerId, ...rest } = authentication; const auth = await UserAuthentication.findOne({ where: { - authenticationProviderId, providerId, }, include: [ @@ -53,10 +52,27 @@ export default async function userCreator({ if (auth) { const { user } = auth; - await user.update({ email }); - await auth.update(rest); + // We found an authentication record that matches the user id, but it's + // associated with a different authentication provider, (eg a different + // hosted google domain). This is possible in Google Auth when moving domains. + // In the future we may auto-migrate these. + if (auth.authenticationProviderId !== authenticationProviderId) { + throw new Error( + `User authentication ${providerId} already exists for ${auth.authenticationProviderId}, tried to assign to ${authenticationProviderId}` + ); + } - return { user, authentication: auth, isNewUser: false }; + if (user) { + await user.update({ email }); + await auth.update(rest); + + return { user, authentication: auth, isNewUser: false }; + } + + // We found an authentication record, but the associated user was deleted or + // otherwise didn't exist. Cleanup the auth record and proceed with creating + // a new user. See: https://github.com/outline/outline/issues/2022 + await auth.destroy(); } // A `user` record might exist in the form of an invite even if there is no diff --git a/server/commands/userCreator.test.js b/server/commands/userCreator.test.js index a7d2bd7a4..b40abe7c1 100644 --- a/server/commands/userCreator.test.js +++ b/server/commands/userCreator.test.js @@ -37,6 +37,62 @@ describe("userCreator", () => { expect(isNewUser).toEqual(false); }); + it("should create user with deleted user matching providerId", async () => { + const existing = await buildUser(); + const authentications = await existing.getAuthentications(); + const existingAuth = authentications[0]; + const newEmail = "test@example.com"; + + await existing.destroy(); + + const result = await userCreator({ + name: "Test Name", + email: "test@example.com", + teamId: existing.teamId, + ip, + authentication: { + authenticationProviderId: existingAuth.authenticationProviderId, + providerId: existingAuth.providerId, + 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"); + expect(user.email).toEqual(newEmail); + expect(isNewUser).toEqual(true); + }); + + it("should handle duplicate providerId for different iDP", async () => { + const existing = await buildUser(); + const authentications = await existing.getAuthentications(); + const existingAuth = authentications[0]; + let error; + + try { + await userCreator({ + name: "Test Name", + email: "test@example.com", + teamId: existing.teamId, + ip, + authentication: { + authenticationProviderId: "example.org", + providerId: existingAuth.providerId, + accessToken: "123", + scopes: ["read"], + }, + }); + } catch (err) { + error = err; + } + + expect(error && error.toString()).toContain("already exists for"); + }); + it("should create a new user", async () => { const team = await buildTeam(); const authenticationProviders = await team.getAuthenticationProviders(); diff --git a/server/models/User.js b/server/models/User.js index 2d875e32a..8842c56a7 100644 --- a/server/models/User.js +++ b/server/models/User.js @@ -10,6 +10,7 @@ import { sendEmail } from "../mailer"; import { DataTypes, sequelize, encryptedFields, Op } from "../sequelize"; import { DEFAULT_AVATAR_HOST } from "../utils/avatars"; import { publicS3Endpoint, uploadToS3FromUrl } from "../utils/s3"; +import UserAuthentication from "./UserAuthentication"; import { Star, Team, Collection, NotificationSetting, ApiKey } from "."; const User = sequelize.define( @@ -208,6 +209,10 @@ const removeIdentifyingInfo = async (model, options) => { where: { userId: model.id }, transaction: options.transaction, }); + await UserAuthentication.destroy({ + where: { userId: model.id }, + transaction: options.transaction, + }); model.email = null; model.name = "Unknown"; diff --git a/server/models/User.test.js b/server/models/User.test.js index 283dd0478..9a8231fab 100644 --- a/server/models/User.test.js +++ b/server/models/User.test.js @@ -1,11 +1,21 @@ // @flow -import { CollectionUser } from "../models"; +import { UserAuthentication, CollectionUser } from "../models"; import { buildUser, buildTeam, buildCollection } from "../test/factories"; import { flushdb } from "../test/support"; beforeEach(() => flushdb()); describe("user model", () => { + describe("destroy", () => { + it("should delete user authentications", async () => { + const user = await buildUser(); + expect(await UserAuthentication.count()).toBe(1); + + await user.destroy(); + expect(await UserAuthentication.count()).toBe(0); + }); + }); + describe("getJwtToken", () => { it("should set JWT secret", async () => { const user = await buildUser();