From 2dd5fcded601390a1c631ddfa2bfcbe87338f3cf Mon Sep 17 00:00:00 2001 From: Sven Efftinge Date: Tue, 26 Sep 2023 12:21:05 +0200 Subject: [PATCH] [fga] don't await spicedb calls when disabled (#18802) --- .../server/src/authorization/authorizer.ts | 72 ++++++++++--------- .../src/authorization/relationship-updater.ts | 35 ++++++--- .../src/authorization/spicedb-authorizer.ts | 54 ++++++++------ 3 files changed, 94 insertions(+), 67 deletions(-) diff --git a/components/server/src/authorization/authorizer.ts b/components/server/src/authorization/authorizer.ts index c6f2b041121fb2..3ec1bc6d3de3c9 100644 --- a/components/server/src/authorization/authorizer.ts +++ b/components/server/src/authorization/authorizer.ts @@ -424,46 +424,50 @@ export class Authorizer { } await this.authorizer.writeRelationships(...rels); - //TODO(se) remove this double checking once we're confident that the above works - // check if the relationships were written - try { - const wsToOrgRel = this.find(rel.workspace(workspaceID).org.organization(orgID)); - const wsToOwnerRel = this.find(rel.workspace(workspaceID).owner.user(userID)); - const wsSharedRel = shared ? this.find(rel.workspace(workspaceID).shared.anyUser) : Promise.resolve(true); - if (!(await wsToOrgRel)) { - log.error("Failed to write workspace to org relationship", { - orgID, - userID, - workspaceID, - - shared, - }); - } - if (!(await wsToOwnerRel)) { - log.error("Failed to write workspace to owner relationship", { + (async () => { + //TODO(se) remove this double checking once we're confident that the above works + // check if the relationships were written + try { + const wsToOrgRel = await this.find(rel.workspace(workspaceID).org.organization(orgID)); + if (!wsToOrgRel) { + log.error("Failed to write workspace to org relationship", { + orgID, + userID, + workspaceID, + + shared, + }); + } + const wsToOwnerRel = await this.find(rel.workspace(workspaceID).owner.user(userID)); + if (!wsToOwnerRel) { + log.error("Failed to write workspace to owner relationship", { + orgID, + userID, + workspaceID, + shared, + }); + } + if (shared) { + const wsSharedRel = await this.find(rel.workspace(workspaceID).shared.anyUser); + if (!wsSharedRel) { + log.error("Failed to write workspace shared relationship", { + orgID, + userID, + workspaceID, + shared, + }); + } + } + } catch (error) { + log.error("Failed to check workspace relationships", { orgID, userID, workspaceID, shared, + error, }); } - if (!(await wsSharedRel)) { - log.error("Failed to write workspace shared relationship", { - orgID, - userID, - workspaceID, - shared, - }); - } - } catch (error) { - log.error("Failed to check workspace relationships", { - orgID, - userID, - workspaceID, - shared, - error, - }); - } + })().catch((error) => log.error({ userId: userID }, "Failed to check workspace relationships", { error })); } async removeWorkspaceFromOrg(orgID: string, userID: string, workspaceID: string): Promise { diff --git a/components/server/src/authorization/relationship-updater.ts b/components/server/src/authorization/relationship-updater.ts index bbb311a30e4d55..9cb64c8604d686 100644 --- a/components/server/src/authorization/relationship-updater.ts +++ b/components/server/src/authorization/relationship-updater.ts @@ -8,7 +8,7 @@ import { ProjectDB, TeamDB, UserDB, WorkspaceDB } from "@gitpod/gitpod-db/lib"; import { AdditionalUserData, Organization, User } from "@gitpod/gitpod-protocol"; import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; import { inject, injectable } from "inversify"; -import { Authorizer, isFgaWritesEnabled } from "./authorizer"; +import { Authorizer, isFgaChecksEnabled, isFgaWritesEnabled } from "./authorizer"; import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error"; import { v1 } from "@authzed/authzed-node"; import { fgaRelationsUpdateClientLatency } from "../prometheus-metrics"; @@ -52,6 +52,23 @@ export class RelationshipUpdater { if (await this.isMigrated(user)) { return user; } + const migrated = this.internalMigrate(user); + // if checks are not enabled, we don't want to wait for the migration to finish + if (!(await isFgaChecksEnabled(user.id))) { + migrated.catch((err) => { + log.error({ userId: user.id }, "Error while migrating user", err); + }); + return user; + } + try { + return await migrated; + } catch (error) { + log.error({ userId: user.id }, "Error while migrating user", error); + throw error; + } + } + + private async internalMigrate(user: User): Promise { const stopTimer = fgaRelationsUpdateClientLatency.startTimer(); try { return await this.mutex.using([`fga-migration-${user.id}`], 2000, async () => { @@ -156,16 +173,12 @@ export class RelationshipUpdater { }); for (const ws of workspaces) { - await this.authorizer - .addWorkspaceToOrg( - ws.workspace.organizationId, - ws.workspace.ownerId, - ws.workspace.id, - !!ws.workspace.shareable, - ) - .catch((err) => { - log.error({ userId: user.id, workspaceId: ws.workspace.id }, "Failed to update workspace", err); - }); + await this.authorizer.addWorkspaceToOrg( + ws.workspace.organizationId, + ws.workspace.ownerId, + ws.workspace.id, + !!ws.workspace.shareable, + ); } } diff --git a/components/server/src/authorization/spicedb-authorizer.ts b/components/server/src/authorization/spicedb-authorizer.ts index c445bdb64d457c..2dfbd81f2ca8f0 100644 --- a/components/server/src/authorization/spicedb-authorizer.ts +++ b/components/server/src/authorization/spicedb-authorizer.ts @@ -12,7 +12,7 @@ import { inject, injectable } from "inversify"; import { observeSpicedbClientLatency, spicedbClientLatency } from "../prometheus-metrics"; import { SpiceDBClientProvider } from "./spicedb"; import * as grpc from "@grpc/grpc-js"; -import { isFgaChecksEnabled } from "./authorizer"; +import { isFgaChecksEnabled, isFgaWritesEnabled } from "./authorizer"; async function tryThree(errMessage: string, code: (attempt: number) => Promise): Promise { let attempt = 0; @@ -56,32 +56,42 @@ export class SpiceDBAuthorizer { }, forceEnablement?: boolean, ): Promise { + if (!(await isFgaWritesEnabled(experimentsFields.userId))) { + return true; + } const featureEnabled = !!forceEnablement || (await isFgaChecksEnabled(experimentsFields.userId)); - const timer = spicedbClientLatency.startTimer(); - let error: Error | undefined; - try { - const response = await tryThree("[spicedb] Failed to perform authorization check.", () => - this.client.checkPermission(req, this.callOptions), - ); - const permitted = response.permissionship === v1.CheckPermissionResponse_Permissionship.HAS_PERMISSION; - if (!permitted && !featureEnabled) { - log.info("[spicedb] Permission denied.", { - response: new TrustedValue(response), + const result = (async () => { + const timer = spicedbClientLatency.startTimer(); + let error: Error | undefined; + try { + const response = await tryThree("[spicedb] Failed to perform authorization check.", () => + this.client.checkPermission(req, this.callOptions), + ); + const permitted = response.permissionship === v1.CheckPermissionResponse_Permissionship.HAS_PERMISSION; + if (!permitted && !featureEnabled) { + log.info("[spicedb] Permission denied.", { + response: new TrustedValue(response), + request: new TrustedValue(req), + }); + return true; + } + + return permitted; + } catch (err) { + error = err; + log.error("[spicedb] Failed to perform authorization check.", err, { request: new TrustedValue(req), }); - return true; + return !featureEnabled; + } finally { + observeSpicedbClientLatency("check", error, timer()); } - - return permitted; - } catch (err) { - error = err; - log.error("[spicedb] Failed to perform authorization check.", err, { - request: new TrustedValue(req), - }); - return !featureEnabled; - } finally { - observeSpicedbClientLatency("check", error, timer()); + })(); + // if the feature is not enabld, we don't await + if (!featureEnabled) { + return true; } + return result; } async writeRelationships(...updates: v1.RelationshipUpdate[]): Promise {