From c24c6ed2be731d2c6377af7997b496c16a1b6ba6 Mon Sep 17 00:00:00 2001
From: Anupam Kumar <kyteinsky@gmail.com>
Date: Fri, 4 Oct 2024 04:51:26 +0530
Subject: [PATCH] fix: store a/c info only when creds are valid

and better error handling

Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
---
 lib/Controller/ConfigController.php | 35 +++++++++++++++++++++--------
 lib/Service/GitlabAPIService.php    | 11 +++++----
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php
index 006a4cb..76a0530 100644
--- a/lib/Controller/ConfigController.php
+++ b/lib/Controller/ConfigController.php
@@ -68,15 +68,22 @@ public function setConfig(array $values): DataResponse {
 	 * @NoAdminRequired
 	 */
 	public function addAccount(string $url, string $token) {
-		try {
 			$account = new GitlabAccount();
 			$account->setUserId($this->userId);
 			$account->setUrl($url);
 			$account->setToken($token);
 			$account->setTokenType('personal');
 
+		try {
+			$userInfo = $this->getUserInfo($account);
+			$account->setUserInfoName($userInfo['username']);
+			$account->setUserInfoDisplayName($userInfo['name']);
+		} catch (Exception $e) {
+			return new DataResponse(['error' => $e->getMessage()], $e->getCode());
+		}
+
+		try{
 			$this->accountMapper->insert($account);
-			$this->storeUserInfo($account);
 			$this->updateAccountsConfig();
 
 			return new DataResponse([
@@ -84,8 +91,8 @@ public function addAccount(string $url, string $token) {
 				'config' => UserConfig::loadConfig($this->userId, $this->config)->toArray(),
 			]);
 		} catch (Exception $e) {
-			$this->logger->error('Failed to query Gitlab account: ' . $e->getMessage(), ['exception' => $e]);
-			return new DataResponse([], 500);
+			$this->logger->error('Failed to save the Gitlab account: ' . $e->getMessage(), ['exception' => $e]);
+			return new DataResponse(['error' => 'Server Error: Failed to save the Gitlab account'], 500);
 		}
 	}
 
@@ -231,12 +238,22 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe
 		);
 	}
 
-	private function storeUserInfo(GitlabAccount $account): void {
+	/**
+	 * @param GitlabAccount $account
+	 * @return array{username: string, name: string}
+	 * @throws Exception
+	 */
+	private function getUserInfo(GitlabAccount $account): array {
 		$info = $this->gitlabAPIService->request($account, $account->getUrl(), 'user');
-		if (isset($info['username']) && isset($info['id'])) {
-			$account->setUserInfoName($info['username']);
-			$account->setUserInfoDisplayName($info['name']);
-			$this->accountMapper->update($account);
+		if (isset($info['error'])) {
+			throw new Exception($info['error'], $info['code'] ?? 500);
+		}
+		if (!isset($info['username'])) {
+			throw new Exception('Invalid response from Gitlab API, missing username', 500);
 		}
+		return [
+			'username' => $info['username'],
+			'name' => $info['name'] ?? $info['username'],
+		];
 	}
 }
diff --git a/lib/Service/GitlabAPIService.php b/lib/Service/GitlabAPIService.php
index 1fa14b2..60d5b45 100644
--- a/lib/Service/GitlabAPIService.php
+++ b/lib/Service/GitlabAPIService.php
@@ -278,22 +278,25 @@ public function request(?GitlabAccount $account, string $baseUrl, string $endPoi
 			} elseif ($method === 'DELETE') {
 				$response = $this->client->delete($url, $options);
 			} else {
-				return ['error' => $this->l10n->t('Bad HTTP method')];
+				return ['error' => $this->l10n->t('Bad HTTP method'), 'code' => 405];
 			}
 			$body = $response->getBody();
 			$respCode = $response->getStatusCode();
 
 			if ($respCode >= 400) {
-				return ['error' => $this->l10n->t('Bad credentials')];
+				return ['error' => $this->l10n->t('Bad credentials'), 'code' => $respCode];
 			} else {
 				return json_decode($body, true);
 			}
 		} catch (ServerException | ClientException $e) {
 			$this->logger->warning('GitLab API error : '.$e->getMessage(), ['app' => Application::APP_ID]);
-			return ['error' => 'Authentication failed'];
+			if ($e->getCode() == 401) {
+				return ['error' => $this->l10n->t('Bad credentials'), 'code' => 401];
+			}
+			return ['error' => 'Gitlab API error, please check the server logs for more details', 'code' => $e->getCode()];
 		} catch (ConnectException $e) {
 			$this->logger->warning('GitLab API error : '.$e->getMessage(), ['app' => Application::APP_ID]);
-			return ['error' => $e->getMessage()];
+			return ['error' => 'Connection error, please check the server logs for more details', 'code' => 500];
 		}
 	}