From 71c53ddcf594efe1997ac2eb31271443c48cf32d Mon Sep 17 00:00:00 2001 From: Sagar Date: Thu, 5 Sep 2024 16:15:38 +0545 Subject: [PATCH 1/4] Added has for the client secret for different NC versions Signed-off-by: Sagar --- CHANGELOG.md | 1 + lib/Service/OauthService.php | 17 ++++++++++++----- tests/acceptance/features/api/setup.feature | 4 +--- 3 files changed, 14 insertions(+), 8 deletions(-) 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..ebad93871 100644 --- a/lib/Service/OauthService.php +++ b/lib/Service/OauthService.php @@ -58,10 +58,18 @@ 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 { + if (version_compare(OC_Util::getVersionString(), '30.0.0') >= 0) { + $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + } elseif (version_compare(OC_Util::getVersionString(), '29.0.7') >= 0 && version_compare(OC_Util::getVersionString(), '30.0.0') < 0) { + $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + } elseif (version_compare(OC_Util::getVersionString(), '28.0.10') >= 0 && version_compare(OC_Util::getVersionString(), '29.0.0') < 0) { + $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + } elseif (version_compare(OC_Util::getVersionString(), '27.1.11.8') >= 0 && version_compare(OC_Util::getVersionString(), '28.0.0') < 0) { + $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + } elseif (version_compare(OC_Util::getVersionString(), '27.0.0') === 0) { $encryptedSecret = $secret; + } else { + $encryptedSecret = $this->crypto->encrypt($secret); } $client->setSecret($encryptedSecret); $client->setClientIdentifier($clientId); @@ -87,8 +95,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]+"} } } From cd5d5178b9651f2e391ed15b5823a3eea9f5272b Mon Sep 17 00:00:00 2001 From: Sagar Date: Fri, 6 Sep 2024 12:54:05 +0545 Subject: [PATCH 2/4] review address Signed-off-by: Sagar --- lib/Service/OauthService.php | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/Service/OauthService.php b/lib/Service/OauthService.php index ebad93871..62feccde8 100644 --- a/lib/Service/OauthService.php +++ b/lib/Service/OauthService.php @@ -58,18 +58,20 @@ 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(), '30.0.0') >= 0) { - $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); - } elseif (version_compare(OC_Util::getVersionString(), '29.0.7') >= 0 && version_compare(OC_Util::getVersionString(), '30.0.0') < 0) { - $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); - } elseif (version_compare(OC_Util::getVersionString(), '28.0.10') >= 0 && version_compare(OC_Util::getVersionString(), '29.0.0') < 0) { - $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); - } elseif (version_compare(OC_Util::getVersionString(), '27.1.11.8') >= 0 && version_compare(OC_Util::getVersionString(), '28.0.0') < 0) { - $encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret)); - } elseif (version_compare(OC_Util::getVersionString(), '27.0.0') === 0) { - $encryptedSecret = $secret; - } else { - $encryptedSecret = $this->crypto->encrypt($secret); + $nextcloudVersion = OC_Util::getVersionString(); + 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; } $client->setSecret($encryptedSecret); $client->setClientIdentifier($clientId); From bf27f4050d377802babc8f425ff3ab58aced79cd Mon Sep 17 00:00:00 2001 From: Sagar Date: Fri, 6 Sep 2024 15:17:53 +0545 Subject: [PATCH 3/4] added unit tests to check encrypt or hash secret for different NC versions Signed-off-by: Sagar --- lib/Service/OauthService.php | 31 ++++--- tests/lib/Service/OauthSeviceTest.php | 123 ++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 11 deletions(-) create mode 100644 tests/lib/Service/OauthSeviceTest.php diff --git a/lib/Service/OauthService.php b/lib/Service/OauthService.php index 62feccde8..936f36fd8 100644 --- a/lib/Service/OauthService.php +++ b/lib/Service/OauthService.php @@ -48,17 +48,11 @@ public function __construct(ClientMapper $clientMapper, } /** - * @param string $name - * @param string $redirectUri - * @return array + * @param string $secret + * @param string $nextcloudVersion + * @return string */ - public function createNcOauthClient(string $name, string $redirectUri): array { - $clientId = $this->secureRandom->generate(64, self::validChars); - $client = new Client(); - $client->setName($name); - $client->setRedirectUri(sprintf($redirectUri, $clientId)); - $secret = $this->secureRandom->generate(64, self::validChars); - $nextcloudVersion = OC_Util::getVersionString(); + public function getHashedOrEncryptedSecretBasedOnNextcloudVersions(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: @@ -73,7 +67,22 @@ public function createNcOauthClient(string $name, string $redirectUri): array { $encryptedSecret = $this->crypto->encrypt($secret); break; } - $client->setSecret($encryptedSecret); + return $encryptedSecret; + } + + /** + * @param string $name + * @param string $redirectUri + * @return array + */ + public function createNcOauthClient(string $name, string $redirectUri): array { + $clientId = $this->secureRandom->generate(64, self::validChars); + $client = new Client(); + $client->setName($name); + $client->setRedirectUri(sprintf($redirectUri, $clientId)); + $secret = $this->secureRandom->generate(64, self::validChars); + $nextcloudVersion = OC_Util::getVersionString(); + $client->setSecret($this->getHashedOrEncryptedSecretBasedOnNextcloudVersions($secret, $nextcloudVersion)); $client->setClientIdentifier($clientId); $client = $this->clientMapper->insert($client); diff --git a/tests/lib/Service/OauthSeviceTest.php b/tests/lib/Service/OauthSeviceTest.php new file mode 100644 index 000000000..4ccd74d88 --- /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 getHashedOrEncryptedSecretBasedOnNextcloudVersionsDataProvider() { + 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 getHashedOrEncryptedSecretBasedOnNextcloudVersionsDataProvider + * @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->getHashedOrEncryptedSecretBasedOnNextcloudVersions("client_secret", $nextcloudVersion); + } +} From 1a45400778b1d947e5665ef461cd56289e838526 Mon Sep 17 00:00:00 2001 From: Sagar Date: Fri, 6 Sep 2024 15:57:54 +0545 Subject: [PATCH 4/4] review address Signed-off-by: Sagar --- lib/Service/OauthService.php | 4 ++-- tests/lib/Service/OauthSeviceTest.php | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/Service/OauthService.php b/lib/Service/OauthService.php index 936f36fd8..21c25000e 100644 --- a/lib/Service/OauthService.php +++ b/lib/Service/OauthService.php @@ -52,7 +52,7 @@ public function __construct(ClientMapper $clientMapper, * @param string $nextcloudVersion * @return string */ - public function getHashedOrEncryptedSecretBasedOnNextcloudVersions(string $secret, string $nextcloudVersion): 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: @@ -82,7 +82,7 @@ public function createNcOauthClient(string $name, string $redirectUri): array { $client->setRedirectUri(sprintf($redirectUri, $clientId)); $secret = $this->secureRandom->generate(64, self::validChars); $nextcloudVersion = OC_Util::getVersionString(); - $client->setSecret($this->getHashedOrEncryptedSecretBasedOnNextcloudVersions($secret, $nextcloudVersion)); + $client->setSecret($this->hashOrEncryptSecretBasedOnNextcloudVersion($secret, $nextcloudVersion)); $client->setClientIdentifier($clientId); $client = $this->clientMapper->insert($client); diff --git a/tests/lib/Service/OauthSeviceTest.php b/tests/lib/Service/OauthSeviceTest.php index 4ccd74d88..e49242989 100644 --- a/tests/lib/Service/OauthSeviceTest.php +++ b/tests/lib/Service/OauthSeviceTest.php @@ -1,8 +1,8 @@ + * @copyright Copyright (c) 2024 Sagar Gurung * - * @author Your name + * @author Your name * * @license GNU AGPL version 3 or any later version * @@ -56,7 +56,7 @@ protected function getOauthServiceMock( /** * @return array */ - public function getHashedOrEncryptedSecretBasedOnNextcloudVersionsDataProvider() { + public function gethashOrEncryptSecretBasedOnNextcloudVersion(): array { return [ [ "30.0.0", @@ -107,7 +107,7 @@ public function getHashedOrEncryptedSecretBasedOnNextcloudVersionsDataProvider() /** - * @dataProvider getHashedOrEncryptedSecretBasedOnNextcloudVersionsDataProvider + * @dataProvider gethashOrEncryptSecretBasedOnNextcloudVersion * @param string $nextcloudVersion * @param string $hashOrEncryptFunction * @@ -118,6 +118,6 @@ public function testGetHashedOrEncryptedClientSecretBasedOnNextcloudVersions(str $iCryptoMock = $this->getMockBuilder(ICrypto::class)->getMock(); $oAuthService = $this->getOauthServiceMock(null, null, $iCryptoMock); $iCryptoMock->expects($this->once())->method($hashOrEncryptFunction); - $oAuthService->getHashedOrEncryptedSecretBasedOnNextcloudVersions("client_secret", $nextcloudVersion); + $oAuthService->hashOrEncryptSecretBasedOnNextcloudVersion("client_secret", $nextcloudVersion); } }