Skip to content

Commit 7d27150

Browse files
authored
[fga] Skip old permission system if centralizedPermissions is enabled (#18519)
* [server] Disable old permission checks if centraliedPermissions is enabled * review comments
1 parent 6cf3aa2 commit 7d27150

File tree

7 files changed

+124
-130
lines changed

7 files changed

+124
-130
lines changed

components/server/src/auth/resource-access.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
2222
import { UnauthorizedError } from "../errors";
2323
import { RepoURL } from "../repohost";
2424
import { HostContextProvider } from "./host-context-provider";
25+
import { isFgaAuthorizerEnabled } from "../authorization/authorizer";
26+
import { reportGuardAccessCheck } from "../prometheus-metrics";
2527

2628
declare let resourceInstance: GuardedResource;
2729
export type GuardedResourceKind = typeof resourceInstance.kind;
@@ -152,6 +154,26 @@ export class CompositeResourceAccessGuard implements ResourceAccessGuard {
152154
}
153155
}
154156

157+
/**
158+
* FGAResourceAccessGuard can disable the delegate if FGA is enabled.
159+
*/
160+
export class FGAResourceAccessGuard implements ResourceAccessGuard {
161+
constructor(private readonly userId: string, private readonly delegate: ResourceAccessGuard) {}
162+
163+
async canAccess(resource: GuardedResource, operation: ResourceAccessOp): Promise<boolean> {
164+
const authorizerEnabled = await isFgaAuthorizerEnabled(this.userId);
165+
if (authorizerEnabled) {
166+
// Authorizer takes over, so we should not check.
167+
reportGuardAccessCheck("fga");
168+
return true;
169+
}
170+
reportGuardAccessCheck("resource-access");
171+
172+
// FGA can't take over yet, so we delegate
173+
return await this.delegate.canAccess(resource, operation);
174+
}
175+
}
176+
155177
export class TeamMemberResourceGuard implements ResourceAccessGuard {
156178
constructor(readonly userId: string) {}
157179

components/server/src/authorization/authorizer.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -213,11 +213,7 @@ export class Authorizer {
213213
}
214214

215215
private async isDisabled(userId: string): Promise<boolean> {
216-
return !(await getExperimentsClientForBackend().getValueAsync("centralizedPermissions", false, {
217-
user: {
218-
id: userId,
219-
},
220-
}));
216+
return !(await isFgaAuthorizerEnabled(userId));
221217
}
222218

223219
async addUser(userId: string, owningOrgId?: string) {
@@ -438,6 +434,15 @@ export class Authorizer {
438434
}
439435
}
440436

437+
// TODO(gpl) remove after FGA rollout
438+
export async function isFgaAuthorizerEnabled(userId: string): Promise<boolean> {
439+
return getExperimentsClientForBackend().getValueAsync("centralizedPermissions", false, {
440+
user: {
441+
id: userId,
442+
},
443+
});
444+
}
445+
441446
function set(rs: v1.Relationship): v1.RelationshipUpdate {
442447
return v1.RelationshipUpdate.create({
443448
operation: v1.RelationshipUpdate_Operation.TOUCH,

components/server/src/prometheus-metrics.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ export function registerServerMetrics(registry: prometheusClient.Registry) {
2121
registry.registerMetric(stripeClientRequestsCompletedDurationSeconds);
2222
registry.registerMetric(imageBuildsStartedTotal);
2323
registry.registerMetric(imageBuildsCompletedTotal);
24-
registry.registerMetric(centralizedPermissionsValidationsTotal);
2524
registry.registerMetric(spicedbClientLatency);
2625
registry.registerMetric(dashboardErrorBoundary);
2726
registry.registerMetric(jwtCookieIssued);
@@ -229,16 +228,6 @@ export function increaseImageBuildsCompletedTotal(outcome: "succeeded" | "failed
229228
imageBuildsCompletedTotal.inc({ outcome });
230229
}
231230

232-
const centralizedPermissionsValidationsTotal = new prometheusClient.Counter({
233-
name: "gitpod_perms_centralized_validations_total",
234-
help: "counter of centralized permission checks validations against existing system",
235-
labelNames: ["operation", "matches_expectation"],
236-
});
237-
238-
export function reportCentralizedPermsValidation(operation: string, matches: boolean) {
239-
centralizedPermissionsValidationsTotal.inc({ operation, matches_expectation: String(matches) });
240-
}
241-
242231
export const fgaRelationsUpdateClientLatency = new prometheusClient.Histogram({
243232
name: "gitpod_fga_relationship_update_seconds",
244233
help: "Histogram of completed relationship updates",
@@ -343,3 +332,14 @@ export const updateSubscribersRegistered = new prometheusClient.Gauge({
343332
help: "Gauge of subscribers registered",
344333
labelNames: ["type"],
345334
});
335+
336+
export const guardAccessChecksTotal = new prometheusClient.Counter({
337+
name: "gitpod_guard_access_checks_total",
338+
help: "Counter for the number of guard access checks we do by type",
339+
labelNames: ["type"],
340+
});
341+
342+
export type GuardAccessCheckType = "fga" | "resource-access";
343+
export function reportGuardAccessCheck(type: GuardAccessCheckType) {
344+
guardAccessChecksTotal.labels(type).inc();
345+
}

components/server/src/user/user-controller.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,12 @@ import { getRequestingClientInfo } from "../express-util";
2121
import { GitpodToken, GitpodTokenType, User } from "@gitpod/gitpod-protocol";
2222
import { HostContextProvider } from "../auth/host-context-provider";
2323
import { reportJWTCookieIssued } from "../prometheus-metrics";
24-
import { OwnerResourceGuard, ResourceAccessGuard, ScopedResourceGuard } from "../auth/resource-access";
24+
import {
25+
FGAResourceAccessGuard,
26+
OwnerResourceGuard,
27+
ResourceAccessGuard,
28+
ScopedResourceGuard,
29+
} from "../auth/resource-access";
2530
import { OneTimeSecretServer } from "../one-time-secret-server";
2631
import { ClientMetadata } from "../websocket/websocket-connection-manager";
2732
import * as fs from "fs/promises";
@@ -387,7 +392,8 @@ export class UserController {
387392
return;
388393
}
389394
const sessionId = req.body.sessionId;
390-
const server = this.createGitpodServer(user, new OwnerResourceGuard(user.id));
395+
const resourceGuard = new FGAResourceAccessGuard(user.id, new OwnerResourceGuard(user.id));
396+
const server = this.createGitpodServer(user, resourceGuard);
391397
try {
392398
await server.sendHeartBeat({}, { wasClosed: true, instanceId: instanceID });
393399
/** no await */ server

components/server/src/websocket/websocket-connection-manager.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import {
3434
TeamMemberResourceGuard,
3535
WithResourceAccessGuard,
3636
RepositoryResourceGuard,
37+
FGAResourceAccessGuard,
3738
} from "../auth/resource-access";
3839
import { clientIp, takeFirst } from "../express-util";
3940
import {
@@ -226,6 +227,7 @@ export class WebsocketConnectionManager implements ConnectionHandler {
226227
new SharedWorkspaceAccessGuard(),
227228
new RepositoryResourceGuard(user, this.hostContextProvider),
228229
]);
230+
resourceGuard = new FGAResourceAccessGuard(user.id, resourceGuard);
229231
} else {
230232
resourceGuard = { canAccess: async () => false };
231233
}

0 commit comments

Comments
 (0)