[server] make first user owner (#18417)

svenefftinge · web-flow · commit 8a8b14832151 · 2023-08-03T16:31:28.000+08:00
diff --git a/components/gitpod-db/src/typeorm/team-db-impl.ts b/components/gitpod-db/src/typeorm/team-db-impl.ts
@@ -291,12 +291,13 @@ export class TeamDBImpl extends TransactionalDBImpl<TeamDB> implements TeamDB {
         const membershipRepo = await this.getMembershipRepo();
 
         if (role != "owner") {
-            const ownerCount = await membershipRepo.count({
+            const allOwners = await membershipRepo.find({
                 teamId,
                 role: "owner",
                 deleted: false,
             });
-            if (ownerCount <= 1) {
+            const otherOwnerCount = allOwners.filter((m) => m.userId != userId).length;
+            if (otherOwnerCount === 0) {
                 throw new ApplicationError(ErrorCodes.CONFLICT, "An organization must retain at least one owner");
             }
         }
diff --git a/components/server/BUILD.yaml b/components/server/BUILD.yaml
@@ -24,9 +24,8 @@ packages:
       yarnLock: ${coreYarnLockBase}/yarn.lock
       tsconfig: tsconfig.json
       commands:
-        # leeway executes the build and test step in the wrong order, so we have to switch them here
-        test: ["yarn", "build"]
-        build: ["yarn", "test"]
+        # leeway executes the build and test step in the wrong order, so we need to call a special script that builds before testing
+        test: ["yarn", "test:leeway"]
   - name: docker
     type: docker
     deps:
diff --git a/components/server/package.json b/components/server/package.json
@@ -7,16 +7,16 @@
     "start": "node ./dist/main.js",
     "start-inspect": "node --inspect=0.0.0.0:9229 ./dist/main.js",
     "generate": "leeway run components/spicedb:generate-ts > src/authorization/definitions.ts && npx prettier --write src/authorization/definitions.ts",
-    "build": "yarn generate && yarn lint && npx tsc",
+    "build": "yarn clean && yarn generate && yarn lint && npx tsc",
     "lint": "yarn eslint src/*.ts src/**/*.ts",
     "lint:fix": "yarn eslint src/*.ts src/**/*.ts --fix",
-    "build:clean": "yarn clean && yarn build",
     "rebuild": "yarn build:clean",
     "build:watch": "watch 'yarn build' .",
     "watch": "leeway exec --package .:app --transitive-dependencies --filter-type yarn --components --parallel -- yarn build -w --preserveWatchOutput",
     "clean": "rimraf dist",
     "clean:node": "rimraf node_modules",
     "purge": "yarn clean && yarn clean:node && yarn run rimraf yarn.lock",
+    "test:leeway": "yarn build && yarn test",
     "test": "cleanup() { echo 'Cleanup started'; yarn stop-services; }; trap cleanup EXIT; yarn test:unit && yarn start-services && yarn test:db",
     "test:unit": "mocha --opts mocha.opts './**/*.spec.js' --exclude './node_modules/**'",
     "test:db": ". $(leeway run components/gitpod-db:db-test-env) && mocha --opts mocha.opts './**/*.spec.db.js'  --exclude './node_modules/**'",
diff --git a/components/server/src/iam/iam-session-app.spec.ts b/components/server/src/iam/iam-session-app.spec.ts
@@ -21,6 +21,7 @@ import { OIDCCreateSessionPayload } from "./iam-oidc-create-session-payload";
 import { TeamMemberInfo, TeamMemberRole, User } from "@gitpod/gitpod-protocol";
 import { OrganizationService } from "../orgs/organization-service";
 import { UserService } from "../user/user-service";
+import { UserDB } from "@gitpod/gitpod-db/lib";
 const expect = chai.expect;
 
 @suite(timeout(10000))
@@ -113,6 +114,11 @@ class TestIamSessionApp {
                 bind(UserAuthentication).toConstantValue(this.userAuthenticationMock as any);
                 bind(UserService).toConstantValue(this.userServiceMock as any);
                 bind(OrganizationService).toConstantValue(this.orgServiceMock as any);
+                bind(UserDB).toConstantValue(<any>{
+                    transaction: async (run: () => void) => {
+                        return run();
+                    },
+                }); // unused
             }),
         );
         this.app = container.get(IamSessionApp);
diff --git a/components/server/src/iam/iam-session-app.ts b/components/server/src/iam/iam-session-app.ts
@@ -16,6 +16,7 @@ import { reportJWTCookieIssued } from "../prometheus-metrics";
 import { ApplicationError } from "@gitpod/gitpod-protocol/lib/messaging/error";
 import { OrganizationService } from "../orgs/organization-service";
 import { UserService } from "../user/user-service";
+import { UserDB } from "@gitpod/gitpod-db/lib";
 
 @injectable()
 export class IamSessionApp {
@@ -26,6 +27,7 @@ export class IamSessionApp {
         @inject(UserService) private readonly userService: UserService,
         @inject(OrganizationService) private readonly orgService: OrganizationService,
         @inject(SessionHandler) private readonly session: SessionHandler,
+        @inject(UserDB) private readonly userDb: UserDB,
     ) {}
 
     public getMiddlewares() {
@@ -108,22 +110,6 @@ export class IamSessionApp {
             }
         }
 
-        if (existingUser?.organizationId) {
-            const members = await this.orgService.listMembers(existingUser.id, existingUser.organizationId);
-            if (!members.some((m) => m.userId === existingUser?.id)) {
-                // In case `createNewOIDCUser` failed to create a membership for this user,
-                // let's try to fix the situation on the fly.
-                // Also, if that step repeatedly fails, it would fail the login process earlier but
-                // in a more consistent state.
-                await this.orgService.addOrUpdateMember(
-                    undefined,
-                    existingUser.organizationId,
-                    existingUser.id,
-                    "member",
-                );
-            }
-        }
-
         return existingUser;
     }
 
@@ -147,17 +133,22 @@ export class IamSessionApp {
     private async createNewOIDCUser(payload: OIDCCreateSessionPayload): Promise<User> {
         const { claims, organizationId } = payload;
 
-        // Until we support SKIM (or any other means to sync accounts) we create new users here as a side-effect of the login
-        const user = await this.userService.createUser({
-            organizationId,
-            identity: { ...this.mapOIDCProfileToIdentity(payload), lastSigninTime: new Date().toISOString() },
-            userUpdate: (user) => {
-                user.name = claims.name;
-                user.avatarUrl = claims.picture;
-            },
+        return this.userDb.transaction(async (_, ctx) => {
+            // Until we support SKIM (or any other means to sync accounts) we create new users here as a side-effect of the login
+            const user = await this.userService.createUser(
+                {
+                    organizationId,
+                    identity: { ...this.mapOIDCProfileToIdentity(payload), lastSigninTime: new Date().toISOString() },
+                    userUpdate: (user) => {
+                        user.name = claims.name;
+                        user.avatarUrl = claims.picture;
+                    },
+                },
+                ctx,
+            );
+
+            await this.orgService.addOrUpdateMember(undefined, organizationId, user.id, "member", ctx);
+            return user;
         });
-
-        await this.orgService.addOrUpdateMember(undefined, organizationId, user.id, "member");
-        return user;
     }
 }
diff --git a/components/server/src/orgs/organization-service.spec.db.ts b/components/server/src/orgs/organization-service.spec.db.ts
@@ -120,7 +120,10 @@ describe("OrganizationService", async () => {
         owner = previouslyMember;
 
         // owner can downgrade themselves only if they are not the last owner
-        await expectError(ErrorCodes.CONFLICT, os.addOrUpdateMember(owner.id, org.id, owner.id, "member"));
+        await os.addOrUpdateMember(owner.id, org.id, owner.id, "member");
+        // verify they are still an owner
+        const members = await os.listMembers(owner.id, org.id);
+        expect(members.some((m) => m.userId === owner.id && m.role === "owner")).to.be.true;
 
         // owner can delete themselves only if they are not the last owner
         await expectError(ErrorCodes.CONFLICT, os.removeOrganizationMember(owner.id, org.id, owner.id));
diff --git a/components/server/src/orgs/organization-service.ts b/components/server/src/orgs/organization-service.ts
@@ -18,6 +18,7 @@ import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 import { inject, injectable } from "inversify";
 import { Authorizer } from "../authorization/authorizer";
 import { ProjectsService } from "../projects/projects-service";
+import { TransactionalContext } from "@gitpod/gitpod-db/lib/typeorm/transactional-db-impl";
 
 @injectable()
 export class OrganizationService {
@@ -183,90 +184,103 @@ export class OrganizationService {
         orgId: string,
         memberId: string,
         role: OrgMemberRole,
+        txCtx?: TransactionalContext,
     ): Promise<void> {
         if (userId) {
             await this.auth.checkPermissionOnOrganization(userId, "write_members", orgId);
         }
-        if (role !== "owner") {
-            const members = await this.teamDB.findMembersByTeam(orgId);
-            if (!members.some((m) => m.userId !== memberId && m.role === "owner")) {
-                throw new ApplicationError(ErrorCodes.CONFLICT, "Cannot remove the last owner of an organization.");
-            }
-        }
-        const members = await this.teamDB.findMembersByTeam(orgId);
-        const firstMember = members.filter((m) => m.userId !== BUILTIN_INSTLLATION_ADMIN_USER_ID).length === 0;
-        if (firstMember) {
-            // first member (that is not an admin) is going to be an owner
-            role = "owner";
-            log.info({ userId: memberId }, "First member of organization, setting role to owner.");
-        }
-
+        let members: OrgMemberInfo[] = [];
         try {
-            await this.teamDB.transaction(async (db) => {
-                await db.addMemberToTeam(memberId, orgId);
-                await db.setTeamMemberRole(memberId, orgId, role);
+            await this.teamDB.transaction(txCtx, async (teamDB, txCtx) => {
+                members = await teamDB.findMembersByTeam(orgId);
+                const hasOtherRegularOwners =
+                    members.filter(
+                        (m) =>
+                            m.userId !== BUILTIN_INSTLLATION_ADMIN_USER_ID && //
+                            m.userId !== memberId && //
+                            m.role === "owner",
+                    ).length > 0;
+                if (!hasOtherRegularOwners) {
+                    // first regular member is going to be an owner
+                    role = "owner";
+                    log.info({ userId: memberId }, "First member of organization, setting role to owner.");
+                }
+
+                await teamDB.addMemberToTeam(memberId, orgId);
+                await teamDB.setTeamMemberRole(memberId, orgId, role);
                 await this.auth.addOrganizationRole(orgId, memberId, role);
+                // we can remove the built-in installation admin if we have added an owner
+                if (!hasOtherRegularOwners && members.some((m) => m.userId === BUILTIN_INSTLLATION_ADMIN_USER_ID)) {
+                    try {
+                        await this.removeOrganizationMember(memberId, orgId, BUILTIN_INSTLLATION_ADMIN_USER_ID, txCtx);
+                    } catch (error) {
+                        log.warn("Failed to remove built-in installation admin from organization.", error);
+                    }
+                }
             });
         } catch (err) {
-            //TODO simply removing the user is not necessarily the right thing to do here, as the user might have been a member before
-            await this.auth.removeOrganizationRole(orgId, memberId, "member");
+            await this.auth.removeOrganizationRole(
+                orgId,
+                memberId,
+                members.find((m) => m.userId === memberId)?.role || "member",
+            );
             throw err;
         }
-        // we can remove the built-in installation admin now
-        if (firstMember) {
-            try {
-                await this.removeOrganizationMember(memberId, orgId, BUILTIN_INSTLLATION_ADMIN_USER_ID);
-            } catch (error) {
-                log.warn("Failed to remove built-in installation admin from organization.", error);
-            }
-        }
     }
 
-    public async removeOrganizationMember(userId: string, orgId: string, memberId: string): Promise<void> {
+    public async removeOrganizationMember(
+        userId: string,
+        orgId: string,
+        memberId: string,
+        txCtx?: TransactionalContext,
+    ): Promise<void> {
         // The user is leaving a team, if they are removing themselves from the team.
         if (userId === memberId) {
             await this.auth.checkPermissionOnOrganization(userId, "read_info", orgId);
         } else {
             await this.auth.checkPermissionOnOrganization(userId, "write_members", orgId);
         }
+        let membership: OrgMemberInfo | undefined;
+        try {
+            await this.teamDB.transaction(txCtx, async (db) => {
+                // Check for existing membership.
+                const members = await db.findMembersByTeam(orgId);
+                // cannot remove last owner
+                if (!members.some((m) => m.userId !== memberId && m.role === "owner")) {
+                    throw new ApplicationError(ErrorCodes.CONFLICT, "Cannot remove the last owner of an organization.");
+                }
 
-        // Check for existing membership.
-        const members = await this.teamDB.findMembersByTeam(orgId);
-        // cannot remove last owner
-        if (!members.some((m) => m.userId !== memberId && m.role === "owner")) {
-            throw new ApplicationError(ErrorCodes.CONFLICT, "Cannot remove the last owner of an organization.");
-        }
-
-        const membership = members.find((m) => m.userId === memberId);
-        if (!membership) {
-            throw new ApplicationError(
-                ErrorCodes.NOT_FOUND,
-                `Could not find membership for user '${memberId}' in organization '${orgId}'`,
-            );
-        }
+                membership = members.find((m) => m.userId === memberId);
+                if (!membership) {
+                    throw new ApplicationError(
+                        ErrorCodes.NOT_FOUND,
+                        `Could not find membership for user '${memberId}' in organization '${orgId}'`,
+                    );
+                }
 
-        // Check if user's account belongs to the Org.
-        const userToBeRemoved = await this.userDB.findUserById(memberId);
-        if (!userToBeRemoved) {
-            throw new ApplicationError(ErrorCodes.NOT_FOUND, `Could not find user '${memberId}'`);
-        }
-        // Only invited members can be removed from the Org, but organizational accounts cannot.
-        if (userToBeRemoved.organizationId && orgId === userToBeRemoved.organizationId) {
-            throw new ApplicationError(
-                ErrorCodes.PERMISSION_DENIED,
-                `User's account '${memberId}' belongs to the organization '${orgId}'`,
-            );
-        }
+                // Check if user's account belongs to the Org.
+                const userToBeRemoved = await this.userDB.findUserById(memberId);
+                if (!userToBeRemoved) {
+                    throw new ApplicationError(ErrorCodes.NOT_FOUND, `Could not find user '${memberId}'`);
+                }
+                // Only invited members can be removed from the Org, but organizational accounts cannot.
+                if (userToBeRemoved.organizationId && orgId === userToBeRemoved.organizationId) {
+                    throw new ApplicationError(
+                        ErrorCodes.PERMISSION_DENIED,
+                        `User's account '${memberId}' belongs to the organization '${orgId}'`,
+                    );
+                }
 
-        try {
-            await this.teamDB.transaction(async (db) => {
                 await db.removeMemberFromTeam(userToBeRemoved.id, orgId);
                 await this.auth.removeOrganizationRole(orgId, memberId, "member");
             });
         } catch (err) {
-            // Rollback to the original role the user had
-            await this.auth.addOrganizationRole(orgId, memberId, membership.role);
-            throw new ApplicationError(ErrorCodes.INTERNAL_SERVER_ERROR, err);
+            if (membership) {
+                // Rollback to the original role the user had
+                await this.auth.addOrganizationRole(orgId, memberId, membership.role);
+            }
+            const code = ApplicationError.hasErrorCode(err) ? err.code : ErrorCodes.INTERNAL_SERVER_ERROR;
+            throw new ApplicationError(code, err);
         }
         this.analytics.track({
             userId,
diff --git a/components/server/src/user/user-controller.ts b/components/server/src/user/user-controller.ts
@@ -138,10 +138,7 @@ export class UserController {
 
                 // The user has supplied a valid token, we need to sign them in.
                 // Login this user (sets cookie as side-effect)
-                const user = await this.userService.findUserById(
-                    BUILTIN_INSTLLATION_ADMIN_USER_ID,
-                    BUILTIN_INSTLLATION_ADMIN_USER_ID,
-                );
+                const user = await this.userDb.findUserById(BUILTIN_INSTLLATION_ADMIN_USER_ID);
                 if (!user) {
                     // We respond with NOT_AUTHENTICATED to prevent gleaning whether the user, or token are invalid.
                     throw new ApplicationError(ErrorCodes.NOT_AUTHENTICATED, "Admin user not found");
diff --git a/components/server/src/user/user-service.ts b/components/server/src/user/user-service.ts
