Use User.fgaRelationshipsVersion instead of User.additionalAttributes.fgaRelationshipsVersion (#18838)

* [db] Add User.fgaRelationshipsVersion

* [server] Use User.fgaRelationshipsVersion

Author: geropl

components/gitpod-db/src/typeorm/entity/db-user.ts

@@ -103,4 +103,10 @@ export class DBUser implements User {
         transformer: Transformer.MAP_EMPTY_STR_TO_UNDEFINED,
     })
     verificationPhoneNumber?: string;
+
+    @Column({
+        default: 0,
+        transformer: Transformer.MAP_ZERO_TO_UNDEFINED,
+    })
+    fgaRelationshipsVersion?: number;
 }

components/gitpod-db/src/typeorm/migration/1694775547603-AddOrgDefaultImageSetting.ts

@@ -10,7 +10,7 @@ import { columnExists, tableExists } from "./helper/helper";
 export class AddOrgDefaultImageSetting1694775547603 implements MigrationInterface {
     public async up(queryRunner: QueryRunner): Promise<void> {
         if (await tableExists(queryRunner, "d_b_org_settings")) {
-            if (!(await columnExists(queryRunner, "d_b_org_settings", "d_b_org_settings"))) {
+            if (!(await columnExists(queryRunner, "d_b_org_settings", "defaultWorkspaceImage"))) {
                 await queryRunner.query(
                     "ALTER TABLE `d_b_org_settings` ADD COLUMN `defaultWorkspaceImage` varchar(255) NULL",
                 );

components/gitpod-db/src/typeorm/migration/1695906381289-UserFgaRelationShipVersion.ts

@@ -0,0 +1,38 @@
+/**
+ * Copyright (c) 2023 Gitpod GmbH. All rights reserved.
+ * Licensed under the GNU Affero General Public License (AGPL).
+ * See License.AGPL.txt in the project root for license information.
+ */
+
+import { MigrationInterface, QueryRunner } from "typeorm";
+import { columnExists, indexExists } from "./helper/helper";
+
+const TABLE_NAME = "d_b_user";
+const COLUMN_NAME = "fgaRelationshipsVersion";
+const INDEX_NAME = "ind_fgaRelationshipsVersion";
+
+export class UserFgaRelationShipVersion1695906381289 implements MigrationInterface {
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        if (!(await columnExists(queryRunner, TABLE_NAME, COLUMN_NAME))) {
+            await queryRunner.query(
+                `ALTER TABLE \`${TABLE_NAME}\` ADD COLUMN \`${COLUMN_NAME}\` int NOT NULL DEFAULT 0`,
+            );
+        }
+
+        if (!(await indexExists(queryRunner, TABLE_NAME, INDEX_NAME))) {
+            await queryRunner.query(
+                `ALTER TABLE \`${TABLE_NAME}\` ADD INDEX \`${INDEX_NAME}\` (${COLUMN_NAME}), ALGORITHM=INPLACE, LOCK=NONE`,
+            );
+        }
+    }
+
+    public async down(queryRunner: QueryRunner): Promise<void> {
+        if (await columnExists(queryRunner, TABLE_NAME, COLUMN_NAME)) {
+            await queryRunner.query(`ALTER TABLE \`${TABLE_NAME}\` DROP COLUMN \`${COLUMN_NAME}\``);
+        }
+
+        if (await indexExists(queryRunner, TABLE_NAME, INDEX_NAME)) {
+            await queryRunner.query(`ALTER TABLE \`${TABLE_NAME}\` DROP INDEX \`${INDEX_NAME}\``);
+        }
+    }
+}

components/gitpod-db/src/typeorm/transformer.ts

@@ -23,6 +23,21 @@ export namespace Transformer {
         },
     };
 
+    export const MAP_ZERO_TO_UNDEFINED: ValueTransformer = {
+        to(value: any): any {
+            if (value === undefined) {
+                return 0;
+            }
+            return value;
+        },
+        from(value: any): any {
+            if (value === 0) {
+                return undefined;
+            }
+            return value;
+        },
+    };
+
     export const MAP_NULL_TO_UNDEFINED: ValueTransformer = {
         to(value: any): any {
             if (value === undefined) {

components/gitpod-db/src/typeorm/user-db-impl.ts

@@ -647,4 +647,19 @@ export class TypeORMUserDBImpl extends TransactionalDBImpl<UserDB> implements Us
         }
         return this.mapDBUserToUser(result);
     }
+
+    async findUserIdsNotYetMigratedToFgaVersion(fgaRelationshipsVersion: number, limit: number): Promise<string[]> {
+        const userRepo = await this.getUserRepo();
+        const ids = (await userRepo
+            .createQueryBuilder("user")
+            .select(["id"])
+            .where({
+                fgaRelationshipsVersion: Not(Equal(fgaRelationshipsVersion)),
+                markedDeleted: Equal(false),
+            })
+            .orderBy("_lastModified", "DESC")
+            .limit(limit)
+            .getMany()) as Pick<DBUser, "id">[];
+        return ids.map(({ id }) => id);
+    }
 }

components/gitpod-db/src/user-db.ts

@@ -150,6 +150,8 @@ export interface UserDB extends OAuthUserRepository, OAuthTokenRepository, Trans
     isBlockedPhoneNumber(phoneNumber: string): Promise<boolean>;
 
     findOrgOwnedUser(organizationId: string, email: string): Promise<MaybeUser>;
+
+    findUserIdsNotYetMigratedToFgaVersion(fgaRelationshipsVersion: number, limit: number): Promise<string[]>;
 }
 export type PartialUserUpdate = Partial<Omit<User, "identities">> & Pick<User, "id">;
 

components/gitpod-protocol/src/protocol.ts

@@ -58,6 +58,9 @@ export interface User {
 
     // The phone number used for the last phone verification.
     verificationPhoneNumber?: string;
+
+    // The FGA relationships version of this user
+    fgaRelationshipsVersion?: number;
 }
 
 export namespace User {
@@ -268,8 +271,6 @@ export interface AdditionalUserData extends Partial<WorkspaceTimeoutSetting> {
     // additional user profile data
     profile?: ProfileDetails;
     shouldSeeMigrationMessage?: boolean;
-    // fgaRelationshipsVersion is the version of the spicedb relationships
-    fgaRelationshipsVersion?: number;
     // remembered workspace auto start options
     workspaceAutostartOptions?: WorkspaceAutostartOption[];
 }

components/server/src/authorization/relationship-updater-job.ts

@@ -8,40 +8,31 @@ import { injectable, inject } from "inversify";
 import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 import { Job } from "../jobs/runner";
 import { RelationshipUpdater } from "./relationship-updater";
-import { TypeORM, UserDB } from "@gitpod/gitpod-db/lib";
+import { UserDB } from "@gitpod/gitpod-db/lib";
 
 @injectable()
 export class RelationshipUpdateJob implements Job {
     constructor(
         @inject(RelationshipUpdater) private relationshipUpdater: RelationshipUpdater,
         @inject(UserDB) private readonly userDB: UserDB,
-        @inject(TypeORM) private readonly db: TypeORM,
     ) {}
 
     public name = "relationship-update-job";
     public frequencyMs = 1000 * 60 * 3; // 3m
 
     public async run(): Promise<void> {
         try {
-            const connection = await this.db.getConnection();
-            const results = await connection.query(`
-                SELECT id FROM d_b_user
-                WHERE
-                    (additionalData->"$.fgaRelationshipsVersion" != ${RelationshipUpdater.version} OR
-                    additionalData->"$.fgaRelationshipsVersion" IS NULL) AND
-                    markedDeleted = 0
-                ORDER BY _lastModified DESC
-                LIMIT 50;`);
+            const ids = await this.userDB.findUserIdsNotYetMigratedToFgaVersion(RelationshipUpdater.version, 50);
             const now = Date.now();
             let migrated = 0;
-            for (const result of results) {
-                const user = await this.userDB.findUserById(result.id);
+            for (const userId of ids) {
+                const user = await this.userDB.findUserById(userId);
                 if (!user) {
                     continue;
                 }
                 try {
                     const resultingUser = await this.relationshipUpdater.migrate(user);
-                    if (resultingUser.additionalData?.fgaRelationshipsVersion === RelationshipUpdater.version) {
+                    if (resultingUser.fgaRelationshipsVersion === RelationshipUpdater.version) {
                         migrated++;
                     }
                 } catch (error) {

components/server/src/authorization/relationship-updater.spec.db.ts

@@ -13,7 +13,7 @@ import {
     WorkspaceDB,
 } from "@gitpod/gitpod-db/lib";
 import { resetDB } from "@gitpod/gitpod-db/lib/test/reset-db";
-import { AdditionalUserData, User, Workspace } from "@gitpod/gitpod-protocol";
+import { User, Workspace } from "@gitpod/gitpod-protocol";
 import { Experiments } from "@gitpod/gitpod-protocol/lib/experiments/configcat-server";
 import * as chai from "chai";
 import { Container } from "inversify";
@@ -72,21 +72,21 @@ describe("RelationshipUpdater", async () => {
         let user = await userDB.newUser();
 
         user = await migrator.migrate(user);
-        expect(user.additionalData?.fgaRelationshipsVersion).to.not.be.undefined;
+        expect(user.fgaRelationshipsVersion).to.not.be.undefined;
 
         Experiments.configureTestingClient({
             centralizedPermissions: false,
         });
 
         user = await migrator.migrate(user);
-        expect(user.additionalData?.fgaRelationshipsVersion).to.be.undefined;
+        expect(user.fgaRelationshipsVersion).to.be.undefined;
 
         Experiments.configureTestingClient({
             centralizedPermissions: true,
         });
 
         user = await migrator.migrate(user);
-        expect(user.additionalData?.fgaRelationshipsVersion).to.not.be.undefined;
+        expect(user.fgaRelationshipsVersion).to.not.be.undefined;
     });
 
     it("should correctly update a simple user after it moves between org and installation level", async () => {
@@ -329,15 +329,18 @@ describe("RelationshipUpdater", async () => {
 
     async function notExpected(relation: v1.Relationship): Promise<void> {
         const rs = await authorizer.find(relation);
-        expect(rs).to.be.undefined;
+        expect(
+            rs,
+            `Unexpected relation: ${rs?.subject?.object?.objectType}:${rs?.subject?.object?.objectId}#${rs?.relation}@${rs?.resource?.objectType}:${rs?.resource?.objectId}`,
+        ).to.be.undefined;
     }
 
     async function migrate(user: User): Promise<User> {
         // reset fgaRelationshipsVersion to force update
-        AdditionalUserData.set(user, { fgaRelationshipsVersion: undefined });
-        user = await userDB.storeUser(user);
-        user = await migrator.migrate(user);
-        expect(user.additionalData?.fgaRelationshipsVersion).to.equal(RelationshipUpdater.version);
+        user.fgaRelationshipsVersion = undefined;
+        await userDB.updateUserPartial({ id: user.id, fgaRelationshipsVersion: user.fgaRelationshipsVersion });
+        user = await migrator.migrate(user, true);
+        expect(user.fgaRelationshipsVersion).to.equal(RelationshipUpdater.version);
         return user;
     }
 });

components/server/src/authorization/relationship-updater.ts

@@ -5,7 +5,7 @@
  */
 
 import { ProjectDB, TeamDB, UserDB, WorkspaceDB } from "@gitpod/gitpod-db/lib";
-import { AdditionalUserData, Organization, User } from "@gitpod/gitpod-protocol";
+import { Organization, User } from "@gitpod/gitpod-protocol";
 import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 import { inject, injectable } from "inversify";
 import { Authorizer, isFgaChecksEnabled, isFgaWritesEnabled } from "./authorizer";
@@ -38,14 +38,13 @@ export class RelationshipUpdater {
      * @param user
      * @returns
      */
-    public async migrate(user: User): Promise<User> {
+    public async migrate(user: User, forceAwait: boolean = false): Promise<User> {
         const isEnabled = await isFgaWritesEnabled(user.id);
         if (!isEnabled) {
-            if (user.additionalData?.fgaRelationshipsVersion !== undefined) {
+            if (user.fgaRelationshipsVersion !== undefined) {
                 log.info({ userId: user.id }, `User has been removed from FGA.`);
                 // reset the fgaRelationshipsVersion to undefined, so the migration is triggered again when the feature is enabled
-                AdditionalUserData.set(user, { fgaRelationshipsVersion: undefined });
-                return await this.userDB.storeUser(user);
+                await this.setFgaRelationshipsVersion(user, undefined);
             }
             return user;
         }
@@ -54,7 +53,7 @@ export class RelationshipUpdater {
         }
         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))) {
+        if (!forceAwait && !(await isFgaChecksEnabled(user.id))) {
             migrated.catch((err) => {
                 log.error({ userId: user.id }, "Error while migrating user", err);
             });
@@ -83,7 +82,7 @@ export class RelationshipUpdater {
                     return user;
                 }
                 log.info({ userId: user.id }, `Updating FGA relationships for user.`, {
-                    fromVersion: user?.additionalData?.fgaRelationshipsVersion,
+                    fromVersion: user?.fgaRelationshipsVersion,
                     toVersion: RelationshipUpdater.version,
                 });
                 const orgs = await this.findAffectedOrganizations(user.id);
@@ -94,21 +93,15 @@ export class RelationshipUpdater {
                     await this.updateOrganization(user.id, org);
                 }
                 await this.updateWorkspaces(user);
-                AdditionalUserData.set(user, {
-                    fgaRelationshipsVersion: RelationshipUpdater.version,
-                });
-                await this.userDB.updateUserPartial({
-                    id: user.id,
-                    additionalData: user.additionalData,
-                });
+                await this.setFgaRelationshipsVersion(user, RelationshipUpdater.version);
                 log.info({ userId: user.id }, `Finished updating relationships.`, {
                     duration: new Date().getTime() - before,
                 });
 
                 // let's double check the migration worked.
                 if (!(await this.isMigrated(user))) {
                     log.error({ userId: user.id }, `User migration failed.`, {
-                        markedMigrated: user.additionalData?.fgaRelationshipsVersion === RelationshipUpdater.version,
+                        markedMigrated: user.fgaRelationshipsVersion === RelationshipUpdater.version,
                     });
                 }
                 return user;
@@ -119,18 +112,14 @@ export class RelationshipUpdater {
     }
 
     private async isMigrated(user: User) {
-        const isMigrated = user.additionalData?.fgaRelationshipsVersion === RelationshipUpdater.version;
+        const isMigrated = user.fgaRelationshipsVersion === RelationshipUpdater.version;
         if (isMigrated) {
             const hasSelfRelationship = await this.authorizer.find(rel.user(user.id).self.user(user.id));
             if (!hasSelfRelationship) {
                 log.warn({ userId: user.id }, `User is marked as migrated but doesn't have self relationship.`);
                 //TODO(se) this is an extra safety net to detect
                 // reset the fgaRelationshipsVersion to undefined, so the migration is triggered again when the feature is enabled
-                AdditionalUserData.set(user, { fgaRelationshipsVersion: undefined });
-                await this.userDB.updateUserPartial({
-                    id: user.id,
-                    additionalData: user.additionalData,
-                });
+                await this.setFgaRelationshipsVersion(user, undefined);
                 return false;
             }
         }
@@ -205,4 +194,12 @@ export class RelationshipUpdater {
             projects.map((p) => p.id),
         );
     }
+
+    private async setFgaRelationshipsVersion(user: User, version: number | undefined): Promise<void> {
+        user.fgaRelationshipsVersion = version;
+        await this.userDB.updateUserPartial({
+            id: user.id,
+            fgaRelationshipsVersion: version,
+        });
+    }
 }

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

@@ -254,13 +254,7 @@ export class UserController {
             }
 
             // reset the FGA state
-            await this.userService.updateUser(user.id, {
-                id: user.id,
-                additionalData: {
-                    ...(user.additionalData || {}),
-                    fgaRelationshipsVersion: undefined,
-                },
-            });
+            await this.userService.resetFgaVersion(user.id, user.id);
 
             const redirectToUrl = this.getSafeReturnToParam(req) || this.config.hostUrl.toString();
 

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

@@ -220,4 +220,10 @@ export class UserService {
             throw error;
         }
     }
+
+    async resetFgaVersion(subjectId: string, userId: string) {
+        await this.authorizer.checkPermissionOnUser(subjectId, "write_info", userId);
+
+        await this.userDb.updateUserPartial({ id: userId, fgaRelationshipsVersion: undefined });
+    }
 }