feat: fix wrong message Unauthorized to connect to Openproject

CHANGELOG.md

@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- Fix: Shown wrong message "Unauthorized to connect to Openproject" on Single-Sign-On setup [#1018](https://github.com/nextcloud/integration_openproject/pull/1018)
+
 ### Changed
 
 ### Removed

lib/Controller/ConfigController.php

@@ -155,7 +155,7 @@ public function setConfig(array $values): DataResponse {
 
 		if (isset($values['token'])) {
 			if ($values['token']) {
-				$result = $this->openprojectAPIService->initUserInfo($this->userId);
+				$result = $this->openprojectAPIService->initUserInfo($this->userId, $values['token']);
 			} else {
 				$this->clearUserInfo();
 				$result = [
@@ -536,7 +536,7 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe
 			);
 			if (isset($result['access_token']) && isset($result['refresh_token'])) {
 				// set user info
-				$userInfo = $this->openprojectAPIService->initUserInfo($this->userId);
+				$userInfo = $this->openprojectAPIService->initUserInfo($this->userId, $result['access_token']);
 				if (isset($userInfo['user_name'])) {
 					$this->config->setUserValue(
 						$this->userId, Application::APP_ID, 'oauth_connection_result', 'success'

lib/Service/OpenProjectAPIService.php

@@ -419,7 +419,7 @@ public function rawRequest(
 		string $method = 'GET',
 		array $options = []
 	) {
-		$url = $openprojectUrl . '/api/v3/' . $endPoint;
+		$url = $openprojectUrl . '/api/v3/' . ltrim($endPoint, '/');
 		if (!isset($options['headers']['Authorization'])) {
 			$options['headers']['Authorization'] = 'Bearer ' . $accessToken;
 		}
@@ -1716,13 +1716,6 @@ public function getOIDCToken(string $userId): string {
 		$this->config->setUserValue($userId, Application::APP_ID, 'token', $token->getAccessToken());
 		$this->config->setUserValue($userId, Application::APP_ID, 'token_expires_at', $tokenExpiresAt);
 
-		$savedUserId = $this->config->getUserValue($userId, Application::APP_ID, 'user_id');
-		$savedUsername = $this->config->getUserValue($userId, Application::APP_ID, 'user_name');
-		if (!$savedUserId || !$savedUsername) {
-			// get user info
-			$this->initUserInfo($userId);
-		}
-
 		return $token->getAccessToken();
 	}
 
@@ -1751,16 +1744,22 @@ public function getAccessToken(?string $userId): string {
 			return '';
 		}
 		$token = $this->config->getUserValue($userId, Application::APP_ID, 'token', '');
+		$authMethod = $this->config->getAppValue(Application::APP_ID, 'authorization_method');
+
 		if ($token && !$this->isAccessTokenExpired($userId)) {
+			if ($authMethod === SettingsService::AUTH_METHOD_OIDC) {
+				$this->initUserInfo($userId, $token);
+			}
 			return $token;
 		}
 
 		if ($token) {
 			$this->logger->debug('Token has expired.', ['app' => $this->appName]);
 			$this->logger->debug('Refreshing access token.', ['app' => $this->appName]);
+			$this->config->deleteUserValue($userId, Application::APP_ID, 'user_name');
+			$this->config->deleteUserValue($userId, Application::APP_ID, 'user_id');
 		}
 
-		$authMethod = $this->config->getAppValue(Application::APP_ID, 'authorization_method');
 		// For OAuth2 setup, only try to refresh the expired token.
 		// Token exchange needs to be initiated from the UI.
 		if ($authMethod === SettingsService::AUTH_METHOD_OAUTH && $token) {
@@ -1784,7 +1783,11 @@ public function getAccessToken(?string $userId): string {
 			}
 			return $result['access_token'];
 		} elseif ($authMethod === SettingsService::AUTH_METHOD_OIDC) {
-			return $this->getOIDCToken($userId);
+			$token = $this->getOIDCToken($userId);
+			if ($token) {
+				$this->initUserInfo($userId, $token);
+			}
+			return $token;
 		}
 
 		return '';
@@ -1796,8 +1799,20 @@ public function getAccessToken(?string $userId): string {
 	 * @return array<mixed>
 	 * @throws PreConditionNotMetException
 	 */
-	public function initUserInfo(string $userId): array {
-		$info = $this->request($userId, '/users/me');
+	public function initUserInfo(string $userId, string $accessToken): array {
+		$savedUserId = $this->config->getUserValue($userId, Application::APP_ID, 'user_id');
+		$savedUsername = $this->config->getUserValue($userId, Application::APP_ID, 'user_name');
+		if ($savedUserId && $savedUsername) {
+			return ['user_name' => $savedUsername];
+		}
+		try {
+			$openprojectUrl = $this->config->getAppValue(Application::APP_ID, 'openproject_instance_url');
+			$response = $this->rawRequest($accessToken, $openprojectUrl, '/users/me');
+			$info = json_decode($response->getBody(), true);
+		} catch (Exception $e) {
+			$this->logger->error('OpenProject error : ' . $e->getMessage(), ['app' => $this->appName]);
+			return ['error' => $e->getMessage()];
+		}
 		if (isset($info['lastName'], $info['firstName'], $info['id'])) {
 			$fullName = $info['firstName'] . ' ' . $info['lastName'];
 			$this->config->setUserValue($userId, Application::APP_ID, 'user_id', $info['id']);

tests/lib/Service/OpenProjectAPIServiceTest.php

@@ -783,18 +783,14 @@ private function getOpenProjectAPIService(
 		$tokenExpiryTime = $oAuth2OrOidcToken === 'expired' ? 0 : time() + 7200;
 		$this->defaultConfigMock
 			->method('getUserValue')
-			->withConsecutive(
-				[$userId, 'integration_openproject', 'token'],
-				[$userId, 'integration_openproject', 'token_expires_at'],
-				[$userId, 'integration_openproject', 'refresh_token'],
-				[$userId, 'integration_openproject', 'token'],
-			)
-			->willReturnOnConsecutiveCalls(
-				$oAuth2OrOidcToken,
-				$tokenExpiryTime,
-				'oAuthRefreshToken',
-				'new-Token'
-			);
+			->willReturnMap([
+				[$userId, 'integration_openproject', 'token', $oAuth2OrOidcToken],
+				[$userId, 'integration_openproject', 'token_expires_at', $tokenExpiryTime],
+				[$userId, 'integration_openproject', 'refresh_token', 'oAuthRefreshToken'],
+				[$userId, 'integration_openproject', 'token', 'new-Token'],
+				[$userId, 'integration_openproject', 'user_id', 'some-user-id'],
+				[$userId, 'integration_openproject', 'user_name', 'some-user-name'],
+			]);
 		if ($authMethod === OpenProjectAPIService::AUTH_METHOD_OIDC) {
 			$tokenMock = $this->getMockBuilder(Token::class)->disableOriginalConstructor()->getMock();
 			$exchangeTokenMock->method('getEvent')->willReturn($exchangedTokenRequestedEventMock);
@@ -4565,12 +4561,6 @@ public function testGetOIDCTokenSuccess(): void {
 			->willReturnMap($this->getAppValues([
 				'authorization_method' => OpenProjectAPIService::AUTH_METHOD_OIDC
 			]));
-		$configMock
-			->method('getUserValue')
-			->willReturnMap([
-				['testUser', Application::APP_ID, 'user_id', null],
-				['testUser', Application::APP_ID, 'user_name', null],
-			]);
 		$calls = [];
 		$configMock
 			->method('setUserValue')
@@ -4587,15 +4577,14 @@ public function testGetOIDCTokenSuccess(): void {
 		$eventMock->method('getToken')->willReturn($tokenMock);
 		$tokenMock->method('getAccessToken')->willReturn('exchanged-access-token');
 		$service = $this->getOpenProjectAPIServiceMock(
-			['initUserInfo'],
+			[],
 			[
 				'appManager' => $iAppManagerMock,
 				'config' => $configMock,
 				'manager' => $iManagerMock,
 				'tokenEventFactory' => $exchangeTokenEvent,
 			],
 		);
-		$service->expects($this->once())->method('initUserInfo')->with('testUser');
 		$result = $service->getOIDCToken('testUser');
 		$this->assertEquals('exchanged-access-token', $result);
 		$expectedCalls = [
@@ -4815,7 +4804,7 @@ public function testGetAccessToken(
 				['testUser', Application::APP_ID, 'refresh_token', '', 'refresh-token'],
 			]);
 		$service = $this->getOpenProjectAPIServiceMock(
-			['isAccessTokenExpired', 'getOIDCToken', 'requestOAuthAccessToken'],
+			['isAccessTokenExpired', 'getOIDCToken', 'requestOAuthAccessToken', 'initUserInfo'],
 			[
 				'config' => $configMock,
 			],
@@ -4827,6 +4816,25 @@ public function testGetAccessToken(
 			->willReturn($expired);
 		}
 
+		if ($token && $expired) {
+			$configMock
+				->expects($this->exactly(2))
+				->method("deleteUserValue")
+				->withConsecutive(
+					['testUser', 'integration_openproject', 'user_name'],
+					['testUser', 'integration_openproject', 'user_id'],
+				);
+		}
+
+		if ($token && $authMethod === SettingsService::AUTH_METHOD_OIDC && (!$expired || $expectedToken)) {
+			$expectedTokenForInitUserInfo = $expired ? $expectedToken : $token;
+			$service->expects($this->once())
+				->method('initUserInfo')
+				->with('testUser', $expectedTokenForInitUserInfo);
+		} else {
+			$service->expects($this->never())->method('initUserInfo');
+		}
+
 		if ($authMethod === SettingsService::AUTH_METHOD_OAUTH && $expired) {
 			if ($tokenRefreshFailed) {
 				$service->expects($this->once())
@@ -5239,4 +5247,103 @@ public function testGetNCBaseUrl(string $url, string $expected): void {
 		$baseUrl = $service->getNCBaseUrl();
 		$this->assertEquals($expected, $baseUrl);
 	}
+
+	/**
+	 * Data provider for testInitUserInfo
+	 */
+	public function initUserInfoDataProvider(): array {
+		return [
+			'user info already saved' => [
+				'savedUserId' => 'testUserID',
+				'savedUsername' => 'test User',
+				'expectedResult' => ['user_name' => 'test User'],
+				'expectRawRequestCalled' => false,
+				'apiResponse' => null,
+				'expectedExpection' => null
+			],
+			'user info missing and API succeeds' => [
+				'savedUserId' => '',
+				'savedUsername' => '',
+				'expectedResult' => ['user_name' => 'test User'],
+				'expectRawRequestCalled' => true,
+				'apiResponse' => [
+					'id' => 'testUserID',
+					'firstName' => 'test',
+					'lastName' => 'User',
+				],
+				'expectedExpection' => null
+			],
+			'user info missing and API throws exception' => [
+				'savedUserId' => '',
+				'savedUsername' => '',
+				'expectedResult' => ['error' => 'OpenProject error'],
+				'expectRawRequestCalled' => true,
+				'apiResponse' => null,
+				'expectedExpection' => new \Exception('OpenProject error')
+			],
+			'user info missing and API returns incomplete data' => [
+				'savedUserId' => '',
+				'savedUsername' => '',
+				'expectedResult' => ['id' => 'testUserID', 'error' => 'Failed to get user profile'],
+				'expectRawRequestCalled' => true,
+				'apiResponse' => [
+					'id' => 'testUserID',
+				],
+				'expectedExpection' => null
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider initUserInfoDataProvider
+	 * @param string $savedUserId
+	 * @param string $savedUsername
+	 * @param array $expectedResult
+	 * @param bool $expectRawRequestCalled
+	 * @param array|null $apiResponse
+	 * @param \Exception|null $expectedExpection
+	 * @return void
+	 */
+	public function testInitUserInfo(string $savedUserId, string $savedUsername, array $expectedResult, bool $expectRawRequestCalled, ?array $apiResponse, ?\Exception $expectedExpection): void {
+		$configMock = $this->getMockBuilder(IConfig::class)->getMock();
+		$configMock->method('getAppValue')
+			->willReturnMap($this->getAppValues([
+				'openproject_instance_url' => 'http://test.local',
+			]));
+		$configMock
+			->method('getUserValue')
+			->willReturnMap([
+				['testUser', Application::APP_ID, 'user_id', '', $savedUserId],
+				['testUser', Application::APP_ID, 'user_name','', $savedUsername]
+			]);
+
+		$calls = [];
+		$configMock
+			->method('setUserValue')
+			->willReturnCallback(function ($uid, $app, $key, $value) use (&$calls) {
+				$calls[] = [$uid, $app, $key, $value];
+			});
+
+		$service = $this->getOpenProjectAPIServiceMock(['rawRequest'], [ 'config' => $configMock ]);
+
+		if (!$expectRawRequestCalled) {
+			$service->expects($this->never())->method('rawRequest');
+		} elseif ($expectedExpection !== null) {
+			$service->expects($this->once())->method('rawRequest')->willThrowException($expectedExpection);
+		} else {
+			$mockResponse = $this->createMock(Response::class);
+			$mockResponse->method('getBody')->willReturn(json_encode($apiResponse));
+			$service->expects($this->once())->method('rawRequest')->willReturn(
+				$mockResponse
+			);
+		}
+
+		$result = $service->initUserInfo('testUser', 'token');
+		$this->assertSame($expectedResult, $result);
+
+		if ($expectedResult === ['user_name' => 'test User'] && $expectRawRequestCalled) {
+			$this->assertContains(['testUser', Application::APP_ID, 'user_id', 'testUserID'], $calls);
+			$this->assertContains(['testUser', Application::APP_ID, 'user_name', 'test User'], $calls);
+		}
+	}
 }