Skip to content

Commit 4310844

Browse files
committed
fix(webapp): address review findings in management API and settings routes
Review fixes: * await the async redirect helpers on denial/guard paths so they redirect with a toast instead of rendering the error boundary (settings index, env settings, regions) * apply the directory-sync membership checks to the new PAT invite and remove-member endpoints, matching the dashboard * skip already-invited emails when inviting so re-inviting one address doesn't fail the whole batch * bound org and project rename input lengths * e2e coverage for the above
1 parent 972dd40 commit 4310844

9 files changed

Lines changed: 84 additions & 15 deletions

File tree

apps/webapp/app/models/member.server.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,8 +174,11 @@ export async function inviteMembers({
174174
}) satisfies Prisma.OrgMemberInviteCreateManyInput
175175
);
176176

177+
// Skip already-invited emails (unique org+email) so re-inviting one address
178+
// doesn't P2002 the whole batch.
177179
await prisma.orgMemberInvite.createMany({
178180
data: invites,
181+
skipDuplicates: true,
179182
});
180183

181184
return await prisma.orgMemberInvite.findMany({

apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ export const action = dashboardAction(
110110
);
111111

112112
if (!ability.can("manage", { type: "project" })) {
113-
throw redirectWithErrorMessage(
113+
throw await redirectWithErrorMessage(
114114
redirectPath,
115115
request,
116116
"You don't have permission to change the default region"
@@ -120,14 +120,14 @@ export const action = dashboardAction(
120120
const project = await findProjectBySlug(organizationSlug, projectParam, user.id);
121121

122122
if (!project) {
123-
throw redirectWithErrorMessage(redirectPath, request, "Project not found");
123+
throw await redirectWithErrorMessage(redirectPath, request, "Project not found");
124124
}
125125

126126
const formData = await request.formData();
127127
const parsedFormData = FormSchema.safeParse(Object.fromEntries(formData));
128128

129129
if (!parsedFormData.success) {
130-
throw redirectWithErrorMessage(redirectPath, request, "No region specified");
130+
throw await redirectWithErrorMessage(redirectPath, request, "No region specified");
131131
}
132132

133133
const service = new SetDefaultRegionService();

apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.general/route.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ export const action = dashboardAction(
106106
switch (submission.value.action) {
107107
case "rename": {
108108
if (!ability.can("manage", { type: "project" })) {
109-
throw redirectWithErrorMessage(
109+
throw await redirectWithErrorMessage(
110110
v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }),
111111
request,
112112
"You don't have permission to rename this project"
@@ -139,7 +139,7 @@ export const action = dashboardAction(
139139
}
140140
case "delete": {
141141
if (!ability.can("manage", { type: "project" })) {
142-
throw redirectWithErrorMessage(
142+
throw await redirectWithErrorMessage(
143143
v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }),
144144
request,
145145
"You don't have permission to delete this project"

apps/webapp/app/routes/_app.orgs.$organizationSlug.settings._index/route.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,8 @@ export const action = dashboardAction(
158158
},
159159
},
160160
async ({ ability, request, params }) => {
161+
// clearCurrentProject (delete branch) needs the full UserFromSession
162+
// (dashboardPreferences), which the builder's SessionUser doesn't carry.
161163
const user = await requireUser(request);
162164
const { organizationSlug } = params;
163165

@@ -177,7 +179,7 @@ export const action = dashboardAction(
177179
switch (submission.value.action) {
178180
case "rename": {
179181
if (!ability.can("manage", { type: "organization" })) {
180-
throw redirectWithErrorMessage(
182+
throw await redirectWithErrorMessage(
181183
organizationSettingsPath({ slug: organizationSlug }),
182184
request,
183185
"You don't have permission to rename this organization"
@@ -205,7 +207,7 @@ export const action = dashboardAction(
205207
}
206208
case "delete": {
207209
if (!ability.can("manage", { type: "organization" })) {
208-
throw redirectWithErrorMessage(
210+
throw await redirectWithErrorMessage(
209211
organizationSettingsPath({ slug: organizationSlug }),
210212
request,
211213
"You don't have permission to delete this organization"

apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { logger } from "~/services/logger.server";
77
import { resolveOrganizationForApiUser } from "~/services/organizationApiAccess.server";
88
import { createActionPATApiRoute } from "~/services/routeBuilders/apiBuilder.server";
99
import { scheduleEmail } from "~/services/scheduleEmail.server";
10+
import { ssoController } from "~/services/sso.server";
1011
import { acceptInvitePath } from "~/utils/pathBuilder";
1112

1213
const ParamsSchema = z.object({
@@ -44,6 +45,13 @@ export const action = createActionPATApiRoute(
4445
return json({ error: "Organization not found" }, { status: 404 });
4546
}
4647

48+
// Directory-managed membership: inviting is disabled (mirrors the dashboard
49+
// invite action). Fail-open on a plugin error.
50+
const policy = await ssoController.getMembershipPolicy(organization.id);
51+
if (policy.isOk() && !policy.value.manualMembershipAllowed) {
52+
return json({ error: "Membership is managed by Directory Sync" }, { status: 403 });
53+
}
54+
4755
const invites = await inviteMembers({
4856
slug: organization.slug,
4957
emails: body.emails,

apps/webapp/app/routes/api.v1.orgs.$orgParam.members.$memberId.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { prisma } from "~/db.server";
44
import { removeTeamMember } from "~/models/member.server";
55
import { resolveOrganizationForApiUser } from "~/services/organizationApiAccess.server";
66
import { createActionPATApiRoute } from "~/services/routeBuilders/apiBuilder.server";
7+
import { ssoController } from "~/services/sso.server";
78

89
const ParamsSchema = z.object({
910
orgParam: z.string(),
@@ -36,6 +37,13 @@ export const action = createActionPATApiRoute(
3637
return json({ error: "Organization not found" }, { status: 404 });
3738
}
3839

40+
// Directory-managed membership: manual removal is disabled (mirrors the
41+
// dashboard Team page). Fail-open on a plugin error.
42+
const policy = await ssoController.getMembershipPolicy(organization.id);
43+
if (policy.isOk() && !policy.value.manualMembershipAllowed) {
44+
return json({ error: "Membership is managed by Directory Sync" }, { status: 403 });
45+
}
46+
3947
// removeTeamMember enforces the last-member guard and throws
4048
// ServiceValidationError (member-not-found / last-member), which the
4149
// builder maps to its status.
@@ -45,6 +53,16 @@ export const action = createActionPATApiRoute(
4553
memberId: params.memberId,
4654
});
4755

56+
// Sticky removal: record a tombstone so passive SSO-JIT won't re-add them
57+
// (best-effort; no-op without the SSO plugin).
58+
await ssoController
59+
.recordMembershipRemoval({
60+
organizationId: organization.id,
61+
userId: removed.userId,
62+
reason: "manual_removal",
63+
})
64+
.unwrapOr(undefined);
65+
4866
return json({
4967
id: removed.id,
5068
user: {

apps/webapp/app/routes/api.v1.orgs.$orgParam.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ const ParamsSchema = z.object({
1010
});
1111

1212
const RenameOrgRequestBody = z.object({
13-
title: z.string().min(1),
13+
title: z.string().trim().min(3).max(50),
1414
});
1515

1616
// Multi-method (PATCH rename / DELETE): declare both so other verbs 405, and

apps/webapp/app/routes/api.v1.projects.$projectRef.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ export const loader = createLoaderPATApiRoute(
6666
);
6767

6868
const RenameProjectRequestBody = z.object({
69-
name: z.string().min(1),
69+
name: z.string().trim().min(1).max(255),
7070
});
7171

7272
// Multi-method (PATCH rename / DELETE): declare both so other verbs 405, and

apps/webapp/test/auth-api.e2e.full.test.ts

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3008,12 +3008,10 @@ describe("API", () => {
30083008
{ Authorization: `Bearer ${pat.token}` },
30093009
{ region: "definitely-not-a-region" }
30103010
);
3011-
// No worker groups seeded → the presenter yields no regions → the handler
3012-
// returns 400 "region not found". The point: the builder let the request
3013-
// through to the handler (auth + method + body all passed).
3014-
expect(res.status).not.toBe(401);
3015-
expect(res.status).not.toBe(403);
3016-
expect(res.status).not.toBe(405);
3011+
// No worker groups seeded → the presenter throws → the route returns 400.
3012+
// The point: the builder let the request through to the handler (auth +
3013+
// method + body all passed).
3014+
expect(res.status).toBe(400);
30173015
});
30183016
});
30193017

@@ -3052,4 +3050,44 @@ describe("API", () => {
30523050
expect(res.status).toBe(405);
30533051
});
30543052
});
3053+
3054+
// Invites are idempotent per email: createMany uses skipDuplicates, so
3055+
// re-inviting an already-invited address returns success rather than a
3056+
// P2002-driven 500.
3057+
describe("Member invites — re-invite is idempotent", () => {
3058+
it("inviting the same email twice does not 500", async () => {
3059+
const server = getTestServer();
3060+
const { organization, pat } = await seedTestUserProject(server.prisma);
3061+
const path = `/api/v1/orgs/${organization.id}/invites`;
3062+
const invite = () =>
3063+
server.webapp.fetch(path, {
3064+
method: "POST",
3065+
headers: { Authorization: `Bearer ${pat.token}`, "Content-Type": "application/json" },
3066+
body: JSON.stringify({ emails: ["dup-invite@example.com"] }),
3067+
});
3068+
3069+
const first = await invite();
3070+
expect(first.status).toBe(201);
3071+
3072+
const second = await invite();
3073+
expect(second.status).toBe(201);
3074+
expect(second.status).not.toBe(500);
3075+
});
3076+
});
3077+
3078+
// Org creation via the management API is gated behind
3079+
// ORG_CREATION_API_ENABLED (default "0"). Without it, a valid PAT gets a 404
3080+
// so the endpoint stays invisible.
3081+
describe("Org creation — disabled by default", () => {
3082+
it("POST /api/v1/orgs with a valid PAT returns 404 when the flag is unset", async () => {
3083+
const server = getTestServer();
3084+
const { pat } = await seedTestUserProject(server.prisma);
3085+
const res = await server.webapp.fetch("/api/v1/orgs", {
3086+
method: "POST",
3087+
headers: { Authorization: `Bearer ${pat.token}`, "Content-Type": "application/json" },
3088+
body: JSON.stringify({ title: "New org from API" }),
3089+
});
3090+
expect(res.status).toBe(404);
3091+
});
3092+
});
30553093
});

0 commit comments

Comments
 (0)