History

Surface failed policy and OAuth app mutations (#1417)

morluto committed 3 months ago

Commit 046d67d1 parent f1b617c5 files · +111 −11Browse files

Unified · Split

5 files
  1. .changeset/calm-tools-report-failures.md +5 −0
  2. e2e/scenarios/policies-ui.test.ts +49 −0
  3. e2e/selfhost/oauth-app-modal.test.ts +27 −0
  4. packages/react/src/components/add-account-modal.tsx +7 −3
  5. packages/react/src/hooks/use-policy-actions.ts +23 −8
.changeset/calm-tools-report-failures.md+5−0
@@ -0,0 +1,5 @@
1---
2"executor": patch
3---
4
5Show policy and OAuth app removal failures in the UI, and keep success-only state unchanged when those writes fail.
e2e/scenarios/policies-ui.test.ts+49−0
@@ -158,6 +158,7 @@ scenario( · 157 unmodified lines
158 .getByRole("button")158 .getByRole("button")
159 .filter({ hasText: leaf })159 .filter({ hasText: leaf })
160 .getByLabel(label, { exact: true });160 .getByLabel(label, { exact: true });
161 const internalError = JSON.stringify({ _tag: "InternalError", traceId: "policy-write" });
161 162
162 await step("Open the integration's Tools tab", async () => {163 await step("Open the integration's Tools tab", async () => {
163 await page.goto(`/integrations/${integration}`, { waitUntil: "networkidle" });164 await page.goto(`/integrations/${integration}`, { waitUntil: "networkidle" });
@@ -172,6 +173,30 @@ scenario( · 8 unmodified lines
172 await policyMenuFor(alpha, `${integration}.records.create`).waitFor();173 await policyMenuFor(alpha, `${integration}.records.create`).waitFor();
173 });174 });
174 175
176 await step("A rejected policy create reports the failure and writes nothing", async () => {
177 await page.route("**/api/policies", async (route) => {
178 if (route.request().method() !== "POST") {
179 await route.continue();
180 return;
181 }
182 await route.fulfill({
183 status: 500,
184 contentType: "application/json",
185 body: internalError,
186 });
187 });
188 await policyMenuFor(alpha, `${integration}.records.create`).click();
189 await page.getByText(leafPattern, { exact: true }).waitFor();
190 await page.getByRole("menuitem", { name: "Block" }).click();
191 await page.getByText("Failed to create policy", { exact: true }).waitFor();
192 const afterFailure = await Effect.runPromise(client.policies.list());
193 expect(
194 afterFailure.map((policy) => policy.pattern),
195 "a rejected create does not persist the optimistic policy",
196 ).not.toContain(leafPattern);
197 await page.unroute("**/api/policies");
198 });
199
175 await step("Block records.create from the per-tool menu", async () => {200 await step("Block records.create from the per-tool menu", async () => {
176 await policyMenuFor(alpha, `${integration}.records.create`).click();201 await policyMenuFor(alpha, `${integration}.records.create`).click();
177 // The menu is headed by the exact pattern it will store.202 // The menu is headed by the exact pattern it will store.
@@ -180,6 +205,30 @@ scenario( · 2 unmodified lines
180 await leafIndicator(alpha, "create", `Blocked (matched ${leafPattern})`).waitFor();205 await leafIndicator(alpha, "create", `Blocked (matched ${leafPattern})`).waitFor();
181 });206 });
182 207
208 await step("A rejected clear reports the failure and keeps the policy active", async () => {
209 await page.route("**/api/policies/*", async (route) => {
210 if (route.request().method() !== "DELETE") {
211 await route.continue();
212 return;
213 }
214 await route.fulfill({
215 status: 500,
216 contentType: "application/json",
217 body: internalError,
218 });
219 });
220 await policyMenuFor(alpha, `${integration}.records.create`).click();
221 await page.getByRole("menuitem", { name: "Clear" }).click();
222 await page.getByText("Failed to clear policy", { exact: true }).waitFor();
223 await leafIndicator(alpha, "create", `Blocked (matched ${leafPattern})`).waitFor();
224 const afterFailure = await Effect.runPromise(client.policies.list());
225 expect(
226 afterFailure.map((policy) => policy.pattern),
227 "a rejected clear leaves the stored policy intact",
228 ).toContain(leafPattern);
229 await page.unroute("**/api/policies/*");
230 });
231
183 await step("Require approval for the whole records category", async () => {232 await step("Require approval for the whole records category", async () => {
184 await policyMenuFor(alpha, `${integration}.records.*`).click();233 await policyMenuFor(alpha, `${integration}.records.*`).click();
185 await page.getByText(categoryPattern, { exact: true }).waitFor();234 await page.getByText(categoryPattern, { exact: true }).waitFor();
e2e/selfhost/oauth-app-modal.test.ts+27−0
@@ -129,6 +129,33 @@ scenario( · 128 unmodified lines
129 await actions.waitFor();129 await actions.waitFor();
130 });130 });
131 131
132 await step("A rejected removal reports the failure and keeps the app", async () => {
133 await page.route("**/api/oauth/clients/**", async (route) => {
134 if (route.request().method() !== "DELETE") {
135 await route.continue();
136 return;
137 }
138 await route.fulfill({
139 status: 500,
140 contentType: "application/json",
141 body: JSON.stringify({ _tag: "InternalError", traceId: "oauth-client-remove" }),
142 });
143 });
144 await actions.click();
145 await page.getByRole("menuitem", { name: "Remove" }).click();
146 await page.getByRole("button", { name: "Remove app" }).click();
147 await page.getByText(`Failed to remove ${appName}`, { exact: true }).waitFor();
148 await page.getByRole("heading", { name: `Remove ${appName}?` }).waitFor();
149 const clients = await Effect.runPromise(client.oauth.listClients());
150 expect(
151 clients.map((candidate) => String(candidate.slug)),
152 "a rejected removal leaves the registered app in the catalog",
153 ).toContain(appName);
154 await page.getByRole("button", { name: "Cancel" }).click();
155 await actions.waitFor();
156 await page.unroute("**/api/oauth/clients/**");
157 });
158
132 await step("Remove the app and confirm", async () => {159 await step("Remove the app and confirm", async () => {
133 await actions.click();160 await actions.click();
134 await page.getByRole("menuitem", { name: "Remove" }).click();161 await page.getByRole("menuitem", { name: "Remove" }).click();
packages/react/src/components/add-account-modal.tsx+7−3
@@ -1230,7 +1230,7 @@ function AddAccountModalView(props: AddAccountModalProps) { · 1229 unmodified lines
1230 const doRegisterDynamic = useAtomSet(registerDynamicOAuthClient, {1230 const doRegisterDynamic = useAtomSet(registerDynamicOAuthClient, {
1231 mode: "promiseExit",1231 mode: "promiseExit",
1232 });1232 });
1233 const doRemoveOAuthClient = useAtomSet(removeOAuthClientOptimistic, { mode: "promise" });1233 const doRemoveOAuthClient = useAtomSet(removeOAuthClientOptimistic, { mode: "promiseExit" });
1234 const doValidate = useAtomSet(validateConnection, { mode: "promiseExit" });1234 const doValidate = useAtomSet(validateConnection, { mode: "promiseExit" });
1235 const doCheckConnectionHealth = useAtomSet(checkConnectionHealth, { mode: "promiseExit" });1235 const doCheckConnectionHealth = useAtomSet(checkConnectionHealth, { mode: "promiseExit" });
1236 const doUpdateConnection = useAtomSet(updateConnection, { mode: "promiseExit" });1236 const doUpdateConnection = useAtomSet(updateConnection, { mode: "promiseExit" });
@@ -1610,12 +1610,16 @@ function AddAccountModalView(props: AddAccountModalProps) { · 373 unmodified lines
1610 // reconnect at their next refresh); clear the picked app if it was the one1610 // reconnect at their next refresh); clear the picked app if it was the one
1611 // removed so the connect button doesn't point at a gone slug.1611 // removed so the connect button doesn't point at a gone slug.
1612 const handleRemoveApp = async (client: OAuthClientSummary): Promise<void> => {1612 const handleRemoveApp = async (client: OAuthClientSummary): Promise<void> => {
1613 setRemovingClient(null);1613 const exit = await doRemoveOAuthClient({
1614 await doRemoveOAuthClient({
1615 params: { slug: client.slug },1614 params: { slug: client.slug },
1616 payload: { owner: client.owner },1615 payload: { owner: client.owner },
1617 reactivityKeys: oauthClientWriteKeys,1616 reactivityKeys: oauthClientWriteKeys,
1618 });1617 });
1618 if (Exit.isFailure(exit)) {
1619 toast.error(messageFromExit(exit, `Failed to remove ${String(client.slug)}`));
1620 return;
1621 }
1622 setRemovingClient(null);
1619 trackEvent("oauth_client_removed", { owner: client.owner });1623 trackEvent("oauth_client_removed", { owner: client.owner });
1620 toast.success(`Removed ${String(client.slug)}`);1624 toast.success(`Removed ${String(client.slug)}`);
1621 if (pickedApp === String(client.slug)) setPickedApp(null);1625 if (pickedApp === String(client.slug)) setPickedApp(null);
packages/react/src/hooks/use-policy-actions.ts+23−8
@@ -1,6 +1,8 @@
1import { useCallback, useMemo, useRef } from "react";1import { useCallback, useMemo, useRef } from "react";
2import { useAtomSet, useAtomValue } from "@effect/atom-react";2import { useAtomSet, useAtomValue } from "@effect/atom-react";
3import * as Exit from "effect/Exit";
3import * as AsyncResult from "effect/unstable/reactivity/AsyncResult";4import * as AsyncResult from "effect/unstable/reactivity/AsyncResult";
5import { toast } from "sonner";
4import {6import {
5 PolicyId,7 PolicyId,
6 positionForNewPattern,8 positionForNewPattern,
@@ -16,6 +18,7 @@ import { · 9 unmodified lines
16} from "../api/atoms";18} from "../api/atoms";
17import { policyWriteKeys } from "../api/reactivity-keys";19import { policyWriteKeys } from "../api/reactivity-keys";
18import { trackEvent } from "../api/analytics";20import { trackEvent } from "../api/analytics";
21import { messageFromExit } from "../api/error-reporting";
19 22
20export interface PolicyAction {23export interface PolicyAction {
21 /** Set the action on a pattern. If a user rule with this exact pattern24 /** Set the action on a pattern. If a user rule with this exact pattern
@@ -44,9 +47,9 @@ export interface PolicyAction { · 22 unmodified lines
44 */47 */
45export const usePolicyActions = (owner: Owner = "org"): PolicyAction => {48export const usePolicyActions = (owner: Owner = "org"): PolicyAction => {
46 const policies = useAtomValue(policiesOptimisticAtom);49 const policies = useAtomValue(policiesOptimisticAtom);
47 const doCreate = useAtomSet(createPolicyOptimistic, { mode: "promise" });50 const doCreate = useAtomSet(createPolicyOptimistic, { mode: "promiseExit" });
48 const doUpdate = useAtomSet(updatePolicyOptimistic, { mode: "promise" });51 const doUpdate = useAtomSet(updatePolicyOptimistic, { mode: "promiseExit" });
49 const doRemove = useAtomSet(removePolicyOptimistic, { mode: "promise" });52 const doRemove = useAtomSet(removePolicyOptimistic, { mode: "promiseExit" });
50 53
51 // Sorted by position ASC (lowest position = highest precedence first),54 // Sorted by position ASC (lowest position = highest precedence first),
52 // matching server evaluation order. Optimistic placeholder rows carry55 // matching server evaluation order. Optimistic placeholder rows carry
@@ -105,23 +108,31 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { · 52 unmodified lines
105 const existing = findExact(pattern);108 const existing = findExact(pattern);
106 if (existing) {109 if (existing) {
107 if (existing.action === action) return;110 if (existing.action === action) return;
108 await doUpdate({111 const exit = await doUpdate({
109 params: { policyId: PolicyId.make(existing.id) },112 params: { policyId: PolicyId.make(existing.id) },
110 payload: { owner, action },113 payload: { owner, action },
111 reactivityKeys: policyWriteKeys,114 reactivityKeys: policyWriteKeys,
112 });115 });
116 if (Exit.isFailure(exit)) {
117 toast.error(messageFromExit(exit, "Failed to update policy"));
118 return;
119 }
113 trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner });120 trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner });
114 return;121 return;
115 }122 }
116 const position = computePosition(pattern);123 const position = computePosition(pattern);
117 const created = await doCreate({124 const exit = await doCreate({
118 payload:125 payload:
119 position === undefined126 position === undefined
120 ? { owner, pattern, action }127 ? { owner, pattern, action }
121 : { owner, pattern, action, position },128 : { owner, pattern, action, position },
122 reactivityKeys: policyWriteKeys,129 reactivityKeys: policyWriteKeys,
123 });130 });
124 createdIdByPattern.current.set(pattern, String(created.id));131 if (Exit.isFailure(exit)) {
132 toast.error(messageFromExit(exit, "Failed to create policy"));
133 return;
134 }
135 createdIdByPattern.current.set(pattern, String(exit.value.id));
125 trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner });136 trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner });
126 },137 },
127 [owner, doCreate, doUpdate, findExact, computePosition],138 [owner, doCreate, doUpdate, findExact, computePosition],
@@ -138,12 +149,16 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { · 10 unmodified lines
138 const isReal = (id: string | undefined): id is string => !!id && !id.startsWith("pending-");149 const isReal = (id: string | undefined): id is string => !!id && !id.startsWith("pending-");
139 const id = [existing?.id, createdIdByPattern.current.get(pattern), policyId].find(isReal);150 const id = [existing?.id, createdIdByPattern.current.get(pattern), policyId].find(isReal);
140 if (!id) return;151 if (!id) return;
141 createdIdByPattern.current.delete(pattern);152 const exit = await doRemove({
142 await doRemove({
143 params: { policyId: PolicyId.make(id) },153 params: { policyId: PolicyId.make(id) },
144 payload: { owner },154 payload: { owner },
145 reactivityKeys: policyWriteKeys,155 reactivityKeys: policyWriteKeys,
146 });156 });
157 if (Exit.isFailure(exit)) {
158 toast.error(messageFromExit(exit, "Failed to clear policy"));
159 return;
160 }
161 createdIdByPattern.current.delete(pattern);
147 trackEvent("tool_policy_cleared", {162 trackEvent("tool_policy_cleared", {
148 pattern_kind: pattern.endsWith(".*") ? "group" : "exact",163 pattern_kind: pattern.endsWith(".*") ? "group" : "exact",
149 owner,164 owner,