From 53e3ecb1439c7395915139873f55ca228e851e86 Mon Sep 17 00:00:00 2001
From: Alex Tugarev <alex@gitpod.io>
Date: Wed, 8 Nov 2023 09:34:26 +0000
Subject: [PATCH] address review comments

---
 .../server/src/auth/auth-provider-service.ts       | 14 +++++++++-----
 .../server/src/workspace/gitpod-server-impl.ts     | 11 +----------
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/components/server/src/auth/auth-provider-service.ts b/components/server/src/auth/auth-provider-service.ts
index db5060cc64ef30..86606d7a0b8e18 100644
--- a/components/server/src/auth/auth-provider-service.ts
+++ b/components/server/src/auth/auth-provider-service.ts
@@ -214,10 +214,14 @@ export class AuthProviderService {
         await this.authProviderDB.delete(authProvider);
     }
 
-    async getAuthProvider(userId: string, id: string): Promise<AuthProviderEntry | undefined> {
+    /**
+     * Returns the provider identified by the specified `id`. Throws `NOT_FOUND` error if the resource
+     * is not found.
+     */
+    async getAuthProvider(userId: string, id: string): Promise<AuthProviderEntry> {
         const result = await this.authProviderDB.findById(id);
         if (!result) {
-            return undefined;
+            throw new ApplicationError(ErrorCodes.NOT_FOUND, "Provider resource not found.");
         }
 
         if (result.organizationId) {
@@ -266,7 +270,7 @@ export class AuthProviderService {
         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.");
+            throw new ApplicationError(ErrorCodes.NOT_FOUND, "Provider resource not found.");
         }
         const changed =
             entry.clientId !== existing.oauth.clientId ||
@@ -325,7 +329,7 @@ export class AuthProviderService {
         // TODO can we change this to query for the provider by id and org instead of loading all from org?
         const existing = (await this.authProviderDB.findByOrgId(organizationId)).find((p) => p.id === id);
         if (!existing) {
-            throw new Error("Provider does not exist.");
+            throw new ApplicationError(ErrorCodes.NOT_FOUND, "Provider resource not found.");
         }
         const changed =
             entry.clientId !== existing.oauth.clientId ||
@@ -368,7 +372,7 @@ export class AuthProviderService {
                 break;
         }
         if (!urls) {
-            throw new Error("Unexpected service type.");
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "Unexpected service type.");
         }
         const oauth: AuthProviderEntry["oauth"] = {
             ...urls,
diff --git a/components/server/src/workspace/gitpod-server-impl.ts b/components/server/src/workspace/gitpod-server-impl.ts
index c3ec3ba6ab4712..b2886a6c5006de 100644
--- a/components/server/src/workspace/gitpod-server-impl.ts
+++ b/components/server/src/workspace/gitpod-server-impl.ts
@@ -3070,9 +3070,6 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         const user = await this.checkAndBlockUser("getAuthProvider");
 
         const result = await this.authProviderService.getAuthProvider(user.id, id);
-        if (!result) {
-            throw new ApplicationError(ErrorCodes.NOT_FOUND, "Provider resource not found.");
-        }
         return AuthProviderEntry.redact(result);
     }
 
@@ -3085,11 +3082,8 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
 
         const user = await this.checkAndBlockUser("deleteAuthProvider");
 
+        // TODO(at) get rid of the additional read here when user-level providers are migrated to org-level.
         const authProvider = await this.authProviderService.getAuthProvider(user.id, id);
-        if (!authProvider) {
-            throw new ApplicationError(ErrorCodes.NOT_FOUND, "Provider resource not found.");
-        }
-
         if (authProvider.organizationId) {
             return this.deleteOrgAuthProvider(ctx, { id, organizationId: authProvider.organizationId });
         } else {
@@ -3111,9 +3105,6 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         const user = await this.checkAndBlockUser("updateAuthProvider");
 
         const authProvider = await this.authProviderService.getAuthProvider(user.id, id);
-        if (!authProvider) {
-            throw new ApplicationError(ErrorCodes.NOT_FOUND, "Provider resource not found.");
-        }
 
         if (authProvider.organizationId) {
             return this.updateOrgAuthProvider(ctx, {