From 28439d315dd9c7cf1681d6f2e1251da54a11beae Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 4 Jun 2022 10:46:03 -0700 Subject: [PATCH] fix: Self-hosted should show signin options for all configured authentication methods (#2986) --- server/commands/accountProvisioner.test.ts | 80 +++++++++ server/commands/teamCreator.test.ts | 182 +++++++++++---------- server/commands/teamCreator.ts | 12 +- server/routes/api/auth.test.ts | 32 +++- server/routes/api/auth.ts | 6 +- server/routes/auth/providers/index.ts | 3 +- 6 files changed, 205 insertions(+), 110 deletions(-) diff --git a/server/commands/accountProvisioner.test.ts b/server/commands/accountProvisioner.test.ts index 770bb3080..afd8f6aed 100644 --- a/server/commands/accountProvisioner.test.ts +++ b/server/commands/accountProvisioner.test.ts @@ -1,4 +1,5 @@ import WelcomeEmail from "@server/emails/templates/WelcomeEmail"; +import env from "@server/env"; import { TeamDomain } from "@server/models"; import Collection from "@server/models/Collection"; import UserAuthentication from "@server/models/UserAuthentication"; @@ -14,6 +15,8 @@ describe("accountProvisioner", () => { const ip = "127.0.0.1"; it("should create a new user and team", async () => { + env.DEPLOYMENT = "hosted"; + const spy = jest.spyOn(WelcomeEmail, "schedule"); const { user, team, isNewTeam, isNewUser } = await accountProvisioner({ ip, @@ -286,4 +289,81 @@ describe("accountProvisioner", () => { spy.mockRestore(); }); + + describe("self hosted", () => { + it("should fail if existing team and domain not in allowed list", async () => { + env.DEPLOYMENT = undefined; + let error; + const team = await buildTeam(); + + try { + await accountProvisioner({ + ip, + user: { + name: "Jenny Tester", + email: "jenny@example.com", + avatarUrl: "https://example.com/avatar.png", + username: "jtester", + }, + team: { + name: team.name, + avatarUrl: team.avatarUrl, + subdomain: "example", + }, + authenticationProvider: { + name: "google", + providerId: "example.com", + }, + authentication: { + providerId: "123456789", + accessToken: "123", + scopes: ["read"], + }, + }); + } catch (err) { + error = err; + } + + expect(error.message).toEqual( + "The maximum number of teams has been reached" + ); + }); + + it("should always use existing team if self-hosted", async () => { + env.DEPLOYMENT = undefined; + + const team = await buildTeam(); + const { user, isNewUser } = await accountProvisioner({ + ip, + user: { + name: "Jenny Tester", + email: "jenny@example.com", + avatarUrl: "https://example.com/avatar.png", + username: "jtester", + }, + team: { + name: team.name, + avatarUrl: team.avatarUrl, + subdomain: "example", + domain: "allowed-domain.com", + }, + authenticationProvider: { + name: "google", + providerId: "allowed-domain.com", + }, + authentication: { + providerId: "123456789", + accessToken: "123", + scopes: ["read"], + }, + }); + + expect(user.teamId).toEqual(team.id); + expect(user.username).toEqual("jtester"); + expect(isNewUser).toEqual(true); + + const providers = await team.$get("authenticationProviders"); + expect(providers.length).toEqual(2); + }); + }); }); diff --git a/server/commands/teamCreator.test.ts b/server/commands/teamCreator.test.ts index b641be958..a76657dae 100644 --- a/server/commands/teamCreator.test.ts +++ b/server/commands/teamCreator.test.ts @@ -8,6 +8,7 @@ beforeEach(() => flushdb()); describe("teamCreator", () => { it("should create team and authentication provider", async () => { + env.DEPLOYMENT = "hosted"; const result = await teamCreator({ name: "Test team", subdomain: "example", @@ -25,72 +26,41 @@ describe("teamCreator", () => { expect(isNewTeam).toEqual(true); }); - it("should not allow creating multiple teams in installation", async () => { - env.DEPLOYMENT = undefined; - await buildTeam(); - let error; + describe("self hosted", () => { + it("should not allow creating multiple teams in installation", async () => { + env.DEPLOYMENT = undefined; + await buildTeam(); + let error; - try { - await teamCreator({ - name: "Test team", - subdomain: "example", - avatarUrl: "http://example.com/logo.png", - authenticationProvider: { - name: "google", - providerId: "example.com", - }, + try { + await teamCreator({ + name: "Test team", + subdomain: "example", + avatarUrl: "http://example.com/logo.png", + authenticationProvider: { + name: "google", + providerId: "example.com", + }, + }); + } catch (err) { + error = err; + } + + expect(error).toBeTruthy(); + }); + + it("should return existing team when within allowed domains", async () => { + env.DEPLOYMENT = undefined; + const existing = await buildTeam(); + const user = await buildUser({ + teamId: existing.id, }); - } catch (err) { - error = err; - } - - expect(error).toBeTruthy(); - }); - - it("should return existing team when within allowed domains", async () => { - env.DEPLOYMENT = undefined; - const existing = await buildTeam(); - const user = await buildUser({ - teamId: existing.id, - }); - await TeamDomain.create({ - teamId: existing.id, - name: "allowed-domain.com", - createdById: user.id, - }); - - const result = await teamCreator({ - name: "Updated name", - subdomain: "example", - domain: "allowed-domain.com", - authenticationProvider: { - name: "google", - providerId: "allowed-domain.com", - }, - }); - const { team, authenticationProvider, isNewTeam } = result; - expect(team.id).toEqual(existing.id); - expect(team.name).toEqual(existing.name); - expect(authenticationProvider.name).toEqual("google"); - expect(authenticationProvider.providerId).toEqual("allowed-domain.com"); - expect(isNewTeam).toEqual(false); - const providers = await team.$get("authenticationProviders"); - expect(providers.length).toEqual(2); - }); - - it("should error when NOT within allowed domains", async () => { - const user = await buildUser(); - delete process.env.DEPLOYMENT; - const existing = await buildTeam(); - await TeamDomain.create({ - teamId: existing.id, - name: "other-domain.com", - createdById: user.id, - }); - - let error; - try { - await teamCreator({ + await TeamDomain.create({ + teamId: existing.id, + name: "allowed-domain.com", + createdById: user.id, + }); + const result = await teamCreator({ name: "Updated name", subdomain: "example", domain: "allowed-domain.com", @@ -99,32 +69,66 @@ describe("teamCreator", () => { providerId: "allowed-domain.com", }, }); - } catch (err) { - error = err; - } - - expect(error).toBeTruthy(); - }); - - it("should return exising team", async () => { - env.DEPLOYMENT = undefined; - const authenticationProvider = { - name: "google", - providerId: "example.com", - }; - const existing = await buildTeam({ - subdomain: "example", - authenticationProviders: [authenticationProvider], + const { team, authenticationProvider, isNewTeam } = result; + expect(team.id).toEqual(existing.id); + expect(team.name).toEqual(existing.name); + expect(authenticationProvider.name).toEqual("google"); + expect(authenticationProvider.providerId).toEqual("allowed-domain.com"); + expect(isNewTeam).toEqual(false); + const providers = await team.$get("authenticationProviders"); + expect(providers.length).toEqual(2); }); - const result = await teamCreator({ - name: "Updated name", - subdomain: "example", - authenticationProvider, + + it("should error when NOT within allowed domains", async () => { + env.DEPLOYMENT = undefined; + const existing = await buildTeam(); + const user = await buildUser({ + teamId: existing.id, + }); + await TeamDomain.create({ + teamId: existing.id, + name: "allowed-domain.com", + createdById: user.id, + }); + + let error; + try { + await teamCreator({ + name: "Updated name", + subdomain: "example", + domain: "other-domain.com", + authenticationProvider: { + name: "google", + providerId: "other-domain.com", + }, + }); + } catch (err) { + error = err; + } + + expect(error).toBeTruthy(); + }); + + it("should return exising team", async () => { + env.DEPLOYMENT = undefined; + const authenticationProvider = { + name: "google", + providerId: "example.com", + }; + const existing = await buildTeam({ + subdomain: "example", + authenticationProviders: [authenticationProvider], + }); + const result = await teamCreator({ + name: "Updated name", + subdomain: "example", + authenticationProvider, + }); + const { team, isNewTeam } = result; + expect(team.id).toEqual(existing.id); + expect(team.name).toEqual(existing.name); + expect(team.subdomain).toEqual("example"); + expect(isNewTeam).toEqual(false); }); - const { team, isNewTeam } = result; - expect(team.id).toEqual(existing.id); - expect(team.name).toEqual(existing.name); - expect(team.subdomain).toEqual("example"); - expect(isNewTeam).toEqual(false); }); }); diff --git a/server/commands/teamCreator.ts b/server/commands/teamCreator.ts index 691459c6e..8b76ef6bb 100644 --- a/server/commands/teamCreator.ts +++ b/server/commands/teamCreator.ts @@ -1,4 +1,3 @@ -import invariant from "invariant"; import env from "@server/env"; import { DomainNotAllowedError, MaximumTeamsError } from "@server/errors"; import Logger from "@server/logging/Logger"; @@ -55,15 +54,12 @@ async function teamCreator({ // to the multi-tenant version, we want to restrict to a single team that MAY // have multiple authentication providers if (env.DEPLOYMENT !== "hosted") { - const teamCount = await Team.count(); + const team = await Team.findOne(); // If the self-hosted installation has a single team and the domain for the // new team is allowed then assign the authentication provider to the // existing team - if (teamCount === 1 && domain) { - const team = await Team.findOne(); - invariant(team, "Team should exist"); - + if (team && domain) { if (await team.isDomainAllowed(domain)) { authP = await team.$create( "authenticationProvider", @@ -79,9 +75,7 @@ async function teamCreator({ } } - if (teamCount >= 1) { - throw MaximumTeamsError(); - } + throw MaximumTeamsError(); } // If the service did not provide a logo/avatar then we attempt to generate diff --git a/server/routes/api/auth.test.ts b/server/routes/api/auth.test.ts index a9d9f3ee1..19813cf6c 100644 --- a/server/routes/api/auth.test.ts +++ b/server/routes/api/auth.test.ts @@ -1,4 +1,5 @@ import TestServer from "fetch-test-server"; +import sharedEnv from "@shared/env"; import env from "@server/env"; import webService from "@server/services/web"; import { buildUser, buildTeam } from "@server/test/factories"; @@ -49,6 +50,8 @@ describe("#auth.info", () => { describe("#auth.config", () => { it("should return available SSO providers", async () => { + env.DEPLOYMENT = "hosted"; + const res = await server.post("/api/auth.config"); const body = await res.json(); expect(res.status).toEqual(200); @@ -58,7 +61,10 @@ describe("#auth.config", () => { }); it("should return available providers for team subdomain", async () => { - env.URL = "http://localoutline.com"; + env.URL = sharedEnv.URL = "http://localoutline.com"; + env.SUBDOMAINS_ENABLED = sharedEnv.SUBDOMAINS_ENABLED = true; + env.DEPLOYMENT = "hosted"; + await buildTeam({ guestSignin: false, subdomain: "example", @@ -81,6 +87,8 @@ describe("#auth.config", () => { }); it("should return available providers for team custom domain", async () => { + env.DEPLOYMENT = "hosted"; + await buildTeam({ guestSignin: false, domain: "docs.mycompany.com", @@ -103,7 +111,9 @@ describe("#auth.config", () => { }); it("should return email provider for team when guest signin enabled", async () => { - env.URL = "http://localoutline.com"; + env.URL = sharedEnv.URL = "http://localoutline.com"; + env.DEPLOYMENT = "hosted"; + await buildTeam({ guestSignin: true, subdomain: "example", @@ -127,7 +137,9 @@ describe("#auth.config", () => { }); it("should not return provider when disabled", async () => { - env.URL = "http://localoutline.com"; + env.URL = sharedEnv.URL = "http://localoutline.com"; + env.DEPLOYMENT = "hosted"; + await buildTeam({ guestSignin: false, subdomain: "example", @@ -148,8 +160,9 @@ describe("#auth.config", () => { expect(res.status).toEqual(200); expect(body.data.providers.length).toBe(0); }); + describe("self hosted", () => { - it("should return available providers for team", async () => { + it("should return all configured providers but respect email setting", async () => { env.DEPLOYMENT = ""; await buildTeam({ guestSignin: false, @@ -163,9 +176,11 @@ describe("#auth.config", () => { const res = await server.post("/api/auth.config"); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.providers.length).toBe(1); - expect(body.data.providers[0].name).toBe("Slack"); + expect(body.data.providers.length).toBe(2); + expect(body.data.providers[0].name).toBe("Google"); + expect(body.data.providers[1].name).toBe("Slack"); }); + it("should return email provider for team when guest signin enabled", async () => { env.DEPLOYMENT = ""; await buildTeam({ @@ -180,9 +195,10 @@ describe("#auth.config", () => { const res = await server.post("/api/auth.config"); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.providers.length).toBe(2); + expect(body.data.providers.length).toBe(3); expect(body.data.providers[0].name).toBe("Slack"); - expect(body.data.providers[1].name).toBe("Email"); + expect(body.data.providers[1].name).toBe("Google"); + expect(body.data.providers[2].name).toBe("Email"); }); }); }); diff --git a/server/routes/api/auth.ts b/server/routes/api/auth.ts index 56daeb3a3..470232dc2 100644 --- a/server/routes/api/auth.ts +++ b/server/routes/api/auth.ts @@ -22,6 +22,7 @@ function filterProviders(team?: Team) { return ( !team || + env.DEPLOYMENT !== "hosted" || find(team.authenticationProviders, { name: provider.id, enabled: true, @@ -40,10 +41,9 @@ router.post("auth.config", async (ctx) => { // brand for the knowledge base and it's guest signin option is used for the // root login page. if (env.DEPLOYMENT !== "hosted") { - const teams = await Team.scope("withAuthenticationProviders").findAll(); + const team = await Team.scope("withAuthenticationProviders").findOne(); - if (teams.length === 1) { - const team = teams[0]; + if (team) { ctx.body = { data: { name: team.name, diff --git a/server/routes/auth/providers/index.ts b/server/routes/auth/providers/index.ts index 831437d39..63d50ffc1 100644 --- a/server/routes/auth/providers/index.ts +++ b/server/routes/auth/providers/index.ts @@ -1,4 +1,5 @@ import Router from "koa-router"; +import { sortBy } from "lodash"; import { signin } from "@shared/utils/urlHelpers"; import { requireDirectory } from "@server/utils/fs"; @@ -43,4 +44,4 @@ requireDirectory(__dirname).forEach(([module, id]) => { } }); -export default providers; +export default sortBy(providers, "id");