diff --git a/app/scenes/Settings/Security.tsx b/app/scenes/Settings/Security.tsx index 93445473b46f..936b574519d4 100644 --- a/app/scenes/Settings/Security.tsx +++ b/app/scenes/Settings/Security.tsx @@ -5,7 +5,9 @@ import { useState } from "react"; import * as React from "react"; import { useTranslation, Trans } from "react-i18next"; import { toast } from "sonner"; +import { AUTH_TYPE_SSO } from "@shared/constants"; import { TeamPreference, EmailDisplay } from "@shared/types"; +import env from "~/env"; import ConfirmationDialog from "~/components/ConfirmationDialog"; import Heading from "~/components/Heading"; import type { Option } from "~/components/InputSelect"; @@ -306,19 +308,21 @@ function Security() { onChange={handleViewersCanExportChange} /> - - - + {env.AUTH_TYPE !== AUTH_TYPE_SSO && ( + + + + )} { // throws into the ErrorBoundary. This is belt-and-suspenders on // top of ApiClient.fetch's primary detection; either path lands // us on the same wipeAndReload helper which is idempotent. - if (env.AUTH_TYPE === "SSO" && !res?.data?.user) { + if (env.AUTH_TYPE === AUTH_TYPE_SSO && !res?.data?.user) { Logger.warn( "/auth.info returned no user payload — assuming stale session" ); diff --git a/app/utils/ApiClient.ts b/app/utils/ApiClient.ts index d0e9ee5c6697..270c13fc6c7f 100644 --- a/app/utils/ApiClient.ts +++ b/app/utils/ApiClient.ts @@ -1,6 +1,7 @@ import retry from "fetch-retry"; import trim from "lodash/trim"; import queryString from "query-string"; +import { AUTH_TYPE_SSO } from "@shared/constants"; import EDITOR_VERSION from "@shared/editor/version"; import type { JSONObject } from "@shared/types"; import { Scope } from "@shared/types"; @@ -203,7 +204,7 @@ class ApiClient { // Either signal is sufficient. Both can be true together but only // the first qualifying detection matters since wipeAndReload is // idempotent. - if (env.AUTH_TYPE === "SSO") { + if (env.AUTH_TYPE === AUTH_TYPE_SSO) { const contentType = response.headers.get("content-type") || ""; const finalUrlOffApi = !response.url.includes("/api/"); const wasRedirected = response.redirected && finalUrlOffApi; @@ -244,7 +245,7 @@ class ApiClient { // Handle 401, log out user if (response.status === 401) { if (!this.shareId) { - if (env.AUTH_TYPE === "SSO") { + if (env.AUTH_TYPE === AUTH_TYPE_SSO) { // In ForwardAuth mode, the stale JWT cookie has been cleared by the // server. Navigate to the current URL so the browser makes a fresh // HTTP request — the proxy will inject new identity headers and a new diff --git a/server/env.ts b/server/env.ts index 4864e06bdb6b..4bab9883584d 100644 --- a/server/env.ts +++ b/server/env.ts @@ -543,10 +543,11 @@ export class Environment { /** * The authentication type to use. When set to "SSO", the server will trust - * X-Auth-Request-Email and X-Auth-Request-User headers injected by a reverse - * proxy (e.g. oauth2-proxy, Authelia) for authentication and automatic user - * provisioning. Only enable this when Outline is deployed behind a trusted - * authenticating proxy on a self-hosted instance. + * the X-Auth-Request-Email header injected by a reverse proxy + * (e.g. oauth2-proxy, Authelia) for authentication and automatic user + * provisioning. The display name is derived from the email local-part. + * Only enable this when Outline is deployed behind a trusted authenticating + * proxy on a self-hosted instance. */ @Public @IsOptional() diff --git a/server/middlewares/authentication.test.ts b/server/middlewares/authentication.test.ts index 9365f6742bd5..802485fa281b 100644 --- a/server/middlewares/authentication.test.ts +++ b/server/middlewares/authentication.test.ts @@ -1,4 +1,5 @@ import type { DefaultState } from "koa"; +import { AUTH_TYPE_SSO } from "@shared/constants"; import { randomString } from "@shared/random"; import { Scope } from "@shared/types"; import env from "@server/env"; @@ -417,7 +418,7 @@ describe("Authentication middleware", () => { describe("with ForwardAuth headers", () => { beforeEach(() => { - env.AUTH_TYPE = "SSO"; + env.AUTH_TYPE = AUTH_TYPE_SSO; }); afterEach(() => { @@ -510,9 +511,6 @@ describe("Authentication middleware", () => { if (header === "x-auth-request-email") { return newEmail; } - if (header === "x-auth-request-user") { - return "New User"; - } return ""; }), }, @@ -529,36 +527,6 @@ describe("Authentication middleware", () => { where: { email: newEmail.toLowerCase() }, }); expect(provisioned).not.toBeNull(); - expect(state.auth.user.email).toEqual(newEmail.toLowerCase()); - expect(state.auth.user.name).toEqual("New User"); - }); - - it("should use email prefix as name when X-Auth-Request-User is absent", async () => { - await buildTeam(); - const state = {} as DefaultState; - const authMiddleware = auth(); - const newEmail = `prefix-${randomString(6)}@example.com`; - - await authMiddleware( - { - // @ts-expect-error mock request - request: { - get: jest.fn((header: string) => { - if (header === "x-auth-request-email") { - return newEmail; - } - return ""; - }), - }, - // @ts-expect-error mock cookies - cookies: { get: jest.fn(() => undefined), set: jest.fn() }, - state, - ip: "127.0.0.1", - cache: {}, - }, - jest.fn() - ); - expect(state.auth.user.email).toEqual(newEmail.toLowerCase()); expect(state.auth.user.name).toEqual( newEmail.toLowerCase().split("@")[0] @@ -600,6 +568,37 @@ describe("Authentication middleware", () => { } }); + it("should reject a forwarded email with no local part", async () => { + await buildTeam(); + const state = {} as DefaultState; + const authMiddleware = auth(); + + // "@example.com" normalises to "@" with an empty + // local part. User.name enforces min length 1, so we reject up front + // rather than letting provisioning fail with an opaque validation error. + await expect( + authMiddleware( + { + // @ts-expect-error mock request + request: { + get: jest.fn((header: string) => { + if (header === "x-auth-request-email") { + return "@example.com"; + } + return ""; + }), + }, + // @ts-expect-error mock cookies + cookies: { get: jest.fn(() => undefined), set: jest.fn() }, + state, + ip: "127.0.0.1", + cache: {}, + }, + jest.fn() + ) + ).rejects.toThrow("Invalid forwarded email: missing local part"); + }); + it("should not match existing users via SQL LIKE wildcard characters", async () => { const team = await buildTeam(); const existingUser = await buildUser({ teamId: team.id }); diff --git a/server/middlewares/authentication.ts b/server/middlewares/authentication.ts index 4452bca17df1..9e68c502c82b 100644 --- a/server/middlewares/authentication.ts +++ b/server/middlewares/authentication.ts @@ -1,6 +1,7 @@ import { addDays } from "date-fns"; import type { Next } from "koa"; import capitalize from "lodash/capitalize"; +import { AUTH_TYPE_SSO } from "@shared/constants"; import { UserRole } from "@shared/types"; import { slugifyDomain } from "@shared/utils/domains"; import { parseEmail } from "@shared/utils/email"; @@ -29,9 +30,6 @@ import { /** Service identifier used by the ForwardAuth authentication flow. */ export const FORWARDAUTH_SERVICE = "forwardauth"; -/** The {@link env.AUTH_TYPE} value that activates ForwardAuth/SSO mode. */ -const AUTH_TYPE_SSO = "SSO"; - type AuthenticationOptions = { /** Role required to access the route. */ role?: UserRole; @@ -372,9 +370,16 @@ async function validateAuthentication( service = FORWARDAUTH_SERVICE; const email = normalizeProxyEmail(token.slice(4)); - const localPart = email.split("@")[0]; - const displayName = ctx.request.get("x-auth-request-user") || localPart; - const { domain } = parseEmail(email); + const { local: localPart, domain } = parseEmail(email); + + // A malformed forwarded email with no local part (e.g. "@example.com") + // normalises to "@" and yields an empty localPart. + // User.name enforces a min length of 1, so provisioning would otherwise + // fail with an opaque validation error — reject explicitly so the failure + // mode is deterministic. + if (!localPart) { + throw AuthenticationError("Invalid forwarded email: missing local part"); + } // Concurrent-creation race guard. The SPA on first-ever login fires // multiple parallel API requests (docs, team, access tokens, …) with @@ -446,7 +451,7 @@ async function validateAuthentication( }); const created = await User.create( { - name: displayName, + name: localPart, email, teamId: team.id, // First user into a brand-new team becomes admin. diff --git a/server/policies/user.test.ts b/server/policies/user.test.ts index 032bc6980518..e65fce238d74 100644 --- a/server/policies/user.test.ts +++ b/server/policies/user.test.ts @@ -1,4 +1,6 @@ +import { AUTH_TYPE_SSO } from "@shared/constants"; import { TeamPreference, EmailDisplay, UserRole } from "@shared/types"; +import env from "@server/env"; import { buildUser, buildTeam, @@ -8,6 +10,51 @@ import { import { serialize } from "./index"; describe("policies/user", () => { + describe("delete", () => { + it("should allow users to delete their own account by default", async () => { + const user = await buildUser(); + const abilities = serialize(user, user); + expect(abilities.delete).toBeTruthy(); + }); + + describe("when AUTH_TYPE is SSO", () => { + let originalAuthType: string | undefined; + + beforeEach(() => { + originalAuthType = env.AUTH_TYPE; + env.AUTH_TYPE = AUTH_TYPE_SSO; + }); + + afterEach(() => { + env.AUTH_TYPE = originalAuthType; + }); + + it("should not allow users to delete their own account", async () => { + const user = await buildUser(); + const abilities = serialize(user, user); + expect(abilities.delete).toBeFalsy(); + }); + + it("should not allow admins to delete their own account", async () => { + const admin = await buildAdmin(); + await buildUser({ + teamId: admin.teamId, + }); + const abilities = serialize(admin, admin); + expect(abilities.delete).toBeFalsy(); + }); + + it("should still allow admins to delete other users", async () => { + const admin = await buildAdmin(); + const user = await buildUser({ + teamId: admin.teamId, + }); + const abilities = serialize(admin, user); + expect(abilities.delete).toBeTruthy(); + }); + }); + }); + describe("readEmail", () => { it("should allow user to read their own email", async () => { const team = await buildTeam(); diff --git a/server/policies/user.ts b/server/policies/user.ts index 170507945505..0290836b3cf4 100644 --- a/server/policies/user.ts +++ b/server/policies/user.ts @@ -1,4 +1,6 @@ +import { AUTH_TYPE_SSO } from "@shared/constants"; import { TeamPreference, EmailDisplay } from "@shared/types"; +import env from "@server/env"; import { User, Team } from "@server/models"; import { allow } from "./cancan"; import { @@ -63,15 +65,19 @@ allow(User, "readEmail", User, (actor, user) => { ); }); -allow(User, "delete", User, (actor, user) => - or( +allow(User, "delete", User, (actor, user) => { + if (env.AUTH_TYPE === AUTH_TYPE_SSO && actor.id === user?.id) { + return false; + } + + return or( isTeamAdmin(actor, user), and( actor.id === user?.id, !!actor.team.getPreference(TeamPreference.MembersCanDeleteAccount) ) - ) -); + ); +}); allow(User, ["activate", "suspend"], User, (actor, user) => and(isTeamAdmin(actor, user), user?.id !== actor.id) diff --git a/server/routes/api/users/users.test.ts b/server/routes/api/users/users.test.ts index f5759a0ad55f..a7f12877b644 100644 --- a/server/routes/api/users/users.test.ts +++ b/server/routes/api/users/users.test.ts @@ -1,5 +1,7 @@ import { faker } from "@faker-js/faker"; +import { AUTH_TYPE_SSO } from "@shared/constants"; import { TeamPreference, UserRole } from "@shared/types"; +import env from "@server/env"; import ConfirmUpdateEmail from "@server/emails/templates/ConfirmUpdateEmail"; import { TeamDomain } from "@server/models"; import { @@ -647,6 +649,71 @@ describe("#users.delete", () => { expect(res.status).toEqual(200); }); + describe("when AUTH_TYPE is SSO", () => { + let originalAuthType: string | undefined; + + beforeEach(() => { + originalAuthType = env.AUTH_TYPE; + env.AUTH_TYPE = AUTH_TYPE_SSO; + }); + + afterEach(() => { + env.AUTH_TYPE = originalAuthType; + }); + + it("should not allow users to delete their own account", async () => { + const user = await buildUser(); + await buildUser({ + teamId: user.teamId, + }); + const res = await server.post("/api/users.delete", { + body: { + code: user.deleteConfirmationCode, + token: user.getJwtToken(), + }, + }); + expect(res.status).toEqual(403); + }); + + it("should not allow admins to delete their own account", async () => { + const admin = await buildAdmin(); + await buildUser({ + teamId: admin.teamId, + }); + const res = await server.post("/api/users.delete", { + body: { + code: admin.deleteConfirmationCode, + token: admin.getJwtToken(), + }, + }); + expect(res.status).toEqual(403); + }); + + it("should not allow users to request their own deletion", async () => { + const user = await buildUser(); + const res = await server.post("/api/users.requestDelete", { + body: { + token: user.getJwtToken(), + }, + }); + expect(res.status).toEqual(403); + }); + + it("should still allow admins to delete other users", async () => { + const admin = await buildAdmin(); + const user = await buildUser({ + teamId: admin.teamId, + }); + const res = await server.post("/api/users.delete", { + body: { + id: user.id, + token: admin.getJwtToken(), + }, + }); + expect(res.status).toEqual(200); + }); + }); + it("should require authentication", async () => { const res = await server.post("/api/users.delete"); const body = await res.json(); diff --git a/shared/constants.ts b/shared/constants.ts index 6439bf0be389..3fc92962380b 100644 --- a/shared/constants.ts +++ b/shared/constants.ts @@ -7,6 +7,9 @@ import { NotificationBadgeType, } from "./types"; +/** The {@link PublicEnv.AUTH_TYPE} value that activates ForwardAuth/SSO mode. */ +export const AUTH_TYPE_SSO = "SSO"; + export const MAX_AVATAR_DISPLAY = 6; export const Pagination = {