From 1c0c694c22eb1b3eb4fd9b3f6671f4524d01b8aa Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Fri, 2 Jul 2021 12:07:43 -0700 Subject: [PATCH] fix: Email auth should allow same guest user on multiple subdomains (#2252) * test: Add email auth tests to establish current state of system * fix: Update logic to account for dupe emails used between subdomains * test * test --- server/auth/providers/email.js | 41 ++++- server/auth/providers/email.test.js | 156 ++++++++++++++++++ .../20170712055148-non-unique-email.js | 12 +- server/test/factories.js | 17 ++ server/test/support.js | 18 +- 5 files changed, 218 insertions(+), 26 deletions(-) create mode 100644 server/auth/providers/email.test.js diff --git a/server/auth/providers/email.js b/server/auth/providers/email.js index fc20ff52e..60ed4af10 100644 --- a/server/auth/providers/email.js +++ b/server/auth/providers/email.js @@ -2,12 +2,15 @@ import { subMinutes } from "date-fns"; import Router from "koa-router"; import { find } from "lodash"; +import { parseDomain, isCustomSubdomain } from "../../../shared/utils/domains"; import { AuthorizationError } from "../../errors"; import mailer, { sendEmail } from "../../mailer"; +import errorHandling from "../../middlewares/errorHandling"; import methodOverride from "../../middlewares/methodOverride"; import validation from "../../middlewares/validation"; import { User, Team } from "../../models"; import { signIn } from "../../utils/authentication"; +import { isCustomDomain } from "../../utils/domains"; import { getUserForEmailSigninToken } from "../../utils/jwt"; const router = new Router(); @@ -20,19 +23,45 @@ export const config = { router.use(methodOverride()); router.use(validation()); -router.post("email", async (ctx) => { +router.post("email", errorHandling(), async (ctx) => { const { email } = ctx.body; ctx.assertEmail(email, "email is required"); - const user = await User.scope("withAuthentications").findOne({ + const users = await User.scope("withAuthentications").findAll({ where: { email: email.toLowerCase() }, }); - if (user) { - const team = await Team.scope("withAuthenticationProviders").findByPk( - user.teamId - ); + if (users.length) { + let team; + + if (isCustomDomain(ctx.request.hostname)) { + team = await Team.scope("withAuthenticationProviders").findOne({ + where: { domain: ctx.request.hostname }, + }); + } + + if ( + process.env.SUBDOMAINS_ENABLED === "true" && + isCustomSubdomain(ctx.request.hostname) && + !isCustomDomain(ctx.request.hostname) + ) { + const domain = parseDomain(ctx.request.hostname); + const subdomain = domain ? domain.subdomain : undefined; + team = await Team.scope("withAuthenticationProviders").findOne({ + where: { subdomain }, + }); + } + + const user = + users.find((user) => team && user.teamId === team.id) || users[0]; + + if (!team) { + team = await Team.scope("withAuthenticationProviders").findByPk( + user.teamId + ); + } + if (!team) { ctx.redirect(`/?notice=auth-error`); return; diff --git a/server/auth/providers/email.test.js b/server/auth/providers/email.test.js new file mode 100644 index 000000000..e305c09a9 --- /dev/null +++ b/server/auth/providers/email.test.js @@ -0,0 +1,156 @@ +// @flow +import TestServer from "fetch-test-server"; +import app from "../../app"; +import mailer from "../../mailer"; +import { buildUser, buildGuestUser, buildTeam } from "../../test/factories"; +import { flushdb } from "../../test/support"; + +const server = new TestServer(app.callback()); + +jest.mock("../../mailer"); + +beforeEach(async () => { + await flushdb(); + + // $FlowFixMe – does not understand Jest mocks + mailer.signin.mockReset(); +}); +afterAll(() => server.close()); + +describe("email", () => { + it("should require email param", async () => { + const res = await server.post("/auth/email", { + body: {}, + }); + const body = await res.json(); + + expect(res.status).toEqual(400); + expect(body.error).toEqual("validation_error"); + expect(body.ok).toEqual(false); + }); + + it("should respond with redirect location when user is SSO enabled", async () => { + const user = await buildUser(); + + const res = await server.post("/auth/email", { + body: { email: user.email }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.redirect).toMatch("slack"); + expect(mailer.signin).not.toHaveBeenCalled(); + }); + + it("should respond with success when user is not SSO enabled", async () => { + const user = await buildGuestUser(); + + const res = await server.post("/auth/email", { + body: { email: user.email }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.success).toEqual(true); + expect(mailer.signin).toHaveBeenCalled(); + }); + + it("should respond with success regardless of whether successful to prevent crawling email logins", async () => { + const res = await server.post("/auth/email", { + body: { email: "user@example.com" }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.success).toEqual(true); + expect(mailer.signin).not.toHaveBeenCalled(); + }); + + describe("with multiple users matching email", () => { + it("should default to current subdomain with SSO", async () => { + process.env.URL = "http://localoutline.com"; + process.env.SUBDOMAINS_ENABLED = "true"; + + const email = "sso-user@example.org"; + const team = await buildTeam({ + subdomain: "example", + }); + + await buildGuestUser({ email }); + await buildUser({ email, teamId: team.id }); + + const res = await server.post("/auth/email", { + body: { email }, + headers: { host: "example.localoutline.com" }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.redirect).toMatch("slack"); + expect(mailer.signin).not.toHaveBeenCalled(); + }); + + it("should default to current subdomain with guest email", async () => { + process.env.URL = "http://localoutline.com"; + process.env.SUBDOMAINS_ENABLED = "true"; + + const email = "guest-user@example.org"; + const team = await buildTeam({ + subdomain: "example", + }); + + await buildUser({ email }); + await buildGuestUser({ email, teamId: team.id }); + + const res = await server.post("/auth/email", { + body: { email }, + headers: { host: "example.localoutline.com" }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.success).toEqual(true); + expect(mailer.signin).toHaveBeenCalled(); + }); + + it("should default to custom domain with SSO", async () => { + const email = "sso-user-2@example.org"; + const team = await buildTeam({ + domain: "docs.mycompany.com", + }); + + await buildGuestUser({ email }); + await buildUser({ email, teamId: team.id }); + + const res = await server.post("/auth/email", { + body: { email }, + headers: { host: "docs.mycompany.com" }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.redirect).toMatch("slack"); + expect(mailer.signin).not.toHaveBeenCalled(); + }); + + it("should default to custom domain with guest email", async () => { + const email = "guest-user-2@example.org"; + const team = await buildTeam({ + domain: "docs.mycompany.com", + }); + + await buildUser({ email }); + await buildGuestUser({ email, teamId: team.id }); + + const res = await server.post("/auth/email", { + body: { email }, + headers: { host: "docs.mycompany.com" }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.success).toEqual(true); + expect(mailer.signin).toHaveBeenCalled(); + }); + }); +}); diff --git a/server/migrations/20170712055148-non-unique-email.js b/server/migrations/20170712055148-non-unique-email.js index 169c0c69b..5a4032e52 100644 --- a/server/migrations/20170712055148-non-unique-email.js +++ b/server/migrations/20170712055148-non-unique-email.js @@ -1,15 +1,7 @@ module.exports = { up: async (queryInterface, Sequelize) => { - await queryInterface.changeColumn('users', 'email', { - type: Sequelize.STRING, - unique: false, - allowNull: false, - }); - await queryInterface.changeColumn('users', 'username', { - type: Sequelize.STRING, - unique: false, - allowNull: false, - }); + await queryInterface.removeConstraint('users', 'users_email_key', {}) + await queryInterface.removeConstraint('users', 'users_username_key', {}) }, down: async (queryInterface, Sequelize) => { diff --git a/server/test/factories.js b/server/test/factories.js index 576441dfc..c053ae825 100644 --- a/server/test/factories.js +++ b/server/test/factories.js @@ -68,6 +68,23 @@ export function buildEvent(overrides: Object = {}) { }); } +export async function buildGuestUser(overrides: Object = {}) { + if (!overrides.teamId) { + const team = await buildTeam(); + overrides.teamId = team.id; + } + + count++; + + return User.create({ + email: `user${count}@example.com`, + name: `User ${count}`, + createdAt: new Date("2018-01-01T00:00:00.000Z"), + lastActiveAt: new Date("2018-01-01T00:00:00.000Z"), + ...overrides, + }); +} + export async function buildUser(overrides: Object = {}) { if (!overrides.teamId) { const team = await buildTeam(); diff --git a/server/test/support.js b/server/test/support.js index c63f33872..b82b4f23c 100644 --- a/server/test/support.js +++ b/server/test/support.js @@ -3,17 +3,15 @@ import { v4 as uuidv4 } from "uuid"; import { User, Document, Collection, Team } from "../models"; import { sequelize } from "../sequelize"; -export function flushdb() { - const sql = sequelize.getQueryInterface(); - const tables = Object.keys(sequelize.models).map((model) => { - const n = sequelize.models[model].getTableName(); - return sql.queryGenerator.quoteTable( - typeof n === "string" ? n : n.tableName - ); - }); +const sql = sequelize.getQueryInterface(); +const tables = Object.keys(sequelize.models).map((model) => { + const n = sequelize.models[model].getTableName(); + return sql.queryGenerator.quoteTable(typeof n === "string" ? n : n.tableName); +}); +const flushQuery = `TRUNCATE ${tables.join(", ")}`; - const query = `TRUNCATE ${tables.join(", ")} CASCADE`; - return sequelize.query(query); +export function flushdb() { + return sequelize.query(flushQuery); } export const seed = async () => {