address review comments

Author: AlexTugarev

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,

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, {