From 1b972070d7ca5562eb92ef85224b4c1ce9964cd2 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Thu, 18 Mar 2021 21:56:24 -0700 Subject: [PATCH] feat: Enforce single team when self-hosted (#1954) * fix: Enforce single team when self hosting * test: positive case * refactor * fix: Visible error message on login screen for max teams scenario * Update Notices.js * lint --- .env.sample | 2 +- app.json | 4 +-- app/scenes/Login/Notices.js | 6 ++++ server/auth/google.js | 4 +-- server/commands/teamCreator.js | 32 ++++++++++++++++++-- server/commands/teamCreator.test.js | 46 +++++++++++++++++++++++++++++ server/errors.js | 6 ++++ server/test/helper.js | 2 ++ server/utils/authentication.js | 7 +++++ 9 files changed, 101 insertions(+), 8 deletions(-) create mode 100644 server/utils/authentication.js diff --git a/.env.sample b/.env.sample index 1b41ae5f..d83d1768 100644 --- a/.env.sample +++ b/.env.sample @@ -71,7 +71,7 @@ DEBUG=cache,presenters,events,emails,mailer,utils,multiplayer,server,services # Comma separated list of domains to be allowed to signin to the wiki. If not # set, all domains are allowed by default when using Google OAuth to signin -GOOGLE_ALLOWED_DOMAINS= +ALLOWED_DOMAINS= # For a complete Slack integration with search and posting to channels the # following configs are also needed, some more details diff --git a/app.json b/app.json index 4af94bb0..76d8e094 100644 --- a/app.json +++ b/app.json @@ -55,7 +55,7 @@ "description": "", "required": false }, - "GOOGLE_ALLOWED_DOMAINS": { + "ALLOWED_DOMAINS": { "description": "Comma separated list of domains to be allowed (optional). If not set, all Google apps domains are allowed by default", "required": false }, @@ -148,4 +148,4 @@ "required": false } } -} +} \ No newline at end of file diff --git a/app/scenes/Login/Notices.js b/app/scenes/Login/Notices.js index b45a2e8a..d7be46d3 100644 --- a/app/scenes/Login/Notices.js +++ b/app/scenes/Login/Notices.js @@ -15,6 +15,12 @@ export default function Notices({ notice }: Props) { signing in with your Google Workspace account. )} + {notice === "maximum-teams" && ( + + The team you authenticated with is not authorized on this + installation. Try another? + + )} {notice === "hd-not-allowed" && ( Sorry, your Google apps domain is not allowed. Please try again with diff --git a/server/auth/google.js b/server/auth/google.js index 9f85c227..8a100cf5 100644 --- a/server/auth/google.js +++ b/server/auth/google.js @@ -11,14 +11,14 @@ import { } from "../errors"; import auth from "../middlewares/authentication"; import passportMiddleware from "../middlewares/passport"; +import { getAllowedDomains } from "../utils/authentication"; import { StateStore } from "../utils/passport"; const router = new Router(); const providerName = "google"; const GOOGLE_CLIENT_ID = process.env.GOOGLE_CLIENT_ID; const GOOGLE_CLIENT_SECRET = process.env.GOOGLE_CLIENT_SECRET; -const allowedDomainsEnv = process.env.GOOGLE_ALLOWED_DOMAINS; -const allowedDomains = allowedDomainsEnv ? allowedDomainsEnv.split(",") : []; +const allowedDomains = getAllowedDomains(); const scopes = [ "https://www.googleapis.com/auth/userinfo.profile", diff --git a/server/commands/teamCreator.js b/server/commands/teamCreator.js index a7e18037..3bd113d3 100644 --- a/server/commands/teamCreator.js +++ b/server/commands/teamCreator.js @@ -1,11 +1,12 @@ // @flow import debug from "debug"; +import { MaximumTeamsError } from "../errors"; import { Team, AuthenticationProvider } from "../models"; import { sequelize } from "../sequelize"; +import { getAllowedDomains } from "../utils/authentication"; import { generateAvatarUrl } from "../utils/avatars"; const log = debug("server"); - type TeamCreatorResult = {| team: Team, authenticationProvider: AuthenticationProvider, @@ -28,7 +29,7 @@ export default async function teamCreator({ providerId: string, |}, |}): Promise { - const authP = await AuthenticationProvider.findOne({ + let authP = await AuthenticationProvider.findOne({ where: authenticationProvider, include: [ { @@ -49,6 +50,31 @@ export default async function teamCreator({ }; } + // This team has never been seen before, if self hosted the logic is different + // to the multi-tenant version, we want to restrict to a single team that MAY + // have multiple authentication providers + if (process.env.DEPLOYMENT !== "hosted") { + const teamCount = await Team.count(); + + // If the self-hosted installation has a single team and the domain for the + // new team matches one in the allowed domains env variable then assign the + // authentication provider to the existing team + if (teamCount === 1 && domain && getAllowedDomains().includes(domain)) { + const team = await Team.findOne(); + authP = await team.createAuthenticationProvider(authenticationProvider); + + return { + authenticationProvider: authP, + team, + isNewTeam: false, + }; + } + + if (teamCount >= 1) { + throw new MaximumTeamsError(); + } + } + // If the service did not provide a logo/avatar then we attempt to generate // one via ClearBit, or fallback to colored initials in worst case scenario if (!avatarUrl) { @@ -59,9 +85,9 @@ export default async function teamCreator({ }); } - // This team has never been seen before, time to create all the new stuff let transaction = await sequelize.transaction(); let team; + try { team = await Team.create( { diff --git a/server/commands/teamCreator.test.js b/server/commands/teamCreator.test.js index 336aecfd..1c2dd578 100644 --- a/server/commands/teamCreator.test.js +++ b/server/commands/teamCreator.test.js @@ -34,6 +34,52 @@ describe("teamCreator", () => { expect(isNewTeam).toEqual(true); }); + it("should now allow creating multiple teams in installation", async () => { + await buildTeam(); + let error; + + 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 () => { + const existing = await buildTeam(); + + 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.getAuthenticationProviders(); + expect(providers.length).toEqual(2); + }); + it("should return exising team", async () => { const authenticationProvider = { name: "google", diff --git a/server/errors.js b/server/errors.js index 412195d7..7aab2f8d 100644 --- a/server/errors.js +++ b/server/errors.js @@ -69,6 +69,12 @@ export function OAuthStateMismatchError( return httpErrors(400, message, { id: "state_mismatch" }); } +export function MaximumTeamsError( + message: string = "The maximum number of teams has been reached" +) { + return httpErrors(400, message, { id: "maximum_teams" }); +} + export function EmailAuthenticationRequiredError( message: string = "User must authenticate with email", redirectUrl: string = env.URL diff --git a/server/test/helper.js b/server/test/helper.js index 7572493f..dd87fa97 100644 --- a/server/test/helper.js +++ b/server/test/helper.js @@ -6,6 +6,8 @@ process.env.DATABASE_URL = process.env.DATABASE_URL_TEST; process.env.NODE_ENV = "test"; process.env.GOOGLE_CLIENT_ID = "123"; process.env.SLACK_KEY = "123"; +process.env.DEPLOYMENT = ""; +process.env.ALLOWED_DOMAINS = "allowed-domain.com"; // This is needed for the relative manual mock to be picked up jest.mock("../events"); diff --git a/server/utils/authentication.js b/server/utils/authentication.js new file mode 100644 index 00000000..baf3cdc8 --- /dev/null +++ b/server/utils/authentication.js @@ -0,0 +1,7 @@ +// @flow + +export function getAllowedDomains(): string[] { + // GOOGLE_ALLOWED_DOMAINS included here for backwards compatability + const env = process.env.ALLOWED_DOMAINS || process.env.GOOGLE_ALLOWED_DOMAINS; + return env ? env.split(",") : []; +}