split internal upsert method updateAuthProvider into create and update

Author: AlexTugarev

components/server/src/auth/auth-provider-service.ts

@@ -112,45 +112,61 @@ export class AuthProviderService {
         return result;
     }
 
-    async updateAuthProvider(
-        userId: string,
-        entry: AuthProviderEntry.UpdateEntry | AuthProviderEntry.NewEntry,
-    ): Promise<AuthProviderEntry> {
+    async createAuthProviderOfUser(userId: string, entry: AuthProviderEntry.NewEntry): Promise<AuthProviderEntry> {
         await this.auth.checkPermissionOnUser(userId, "write_info", userId);
 
-        let authProvider: AuthProviderEntry;
-        if ("id" in entry) {
-            const { id, ownerId } = entry;
-            const existing = (await this.authProviderDB.findByUserId(ownerId)).find((p) => p.id === id);
-            if (!existing) {
-                throw new Error("Provider does not exist.");
-            }
-            const changed =
-                entry.clientId !== existing.oauth.clientId ||
-                (entry.clientSecret && entry.clientSecret !== existing.oauth.clientSecret);
+        const host = entry.host && entry.host.toLowerCase();
 
-            if (!changed) {
-                return existing;
-            }
+        // reachability test
+        if (!(await this.isHostReachable(host))) {
+            log.info(`Host could not be reached.`, { entry });
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, `Host could not be reached.`);
+        }
 
-            // update config on demand
-            const oauth = {
-                ...existing.oauth,
-                clientId: entry.clientId,
-                clientSecret: entry.clientSecret || existing.oauth.clientSecret, // FE may send empty ("") if not changed
-            };
-            authProvider = {
-                ...existing,
-                oauth,
-                status: "pending",
-            };
-        } else {
-            const existing = await this.authProviderDB.findByHost(entry.host);
-            if (existing) {
-                throw new Error("Provider for this host already exists.");
-            }
-            authProvider = this.initializeNewProvider(entry);
+        // checking for already existing runtime providers
+        const hostContext = this.hostContexts.get(host);
+        if (hostContext) {
+            const builtInExists = hostContext.authProvider.params.ownerId === undefined;
+            log.info(`Attempt to override an existing provider.`, { entry, builtInExists });
+            throw new ApplicationError(ErrorCodes.CONFLICT, `Attempt to override an existing provider.`);
+        }
+
+        const existing = await this.authProviderDB.findByHost(entry.host);
+        if (existing) {
+            log.info(`Attempt to override an existing provider.`, { entry });
+            throw new ApplicationError(ErrorCodes.CONFLICT, `Provider for this host already exists.`);
         }
+        const authProvider = this.initializeNewProvider(entry);
+        return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
+    }
+
+    async updateAuthProviderOfUser(userId: string, entry: AuthProviderEntry.UpdateEntry): Promise<AuthProviderEntry> {
+        await this.auth.checkPermissionOnUser(userId, "write_info", userId);
+
+        const { id, ownerId } = entry;
+        const existing = (await this.authProviderDB.findByUserId(ownerId)).find((p) => p.id === id);
+        if (!existing) {
+            throw new Error("Provider does not exist.");
+        }
+        const changed =
+            entry.clientId !== existing.oauth.clientId ||
+            (entry.clientSecret && entry.clientSecret !== existing.oauth.clientSecret);
+
+        if (!changed) {
+            return existing;
+        }
+
+        // update config on demand
+        const oauth = {
+            ...existing.oauth,
+            clientId: entry.clientId,
+            clientSecret: entry.clientSecret || existing.oauth.clientSecret, // FE may send empty ("") if not changed
+        };
+        const authProvider = {
+            ...existing,
+            oauth,
+            status: "pending",
+        };
         return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
     }
 

components/server/src/workspace/gitpod-server-impl.ts

@@ -2939,25 +2939,15 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
 
         const safeProvider = this.redactUpdateOwnAuthProviderParams({ entry });
         try {
+            if ("id" in safeProvider) {
+                const result = await this.authProviderService.updateAuthProviderOfUser(user.id, safeProvider);
+                return AuthProviderEntry.redact(result);
+            }
             if ("host" in safeProvider) {
-                // on creating we're are checking for already existing runtime providers
-
-                const host = safeProvider.host && safeProvider.host.toLowerCase();
-
-                if (!(await this.authProviderService.isHostReachable(host))) {
-                    log.debug(`Host could not be reached.`, { entry, safeProvider });
-                    throw new Error("Host could not be reached.");
-                }
-
-                const hostContext = this.hostContextProvider.get(host);
-                if (hostContext) {
-                    const builtInExists = hostContext.authProvider.params.ownerId === undefined;
-                    log.debug(`Attempt to override existing auth provider.`, { entry, safeProvider, builtInExists });
-                    throw new Error("Provider for this host already exists.");
-                }
+                const result = await this.authProviderService.createAuthProviderOfUser(user.id, safeProvider);
+                return AuthProviderEntry.redact(result);
             }
-            const result = await this.authProviderService.updateAuthProvider(safeProvider);
-            return AuthProviderEntry.redact(result);
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "Unexpected parameters.");
         } catch (error) {
             if (ApplicationError.hasErrorCode(error)) {
                 throw error;