Skip to content

Commit 5547e66

Browse files
committed
Address PR review: fallback grace window + multi-table query AND + UX
Five real-bug fixes from CodeRabbit + Devin review of #3499: #1 personalAccessToken.server.ts: FALLBACK_NOT_INSTALLED_ERROR string was 'RBAC fallback not installed' but the OSS fallback actually returns 'RBAC plugin not installed'. The mismatch made every PAT creation with a roleId hit the compensating-delete branch on self-hosters with no plugin installed — including the dashboard PAT-creation flow. Aligns the constant with the canonical string. #2 internal-packages/rbac/src/fallback.ts: authenticateBearer skipped the revoked-API-key grace window (RevokedApiKey table), so a freshly-rotated env API key would 401 immediately on the new auth path. Mirrors findEnvironmentByApiKey's fallback-to-RevokedApiKey logic so the auth-cross-cutting e2e tests pass. #3 api.v1.query.ts: multi-table queries built a plain RbacResource array, which checkAuth treats as 'any element passes'. A JWT scoped to one detected table could submit a query that also reads another table it isn't scoped to. Wrap with everyResource — same AND-semantics fix as the batch trigger routes. #4 account.tokens/route.tsx: defaultRoleId could land on a custom or plan-blocked role when userRoleId wasn't in the picker's assignable set. The action's submit-revalidation would then 400 until the user manually changed the dropdown. Clamp the default to roles the picker actually renders. #5 settings.team/route.tsx: the role Select used defaultValue, so a failed set-role submit left the attempted role visible while the server kept the old one. Switch to a controlled value bound to currentRoleId.
1 parent cf3d6e4 commit 5547e66

5 files changed

Lines changed: 49 additions & 13 deletions

File tree

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -595,7 +595,11 @@ function RolePicker({
595595

596596
const picker = (
597597
<Select
598-
defaultValue={currentRoleId ?? ""}
598+
// Controlled — keeps the dropdown in sync with the persisted
599+
// role even after a failed set-role fetcher submit (the server
600+
// kept the old role; without `value` the UI would show the
601+
// attempted change).
602+
value={currentRoleId ?? ""}
599603
items={roles}
600604
variant="tertiary/small"
601605
disabled={!canManageMembers || isSubmitting}

apps/webapp/app/routes/account.tokens/route.tsx

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,9 +118,15 @@ export const loader = async ({ request }: LoaderFunctionArgs) => {
118118
// user isn't a member of any org or no RBAC plugin is installed,
119119
// the picker is hidden anyway, so defaultRoleId is just a
120120
// placeholder.
121-
const sys = orgId ? await rbac.systemRoles(orgId) : null;
122-
const lowestAvailable = (sys ?? []).filter((r) => r.available).at(-1)?.id ?? "";
123-
const defaultRoleId = userRoleId ?? lowestAvailable;
121+
// Clamp to roles the picker actually renders (`roles` already
122+
// joins systemRoles ∩ assignableRoleIds). If userRoleId points at
123+
// a custom or plan-blocked role, the hidden form value would
124+
// otherwise post a roleId the action's revalidation rejects with
125+
// 400. Fall through to the most-restrictive assignable role.
126+
const assignableIds = new Set(roles.map((r) => r.id));
127+
const lowestAssignable = roles.at(-1)?.id ?? "";
128+
const defaultRoleId =
129+
userRoleId && assignableIds.has(userRoleId) ? userRoleId : lowestAssignable;
124130

125131
return typedjson({
126132
personalAccessTokens,

apps/webapp/app/routes/api.v1.query.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
import { json } from "@remix-run/server-runtime";
22
import { QueryError } from "@internal/clickhouse";
33
import { z } from "zod";
4-
import { createActionApiRoute } from "~/services/routeBuilders/apiBuilder.server";
4+
import {
5+
createActionApiRoute,
6+
everyResource,
7+
} from "~/services/routeBuilders/apiBuilder.server";
58
import { executeQuery, type QueryScope } from "~/services/queryService.server";
69
import { logger } from "~/services/logger.server";
710
import { rowsToCSV } from "~/utils/dataExport";
@@ -34,10 +37,14 @@ const { action, loader } = createActionApiRoute(
3437
findResource: async () => 1,
3538
authorization: {
3639
action: "read",
40+
// A multi-table query reads from every detected table. Wrap with
41+
// everyResource so a JWT scoped to one table can't pass auth for
42+
// a query that also reads tables it isn't scoped to (would be the
43+
// same OR-loophole the batch trigger route had pre-fix).
3744
resource: (_, __, ___, body) => {
3845
const tables = detectTables(body.query);
3946
return tables.length > 0
40-
? tables.map((id) => ({ type: "query", id }))
47+
? everyResource(tables.map((id) => ({ type: "query", id })))
4148
: { type: "query", id: "all" };
4249
},
4350
},

apps/webapp/app/services/personalAccessToken.server.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,11 @@ export const PAT_LAST_ACCESSED_THROTTLE_MS = 5 * 60 * 1000;
2222
// the PAT is still valid; auth just falls through to legacy permissive
2323
// behaviour. Any other error is treated as a real failure and triggers
2424
// the compensating delete below.
25-
const FALLBACK_NOT_INSTALLED_ERROR = "RBAC fallback not installed";
25+
// Must match the OSS fallback's exact error string (see
26+
// `internal-packages/rbac/src/fallback.ts`'s `setTokenRole`). The
27+
// match is how we detect "no plugin installed" and skip the
28+
// compensating delete.
29+
const FALLBACK_NOT_INSTALLED_ERROR = "RBAC plugin not installed";
2630

2731
type CreatePersonalAccessTokenOptions = {
2832
name: string;

internal-packages/rbac/src/fallback.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,30 @@ class RoleBaseAccessFallbackController implements RoleBaseAccessController {
8080
};
8181
}
8282

83-
const env = await this.prisma.runtimeEnvironment.findFirst({
83+
const include = {
84+
project: true,
85+
organization: true,
86+
orgMember: { select: { userId: true } },
87+
} as const;
88+
let env = await this.prisma.runtimeEnvironment.findFirst({
8489
where: { apiKey: rawToken },
85-
include: {
86-
project: true,
87-
organization: true,
88-
orgMember: { select: { userId: true } },
89-
},
90+
include,
9091
});
9192

93+
// Revoked API key grace window — mirrors `findEnvironmentByApiKey`
94+
// in apps/webapp/app/models/runtimeEnvironment.server.ts. Recently
95+
// rotated keys keep working until their `expiresAt`; without this
96+
// branch a customer who rotates an env API key gets immediate 401s
97+
// on the new auth path. The PR's e2e suite covers this in
98+
// auth-cross-cutting.e2e.full.test.ts ("revoked key within grace").
99+
if (!env) {
100+
const revoked = await this.prisma.revokedApiKey.findFirst({
101+
where: { apiKey: rawToken, expiresAt: { gt: new Date() } },
102+
include: { runtimeEnvironment: { include } },
103+
});
104+
env = revoked?.runtimeEnvironment ?? null;
105+
}
106+
92107
if (!env || env.project.deletedAt !== null) {
93108
return { ok: false, status: 401, error: "Invalid API key" };
94109
}

0 commit comments

Comments
 (0)