fix: Improved handling of authentication edge-cases (#2023)

* fix: authentication records not cleaned up for deleted user
closes #2022

* fix: Improve debugging for duplicate providerId sign-in requests

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

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();

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";

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();