diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d2cd26b6..c4992f7e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Added +### Fixed + +- Fix: Do not create OAuth client when setting up with OIDC auth method [#1049](https://github.com/nextcloud/integration_openproject/pull/1049) + ### Changed ### Removed diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 8d924bdf3..36eb7f1a4 100755 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -651,29 +651,38 @@ public function setUpIntegration(?array $values): DataResponse { // check all required settings $this->settingsService->validateAdminSettingsForm($values, true); - $status = $this->setIntegrationConfig($values); - $result = $this->recreateOauthClientInformation(); - if ($status['oPOAuthTokenRevokeStatus'] !== '') { - $result['openproject_revocation_status'] = $status['oPOAuthTokenRevokeStatus']; + $setup = $this->setIntegrationConfig($values); + $response = ['status' => $setup['status']]; + + if ($values['authorization_method'] === OpenProjectAPIService::AUTH_METHOD_OAUTH) { + $response = \array_merge($response, $this->recreateOauthClientInformation()); } - if ($status['oPUserAppPassword'] !== null) { - $result['openproject_user_app_password'] = $status['oPUserAppPassword']; + + if ($setup['oPOAuthTokenRevokeStatus']) { + $response['openproject_revocation_status'] = $setup['oPOAuthTokenRevokeStatus']; } - return new DataResponse($result); + if ($setup['oPUserAppPassword']) { + $response['openproject_user_app_password'] = $setup['oPUserAppPassword']; + } + return new DataResponse($response); } catch (OpenprojectGroupfolderSetupConflictException $e) { return new DataResponse([ + 'status' => false, 'error' => $this->l->t($e->getMessage()), ], Http::STATUS_CONFLICT); } catch (NoUserException $e) { return new DataResponse([ + 'status' => false, 'error' => $this->l->t($e->getMessage()) ], Http::STATUS_NOT_FOUND); } catch (InvalidArgumentException $e) { return new DataResponse([ + 'status' => false, "error" => $e->getMessage() ], Http::STATUS_BAD_REQUEST); } catch (\Exception $e) { return new DataResponse([ + 'status' => false, "error" => $e->getMessage() ], Http::STATUS_INTERNAL_SERVER_ERROR); } diff --git a/psalm.xml b/psalm.xml index fdcf738b0..8113062e7 100644 --- a/psalm.xml +++ b/psalm.xml @@ -21,6 +21,9 @@ + + + diff --git a/tests/lib/Controller/ConfigControllerTest.php b/tests/lib/Controller/ConfigControllerTest.php index 851418f1c..6757d2dde 100644 --- a/tests/lib/Controller/ConfigControllerTest.php +++ b/tests/lib/Controller/ConfigControllerTest.php @@ -154,11 +154,10 @@ private function getConfigControllerMock( array $constructParams = [], ): MockObject { $constructArgs = $this->getConfigControllerConstructArgs($constructParams); - $mock = $this->getMockBuilder(ConfigController::class) + return $this->getMockBuilder(ConfigController::class) ->setConstructorArgs($constructArgs) ->onlyMethods($mockMethods) ->getMock(); - return $mock; } public function testOauthRedirectSuccess():void { @@ -1875,8 +1874,8 @@ public function integrationDefaultSettingsProvider() { } /** - * @param array $oldCreds - * @param array $credsToUpdate + * @param array $settings + * @param array $expectedSettings * @return void * @dataProvider integrationDefaultSettingsProvider */ @@ -1899,4 +1898,129 @@ public function testSetUpIntegrationDefaultSettings(array $settings, array $expe $configController = new ConfigController(...$constructArgs); $configController->setUpIntegration($settings); } + + /** + * @return array + */ + public function setUpIntegrationSuccessProvider(): array { + $defaultSettings = [ + 'openproject_instance_url' => 'https://test.example.com', + 'default_enable_navigation' => false, + 'default_enable_unified_search' => false, + 'setup_project_folder' => false, + 'setup_app_password' => false, + ]; + return [ + "complete oauth2 setup" => [ + "authMethod" => OpenProjectAPIService::AUTH_METHOD_OAUTH, + 'settings' => [ + ...$defaultSettings, + 'authorization_method' => OpenProjectAPIService::AUTH_METHOD_OAUTH, + 'openproject_client_id' => 'test', + 'openproject_client_secret' => 'test', + ], + 'response' => [ + 'status', + 'nextcloud_oauth_client_name', + 'nextcloud_client_id', + 'nextcloud_client_secret', + 'openproject_redirect_uri', + ], + ], + "complete oidc setup" => [ + "authMethod" => OpenProjectAPIService::AUTH_METHOD_OIDC, + 'settings' => [ + ...$defaultSettings, + 'authorization_method' => OpenProjectAPIService::AUTH_METHOD_OIDC, + 'sso_provider_type' => SettingsService::NEXTCLOUDHUB_OIDC_PROVIDER_TYPE, + 'targeted_audience_client_id' => 'test', + ], + 'response' => [ + 'status', + ], + ], + "with team folder" => [ + "authMethod" => OpenProjectAPIService::AUTH_METHOD_OIDC, + 'settings' => array_merge($defaultSettings, [ + 'authorization_method' => OpenProjectAPIService::AUTH_METHOD_OIDC, + 'sso_provider_type' => SettingsService::NEXTCLOUDHUB_OIDC_PROVIDER_TYPE, + 'targeted_audience_client_id' => 'test', + 'setup_project_folder' => true, + 'setup_app_password' => true, + ]), + 'response' => [ + 'status', + 'openproject_user_app_password', + ], + ], + ]; + } + + /** + * @param string $authMethod + * @param array $settings + * @param array $responseProps + * @return void + * @dataProvider setUpIntegrationSuccessProvider + */ + public function testSetUpIntegrationSuccess(string $authMethod, array $settings, array $responseProps): void { + $userManagerMock = $this->createMock(IUserManager::class); + $userManagerMock->method('userExists')->willReturn(true); + $oauthMock = $this->createMock(OauthService::class); + + if ($authMethod === OpenProjectAPIService::AUTH_METHOD_OAUTH) { + $oauthMock->expects($this->once()) + ->method('createNcOauthClient')->willReturn([ + 'id' => '1234', + 'nextcloud_oauth_client_name' => 'Openproject Client', + 'openproject_redirect_uri' => 'http://openproject.test/oauth/callback', + 'nextcloud_client_id' => 'client_id', + 'nextcloud_client_secret' => 'client_secret', + ]); + } else { + $oauthMock->expects($this->never()) + ->method('createNcOauthClient'); + } + + $openprojectAPIServiceMock = $this->createMock(OpenProjectAPIService::class); + if ($settings['setup_app_password']) { + $openprojectAPIServiceMock->expects($this->once()) + ->method('generateAppPasswordTokenForUser') + ->willReturn('app_pass'); + } else { + $openprojectAPIServiceMock->expects($this->never()) + ->method('generateAppPasswordTokenForUser'); + } + $settingsServiceMock = $this->createMock(SettingsService::class); + $settingsServiceMock->expects($this->once()) + ->method('validateAdminSettingsForm'); + + $constructArgs = [ + 'oauthService' => $oauthMock, + 'settingsService' => $settingsServiceMock, + 'openprojectAPIService' => $openprojectAPIServiceMock, + 'userManager' => $userManagerMock, + ]; + $constructArgs = $this->getConfigControllerConstructArgs($constructArgs); + $configController = new ConfigController(...$constructArgs); + + $response = $configController->setUpIntegration($settings); + $data = $response->getData(); + + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); + $this->assertArrayHasKey('status', $data); + + foreach ($responseProps as $prop) { + $this->assertArrayHasKey($prop, $data); + } + if ($authMethod === OpenProjectAPIService::AUTH_METHOD_OIDC) { + $this->assertArrayNotHasKey('nextcloud_oauth_client_name', $data); + $this->assertArrayNotHasKey('nextcloud_client_id', $data); + $this->assertArrayNotHasKey('nextcloud_client_secret', $data); + $this->assertArrayNotHasKey('openproject_redirect_uri', $data); + } + if (!$settings['setup_app_password']) { + $this->assertArrayNotHasKey('openproject_user_app_password', $data); + } + } }