From fbd4ded5b43bf3e8f281fcd8beb311ab6a9d85d0 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Wed, 24 Dec 2025 09:09:24 -0500 Subject: [PATCH] feat: Add authentication provider management (#10997) * Gemini first-pass * Prevent post-connect login * stash * stash * Add OIDC logo * Separate security page * test * Update icon * test * ui * Add extra guards for disabling auth provider * refactor * test --- AGENTS.md | 16 +- app/components/Icons/ConnectedIcon.tsx | 19 ++ app/hooks/useSettingsConfig.ts | 13 +- app/models/AuthenticationProvider.ts | 12 ++ app/scenes/Invite.tsx | 2 +- app/scenes/Settings/Authentication.tsx | 193 ++++++++++++++++++ app/scenes/Settings/Security.tsx | 90 +------- .../Settings/components/ConnectedButton.tsx | 21 +- .../Settings/components/IntegrationCard.tsx | 24 +-- app/scenes/Settings/components/Status.tsx | 26 +++ app/stores/AuthenticationProvidersStore.ts | 9 +- package.json | 2 +- plugins/azure/server/auth/azure.ts | 13 +- plugins/discord/server/auth/discord.ts | 13 +- plugins/google/server/auth/google.ts | 13 +- plugins/oidc/client/Icon.tsx | 20 ++ plugins/oidc/client/index.tsx | 11 + plugins/oidc/server/auth/oidcRouter.ts | 13 +- plugins/slack/server/auth/slack.ts | 13 +- server/commands/accountProvisioner.test.ts | 37 ++++ server/commands/accountProvisioner.ts | 28 +++ server/middlewares/authentication.ts | 1 + server/models/AuthenticationProvider.test.ts | 104 ++++++++++ server/models/AuthenticationProvider.ts | 73 +++++-- .../authenticationProviders.test.ts | 60 +++++- .../authenticationProviders.ts | 31 ++- .../api/authenticationProviders/schema.ts | 11 + server/routes/auth/index.ts | 6 +- server/utils/MutexLock.ts | 6 +- server/utils/passport.ts | 92 ++++++++- shared/i18n/locales/en_US/translation.json | 23 ++- yarn.lock | 8 +- 32 files changed, 807 insertions(+), 196 deletions(-) create mode 100644 app/components/Icons/ConnectedIcon.tsx create mode 100644 app/scenes/Settings/Authentication.tsx create mode 100644 app/scenes/Settings/components/Status.tsx create mode 100644 plugins/oidc/client/Icon.tsx create mode 100644 plugins/oidc/client/index.tsx create mode 100644 server/models/AuthenticationProvider.test.ts diff --git a/AGENTS.md b/AGENTS.md index 210c6b8fa2..bad40846e8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,16 +142,16 @@ yarn sequelize migration:create --name=add-field-to-table - Run tests with Jest: ```bash -# Run all tests +# Run a specific test file (preferred) +yarn test path/to/test.spec.ts + +# Run every test (avoid) yarn test -# Run specific test suites -yarn test:app # Frontend tests -yarn test:server # Backend tests -yarn test:shared # Shared code tests - -# Run specific test file -yarn test path/to/test.spec.ts +# Run test suites (avoid) +yarn test:app # All frontend tests +yarn test:server # All backend tests +yarn test:shared # All shared code tests ``` - Write unit tests for utilities and business logic in a collocated .test.ts file. diff --git a/app/components/Icons/ConnectedIcon.tsx b/app/components/Icons/ConnectedIcon.tsx new file mode 100644 index 0000000000..8baa10ac4e --- /dev/null +++ b/app/components/Icons/ConnectedIcon.tsx @@ -0,0 +1,19 @@ +import styled from "styled-components"; + +export const ConnectedIcon = styled.div<{ color?: string }>` + width: 24px; + height: 24px; + position: relative; + + &::after { + content: ""; + position: absolute; + top: 50%; + left: 50%; + width: 8px; + height: 8px; + background-color: ${(props) => props.color ?? props.theme.accent}; + border-radius: 50%; + transform: translate(-50%, -50%); + } +`; diff --git a/app/hooks/useSettingsConfig.ts b/app/hooks/useSettingsConfig.ts index ebc3ff6b97..7402ab5252 100644 --- a/app/hooks/useSettingsConfig.ts +++ b/app/hooks/useSettingsConfig.ts @@ -7,6 +7,7 @@ import { UserIcon, GroupIcon, GlobeIcon, + ShieldIcon, TeamIcon, BeakerIcon, SettingsIcon, @@ -33,6 +34,7 @@ import useStores from "./useStores"; const ApiKeys = lazy(() => import("~/scenes/Settings/ApiKeys")); const Applications = lazy(() => import("~/scenes/Settings/Applications")); const APIAndApps = lazy(() => import("~/scenes/Settings/APIAndApps")); +const Authentication = lazy(() => import("~/scenes/Settings/Authentication")); const Details = lazy(() => import("~/scenes/Settings/Details")); const Export = lazy(() => import("~/scenes/Settings/Export")); const Features = lazy(() => import("~/scenes/Settings/Features")); @@ -120,6 +122,15 @@ const useSettingsConfig = () => { group: t("Workspace"), icon: TeamIcon, }, + { + name: t("Authentication"), + path: settingsPath("authentication"), + component: Authentication.Component, + preload: Authentication.preload, + enabled: can.update, + group: t("Workspace"), + icon: PadlockIcon, + }, { name: t("Security"), path: settingsPath("security"), @@ -127,7 +138,7 @@ const useSettingsConfig = () => { preload: Security.preload, enabled: can.update, group: t("Workspace"), - icon: PadlockIcon, + icon: ShieldIcon, }, { name: t("Features"), diff --git a/app/models/AuthenticationProvider.ts b/app/models/AuthenticationProvider.ts index 34418022bc..aad3aad92c 100644 --- a/app/models/AuthenticationProvider.ts +++ b/app/models/AuthenticationProvider.ts @@ -1,6 +1,8 @@ import { computed, observable } from "mobx"; import Model from "./base/Model"; import Field from "./decorators/Field"; +import { AfterDelete } from "./decorators/Lifecycle"; +import type AuthenticationProvidersStore from "~/stores/AuthenticationProvidersStore"; class AuthenticationProvider extends Model { static modelName = "AuthenticationProvider"; @@ -20,6 +22,16 @@ class AuthenticationProvider extends Model { get isActive() { return this.isEnabled && this.isConnected; } + + @AfterDelete + static afterDelete(model: AuthenticationProvider) { + // Restore a placeholder record to allow re-connection + return (model.store as AuthenticationProvidersStore).add({ + ...model, + isEnabled: false, + isConnected: false, + }); + } } export default AuthenticationProvider; diff --git a/app/scenes/Invite.tsx b/app/scenes/Invite.tsx index 416da44581..b6a3885187 100644 --- a/app/scenes/Invite.tsx +++ b/app/scenes/Invite.tsx @@ -188,7 +188,7 @@ function Invite({ onSubmit }: Props) { {can.update && ( As an admin you can also{" "} - enable email sign-in. + enable email sign-in. )} diff --git a/app/scenes/Settings/Authentication.tsx b/app/scenes/Settings/Authentication.tsx new file mode 100644 index 0000000000..879a58fe88 --- /dev/null +++ b/app/scenes/Settings/Authentication.tsx @@ -0,0 +1,193 @@ +import { observer } from "mobx-react"; +import { EmailIcon, PadlockIcon } from "outline-icons"; +import * as React from "react"; +import { useTranslation, Trans } from "react-i18next"; +import { toast } from "sonner"; +import ConfirmationDialog from "~/components/ConfirmationDialog"; +import Flex from "~/components/Flex"; +import Heading from "~/components/Heading"; +import type AuthenticationProvider from "~/models/AuthenticationProvider"; +import PluginIcon from "~/components/PluginIcon"; +import Scene from "~/components/Scene"; +import Switch from "~/components/Switch"; +import Text from "~/components/Text"; +import env from "~/env"; +import useCurrentTeam from "~/hooks/useCurrentTeam"; +import useRequest from "~/hooks/useRequest"; +import useStores from "~/hooks/useStores"; +import SettingRow from "./components/SettingRow"; +import { setPostLoginPath } from "~/hooks/useLastVisitedPath"; +import { settingsPath } from "~/utils/routeHelpers"; +import DomainManagement from "./components/DomainManagement"; +import Button from "~/components/Button"; +import { ConnectedIcon } from "~/components/Icons/ConnectedIcon"; +import { useTheme } from "styled-components"; + +function Authentication() { + const { authenticationProviders, dialogs } = useStores(); + const team = useCurrentTeam(); + const { t } = useTranslation(); + const theme = useTheme(); + + const { + data: providers, + loading, + request, + } = useRequest(authenticationProviders.fetchPage); + + React.useEffect(() => { + if (!providers && !loading) { + void request(); + } + }, [loading, providers, request]); + + const handleGuestSigninChange = React.useCallback( + async (checked: boolean) => { + try { + await team.save({ guestSignin: checked }); + toast.success(t("Settings saved")); + } catch (err) { + toast.error(err.message); + } + }, + [team, t] + ); + + const handleToggleProvider = React.useCallback( + async (provider: AuthenticationProvider, isEnabled: boolean) => { + try { + await provider.save({ isEnabled }); + toast.success(t("Settings saved")); + } catch (err) { + toast.error(err.message); + } + }, + [t] + ); + + const handleRemoveProvider = React.useCallback( + async (provider: AuthenticationProvider) => { + dialogs.openModal({ + title: t("Are you sure?"), + content: ( + { + await provider.delete(); + toast.success(t("Settings saved")); + }} + savingText={`${t("Removing")}…`} + danger + > + {t( + "Removing this authentication provider will prevent members from signing in with {{ authProvider }}.", + { + authProvider: provider.displayName, + } + )} + + ), + }); + }, + [dialogs, t] + ); + + const handleConnectProvider = React.useCallback((name: string) => { + setPostLoginPath(settingsPath("authentication")); + window.location.href = `/auth/${name}?host=${window.location.host}`; + }, []); + + const showSuccessMessage = React.useMemo( + () => () => toast.success(t("Settings saved")), + [t] + ); + + return ( + }> + {t("Authentication")} + + + Manage how members sign-in to your workspace and which authentication + providers are enabled. + + + + {t("Sign In")} + + {authenticationProviders.orderedData.map((provider) => ( + + {provider.displayName} + + } + name={provider.name} + description={ + provider.isConnected + ? t("Allow members to sign-in with {{ authProvider }}", { + authProvider: provider.displayName, + }) + : t("Connect {{ authProvider }} to allow members to sign-in", { + authProvider: provider.displayName, + }) + } + > + + {provider.isConnected ? ( + + ) : ( + + )} + + + ))} + + {t("Email")} + + } + name="guestSignin" + description={ + env.EMAIL_ENABLED + ? t("Allow members to sign-in using their email address") + : t("The server must have SMTP configured to enable this setting") + } + border={false} + > + + + + {t("Restrictions")} + + + ); +} + +export default observer(Authentication); diff --git a/app/scenes/Settings/Security.tsx b/app/scenes/Settings/Security.tsx index 3380f26597..75a7b00478 100644 --- a/app/scenes/Settings/Security.tsx +++ b/app/scenes/Settings/Security.tsx @@ -1,50 +1,37 @@ import debounce from "lodash/debounce"; import { observer } from "mobx-react"; -import { CheckboxIcon, EmailIcon, PadlockIcon } from "outline-icons"; +import { ShieldIcon } from "outline-icons"; import { useState } from "react"; import * as React from "react"; import { useTranslation, Trans } from "react-i18next"; import { toast } from "sonner"; -import { useTheme } from "styled-components"; import { TeamPreference } from "@shared/types"; import ConfirmationDialog from "~/components/ConfirmationDialog"; -import Flex from "~/components/Flex"; import Heading from "~/components/Heading"; import type { Option } from "~/components/InputSelect"; import { InputSelect } from "~/components/InputSelect"; -import PluginIcon from "~/components/PluginIcon"; import Scene from "~/components/Scene"; import Switch from "~/components/Switch"; import Text from "~/components/Text"; -import env from "~/env"; import useCurrentTeam from "~/hooks/useCurrentTeam"; -import useRequest from "~/hooks/useRequest"; import useStores from "~/hooks/useStores"; import isCloudHosted from "~/utils/isCloudHosted"; -import DomainManagement from "./components/DomainManagement"; import SettingRow from "./components/SettingRow"; function Security() { - const { authenticationProviders, dialogs } = useStores(); + const { dialogs } = useStores(); const team = useCurrentTeam(); const { t } = useTranslation(); - const theme = useTheme(); + const [data, setData] = useState({ sharing: team.sharing, documentEmbeds: team.documentEmbeds, - guestSignin: team.guestSignin, defaultUserRole: team.defaultUserRole, memberCollectionCreate: team.memberCollectionCreate, memberTeamCreate: team.memberTeamCreate, inviteRequired: team.inviteRequired, }); - const { - data: providers, - loading, - request, - } = useRequest(authenticationProviders.fetchPage); - const userRoleOptions: Option[] = React.useMemo( () => [ @@ -62,12 +49,6 @@ function Security() { [t] ); - React.useEffect(() => { - if (!providers && !loading) { - void request(); - } - }, [loading, providers, request]); - const showSuccessMessage = React.useMemo( () => debounce(() => { @@ -96,13 +77,6 @@ function Security() { [saveData] ); - const handleGuestSigninChange = React.useCallback( - async (checked: boolean) => { - await saveData({ guestSignin: checked }); - }, - [saveData] - ); - const handleSharingChange = React.useCallback( async (checked: boolean) => { await saveData({ sharing: checked }); @@ -200,7 +174,7 @@ function Security() { ); return ( - }> + }> {t("Security")} @@ -209,57 +183,7 @@ function Security() { -

{t("Sign In")}

- {authenticationProviders.orderedData - // filtering unconnected, until we have ability to connect from this screen - .filter((provider) => provider.isConnected) - .map((provider) => ( - - {provider.displayName} - - } - name={provider.name} - description={t("Allow members to sign-in with {{ authProvider }}", { - authProvider: provider.displayName, - })} - > - - {" "} - - {provider.isActive ? t("Connected") : t("Disabled")} - - - - ))} - - {t("Email")} - - } - name="guestSignin" - description={ - env.EMAIL_ENABLED - ? t("Allow members to sign-in using their email address") - : t("The server must have SMTP configured to enable this setting") - } - border={false} - > - - - -

{t("Access")}

+ {t("Invites")} )} - - -

{t("Behavior")}

+ {t("Behavior")} ); } - -const ConnectedIcon = styled.div` - width: 24px; - height: 24px; - position: relative; - - &::after { - content: ""; - position: absolute; - top: 50%; - left: 50%; - width: 8px; - height: 8px; - background-color: ${s("accent")}; - border-radius: 50%; - transform: translate(-50%, -50%); - } -`; diff --git a/app/scenes/Settings/components/IntegrationCard.tsx b/app/scenes/Settings/components/IntegrationCard.tsx index c75f3775bf..87570df09a 100644 --- a/app/scenes/Settings/components/IntegrationCard.tsx +++ b/app/scenes/Settings/components/IntegrationCard.tsx @@ -6,6 +6,7 @@ import type { ConfigItem } from "~/hooks/useSettingsConfig"; import Button from "../../../components/Button"; import Flex from "../../../components/Flex"; import Text from "../../../components/Text"; +import { Status } from "./Status"; type Props = { integration: ConfigItem; @@ -69,26 +70,3 @@ const Description = styled(Text)` max-width: 100%; color: ${s("textTertiary")}; `; - -const Status = styled(Text).attrs({ - type: "secondary", - size: "small", - as: "span", -})` - display: inline-flex; - align-items: center; - - &::after { - content: ""; - display: inline-block; - width: 17px; - height: 17px; - - background: radial-gradient( - circle at center, - ${s("accent")} 0 33%, - transparent 33% - ); - border-radius: 50%; - } -`; diff --git a/app/scenes/Settings/components/Status.tsx b/app/scenes/Settings/components/Status.tsx new file mode 100644 index 0000000000..05c76fcad0 --- /dev/null +++ b/app/scenes/Settings/components/Status.tsx @@ -0,0 +1,26 @@ +import Text from "@shared/components/Text"; +import { s } from "@shared/styles"; +import styled from "styled-components"; + +export const Status = styled(Text).attrs({ + type: "secondary", + size: "small", + as: "span", +})` + display: inline-flex; + align-items: center; + + &::after { + content: ""; + display: inline-block; + width: 17px; + height: 17px; + + background: radial-gradient( + circle at center, + ${s("accent")} 0 33%, + transparent 33% + ); + border-radius: 50%; + } +`; diff --git a/app/stores/AuthenticationProvidersStore.ts b/app/stores/AuthenticationProvidersStore.ts index 20e3e83915..fc5a051bbe 100644 --- a/app/stores/AuthenticationProvidersStore.ts +++ b/app/stores/AuthenticationProvidersStore.ts @@ -1,11 +1,18 @@ +import orderBy from "lodash/orderBy"; +import { computed } from "mobx"; import AuthenticationProvider from "~/models/AuthenticationProvider"; import type RootStore from "./RootStore"; import Store, { RPCAction } from "./base/Store"; export default class AuthenticationProvidersStore extends Store { - actions = [RPCAction.List, RPCAction.Update]; + actions = [RPCAction.List, RPCAction.Update, RPCAction.Delete]; constructor(rootStore: RootStore) { super(rootStore, AuthenticationProvider); } + + @computed + get orderedData(): AuthenticationProvider[] { + return orderBy(Array.from(this.data.values()), ["desc", "asc"]); + } } diff --git a/package.json b/package.json index ce7b5a518d..4cf9db9194 100644 --- a/package.json +++ b/package.json @@ -182,7 +182,7 @@ "node-fetch": "2.7.0", "nodemailer": "^7.0.11", "octokit": "^3.2.2", - "outline-icons": "^3.16.0", + "outline-icons": "^3.17.0", "oy-vey": "^0.12.1", "pako": "^2.1.0", "passport": "^0.7.0", diff --git a/plugins/azure/server/auth/azure.ts b/plugins/azure/server/auth/azure.ts index a2e06b2c5b..1ab4ee0f4c 100644 --- a/plugins/azure/server/auth/azure.ts +++ b/plugins/azure/server/auth/azure.ts @@ -15,7 +15,8 @@ import { StateStore, request, getTeamFromContext, - getClientFromContext, + getClientFromOAuthState, + getUserFromOAuthState, } from "@server/utils/passport"; import config from "../../plugin.json"; import env from "../env"; @@ -96,13 +97,19 @@ if (env.AZURE_CLIENT_ID && env.AZURE_CLIENT_SECRET) { } const team = await getTeamFromContext(context); - const client = getClientFromContext(context); + const client = getClientFromOAuthState(context); + const user = + context.state?.auth?.user ?? (await getUserFromOAuthState(context)); const domain = parseEmail(email).domain; const subdomain = slugifyDomain(domain); const teamName = organization.displayName; - const ctx = createContext({ ip: context.ip }); + const ctx = createContext({ + ip: context.ip, + user, + authType: context.state?.auth?.type, + }); const result = await accountProvisioner(ctx, { team: { teamId: team?.id, diff --git a/plugins/discord/server/auth/discord.ts b/plugins/discord/server/auth/discord.ts index 5cf1616150..c91eae8787 100644 --- a/plugins/discord/server/auth/discord.ts +++ b/plugins/discord/server/auth/discord.ts @@ -21,7 +21,8 @@ import type { AuthenticationResult } from "@server/types"; import { StateStore, getTeamFromContext, - getClientFromContext, + getClientFromOAuthState, + getUserFromOAuthState, request, } from "@server/utils/passport"; import config from "../../plugin.json"; @@ -68,7 +69,7 @@ if (env.DISCORD_CLIENT_ID && env.DISCORD_CLIENT_SECRET) { ) { try { const team = await getTeamFromContext(context); - const client = getClientFromContext(context); + const client = getClientFromOAuthState(context); /** Fetch the user's profile */ const profile: RESTGetAPICurrentUserResult = await request( "GET", @@ -177,11 +178,17 @@ if (env.DISCORD_CLIENT_ID && env.DISCORD_CLIENT_SECRET) { } } } + const user = + context.state?.auth?.user ?? (await getUserFromOAuthState(context)); // if a team can be inferred, we assume the user is only interested in signing into // that team in particular; otherwise, we will do a best effort at finding their account // or provisioning a new one (within AccountProvisioner) - const ctx = createContext({ ip: context.ip }); + const ctx = createContext({ + ip: context.ip, + user, + authType: context.state?.auth?.type, + }); const result = await accountProvisioner(ctx, { team: { teamId: team?.id, diff --git a/plugins/google/server/auth/google.ts b/plugins/google/server/auth/google.ts index 81cf9ebe4e..2d55b27328 100644 --- a/plugins/google/server/auth/google.ts +++ b/plugins/google/server/auth/google.ts @@ -17,7 +17,8 @@ import type { AuthenticationResult } from "@server/types"; import { StateStore, getTeamFromContext, - getClientFromContext, + getClientFromOAuthState, + getUserFromOAuthState, } from "@server/utils/passport"; import config from "../../plugin.json"; import env from "../env"; @@ -67,7 +68,9 @@ if (env.GOOGLE_CLIENT_ID && env.GOOGLE_CLIENT_SECRET) { // "domain" is the Google Workspaces domain const domain = profile._json.hd; const team = await getTeamFromContext(context); - const client = getClientFromContext(context); + const client = getClientFromOAuthState(context); + const user = + context.state?.auth?.user ?? (await getUserFromOAuthState(context)); // No profile domain means personal gmail account // No team implies the request came from the apex domain @@ -109,7 +112,11 @@ if (env.GOOGLE_CLIENT_ID && env.GOOGLE_CLIENT_SECRET) { // if a team can be inferred, we assume the user is only interested in signing into // that team in particular; otherwise, we will do a best effort at finding their account // or provisioning a new one (within AccountProvisioner) - const ctx = createContext({ ip: context.ip }); + const ctx = createContext({ + ip: context.ip, + user, + authType: context.state?.auth?.type, + }); const result = await accountProvisioner(ctx, { team: { teamId: team?.id, diff --git a/plugins/oidc/client/Icon.tsx b/plugins/oidc/client/Icon.tsx new file mode 100644 index 0000000000..ee3b41df22 --- /dev/null +++ b/plugins/oidc/client/Icon.tsx @@ -0,0 +1,20 @@ +type Props = { + /** The size of the icon, 24px is default to match standard icons */ + size?: number; + /** The color of the icon, defaults to the current text color */ + color?: string; +}; + +export default function OIDCIcon({ size = 24, color = "currentColor" }: Props) { + return ( + + + + ); +} diff --git a/plugins/oidc/client/index.tsx b/plugins/oidc/client/index.tsx new file mode 100644 index 0000000000..b3dbea4a24 --- /dev/null +++ b/plugins/oidc/client/index.tsx @@ -0,0 +1,11 @@ +import { Hook, PluginManager } from "~/utils/PluginManager"; +import config from "../plugin.json"; +import Icon from "./Icon"; + +PluginManager.add([ + { + ...config, + type: Hook.Icon, + value: Icon, + }, +]); diff --git a/plugins/oidc/server/auth/oidcRouter.ts b/plugins/oidc/server/auth/oidcRouter.ts index 765e162820..76b278b570 100644 --- a/plugins/oidc/server/auth/oidcRouter.ts +++ b/plugins/oidc/server/auth/oidcRouter.ts @@ -19,7 +19,8 @@ import type { AuthenticationResult } from "@server/types"; import { StateStore, getTeamFromContext, - getClientFromContext, + getClientFromOAuthState, + getUserFromOAuthState, request, } from "@server/utils/passport"; import config from "../../plugin.json"; @@ -121,7 +122,9 @@ export function createOIDCRouter( } const team = await getTeamFromContext(context); - const client = getClientFromContext(context); + const client = getClientFromOAuthState(context); + const user = + context.state?.auth?.user ?? (await getUserFromOAuthState(context)); const { domain } = parseEmail(email); // Only a single OIDC provider is supported – find the existing, if any. @@ -187,7 +190,11 @@ export function createOIDCRouter( avatarUrl = null; } - const ctx = createContext({ ip: context.ip }); + const ctx = createContext({ + ip: context.ip, + user, + authType: context.state?.auth?.type, + }); const result = await accountProvisioner(ctx, { team: { teamId: team?.id, diff --git a/plugins/slack/server/auth/slack.ts b/plugins/slack/server/auth/slack.ts index 8ed3f5c817..f7ce02569a 100644 --- a/plugins/slack/server/auth/slack.ts +++ b/plugins/slack/server/auth/slack.ts @@ -20,8 +20,9 @@ import { authorize } from "@server/policies"; import { sequelize } from "@server/storage/database"; import type { APIContext, AuthenticationResult } from "@server/types"; import { - getClientFromContext, + getClientFromOAuthState, getTeamFromContext, + getUserFromOAuthState, StateStore, } from "@server/utils/passport"; import { parseEmail } from "@shared/utils/email"; @@ -82,11 +83,17 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { ) { try { const team = await getTeamFromContext(context); - const client = getClientFromContext(context); + const client = getClientFromOAuthState(context); + const user = + context.state?.auth?.user ?? (await getUserFromOAuthState(context)); const { domain } = parseEmail(profile.user.email); - const ctx = createContext({ ip: context.ip }); + const ctx = createContext({ + ip: context.ip, + user, + authType: context.state?.auth?.type, + }); const result = await accountProvisioner(ctx, { team: { teamId: team?.id, diff --git a/server/commands/accountProvisioner.test.ts b/server/commands/accountProvisioner.test.ts index 0a11d0d2f6..b9cbcab2f7 100644 --- a/server/commands/accountProvisioner.test.ts +++ b/server/commands/accountProvisioner.test.ts @@ -431,6 +431,43 @@ describe("accountProvisioner", () => { spy.mockRestore(); }); + + it("should allow connecting a new authentication provider while logged in", async () => { + const admin = await buildAdmin(); + const team = admin.team; + const ctxWithAdmin = createContext({ ip, user: admin }); + + const providerId = faker.internet.domainName(); + const { user, isNewTeam, isNewUser } = await accountProvisioner( + ctxWithAdmin, + { + user: { + name: admin.name, + email: admin.email!, + }, + team: { + teamId: team.id, + subdomain: team.subdomain!, + }, + authenticationProvider: { + name: "google", + providerId, + }, + authentication: { + providerId: randomUUID(), + accessToken: "456", + scopes: ["read"], + }, + } + ); + + expect(user.id).toEqual(admin.id); + expect(isNewUser).toEqual(false); + expect(isNewTeam).toEqual(false); + + const providers = await team.$get("authenticationProviders"); + expect(providers.find((p) => p.name === "google")).toBeTruthy(); + }); }); describe("self hosted", () => { diff --git a/server/commands/accountProvisioner.ts b/server/commands/accountProvisioner.ts index b9585e5924..36bda9f17b 100644 --- a/server/commands/accountProvisioner.ts +++ b/server/commands/accountProvisioner.ts @@ -93,6 +93,34 @@ async function accountProvisioner( let result; let emailMatchOnly; + const actor = ctx.state.auth?.user; + + // If the user is already logged in and is an admin of the team then we + // allow them to connect a new authentication provider + if (actor && actor.teamId === teamParams.teamId && actor.isAdmin) { + const team = actor.team; + let authenticationProvider = await AuthenticationProvider.findOne({ + where: { + ...authenticationProviderParams, + teamId: team.id, + }, + }); + + if (!authenticationProvider) { + authenticationProvider = await team.$create( + "authenticationProvider", + authenticationProviderParams + ); + } + + return { + user: actor, + team, + isNewUser: false, + isNewTeam: false, + }; + } + try { result = await teamProvisioner(ctx, { ...teamParams, diff --git a/server/middlewares/authentication.ts b/server/middlewares/authentication.ts index f802cebd8e..0da8bb1963 100644 --- a/server/middlewares/authentication.ts +++ b/server/middlewares/authentication.ts @@ -69,6 +69,7 @@ export default function auth(options: AuthenticationOptions = {}) { } Object.defineProperty(ctx, "context", { + configurable: true, get() { return { auth: ctx.state.auth, diff --git a/server/models/AuthenticationProvider.test.ts b/server/models/AuthenticationProvider.test.ts new file mode 100644 index 0000000000..57c0e809c3 --- /dev/null +++ b/server/models/AuthenticationProvider.test.ts @@ -0,0 +1,104 @@ +import { buildTeam } from "@server/test/factories"; +import { AuthenticationProvider } from "@server/models"; +import { createContext } from "@server/context"; + +describe("AuthenticationProvider", () => { + describe("checkCanBeDisabled", () => { + it("should allow disabling if email sign-in is enabled", async () => { + const team = await buildTeam({ + guestSignin: true, + }); + const provider = await AuthenticationProvider.create({ + name: "google", + providerId: "google-id", + teamId: team.id, + enabled: true, + }); + + const ctx = createContext({ user: { team } as any }); + await expect(provider.disable(ctx)).resolves.not.toThrow(); + expect(provider.enabled).toBe(false); + }); + + it("should allow disabling if another provider is enabled", async () => { + const team = await buildTeam({ + guestSignin: false, + }); + const provider1 = await AuthenticationProvider.create({ + name: "google", + providerId: "google-id", + teamId: team.id, + enabled: true, + }); + await AuthenticationProvider.create({ + name: "slack", + providerId: "slack-id", + teamId: team.id, + enabled: true, + }); + + const ctx = createContext({ user: { team } as any }); + await expect(provider1.disable(ctx)).resolves.not.toThrow(); + expect(provider1.enabled).toBe(false); + }); + + it("should prevent disabling if it is the last enabled method", async () => { + const team = await buildTeam({ + guestSignin: false, + }); + // buildTeam creates a default 'slack' provider, let's disable it first + await AuthenticationProvider.update( + { enabled: false }, + { where: { teamId: team.id } } + ); + + const provider = await AuthenticationProvider.create({ + name: "google", + providerId: "google-id", + teamId: team.id, + enabled: true, + }); + + const ctx = createContext({ user: { team } as any }); + await expect(provider.disable(ctx)).rejects.toThrow( + "At least one authentication provider is required" + ); + }); + + it("should prevent destruction if it is the last enabled method", async () => { + const team = await buildTeam({ + guestSignin: false, + }); + // Disable existing default providers + await AuthenticationProvider.update( + { enabled: false }, + { where: { teamId: team.id } } + ); + + const provider = await AuthenticationProvider.create({ + name: "google", + providerId: "google-id", + teamId: team.id, + enabled: true, + }); + + await expect(provider.destroy()).rejects.toThrow( + "At least one authentication provider is required" + ); + }); + + it("should allow destruction if it is not enabled", async () => { + const team = await buildTeam({ + guestSignin: false, + }); + const provider = await AuthenticationProvider.create({ + name: "google", + providerId: "google-id", + teamId: team.id, + enabled: false, + }); + + await expect(provider.destroy()).resolves.not.toThrow(); + }); + }); +}); diff --git a/server/models/AuthenticationProvider.ts b/server/models/AuthenticationProvider.ts index 2a565900e0..96a8d993d2 100644 --- a/server/models/AuthenticationProvider.ts +++ b/server/models/AuthenticationProvider.ts @@ -1,6 +1,11 @@ -import type { InferAttributes, InferCreationAttributes } from "sequelize"; +import type { + InferAttributes, + InferCreationAttributes, + Transaction, +} from "sequelize"; import { Op } from "sequelize"; import { + BeforeDestroy, BelongsTo, Column, CreatedAt, @@ -25,6 +30,7 @@ import AzureClient from "plugins/azure/server/azure"; import GoogleClient from "plugins/google/server/google"; import OIDCClient from "plugins/oidc/server/oidc"; import type { APIContext } from "@server/types"; +import type { DestroyOptions } from "sequelize"; @Scopes(() => ({ withUserAuthentication: (userId: string) => ({ @@ -109,19 +115,30 @@ class AuthenticationProvider extends Model< } } - disable: (ctx: APIContext) => Promise = async ( - ctx - ) => { - const { transaction } = ctx.state; - if (!transaction) { - throw new Error("Transaction required"); + /** + * Check if this provider can be disabled or destroyed. + * Throws an error if this is the last enabled authentication provider. + * + * @param transaction - Database transaction to use for the check. + * @throws ValidationError if disabling is not allowed. + */ + private async checkCanBeDisabled( + transaction?: Transaction | null + ): Promise { + // Check if email sign-in is enabled for the team first + const team = await Team.findByPk(this.teamId, { + transaction, + lock: transaction?.LOCK.SHARE, + }); + if (team?.emailSigninEnabled) { + return; } const otherEnabledProviders = await ( this.constructor as typeof AuthenticationProvider ).findAll({ transaction, - lock: transaction.LOCK.SHARE, + lock: transaction?.LOCK.SHARE, where: { teamId: this.teamId, enabled: true, @@ -132,15 +149,45 @@ class AuthenticationProvider extends Model< limit: 1, }); - if (otherEnabledProviders.length >= 1) { - return this.updateWithCtx(ctx, { - enabled: false, - }); - } else { + if (otherEnabledProviders.length === 0) { throw ValidationError("At least one authentication provider is required"); } + } + + @BeforeDestroy + static async checkBeforeDestroy( + instance: AuthenticationProvider, + options: DestroyOptions + ) { + if (instance.enabled) { + await instance.checkCanBeDisabled(options.transaction); + } + } + + /** + * Disable this authentication provider after ensuring it's allowed. + * + * @param ctx - API context containing the transaction. + * @returns The updated AuthenticationProvider instance. + * @throws ValidationError if disabling is not allowed. + */ + disable: (ctx: APIContext) => Promise = async ( + ctx + ) => { + const { transaction } = ctx.state; + await this.checkCanBeDisabled(transaction); + + return this.updateWithCtx(ctx, { + enabled: false, + }); }; + /** + * Enable this authentication provider. + * + * @param ctx - API context containing the transaction. + * @returns The updated AuthenticationProvider instance. + */ enable: (ctx: APIContext) => Promise = async (ctx) => this.updateWithCtx(ctx, { enabled: true, diff --git a/server/routes/api/authenticationProviders/authenticationProviders.test.ts b/server/routes/api/authenticationProviders/authenticationProviders.test.ts index 7561c66d18..1fb671f90d 100644 --- a/server/routes/api/authenticationProviders/authenticationProviders.test.ts +++ b/server/routes/api/authenticationProviders/authenticationProviders.test.ts @@ -55,7 +55,9 @@ describe("#authenticationProviders.info", () => { describe("#authenticationProviders.update", () => { it("should not allow admins to disable when last authentication provider", async () => { - const team = await buildTeam(); + const team = await buildTeam({ + guestSignin: false, + }); const user = await buildAdmin({ teamId: team.id, }); @@ -150,3 +152,59 @@ describe("#authenticationProviders.list", () => { expect(res.status).toEqual(401); }); }); + +describe("#authenticationProviders.delete", () => { + it("should allow admins to delete authentication provider", async () => { + const team = await buildTeam(); + const user = await buildAdmin({ + teamId: team.id, + }); + const googleProvider = await team.$create("authenticationProvider", { + name: "google", + providerId: randomUUID(), + }); + const res = await server.post("/api/authenticationProviders.delete", { + body: { + id: googleProvider.id, + token: user.getJwtToken(), + }, + }); + expect(res.status).toEqual(200); + const count = await team.$count("authenticationProviders", { + where: { + id: googleProvider.id, + }, + }); + expect(count).toBe(0); + }); + + it("should require authorization", async () => { + const team = await buildTeam(); + const user = await buildUser(); + const googleProvider = await team.$create("authenticationProvider", { + name: "google", + providerId: randomUUID(), + }); + const res = await server.post("/api/authenticationProviders.delete", { + body: { + id: googleProvider.id, + token: user.getJwtToken(), + }, + }); + expect(res.status).toEqual(403); + }); + + it("should require authentication", async () => { + const team = await buildTeam(); + const googleProvider = await team.$create("authenticationProvider", { + name: "google", + providerId: randomUUID(), + }); + const res = await server.post("/api/authenticationProviders.delete", { + body: { + id: googleProvider.id, + }, + }); + expect(res.status).toEqual(401); + }); +}); diff --git a/server/routes/api/authenticationProviders/authenticationProviders.ts b/server/routes/api/authenticationProviders/authenticationProviders.ts index 897dfde802..312f8ab88f 100644 --- a/server/routes/api/authenticationProviders/authenticationProviders.ts +++ b/server/routes/api/authenticationProviders/authenticationProviders.ts @@ -64,6 +64,35 @@ router.post( } ); +router.post( + "authenticationProviders.delete", + auth({ role: UserRole.Admin }), + validate(T.AuthenticationProvidersDeleteSchema), + transaction(), + async (ctx: APIContext) => { + const { transaction } = ctx.state; + const { id } = ctx.input.body; + const { user } = ctx.state.auth; + + const authenticationProvider = await AuthenticationProvider.findByPk(id, { + transaction, + lock: transaction.LOCK.UPDATE, + }); + + authorize(user, "delete", authenticationProvider); + + if (authenticationProvider.enabled) { + await authenticationProvider.disable(ctx); + } + + await authenticationProvider.destroy({ transaction }); + + ctx.body = { + success: true, + }; + } +); + router.post( "authenticationProviders.list", auth({ role: UserRole.Admin }), @@ -91,7 +120,7 @@ router.post( ...(row ? presentAuthenticationProvider(row) : {}), }; }) - .sort((a) => (a.isEnabled ? -1 : 1)); + .sort((a, b) => (a.isEnabled === b.isEnabled ? 0 : a.isEnabled ? -1 : 1)); ctx.body = { data, diff --git a/server/routes/api/authenticationProviders/schema.ts b/server/routes/api/authenticationProviders/schema.ts index efebdcd101..78cf5a4800 100644 --- a/server/routes/api/authenticationProviders/schema.ts +++ b/server/routes/api/authenticationProviders/schema.ts @@ -25,3 +25,14 @@ export const AuthenticationProvidersUpdateSchema = BaseSchema.extend({ export type AuthenticationProvidersUpdateReq = z.infer< typeof AuthenticationProvidersUpdateSchema >; + +export const AuthenticationProvidersDeleteSchema = BaseSchema.extend({ + body: z.object({ + /** Authentication Provider Id */ + id: z.string().uuid(), + }), +}); + +export type AuthenticationProvidersDeleteReq = z.infer< + typeof AuthenticationProvidersDeleteSchema +>; diff --git a/server/routes/auth/index.ts b/server/routes/auth/index.ts index 6fe5584443..a58eed70b9 100644 --- a/server/routes/auth/index.ts +++ b/server/routes/auth/index.ts @@ -21,7 +21,11 @@ void (async () => { for (const provider of AuthenticationHelper.providers) { const resolvedRouter = await provider.value.router; if (resolvedRouter) { - router.use("/", resolvedRouter.routes()); + router.use( + "/", + authMiddleware({ optional: true }), + resolvedRouter.routes() + ); } } })(); diff --git a/server/utils/MutexLock.ts b/server/utils/MutexLock.ts index 0e276cb12c..09ca631086 100644 --- a/server/utils/MutexLock.ts +++ b/server/utils/MutexLock.ts @@ -16,9 +16,9 @@ export class MutexLock { */ public static get lock(): Redlock { this.redlock ??= new Redlock([Redis.defaultClient], { - retryJitter: 10, - retryCount: 20, - retryDelay: 200, + retryJitter: 100, + retryCount: 120, + retryDelay: 1000, }); return this.redlock; diff --git a/server/utils/passport.ts b/server/utils/passport.ts index 87f6802214..0243be604d 100644 --- a/server/utils/passport.ts +++ b/server/utils/passport.ts @@ -12,6 +12,7 @@ import env from "@server/env"; import { Team } from "@server/models"; import { InternalError, OAuthStateMismatchError } from "../errors"; import fetch from "./fetch"; +import { getUserForJWT } from "./jwt"; export class StateStore { constructor(private pkce = false) {} @@ -45,7 +46,14 @@ export class StateStore { const clientInput = ctx.query.client?.toString(); const client = clientInput === Client.Desktop ? Client.Desktop : Client.Web; const host = ctx.query.host?.toString() || parseDomain(ctx.hostname).host; - const state = buildState(host, token, client, codeVerifier); + const accessToken = ctx.cookies.get("accessToken"); + const state = buildState({ + host, + token, + client, + codeVerifier, + accessToken, + }); ctx.cookies.set(this.key, state, { expires: addMinutes(new Date(), 10), @@ -115,27 +123,82 @@ export async function request( } } -function buildState( - host: string, - token: string, - client?: Client, - codeVerifier?: string -) { - return [host, token, client, codeVerifier].join("|"); +function buildState({ + host, + token, + client, + codeVerifier, + accessToken, +}: { + host: string; + token: string; + client?: Client; + codeVerifier?: string; + accessToken?: string; +}) { + return [host, token, client, codeVerifier, accessToken].join("|"); } +/** + * Parses the state string into its components. + * + * @param state The state string + * @returns An object containing the parsed components + */ export function parseState(state: string) { - const [host, token, client, rawCodeVerifier] = state.split("|"); + const [host, token, client, rawCodeVerifier, rawAccessToken] = + state.split("|"); const codeVerifier = rawCodeVerifier ? rawCodeVerifier : undefined; - return { host, token, client, codeVerifier }; + const accessToken = rawAccessToken ? rawAccessToken : undefined; + return { host, token, client, codeVerifier, accessToken }; } -export function getClientFromContext(ctx: Context): Client { +/** + * Returns the client type from the context if available. Used to redirect + * the user back to the correct client after the OAuth flow. + * + * @param ctx The Koa context + * @returns The client type, defaults to Client.Web + */ +export function getClientFromOAuthState(ctx: Context): Client { const state = ctx.cookies.get("state"); const client = state ? parseState(state).client : undefined; return client === Client.Desktop ? Client.Desktop : Client.Web; } +/** + * Returns the access token from the context if available. This is used + * to restore the session during the OAuth flow when connecting additional + * providers to an existing team. + * + * @param ctx The Koa context + * @returns The access token if available, otherwise undefined + */ +export function getAccessTokenFromOAuthState(ctx: Context): string | undefined { + const state = ctx.cookies.get("state"); + return state ? parseState(state).accessToken : undefined; +} + +/** + * Returns the user from the context if they are authenticated. This is used + * to restore the session during the OAuth flow. + * + * @param ctx The Koa context + * @returns The user if authenticated, otherwise undefined + */ +export async function getUserFromOAuthState(ctx: Context) { + const token = getAccessTokenFromOAuthState(ctx); + if (!token) { + return undefined; + } + + try { + return await getUserForJWT(token); + } catch (_err) { + return undefined; + } +} + type TeamFromContextOptions = { /** * Whether to consider the state cookie in the context when determining the team. @@ -145,6 +208,13 @@ type TeamFromContextOptions = { includeStateCookie?: boolean; }; +/** + * Infers the team from the context based on the hostname or state cookie. + * + * @param ctx The Koa context + * @param options Options for determining the team + * @returns The inferred team or undefined if not found + */ export async function getTeamFromContext( ctx: Context, options: TeamFromContextOptions = { includeStateCookie: true } diff --git a/shared/i18n/locales/en_US/translation.json b/shared/i18n/locales/en_US/translation.json index ecd20f8464..eed8af8174 100644 --- a/shared/i18n/locales/en_US/translation.json +++ b/shared/i18n/locales/en_US/translation.json @@ -617,6 +617,7 @@ "Account": "Account", "API & Apps": "API & Apps", "Details": "Details", + "Authentication": "Authentication", "Security": "Security", "Features": "Features", "Members": "Members", @@ -1004,6 +1005,19 @@ "Authorization URL": "Authorization URL", "Where users are redirected to authorize this app": "Where users are redirected to authorize this app", "Applications allow you to build internal or public integrations with Outline and provide secure access via OAuth. For more details see the developer documentation.": "Applications allow you to build internal or public integrations with Outline and provide secure access via OAuth. For more details see the developer documentation.", + "Settings saved": "Settings saved", + "Are you sure?": "Are you sure?", + "Removing": "Removing", + "Removing this authentication provider will prevent members from signing in with {{ authProvider }}.": "Removing this authentication provider will prevent members from signing in with {{ authProvider }}.", + "Manage how members sign-in to your workspace and which authentication providers are enabled.": "Manage how members sign-in to your workspace and which authentication providers are enabled.", + "Allow members to sign-in with {{ authProvider }}": "Allow members to sign-in with {{ authProvider }}", + "Connect {{ authProvider }} to allow members to sign-in": "Connect {{ authProvider }} to allow members to sign-in", + "Connected": "Connected", + "Disabled": "Disabled", + "Connect": "Connect", + "Allow members to sign-in using their email address": "Allow members to sign-in using their email address", + "The server must have SMTP configured to enable this setting": "The server must have SMTP configured to enable this setting", + "Restrictions": "Restrictions", "by {{ name }}": "by {{ name }}", "Last used": "Last used", "No expiry": "No expiry", @@ -1012,7 +1026,6 @@ "Copied": "Copied", "Are you sure you want to revoke the {{ tokenName }} token?": "Are you sure you want to revoke the {{ tokenName }} token?", "Disconnect integration": "Disconnect integration", - "Connected": "Connected", "Disconnecting": "Disconnecting", "Are you sure you want to disconnect the {{ service }} integration?": "Are you sure you want to disconnect the {{ service }} integration?", "This will stop sending analytics events to the configured instance.": "This will stop sending analytics events to the configured instance.", @@ -1083,7 +1096,6 @@ "You can import a zip file that was previously exported from an Outline installation – collections, documents, and images will be imported. In Outline, open Export in the Settings sidebar and click on Export Data.": "You can import a zip file that was previously exported from an Outline installation – collections, documents, and images will be imported. In Outline, open Export in the Settings sidebar and click on Export Data.", "Drag and drop the zip file from the Markdown export option in {{appName}}, or click to upload": "Drag and drop the zip file from the Markdown export option in {{appName}}, or click to upload", "Configure": "Configure", - "Connect": "Connect", "Last active": "Last active", "Role": "Role", "Guest": "Guest", @@ -1104,7 +1116,6 @@ "Custom emojis can be used throughout your workspace in documents, comments, and reactions.": "Custom emojis can be used throughout your workspace in documents, comments, and reactions.", "Left": "Left", "Right": "Right", - "Settings saved": "Settings saved", "Logo updated": "Logo updated", "Unable to upload new logo": "Unable to upload new logo", "Delete workspace": "Delete workspace", @@ -1209,11 +1220,7 @@ "Are you sure you want to require invites?": "Are you sure you want to require invites?", "New users will first need to be invited to create an account. Default role and Allowed domains will no longer apply.": "New users will first need to be invited to create an account. Default role and Allowed domains will no longer apply.", "Settings that impact the access, security, and content of your workspace.": "Settings that impact the access, security, and content of your workspace.", - "Allow members to sign-in with {{ authProvider }}": "Allow members to sign-in with {{ authProvider }}", - "Disabled": "Disabled", - "Allow members to sign-in using their email address": "Allow members to sign-in using their email address", - "The server must have SMTP configured to enable this setting": "The server must have SMTP configured to enable this setting", - "Access": "Access", + "Invites": "Invites", "Allow users to send invites": "Allow users to send invites", "Allow editors to invite other people to the workspace": "Allow editors to invite other people to the workspace", "Require invites": "Require invites", diff --git a/yarn.lock b/yarn.lock index ad5866e643..e0f162c871 100644 --- a/yarn.lock +++ b/yarn.lock @@ -12133,10 +12133,10 @@ os-tmpdir@~1.0.2: resolved "https://registry.yarnpkg.com/os-tmpdir/-/os-tmpdir-1.0.2.tgz#bbe67406c79aa85c5cfec766fe5734555dfa1274" integrity "sha1-u+Z0BseaqFxc/sdm/lc0VV36EnQ= sha512-D2FR03Vir7FIu45XBY20mTb+/ZSWB00sjU9jdQXt83gDrI4Ztz5Fs7/yy74g2N5SVQY4xY1qDr4rNddwYRVX0g==" -outline-icons@^3.16.0: - version "3.16.0" - resolved "https://registry.yarnpkg.com/outline-icons/-/outline-icons-3.16.0.tgz#bc42017fc86d7e5378e66626a6a241a343b87fa7" - integrity sha512-w/D1kGCRQSDqIUtFwpsmdmF3GBsOe8fmph7ikyjsoM5wSWA7BJGotDscy/1OnSWdIqnJ41EyGKLDrskZBo27ug== +outline-icons@^3.17.0: + version "3.17.0" + resolved "https://registry.yarnpkg.com/outline-icons/-/outline-icons-3.17.0.tgz#6a7cbfaebd89168d4acc81119a21827dd25eeb90" + integrity sha512-H6p2MqJAqi6bkjX1ZIYy9Qw/1mYEz2akm44t5jg7ncaWdBC8ey0Tcbc4oKN6mtjQPCjm8km+qd5dXy3tsZstMA== own-keys@^1.0.0: version "1.0.1"