Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions app/scenes/Settings/Security.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -306,19 +308,21 @@ function Security() {
onChange={handleViewersCanExportChange}
/>
</SettingRow>
<SettingRow
label={t("Users can delete account")}
name={TeamPreference.MembersCanDeleteAccount}
description={t(
"When enabled, users can delete their own account from the workspace"
)}
>
<Switch
id={TeamPreference.MembersCanDeleteAccount}
checked={team.getPreference(TeamPreference.MembersCanDeleteAccount)}
onChange={handleMembersCanDeleteAccountChange}
/>
</SettingRow>
{env.AUTH_TYPE !== AUTH_TYPE_SSO && (
<SettingRow
label={t("Users can delete account")}
name={TeamPreference.MembersCanDeleteAccount}
description={t(
"When enabled, users can delete their own account from the workspace"
)}
>
<Switch
id={TeamPreference.MembersCanDeleteAccount}
checked={team.getPreference(TeamPreference.MembersCanDeleteAccount)}
onChange={handleMembersCanDeleteAccountChange}
/>
</SettingRow>
)}
<SettingRow
label={t("Email address visibility")}
name={TeamPreference.EmailDisplay}
Expand Down
3 changes: 2 additions & 1 deletion app/stores/AuthStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import invariant from "invariant";
import isNil from "lodash/isNil";
import { observable, action, computed, autorun, runInAction } from "mobx";
import { getCookie, setCookie } from "tiny-cookie";
import { AUTH_TYPE_SSO } from "@shared/constants";
import type { CustomTheme } from "@shared/types";
import Storage from "@shared/utils/Storage";
import { getCookieDomain, parseDomain } from "@shared/utils/domains";
Expand Down Expand Up @@ -214,7 +215,7 @@ export default class AuthStore extends Store<Team> {
// 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"
);
Expand Down
5 changes: 3 additions & 2 deletions app/utils/ApiClient.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion server/middlewares/authentication.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -417,7 +418,7 @@ describe("Authentication middleware", () => {

describe("with ForwardAuth headers", () => {
beforeEach(() => {
env.AUTH_TYPE = "SSO";
env.AUTH_TYPE = AUTH_TYPE_SSO;
});

afterEach(() => {
Expand Down
4 changes: 1 addition & 3 deletions server/middlewares/authentication.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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;
Expand Down
47 changes: 47 additions & 0 deletions server/policies/user.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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();
});
});
Comment on lines +20 to +55
});

describe("readEmail", () => {
it("should allow user to read their own email", async () => {
const team = await buildTeam();
Expand Down
14 changes: 10 additions & 4 deletions server/policies/user.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
67 changes: 67 additions & 0 deletions server/routes/api/users/users.test.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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);
});
Comment on lines +652 to +714

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two gaps in this new block, both worth closing in the same PR:

  1. Admin self-delete in SSO mode is unpinned. Once the policy gap (see inline on server/policies/user.ts) is resolved, add a test where an admin calls POST /api/users.delete with no id (or with their own id) and assert the expected status — 403 if you adopt the early-return fix.

  2. users.requestDelete is listed in the PR description's test plan but isn't exercised here. Same authorize(user, "delete", user) call, so the policy unit test covers the mechanism, but one integration test locks the behaviour against future handler changes:

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 require authentication", async () => {
const res = await server.post("/api/users.delete");
const body = await res.json();
Expand Down
3 changes: 3 additions & 0 deletions shared/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
Loading