fix: store a/c info only when creds are valid

and better error handling Signed-off-by: Anupam Kumar <[email protected]>
nextcloud · Oct 3, 2024 · 5fa487c · 5fa487c
1 parent 3391464
commit 5fa487c
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 19 deletions.
diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php
@@ -68,24 +68,31 @@ public function setConfig(array $values): DataResponse {
 	 * @NoAdminRequired
 	 */
 	public function addAccount(string $url, string $token) {
+		$account = new GitlabAccount();
+		$account->setUserId($this->userId);
+		$account->setUrl($url);
+		$account->setToken($token);
+		$account->setTokenType('personal');
+
 		try {
-			$account = new GitlabAccount();
-			$account->setUserId($this->userId);
-			$account->setUrl($url);
-			$account->setToken($token);
-			$account->setTokenType('personal');
+			$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([
 				'account' => $account->jsonSerialize(),
 				'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);
 		}
 	}
 
@@ -198,9 +205,18 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe
 					$expiresAt = $nowTs + (int)$result['expires_in'];
 					$account->setTokenExpiresAt($expiresAt);
 				}
-				$this->accountMapper->insert($account);
-				$this->storeUserInfo($account);
 
+				try {
+					$userInfo = $this->getUserInfo($account);
+					$account->setUserInfoName($userInfo['username']);
+					$account->setUserInfoDisplayName($userInfo['name']);
+				} catch (Exception $e) {
+					return new RedirectResponse(
+						$this->urlGenerator->linkToRoute('settings.PersonalSettings.index', ['section' => 'connected-accounts']) .
+						'?gitlabToken=error&message=' . urlencode($e->getMessage())
+					);
+				}
+				$this->accountMapper->insert($account);
 				$this->updateAccountsConfig();
 
 				$oauthOrigin = $this->config->getUserOauthOrigin($this->userId);
@@ -231,12 +247,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
@@ -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];
 		}
 	}