diff --git a/CHANGELOG.md b/CHANGELOG.md index 74e002231..64de5ff60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Enhance project search when creating workpackages from Nextcloud - Drop application's support for Nextcloud 26 - Fix issue preventing direct uploading of resources in Nextcloud that are managed by app `Files Access Control` +- Hash or encrypt `client_secret` for different Nextcloud versions ## 2.6.4 - 2024-08-15 ### Changed diff --git a/lib/Service/OauthService.php b/lib/Service/OauthService.php index 547f64a4d..21c25000e 100644 --- a/lib/Service/OauthService.php +++ b/lib/Service/OauthService.php @@ -47,6 +47,29 @@ public function __construct(ClientMapper $clientMapper, $this->crypto = $crypto; } + /** + * @param string $secret + * @param string $nextcloudVersion + * @return string + */ + public function hashOrEncryptSecretBasedOnNextcloudVersion(string $secret, string $nextcloudVersion): string { + switch (true) { + case version_compare($nextcloudVersion, '30.0.0') >= 0: + case version_compare($nextcloudVersion, '29.0.7') >= 0 && version_compare($nextcloudVersion, '30.0.0') < 0: + case version_compare($nextcloudVersion, '28.0.10') >= 0 && version_compare($nextcloudVersion, '29.0.0') < 0: + case version_compare($nextcloudVersion, '27.1.11.8') >= 0 && version_compare($nextcloudVersion, '28.0.0') < 0: + $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + break; + case version_compare($nextcloudVersion, '27.0.0') === 0: + $encryptedSecret = $secret; + break; + default: + $encryptedSecret = $this->crypto->encrypt($secret); + break; + } + return $encryptedSecret; + } + /** * @param string $name * @param string $redirectUri @@ -58,12 +81,8 @@ public function createNcOauthClient(string $name, string $redirectUri): array { $client->setName($name); $client->setRedirectUri(sprintf($redirectUri, $clientId)); $secret = $this->secureRandom->generate(64, self::validChars); - if (version_compare(OC_Util::getVersionString(), '27.0.1') >= 0) { - $encryptedSecret = $this->crypto->encrypt($secret); - } else { - $encryptedSecret = $secret; - } - $client->setSecret($encryptedSecret); + $nextcloudVersion = OC_Util::getVersionString(); + $client->setSecret($this->hashOrEncryptSecretBasedOnNextcloudVersion($secret, $nextcloudVersion)); $client->setClientIdentifier($clientId); $client = $this->clientMapper->insert($client); @@ -87,8 +106,7 @@ public function getClientInfo(int $id): ?array { 'id' => $client->getId(), 'nextcloud_oauth_client_name' => $client->getName(), 'openproject_redirect_uri' => $client->getRedirectUri(), - 'nextcloud_client_id' => $client->getClientIdentifier(), - 'nextcloud_client_secret' => $this->crypto->decrypt($client->getSecret()), + 'nextcloud_client_id' => $client->getClientIdentifier() ]; } catch (ClientNotFoundException $e) { return null; diff --git a/tests/acceptance/features/api/setup.feature b/tests/acceptance/features/api/setup.feature index c7d01bb7e..40f165560 100644 --- a/tests/acceptance/features/api/setup.feature +++ b/tests/acceptance/features/api/setup.feature @@ -656,14 +656,12 @@ Feature: setup the integration through an API "required": [ "nextcloud_oauth_client_name", "openproject_redirect_uri", - "nextcloud_client_id", - "nextcloud_client_secret" + "nextcloud_client_id" ], "properties": { "nextcloud_oauth_client_name": {"type": "string", "pattern": "^OpenProject client$"}, "openproject_redirect_uri": {"type": "string", "pattern": "^http:\/\/some-host.de\/oauth_clients\/[A-Za-z0-9]+\/callback$"}, "nextcloud_client_id": {"type": "string", "pattern": "[A-Za-z0-9]+"}, - "nextcloud_client_secret": {"type": "string", "pattern": "[A-Za-z0-9]+"}, "openproject_user_app_password": {"type": "string", "pattern": "[A-Za-z0-9]+"} } } diff --git a/tests/lib/Service/OauthSeviceTest.php b/tests/lib/Service/OauthSeviceTest.php new file mode 100644 index 000000000..e49242989 --- /dev/null +++ b/tests/lib/Service/OauthSeviceTest.php @@ -0,0 +1,123 @@ + + * + * @author Your name + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\OpenProject\Service; + +use OCA\OAuth2\Db\ClientMapper; +use OCP\Security\ICrypto; +use OCP\Security\ISecureRandom; +use PHPUnit\Framework\TestCase; + +class OauthServiceTest extends TestCase { + protected function getOauthServiceMock( + $clientMapperMock = null, + $iSecureRandomMock = null, + $iCryptoMock = null, + ): OauthService { + + if ($clientMapperMock === null) { + $clientMapperMock = $this->getMockBuilder(ClientMapper::class)->disableOriginalConstructor()->getMock(); + } + if ($iSecureRandomMock === null) { + $iSecureRandomMock = $this->getMockBuilder(ISecureRandom::class)->getMock(); + } + if ($iCryptoMock === null) { + $iCryptoMock = $this->getMockBuilder(ICrypto::class)->getMock(); + } + + return new OauthService( + $clientMapperMock, + $iSecureRandomMock, + $iCryptoMock + ); + } + + + /** + * @return array + */ + public function gethashOrEncryptSecretBasedOnNextcloudVersion(): array { + return [ + [ + "30.0.0", + "calculateHMAC" + ], + [ + "29.0.7", + "calculateHMAC" + ], + [ + "29.1.0", + "calculateHMAC" + ], + [ + "29.0.6", + "encrypt" + ], + [ + "28.0.10", + "calculateHMAC" + ], + [ + "28.2.0", + "calculateHMAC" + ], + [ + "28.0.0", + "encrypt" + ], + [ + "29.0.0", + "encrypt" + ], + [ + "27.1.11.8", + "calculateHMAC" + ], + [ + "27.1.12.0", + "calculateHMAC" + ], + [ + "27.1.1.0", + "encrypt" + ] + ]; + } + + + /** + * @dataProvider gethashOrEncryptSecretBasedOnNextcloudVersion + * @param string $nextcloudVersion + * @param string $hashOrEncryptFunction + * + * @return void + * + */ + public function testGetHashedOrEncryptedClientSecretBasedOnNextcloudVersions(string $nextcloudVersion, string $hashOrEncryptFunction) { + $iCryptoMock = $this->getMockBuilder(ICrypto::class)->getMock(); + $oAuthService = $this->getOauthServiceMock(null, null, $iCryptoMock); + $iCryptoMock->expects($this->once())->method($hashOrEncryptFunction); + $oAuthService->hashOrEncryptSecretBasedOnNextcloudVersion("client_secret", $nextcloudVersion); + } +}