From b082e44389337dc6505d590737a6df70867cc8f4 Mon Sep 17 00:00:00 2001 From: nicktrn <55853254+nicktrn@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:27:52 +0100 Subject: [PATCH] fix(webapp): write-path and appearance-control fixes for the theme work (#4756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes found while reviewing #4547, stacked on that branch so they can be reviewed on their own and merged into it. One commit per fix. ## Write-path correctness **Refuse account writes while impersonating.** The five `dashboardPreferences` writers already no-op for an impersonating admin, but the three profile writers added next to them did not, and `requireUserId` returns the impersonated user's id. Both gates now refuse up front and say so, rather than the preference writers silently no-opping while the page reports success. **Preserve unknown keys on a full-blob write.** `mutateDashboardPreferences` parses the JSON column, hands the result to a mutator and persists the whole object back. zod strips keys it does not declare, so a deploy that predates a preference field drops it on the next write through that path — and `updateCurrentProjectEnvironmentId` sits on the navigation hot path. `preserveUnknownKeys` re-attaches them at the write. Note this cannot help deploys already running, so it makes this the last release able to strip rather than retroactively protecting the fields added in #4547. **Scope hidden-sidebar writes to what was shown.** The customize dialog builds its hidden map from the sections it can see and the write replaced `hiddenItems` wholesale. The profile page has no org in scope, so it resolves sections from the most-recently-updated project's org: confirming there dropped hidden ids belonging to sections that org's flags exclude. The payload now carries the ids the dialog rendered and the write only replaces those. Submissions without the list stay authoritative. **Consider both addresses when checking email ownership.** The check only looked at the address the user already had; it now considers the current and submitted address together, so an org managing either one governs the change. Validation moved ahead of the check, and `emailDomainOf` splits on the last `@`. ## Interaction **Revert unsaved themes, debounce contrast saves.** The theme and system-theme selects stamp `data-theme` before the write lands. When it fails, the loader returns the value it always had — so `useSystemThemeSync`'s effect deps are unchanged and React's vdom diff sees no change either, and nothing rewrites the attribute. The page kept rendering a theme that was never stored while the select showed the stored one. The stored pair is now re-applied explicitly, as the side menu's switcher already did. The contrast slider is debounced because Radix commits on every arrow keypress, so a keyboard user crossing the range fired one write per step. **Tick More options for themes outside the short list.** The appearance submenu offers System, Light and Dark; Black and White live on the profile page. With one of those stored, every row read as unselected. ## Subtraction **Drop the profile update rate limiter.** It covered one of four paths that write the same column — `resources.preferences.sidemenu` and `.favorites` take unlimited authenticated writes and go through the locked read-modify-write, which is more expensive than the single narrow `jsonb_set` this capped. It was also what made the contrast slider unusable by keyboard. If preference writes want limiting, it belongs in one place covering all of them. **Resolve email ownership when the dialog opens.** It fans out one SSO status lookup per organization the user belongs to and ran in the profile loader on every page view, purely to pick which body the dialog renders. The action re-derives it before writing either way, so the check that guards the write now has one call site instead of two. ## Testing `typecheck --filter webapp` and `lint` clean. New unit tests for `preserveUnknownKeys`, `mergeHiddenItems` and `emailDomainOf`; `themePreference`, `mergeHiddenItems` and `ssoManagedIdentity` suites pass locally (26 tests). The rest of the webapp suite needs testcontainers and is left to CI. No changeset or `.server-changes` entry: everything here fixes code on the parent branch that has not shipped. The one exception worth a maintainer's call is `mergeHiddenItems`, which also touches the side menu's own customize path. --- .../navigation/AppearanceMenuItem.tsx | 1 + .../navigation/CustomizeSidebarDialog.tsx | 3 + .../app/routes/account._index/route.tsx | 165 +++++++++++------- .../resources.account.email-ownership.ts | 9 + .../routes/resources.preferences.sidemenu.tsx | 12 +- .../services/dashboardPreferences.server.ts | 18 +- .../profileUpdateRateLimiter.server.ts | 27 --- .../app/services/ssoManagedIdentity.server.ts | 33 ++-- apps/webapp/app/utils/dashboardPreferences.ts | 42 +++++ apps/webapp/test/mergeHiddenItems.test.ts | 29 +++ apps/webapp/test/ssoManagedIdentity.test.ts | 21 ++- apps/webapp/test/themePreference.test.ts | 30 +++- 12 files changed, 279 insertions(+), 111 deletions(-) create mode 100644 apps/webapp/app/routes/resources.account.email-ownership.ts delete mode 100644 apps/webapp/app/services/profileUpdateRateLimiter.server.ts create mode 100644 apps/webapp/test/mergeHiddenItems.test.ts diff --git a/apps/webapp/app/components/navigation/AppearanceMenuItem.tsx b/apps/webapp/app/components/navigation/AppearanceMenuItem.tsx index 4b1f056d2..6894d787d 100644 --- a/apps/webapp/app/components/navigation/AppearanceMenuItem.tsx +++ b/apps/webapp/app/components/navigation/AppearanceMenuItem.tsx @@ -65,6 +65,7 @@ export function AppearanceMenuItem() { icon={EllipsisHorizontalIcon} leadingIconClassName={SIDE_MENU_POPOVER_ITEM_ICON} className={SIDE_MENU_POPOVER_ITEM_LABEL} + isSelected={!THEME_OPTIONS.some((option) => option.value === theme)} /> diff --git a/apps/webapp/app/components/navigation/CustomizeSidebarDialog.tsx b/apps/webapp/app/components/navigation/CustomizeSidebarDialog.tsx index ff892b707..89d365cd3 100644 --- a/apps/webapp/app/components/navigation/CustomizeSidebarDialog.tsx +++ b/apps/webapp/app/components/navigation/CustomizeSidebarDialog.tsx @@ -45,6 +45,8 @@ export type SidebarCustomizationPayload = { sectionItemOrder: Record | null; favorites?: Array<{ id: string; label: string }>; removedFavoriteIds?: string[]; + /** Item ids this dialog rendered, so the write leaves ids it never saw alone. */ + knownItemIds: string[]; }; type DialogState = { @@ -248,6 +250,7 @@ export function CustomizeSidebarDialog({ ? favoriteOrder.map((id) => ({ id, label: state.labels[id] ?? "" })) : undefined, removedFavoriteIds: state.removed.length > 0 ? state.removed : undefined, + knownItemIds: sections.flatMap((section) => section.items.map((item) => item.id)), }; onConfirm(payload); diff --git a/apps/webapp/app/routes/account._index/route.tsx b/apps/webapp/app/routes/account._index/route.tsx index 39456e529..9187d361d 100644 --- a/apps/webapp/app/routes/account._index/route.tsx +++ b/apps/webapp/app/routes/account._index/route.tsx @@ -69,7 +69,6 @@ import { useFeatures } from "~/hooks/useFeatures"; import { useHasAdminAccess, useUser } from "~/hooks/useUser"; import { updateUserEmail, updateUserMarketingEmails, updateUserName } from "~/models/user.server"; import { logger } from "~/services/logger.server"; -import { profileUpdateRateLimiter } from "~/services/profileUpdateRateLimiter.server"; import { type EmailOwnership, getEmailOwnership } from "~/services/ssoManagedIdentity.server"; import { updateContrastPreference, @@ -90,7 +89,7 @@ import { ThemePreference, } from "~/utils/themePreference"; import { cachedFlag, resolveOrganizationFeatureFlags } from "~/v3/featureFlags.server"; -import { requireUser, requireUserId } from "~/services/session.server"; +import { requireUser } from "~/services/session.server"; import { emailSchema, MAX_EMAIL_LENGTH } from "~/utils/emailValidation"; import { pageMeta } from "~/utils/pageTitle"; import { cn } from "~/utils/cn"; @@ -101,6 +100,8 @@ const MIN_CONTRAST = 0; const DEFAULT_CONTRAST_MARK = 0; +const CONTRAST_SAVE_DEBOUNCE_MS = 400; + function themeIcon(value: ThemePreference, appearance: ThemeAppearance) { const Icon = themeOptionIcon(THEME_OPTIONS_BY_VALUE[value], appearance); // shrink-0 stops a long label squashing the icon. @@ -200,31 +201,32 @@ function profileUpdateError(error: string, status: number) { } /** - * Shared gate for the appearance writes: same rate limit as the profile writes, - * then the theme-switcher flag. Returns the user so the caller needn't load it - * a second time. + * Shared gate for every write on this page. Returns the user so the caller + * needn't load it a second time. */ -async function requireAppearanceAccess(request: Request, userId: string) { - const rateLimited = await checkProfileUpdateRateLimit(userId); - if (rateLimited) return { error: rateLimited }; - +async function requireOwnAccountWrite(request: Request) { const user = await requireUser(request); - const showThemeSwitcher = - user.admin || (await cachedFlag({ key: "hasThemeSwitcher", defaultValue: false })); - if (!showThemeSwitcher) { - return { error: profileUpdateError("Not available", 404) }; + if (user.isImpersonating) { + return { + error: profileUpdateError("You can't change this while impersonating another user.", 403), + }; } return { user }; } -/** The only limit a scripted POST can't skip. */ -async function checkProfileUpdateRateLimit(userId: string) { - const limit = await profileUpdateRateLimiter.limit(`user:${userId}`); - if (limit.success) { - return undefined; +/** The gate above, plus the theme-switcher flag. */ +async function requireAppearanceAccess(request: Request) { + const gate = await requireOwnAccountWrite(request); + if ("error" in gate) return gate; + + const showThemeSwitcher = + gate.user.admin || (await cachedFlag({ key: "hasThemeSwitcher", defaultValue: false })); + if (!showThemeSwitcher) { + return { error: profileUpdateError("Not available", 404) }; } - return profileUpdateError("Too many changes at once. Please wait a moment and try again.", 429); + + return gate; } export async function loader({ request }: LoaderFunctionArgs) { @@ -232,9 +234,6 @@ export async function loader({ request }: LoaderFunctionArgs) { const showThemeSwitcher = user.admin || (await cachedFlag({ key: "hasThemeSwitcher", defaultValue: false })); - // Picks the modal only; the action re-checks before writing. - const emailOwnership = await getEmailOwnership(user); - // Null when the user has no project yet; the row hides itself. let sidebarContext: { organization: { slug: string }; @@ -262,16 +261,14 @@ export async function loader({ request }: LoaderFunctionArgs) { }); } - return json({ showThemeSwitcher, sidebarContext, emailOwnership }); + return json({ showThemeSwitcher, sidebarContext }); } export const action: ActionFunction = async ({ request }) => { - const userId = await requireUserId(request); - const formData = await request.formData(); if (formData.get("action") === "update-theme") { - const gate = await requireAppearanceAccess(request, userId); + const gate = await requireAppearanceAccess(request); if ("error" in gate) return gate.error; // Strict, matching /resources/preferences/theme: an unknown value must fail // rather than quietly resetting a saved theme to the default. @@ -282,7 +279,7 @@ export const action: ActionFunction = async ({ request }) => { } if (formData.get("action") === "update-contrast") { - const gate = await requireAppearanceAccess(request, userId); + const gate = await requireAppearanceAccess(request); if ("error" in gate) return gate.error; const contrast = normalizeThemeContrast(formData.get("contrast")); await updateContrastPreference({ user: gate.user, contrast }); @@ -290,7 +287,7 @@ export const action: ActionFunction = async ({ request }) => { } if (formData.get("action") === "update-icon-contrast") { - const gate = await requireAppearanceAccess(request, userId); + const gate = await requireAppearanceAccess(request); if ("error" in gate) return gate.error; await updateIconContrastPreference({ user: gate.user, @@ -300,7 +297,7 @@ export const action: ActionFunction = async ({ request }) => { } if (formData.get("action") === "update-underline-links") { - const gate = await requireAppearanceAccess(request, userId); + const gate = await requireAppearanceAccess(request); if ("error" in gate) return gate.error; await updateUnderlineLinksPreference({ user: gate.user, @@ -310,7 +307,7 @@ export const action: ActionFunction = async ({ request }) => { } if (formData.get("action") === "update-system-theme") { - const gate = await requireAppearanceAccess(request, userId); + const gate = await requireAppearanceAccess(request); if ("error" in gate) return gate.error; // Strict: an unknown value must fail, not silently reset. const end = formData.get("end"); @@ -338,8 +335,8 @@ export const action: ActionFunction = async ({ request }) => { } if (formData.get("action") === "update-name") { - const rateLimited = await checkProfileUpdateRateLimit(userId); - if (rateLimited) return rateLimited; + const gate = await requireOwnAccountWrite(request); + if ("error" in gate) return gate.error; const submission = NameSchema.safeParse({ name: formData.get("name") }); if (!submission.success) { @@ -349,17 +346,26 @@ export const action: ActionFunction = async ({ request }) => { ); } - await updateUserName({ id: userId, name: submission.data.name }); + await updateUserName({ id: gate.user.id, name: submission.data.name }); return json({ success: true as const }); } if (formData.get("action") === "update-email") { - const rateLimited = await checkProfileUpdateRateLimit(userId); - if (rateLimited) return rateLimited; + const gate = await requireOwnAccountWrite(request); + if ("error" in gate) return gate.error; // Re-checked: the loader only picked the modal. - const user = await requireUser(request); - const ownership = await getEmailOwnership(user); + const submission = EmailSchema.safeParse({ email: formData.get("email") }); + if (!submission.success) { + return profileUpdateError( + submission.error.issues[0]?.message ?? "That email address isn't valid.", + 400 + ); + } + + const { email } = submission.data; + + const ownership = await getEmailOwnership(gate.user, email); if (ownership === "idp") { return profileUpdateError( "Your email address is managed by your organization's identity provider.", @@ -372,28 +378,18 @@ export const action: ActionFunction = async ({ request }) => { 503 ); } - - const submission = EmailSchema.safeParse({ email: formData.get("email") }); - if (!submission.success) { - return profileUpdateError( - submission.error.issues[0]?.message ?? "That email address isn't valid.", - 400 - ); - } - - const { email } = submission.data; const existingUser = await prisma.user.findFirst({ where: { email } }); - if (existingUser && existingUser.id !== userId) { + if (existingUser && existingUser.id !== gate.user.id) { return profileUpdateError("Email is already being used by a different account", 400); } - await updateUserEmail({ id: userId, email }); + await updateUserEmail({ id: gate.user.id, email }); return json({ success: true as const }); } if (formData.get("action") === "update-marketing-emails") { - const rateLimited = await checkProfileUpdateRateLimit(userId); - if (rateLimited) return rateLimited; + const gate = await requireOwnAccountWrite(request); + if ("error" in gate) return gate.error; const submission = MarketingEmailsSchema.safeParse({ marketingEmails: formData.get("marketingEmails"), @@ -404,7 +400,7 @@ export const action: ActionFunction = async ({ request }) => { // No-op when the stored value already matches. await updateUserMarketingEmails({ - id: userId, + id: gate.user.id, marketingEmails: submission.data.marketingEmails, }); return json({ success: true as const }); @@ -519,13 +515,17 @@ function EditNameButton() { ); } -function EditEmailButton({ ownership }: { ownership: EmailOwnership }) { +const EMAIL_OWNERSHIP_PATH = "/resources/account/email-ownership"; + +function EditEmailButton() { const user = useUser(); const [isOpen, setIsOpen] = useState(false); const { fetcher, error, setError, isSubmitting } = useProfileFieldUpdate({ successMessage: "Your email address has been updated.", onSuccess: () => setIsOpen(false), }); + const ownershipFetcher = useFetcher<{ ownership: EmailOwnership }>(); + const ownership = ownershipFetcher.data?.ownership; return ( { setIsOpen(open); if (!open) setError(undefined); + if (open && ownershipFetcher.state === "idle" && !ownershipFetcher.data) { + ownershipFetcher.load(EMAIL_OWNERSHIP_PATH); + } }} > @@ -550,7 +553,11 @@ function EditEmailButton({ ownership }: { ownership: EmailOwnership }) { Email address - {ownership === "idp" ? ( + {ownership === undefined ? ( + + Checking your sign-in settings… + + ) : ownership === "idp" ? ( Your organization uses single sign-on, so your email address is managed by your identity provider rather than here. To change it, ask an organization admin to update @@ -773,8 +780,8 @@ function CustomizeSidebarButton({ export default function Page() { const user = useUser(); - const { showThemeSwitcher, sidebarContext, emailOwnership } = useLoaderData(); - const themeFetcher = useFetcher(); + const { showThemeSwitcher, sidebarContext } = useLoaderData(); + const themeFetcher = useFetcher(); const contrastFetcher = useFetcher(); const iconContrastFetcher = useFetcher(); const pendingIconContrast = iconContrastFetcher.formData?.get("iconContrast"); @@ -802,8 +809,8 @@ export default function Page() { const appearance = useThemeAppearance(theme); // One fetcher per end, so picking both quickly can't cancel the first. - const systemLightFetcher = useFetcher(); - const systemDarkFetcher = useFetcher(); + const systemLightFetcher = useFetcher(); + const systemDarkFetcher = useFetcher(); const pendingSystemLight = systemLightFetcher.formData?.get("theme"); const pendingSystemDark = systemDarkFetcher.formData?.get("theme"); const systemLightTheme = normalizeSystemLightTheme( @@ -828,25 +835,51 @@ export default function Page() { fetcher.submit({ action: "update-system-theme", end, theme: value }, { method: "post" }); }; + const storedTheme = normalizeThemePreference(user.dashboardPreferences.theme); + const storedSystemLight = normalizeSystemLightTheme(user.dashboardPreferences.systemLightTheme); + const storedSystemDark = normalizeSystemDarkTheme(user.dashboardPreferences.systemDarkTheme); + const themeWriteFailed = [themeFetcher, systemLightFetcher, systemDarkFetcher].some( + (fetcher) => fetcher.state === "idle" && fetcher.data && !fetcher.data.success + ); + useEffect(() => { + if (themeWriteFailed) { + applyThemePreference(storedTheme, { light: storedSystemLight, dark: storedSystemDark }); + } + }, [themeWriteFailed, storedTheme, storedSystemLight, storedSystemDark]); + // Resnap to the stored value so a failed save leaves no phantom contrast. const [contrastPreview, setContrastPreview] = useState(contrast); + const [contrastToSave, setContrastToSave] = useState(undefined); useEffect(() => { - if (contrastFetcher.state === "idle") { + if (contrastFetcher.state === "idle" && contrastToSave === undefined) { // oxlint-disable-next-line react/set-state-in-effect -- This effect intentionally synchronizes route state after an external or lifecycle change. setContrastPreview(contrast); applyThemeContrast(contrast); } - }, [contrastFetcher.state, contrast]); + }, [contrastFetcher.state, contrast, contrastToSave]); + + const contrastSubmitRef = useRef(contrastFetcher.submit); + useEffect(() => { + contrastSubmitRef.current = contrastFetcher.submit; + }); + useEffect(() => { + if (contrastToSave === undefined) return; + const timer = setTimeout(() => { + contrastSubmitRef.current( + { action: "update-contrast", contrast: String(contrastToSave) }, + { method: "post" } + ); + // oxlint-disable-next-line react/set-state-in-effect -- Clears the debounce slot once the write is away. + setContrastToSave(undefined); + }, CONTRAST_SAVE_DEBOUNCE_MS); + return () => clearTimeout(timer); + }, [contrastToSave]); const previewContrast = (value: number) => { setContrastPreview(value); applyThemeContrast(value); }; - const saveContrast = (value: number) => - contrastFetcher.submit( - { action: "update-contrast", contrast: String(value) }, - { method: "post" } - ); + const saveContrast = (value: number) => setContrastToSave(value); return ( @@ -891,7 +924,7 @@ export default function Page() { {user.email} - + diff --git a/apps/webapp/app/routes/resources.account.email-ownership.ts b/apps/webapp/app/routes/resources.account.email-ownership.ts new file mode 100644 index 000000000..a6b6ab3f3 --- /dev/null +++ b/apps/webapp/app/routes/resources.account.email-ownership.ts @@ -0,0 +1,9 @@ +import { json, type LoaderFunctionArgs } from "@remix-run/node"; +import { getEmailOwnership } from "~/services/ssoManagedIdentity.server"; +import { requireUser } from "~/services/session.server"; + +export async function loader({ request }: LoaderFunctionArgs) { + const user = await requireUser(request); + + return json({ ownership: await getEmailOwnership(user) }); +} diff --git a/apps/webapp/app/routes/resources.preferences.sidemenu.tsx b/apps/webapp/app/routes/resources.preferences.sidemenu.tsx index 35b11a4cf..c9314b9e1 100644 --- a/apps/webapp/app/routes/resources.preferences.sidemenu.tsx +++ b/apps/webapp/app/routes/resources.preferences.sidemenu.tsx @@ -41,6 +41,7 @@ const CustomizationSchema = z.object({ .max(100) .optional(), removedFavoriteIds: z.array(z.string().max(64)).max(100).optional(), + knownItemIds: z.array(z.string().max(64)).max(500).optional(), }); export async function action({ request }: ActionFunctionArgs) { @@ -73,8 +74,14 @@ export async function action({ request }: ActionFunctionArgs) { if (!customizationResult.success) { return json({ success: false, error: "Invalid request data" }, { status: 400 }); } - const { sectionOrder, hiddenItems, sectionItemOrder, favorites, removedFavoriteIds } = - customizationResult.data; + const { + sectionOrder, + hiddenItems, + sectionItemOrder, + favorites, + removedFavoriteIds, + knownItemIds, + } = customizationResult.data; // The modal keeps its "Confirm" pending until this responds, so failures must come back as a // response (never a throw, which would escalate a preferences write to the error boundary). try { @@ -85,6 +92,7 @@ export async function action({ request }: ActionFunctionArgs) { sectionItemOrder, favorites, removedFavoriteIds, + knownItemIds, }); // undefined means nothing was written (impersonating, or the user row is gone) if (!updated) { diff --git a/apps/webapp/app/services/dashboardPreferences.server.ts b/apps/webapp/app/services/dashboardPreferences.server.ts index 2710145b0..ed63a3971 100644 --- a/apps/webapp/app/services/dashboardPreferences.server.ts +++ b/apps/webapp/app/services/dashboardPreferences.server.ts @@ -4,6 +4,8 @@ import { type UserFromSession } from "./session.server"; import { type DashboardPreferences, type FavoritePage, + mergeHiddenItems, + preserveUnknownKeys, parseDashboardPreferences, SideMenuPreferences, } from "~/utils/dashboardPreferences"; @@ -50,7 +52,8 @@ async function mutateDashboardPreferences( return undefined; } - const updated = mutate(getDashboardPreferences(rows[0].dashboardPreferences)); + const raw = rows[0].dashboardPreferences; + const updated = mutate(getDashboardPreferences(raw)); if (!updated) { return undefined; } @@ -60,7 +63,7 @@ async function mutateDashboardPreferences( id: userId, }, data: { - dashboardPreferences: updated, + dashboardPreferences: preserveUnknownKeys(raw, updated), }, }); }, @@ -468,6 +471,7 @@ export async function updateSideMenuCustomization({ sectionItemOrder, favorites, removedFavoriteIds, + knownItemIds, }: { user: UserFromSession; /** undefined = leave unchanged, null = reset to default */ @@ -480,6 +484,13 @@ export async function updateSideMenuCustomization({ favorites?: Array<{ id: string; label: string }>; /** Favorites deleted from the customize modal. */ removedFavoriteIds?: string[]; + /** + * Item ids the submitting dialog rendered. `hiddenItems` only describes these, + * so ids outside the list keep whatever they had: the dialog's section list + * depends on the org whose feature flags were in scope, and a narrower list + * must not un-hide items belonging to a wider one. + */ + knownItemIds?: string[]; }) { if (user.isImpersonating) { return; @@ -494,8 +505,7 @@ export async function updateSideMenuCustomization({ } if (hiddenItems !== undefined) { - next.hiddenItems = - hiddenItems && Object.keys(hiddenItems).length > 0 ? hiddenItems : undefined; + next.hiddenItems = mergeHiddenItems(currentSideMenu.hiddenItems, hiddenItems, knownItemIds); } if (sectionItemOrder !== undefined) { diff --git a/apps/webapp/app/services/profileUpdateRateLimiter.server.ts b/apps/webapp/app/services/profileUpdateRateLimiter.server.ts deleted file mode 100644 index 988979ada..000000000 --- a/apps/webapp/app/services/profileUpdateRateLimiter.server.ts +++ /dev/null @@ -1,27 +0,0 @@ -import { Ratelimit } from "@upstash/ratelimit"; -import { type RedisWithClusterOptions } from "~/redis.server"; -import { createRedisRateLimitClient, RateLimiter } from "~/services/rateLimiter.server"; -import { singleton } from "~/utils/singleton"; - -// Every profile row writes on its own, with no submit button pacing them. The -// client debounce is a courtesy a scripted POST skips, and `/account` sits -// outside `/api/*` so apiRateLimiter doesn't cover it. Exported for the tests. -const PROFILE_UPDATE_RATE_LIMIT_ATTEMPTS = 20; -const PROFILE_UPDATE_RATE_LIMIT_WINDOW = "1 m" as const; - -/** Production uses the env-derived Redis; tests inject a container one. */ -function createProfileUpdateRateLimiter(redisOptions?: RedisWithClusterOptions): RateLimiter { - return new RateLimiter({ - ...(redisOptions ? { redisClient: createRedisRateLimitClient(redisOptions) } : {}), - keyPrefix: "account.profile-update", - limiter: Ratelimit.slidingWindow( - PROFILE_UPDATE_RATE_LIMIT_ATTEMPTS, - PROFILE_UPDATE_RATE_LIMIT_WINDOW - ), - logFailure: true, - }); -} - -export const profileUpdateRateLimiter = singleton("profileUpdateRateLimiter", () => - createProfileUpdateRateLimiter() -); diff --git a/apps/webapp/app/services/ssoManagedIdentity.server.ts b/apps/webapp/app/services/ssoManagedIdentity.server.ts index afae8d605..38869748e 100644 --- a/apps/webapp/app/services/ssoManagedIdentity.server.ts +++ b/apps/webapp/app/services/ssoManagedIdentity.server.ts @@ -28,21 +28,34 @@ export function idpOwnsEmailDomain(status: OrgSsoStatus, emailDomain: string): b ); } -function domainOf(email: string): string | undefined { - const domain = email.toLowerCase().trim().split("@")[1]; - return domain || undefined; +export function emailDomainOf(email: string): string | undefined { + const normalized = email.toLowerCase().trim(); + const at = normalized.lastIndexOf("@"); + return at === -1 ? undefined : normalized.slice(at + 1) || undefined; } -export async function getEmailOwnership(user: { - id: string; - email: string; -}): Promise { +/** + * `candidateEmail` is the address being moved to, when there is one. An org that + * owns either end owns the change: checking only the current address would let a + * member on an unverified domain move onto the org's IdP-managed one. + */ +export async function getEmailOwnership( + user: { + id: string; + email: string; + }, + candidateEmail?: string +): Promise { if (!(await ssoController.isUsingPlugin())) { return "user"; } - const emailDomain = domainOf(user.email); - if (!emailDomain) { + const domains = [ + emailDomainOf(user.email), + candidateEmail ? emailDomainOf(candidateEmail) : undefined, + ]; + const emailDomains = [...new Set(domains.filter((domain): domain is string => !!domain))]; + if (emailDomains.length === 0) { return "user"; } @@ -74,7 +87,7 @@ export async function getEmailOwnership(user: { continue; } - if (idpOwnsEmailDomain(status.value, emailDomain)) { + if (emailDomains.some((domain) => idpOwnsEmailDomain(status.value, domain))) { return "idp"; } } diff --git a/apps/webapp/app/utils/dashboardPreferences.ts b/apps/webapp/app/utils/dashboardPreferences.ts index de858a901..fbe5ddecf 100644 --- a/apps/webapp/app/utils/dashboardPreferences.ts +++ b/apps/webapp/app/utils/dashboardPreferences.ts @@ -97,3 +97,45 @@ export function parseDashboardPreferences( return result.data; } + +/** + * Re-attach keys the schema dropped, so a full-blob write preserves fields this + * deploy was not compiled against. The parsed result wins for every key it + * carries, including ones it deliberately cleared to undefined. + */ +export function preserveUnknownKeys( + raw: unknown, + updated: DashboardPreferences +): DashboardPreferences { + if (!raw || typeof raw !== "object" || Array.isArray(raw)) { + return updated; + } + + const known = new Set(Object.keys(DashboardPreferences.shape)); + const unknownKeys = Object.entries(raw as Record).filter( + ([key]) => !known.has(key) + ); + + return unknownKeys.length > 0 ? { ...Object.fromEntries(unknownKeys), ...updated } : updated; +} + +/** + * Fold a customize-sidebar submission into the stored hidden map. `submitted` + * only describes `knownItemIds`, so ids outside that list keep what they had - + * the dialog's section list depends on which org's feature flags were in scope, + * and a narrower list must not un-hide items belonging to a wider one. Without + * the list the submission is authoritative, as it was before. + */ +export function mergeHiddenItems( + current: Record | undefined, + submitted: Record | null, + knownItemIds: string[] | undefined +): Record | undefined { + const known = knownItemIds ? new Set(knownItemIds) : undefined; + const preserved: Array<[string, boolean]> = known + ? Object.entries(current ?? {}).filter(([id]) => !known.has(id)) + : []; + const merged = { ...Object.fromEntries(preserved), ...(submitted ?? {}) }; + + return Object.keys(merged).length > 0 ? merged : undefined; +} diff --git a/apps/webapp/test/mergeHiddenItems.test.ts b/apps/webapp/test/mergeHiddenItems.test.ts new file mode 100644 index 000000000..4cacacaeb --- /dev/null +++ b/apps/webapp/test/mergeHiddenItems.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from "vitest"; +import { mergeHiddenItems } from "~/utils/dashboardPreferences"; + +describe("mergeHiddenItems", () => { + it("leaves ids the dialog never rendered alone", () => { + const result = mergeHiddenItems({ logs: true, queues: true }, { queues: false }, ["queues"]); + expect(result).toEqual({ logs: true, queues: false }); + }); + + it("keeps out-of-scope ids when the submission resets to defaults", () => { + const result = mergeHiddenItems({ logs: true, queues: true }, null, ["queues"]); + expect(result).toEqual({ logs: true }); + }); + + it("treats the submission as authoritative without a known-id list", () => { + const result = mergeHiddenItems({ logs: true, queues: true }, { queues: false }, undefined); + expect(result).toEqual({ queues: false }); + }); + + it("clears the stored map when nothing is left hidden", () => { + expect(mergeHiddenItems({ queues: true }, null, ["queues"])).toBeUndefined(); + expect(mergeHiddenItems(undefined, null, undefined)).toBeUndefined(); + }); + + it("lets the submission win for ids it did render", () => { + const result = mergeHiddenItems({ queues: true }, { queues: false }, ["queues", "logs"]); + expect(result).toEqual({ queues: false }); + }); +}); diff --git a/apps/webapp/test/ssoManagedIdentity.test.ts b/apps/webapp/test/ssoManagedIdentity.test.ts index fda4f8ec9..51cb5a28d 100644 --- a/apps/webapp/test/ssoManagedIdentity.test.ts +++ b/apps/webapp/test/ssoManagedIdentity.test.ts @@ -1,6 +1,6 @@ import type { OrgSsoStatus } from "@trigger.dev/plugins"; import { describe, expect, it } from "vitest"; -import { idpOwnsEmailDomain } from "~/services/ssoManagedIdentity.server"; +import { emailDomainOf, idpOwnsEmailDomain } from "~/services/ssoManagedIdentity.server"; function status(overrides: Partial = {}): OrgSsoStatus { return { @@ -84,3 +84,22 @@ describe("idpOwnsEmailDomain", () => { expect(idpOwnsEmailDomain(status(), "mail.acme.com")).toBe(false); }); }); + +describe("emailDomainOf", () => { + it("reads the domain off an ordinary address", () => { + expect(emailDomainOf("alice@acme.com")).toBe("acme.com"); + }); + + it("lowercases and trims", () => { + expect(emailDomainOf(" Alice@ACME.com ")).toBe("acme.com"); + }); + + it("splits on the last @, so a quoted local part can't hide the domain", () => { + expect(emailDomainOf('"a@b"@acme.com')).toBe("acme.com"); + }); + + it("returns undefined when there is no domain to read", () => { + expect(emailDomainOf("alice")).toBeUndefined(); + expect(emailDomainOf("alice@")).toBeUndefined(); + }); +}); diff --git a/apps/webapp/test/themePreference.test.ts b/apps/webapp/test/themePreference.test.ts index f89ecf49b..a3a4c438b 100644 --- a/apps/webapp/test/themePreference.test.ts +++ b/apps/webapp/test/themePreference.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { parseDashboardPreferences } from "~/utils/dashboardPreferences"; +import { parseDashboardPreferences, preserveUnknownKeys } from "~/utils/dashboardPreferences"; import { normalizeThemePreference, type ThemePreference } from "~/utils/themePreference"; const VALID_THEMES: ThemePreference[] = ["system", "dark", "light", "black", "white"]; @@ -59,3 +59,31 @@ describe("DashboardPreferences theme schema", () => { expect(result.sideMenu?.isCollapsed).toBe(true); }); }); + +describe("preserveUnknownKeys", () => { + it("re-attaches a key the schema dropped, so a full-blob write can't erase it", () => { + const raw = { + version: "1", + projects: {}, + theme: "dark", + somethingANewerDeployAdded: { nested: true }, + }; + const result = preserveUnknownKeys(raw, parseDashboardPreferences(raw)); + expect(result).toHaveProperty("somethingANewerDeployAdded", { nested: true }); + expect(result.theme).toBe("dark"); + }); + + it("lets the parsed value win for keys the schema does know", () => { + const raw = { version: "1", projects: {}, theme: "dark", contrast: 40 }; + const result = preserveUnknownKeys(raw, { ...parseDashboardPreferences(raw), contrast: 10 }); + expect(result.contrast).toBe(10); + }); + + it("passes the update straight through when there is nothing extra to keep", () => { + const raw = { version: "1", projects: {} }; + const parsed = parseDashboardPreferences(raw); + expect(preserveUnknownKeys(raw, parsed)).toBe(parsed); + expect(preserveUnknownKeys(null, parsed)).toBe(parsed); + expect(preserveUnknownKeys("nonsense", parsed)).toBe(parsed); + }); +});