[fga] migrated user service methods (#18461)

svenefftinge · web-flow · commit 9b06d2e1ee29 · 2023-08-09T09:38:35.000+02:00
diff --git a/components/server/src/orgs/usage-service.spec.db.ts b/components/server/src/orgs/usage-service.spec.db.ts
@@ -82,7 +82,12 @@ describe("UsageService", async () => {
                 authId: "1234",
             },
         });
-        await userService.setAdminRole(BUILTIN_INSTLLATION_ADMIN_USER_ID, admin.id, true);
+        await userService.updateRoleOrPermission(BUILTIN_INSTLLATION_ADMIN_USER_ID, admin.id, [
+            {
+                role: "admin",
+                add: true,
+            },
+        ]);
 
         us = container.get<UsageService>(UsageService);
         await us.getCostCenter(owner.id, org.id);
diff --git a/components/server/src/user/user-service.spec.db.ts b/components/server/src/user/user-service.spec.db.ts
@@ -106,4 +106,101 @@ describe("UserService", async () => {
         expect(updated.avatarUrl).is.undefined;
         expect(updated.additionalData?.disabledClosedTimeout).to.be.true;
     });
+
+    it("should updateWorkspaceTimeoutSetting", async () => {
+        await userService.updateWorkspaceTimeoutSetting(user.id, user.id, {
+            disabledClosedTimeout: true,
+        });
+        let updated = await userService.findUserById(user.id, user.id);
+        expect(updated.additionalData?.disabledClosedTimeout).to.be.true;
+
+        await userService.updateWorkspaceTimeoutSetting(user.id, user.id, {
+            workspaceTimeout: "60m",
+        });
+        updated = await userService.findUserById(user.id, user.id);
+        expect(updated.additionalData?.workspaceTimeout).to.eq("60m");
+
+        await expectError(
+            ErrorCodes.BAD_REQUEST,
+            userService.updateWorkspaceTimeoutSetting(user.id, user.id, {
+                workspaceTimeout: "invalid",
+            }),
+        );
+
+        await expectError(
+            ErrorCodes.PERMISSION_DENIED,
+            userService.updateWorkspaceTimeoutSetting(user2.id, user.id, {
+                workspaceTimeout: "10m",
+            }),
+        );
+
+        await expectError(
+            ErrorCodes.NOT_FOUND,
+            userService.updateWorkspaceTimeoutSetting(nonOrgUser.id, user.id, {
+                workspaceTimeout: "10m",
+            }),
+        );
+    });
+
+    it("should updateRoleOrPermission", async () => {
+        await expectError(
+            ErrorCodes.PERMISSION_DENIED,
+            userService.updateRoleOrPermission(user.id, user.id, [
+                {
+                    role: "admin",
+                    add: false,
+                },
+            ]),
+        );
+
+        await userService.updateRoleOrPermission(BUILTIN_INSTLLATION_ADMIN_USER_ID, user.id, [
+            {
+                role: "admin",
+                add: true,
+            },
+        ]);
+
+        let updated = await userService.findUserById(user.id, user.id);
+        expect(new Set(updated.rolesOrPermissions).has("admin")).to.be.true;
+
+        // can remove role themselves now
+        await userService.updateRoleOrPermission(user.id, user.id, [
+            {
+                role: "admin",
+                add: false,
+            },
+        ]);
+
+        updated = await userService.findUserById(user.id, user.id);
+        expect(new Set(updated.rolesOrPermissions).has("admin")).to.be.false;
+
+        // but not add again
+        await expectError(
+            ErrorCodes.PERMISSION_DENIED,
+            userService.updateRoleOrPermission(user.id, user.id, [
+                {
+                    role: "admin",
+                    add: true,
+                },
+            ]),
+        );
+    });
+
+    it("should listUsers", async () => {
+        let users = await userService.listUsers(user.id, {});
+        expect(users.total).to.eq(2);
+        expect(users.rows.some((u) => u.id === user.id)).to.be.true;
+        expect(users.rows.some((u) => u.id === user2.id)).to.be.true;
+
+        users = await userService.listUsers(BUILTIN_INSTLLATION_ADMIN_USER_ID, {});
+        expect(users.total).to.eq(4);
+        expect(users.rows.some((u) => u.id === user.id)).to.be.true;
+        expect(users.rows.some((u) => u.id === user2.id)).to.be.true;
+        expect(users.rows.some((u) => u.id === nonOrgUser.id)).to.be.true;
+        expect(users.rows.some((u) => u.id === BUILTIN_INSTLLATION_ADMIN_USER_ID)).to.be.true;
+
+        users = await userService.listUsers(nonOrgUser.id, {});
+        expect(users.total).to.eq(1);
+        expect(users.rows.some((u) => u.id === nonOrgUser.id)).to.be.true;
+    });
 });
diff --git a/components/server/src/user/user-service.ts b/components/server/src/user/user-service.ts
@@ -8,13 +8,22 @@ import { inject, injectable } from "inversify";
 import { Config } from "../config";
 import { UserDB } from "@gitpod/gitpod-db/lib";
 import { Authorizer } from "../authorization/authorizer";
-import { AdditionalUserData, Identity, TokenEntry, User } from "@gitpod/gitpod-protocol";
+import {
+    AdditionalUserData,
+    Identity,
+    RoleOrPermission,
+    TokenEntry,
+    User,
+    WorkspaceTimeoutDuration,
+    WorkspaceTimeoutSetting,
+} from "@gitpod/gitpod-protocol";
 import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
 import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 import { CreateUserParams } from "./user-authentication";
 import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
 import { TransactionalContext } from "@gitpod/gitpod-db/lib/typeorm/transactional-db-impl";
 import { RelationshipUpdater } from "../authorization/relationship-updater";
+import { EntitlementService } from "../billing/entitlement-service";
 
 @injectable()
 export class UserService {
@@ -24,6 +33,7 @@ export class UserService {
         @inject(Authorizer) private readonly authorizer: Authorizer,
         @inject(IAnalyticsWriter) private readonly analytics: IAnalyticsWriter,
         @inject(RelationshipUpdater) private readonly relationshipUpdater: RelationshipUpdater,
+        @inject(EntitlementService) private readonly entitlementService: EntitlementService,
     ) {}
 
     public async createUser(
@@ -124,34 +134,104 @@ export class UserService {
         return user;
     }
 
-    async setAdminRole(userId: string, targetUserId: string, admin: boolean): Promise<User> {
-        await this.authorizer.checkPermissionOnUser(userId, "make_admin", targetUserId);
-        const target = await this.findUserById(userId, targetUserId);
-        const rolesAndPermissions = target.rolesOrPermissions || [];
-        const newRoles = [...rolesAndPermissions.filter((r) => r !== "admin")];
-        if (admin) {
-            // add admin role
-            newRoles.push("admin");
+    async updateWorkspaceTimeoutSetting(
+        userId: string,
+        targetUserId: string,
+        setting: Partial<WorkspaceTimeoutSetting>,
+    ): Promise<void> {
+        await this.authorizer.checkPermissionOnUser(userId, "write_info", targetUserId);
+
+        if (setting.workspaceTimeout) {
+            try {
+                WorkspaceTimeoutDuration.validate(setting.workspaceTimeout);
+            } catch (err) {
+                throw new ApplicationError(ErrorCodes.BAD_REQUEST, err.message);
+            }
         }
 
+        if (!(await this.entitlementService.maySetTimeout(targetUserId))) {
+            throw new ApplicationError(
+                ErrorCodes.PERMISSION_DENIED,
+                "Configure workspace timeout only available for paid user.",
+            );
+        }
+
+        const user = await this.findUserById(userId, targetUserId);
+        AdditionalUserData.set(user, setting);
+        await this.userDb.updateUserPartial(user);
+    }
+
+    async listUsers(
+        userId: string,
+        req: {
+            //
+            offset?: number;
+            limit?: number;
+            orderBy?: keyof User;
+            orderDir?: "ASC" | "DESC";
+            searchTerm?: string;
+        },
+    ): Promise<{ total: number; rows: User[] }> {
         try {
-            return await this.userDb.transaction(async (userDb) => {
-                target.rolesOrPermissions = newRoles;
-                const updatedUser = await userDb.storeUser(target);
-                if (admin) {
-                    await this.authorizer.addInstallationAdminRole(target.id);
+            const res = await this.userDb.findAllUsers(
+                req.offset || 0,
+                req.limit || 100,
+                req.orderBy || "creationDate",
+                req.orderDir || "DESC",
+                req.searchTerm,
+            );
+            const result = { total: res.total, rows: [] as User[] };
+            for (const user of res.rows) {
+                if (await this.authorizer.hasPermissionOnUser(userId, "read_info", user.id)) {
+                    result.rows.push(user);
                 } else {
-                    await this.authorizer.removeInstallationAdminRole(target.id);
+                    result.total--;
                 }
-                return updatedUser;
-            });
-        } catch (err) {
-            if (admin) {
-                await this.authorizer.removeInstallationAdminRole(target.id);
+            }
+            return result;
+        } catch (e) {
+            throw new ApplicationError(ErrorCodes.INTERNAL_SERVER_ERROR, e.toString());
+        }
+    }
+
+    async updateRoleOrPermission(
+        userId: string,
+        targetUserId: string,
+        modifications: { role: RoleOrPermission; add?: boolean }[],
+    ): Promise<void> {
+        await this.authorizer.checkPermissionOnUser(userId, "make_admin", targetUserId);
+        const target = await this.findUserById(userId, targetUserId);
+        const rolesOrPermissions = new Set((target.rolesOrPermissions || []) as string[]);
+        const adminBefore = rolesOrPermissions.has("admin");
+        modifications.forEach((e) => {
+            if (e.add) {
+                rolesOrPermissions.add(e.role as string);
             } else {
-                await this.authorizer.addInstallationAdminRole(target.id);
+                rolesOrPermissions.delete(e.role as string);
+            }
+        });
+        target.rolesOrPermissions = Array.from(rolesOrPermissions.values()) as RoleOrPermission[];
+        const adminAfter = new Set(target.rolesOrPermissions).has("admin");
+        try {
+            await this.userDb.transaction(async (userDb) => {
+                await userDb.storeUser(target);
+                if (adminBefore !== adminAfter) {
+                    if (adminAfter) {
+                        await this.authorizer.addInstallationAdminRole(target.id);
+                    } else {
+                        await this.authorizer.removeInstallationAdminRole(target.id);
+                    }
+                }
+            });
+        } catch (error) {
+            if (adminBefore !== adminAfter) {
+                if (adminAfter) {
+                    await this.authorizer.removeInstallationAdminRole(target.id);
+                } else {
+                    await this.authorizer.addInstallationAdminRole(target.id);
+                }
             }
-            throw err;
+            throw error;
         }
     }
 }
diff --git a/components/server/src/workspace/gitpod-server-impl.ts b/components/server/src/workspace/gitpod-server-impl.ts
@@ -144,7 +144,6 @@ import {
 } from "@gitpod/gitpod-protocol/lib/teams-projects-protocol";
 import { ClientMetadata, traceClientMetadata } from "../websocket/websocket-connection-manager";
 import {
-    AdditionalUserData,
     EmailDomainFilterEntry,
     EnvVarWithValue,
     LinkedInProfile,
@@ -661,19 +660,10 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         setting: Partial<WorkspaceTimeoutSetting>,
     ): Promise<void> {
         traceAPIParams(ctx, { setting });
-        if (setting.workspaceTimeout) {
-            WorkspaceTimeoutDuration.validate(setting.workspaceTimeout);
-        }
-
         const user = await this.checkAndBlockUser("updateWorkspaceTimeoutSetting");
         await this.guardAccess({ kind: "user", subject: user }, "update");
 
-        if (!(await this.entitlementService.maySetTimeout(user.id))) {
-            throw new Error("configure workspace timeout only available for paid user.");
-        }
-
-        AdditionalUserData.set(user, setting);
-        await this.userDB.updateUserPartial(user);
+        await this.userService.updateWorkspaceTimeoutSetting(user.id, user.id, setting);
     }
 
     public async sendPhoneNumberVerificationToken(
@@ -3084,20 +3074,12 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
     async adminGetUsers(ctx: TraceContext, req: AdminGetListRequest<User>): Promise<AdminGetListResult<User>> {
         traceAPIParams(ctx, { req: censor(req, "searchTerm") }); // searchTerm may contain PII
 
-        await this.guardAdminAccess("adminGetUsers", { req }, Permission.ADMIN_USERS);
+        const admin = await this.guardAdminAccess("adminGetUsers", { req }, Permission.ADMIN_USERS);
 
-        try {
-            const res = await this.userDB.findAllUsers(
-                req.offset,
-                req.limit,
-                req.orderBy,
-                req.orderDir === "asc" ? "ASC" : "DESC",
-                req.searchTerm,
-            );
-            return res;
-        } catch (e) {
-            throw new ApplicationError(ErrorCodes.INTERNAL_SERVER_ERROR, e.toString());
-        }
+        return this.userService.listUsers(admin.id, {
+            ...req,
+            orderDir: req.orderDir === "asc" ? "ASC" : "DESC",
+        });
     }
 
     async adminGetBlockedRepositories(
@@ -3201,7 +3183,10 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         });
         target.rolesOrPermissions = Array.from(rolesOrPermissions.values()) as RoleOrPermission[];
 
-        return await this.userDB.storeUser(target);
+        await this.userService.updateRoleOrPermission(admin.id, target.id, [
+            ...req.rpp.map((e) => ({ role: e.r, add: e.add })),
+        ]);
+        return this.userService.findUserById(admin.id, req.id);
     }
 
     async adminModifyPermanentWorkspaceFeatureFlag(
diff --git a/components/spicedb/schema/schema.yaml b/components/spicedb/schema/schema.yaml
@@ -13,7 +13,7 @@ schema: |-
     // permissions
     permission read_info = self + organization->member + organization->owner + installation->admin
     permission write_info = self
-    permission make_admin = installation->admin
+    permission make_admin = installation->admin + organization->installation_admin
   }
 
   // There's only one global installation

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ schema: \|-`
`13`	`13`	`// permissions`
`14`	`14`	`permission read_info = self + organization->member + organization->owner + installation->admin`
`15`	`15`	`permission write_info = self`
`16`		`- permission make_admin = installation->admin`
	`16`	`+ permission make_admin = installation->admin + organization->installation_admin`
`17`	`17`	`}`
`18`	`18`
`19`	`19`	`// There's only one global installation`