Skip to content

Commit b49283d

Browse files
SagarGiSagarGi
committed
[WP#57654]switch encryption for oauth secret (#694)
1 parent 0d5a02b commit b49283d

4 files changed

Lines changed: 151 additions & 11 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
1515
- Enhance project search when creating workpackages from Nextcloud
1616
- Drop application's support for Nextcloud 26
1717
- Fix issue preventing direct uploading of resources in Nextcloud that are managed by app `Files Access Control`
18+
- Hash or encrypt `client_secret` for different Nextcloud versions
1819

1920
## 2.6.4 - 2024-08-15
2021
### Changed

‎lib/Service/OauthService.php‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,29 @@ public function __construct(ClientMapper $clientMapper,
4747
$this->crypto = $crypto;
4848
}
4949

50+
/**
51+
* @param string $secret
52+
* @param string $nextcloudVersion
53+
* @return string
54+
*/
55+
public function hashOrEncryptSecretBasedOnNextcloudVersion(string $secret, string $nextcloudVersion): string {
56+
switch (true) {
57+
case version_compare($nextcloudVersion, '30.0.0') >= 0:
58+
case version_compare($nextcloudVersion, '29.0.7') >= 0 && version_compare($nextcloudVersion, '30.0.0') < 0:
59+
case version_compare($nextcloudVersion, '28.0.10') >= 0 && version_compare($nextcloudVersion, '29.0.0') < 0:
60+
case version_compare($nextcloudVersion, '27.1.11.8') >= 0 && version_compare($nextcloudVersion, '28.0.0') < 0:
61+
$encryptedSecret = bin2hex($this->crypto->calculateHMAC($secret));
62+
break;
63+
case version_compare($nextcloudVersion, '27.0.0') === 0:
64+
$encryptedSecret = $secret;
65+
break;
66+
default:
67+
$encryptedSecret = $this->crypto->encrypt($secret);
68+
break;
69+
}
70+
return $encryptedSecret;
71+
}
72+
5073
/**
5174
* @param string $name
5275
* @param string $redirectUri
@@ -58,12 +81,8 @@ public function createNcOauthClient(string $name, string $redirectUri): array {
5881
$client->setName($name);
5982
$client->setRedirectUri(sprintf($redirectUri, $clientId));
6083
$secret = $this->secureRandom->generate(64, self::validChars);
61-
if (version_compare(OC_Util::getVersionString(), '27.0.1') >= 0) {
62-
$encryptedSecret = $this->crypto->encrypt($secret);
63-
} else {
64-
$encryptedSecret = $secret;
65-
}
66-
$client->setSecret($encryptedSecret);
84+
$nextcloudVersion = OC_Util::getVersionString();
85+
$client->setSecret($this->hashOrEncryptSecretBasedOnNextcloudVersion($secret, $nextcloudVersion));
6786
$client->setClientIdentifier($clientId);
6887
$client = $this->clientMapper->insert($client);
6988

@@ -87,8 +106,7 @@ public function getClientInfo(int $id): ?array {
87106
'id' => $client->getId(),
88107
'nextcloud_oauth_client_name' => $client->getName(),
89108
'openproject_redirect_uri' => $client->getRedirectUri(),
90-
'nextcloud_client_id' => $client->getClientIdentifier(),
91-
'nextcloud_client_secret' => $this->crypto->decrypt($client->getSecret()),
109+
'nextcloud_client_id' => $client->getClientIdentifier()
92110
];
93111
} catch (ClientNotFoundException $e) {
94112
return null;

‎tests/acceptance/features/api/setup.feature‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -656,14 +656,12 @@ Feature: setup the integration through an API
656656
"required": [
657657
"nextcloud_oauth_client_name",
658658
"openproject_redirect_uri",
659-
"nextcloud_client_id",
660-
"nextcloud_client_secret"
659+
"nextcloud_client_id"
661660
],
662661
"properties": {
663662
"nextcloud_oauth_client_name": {"type": "string", "pattern": "^OpenProject client$"},
664663
"openproject_redirect_uri": {"type": "string", "pattern": "^http:\/\/some-host.de\/oauth_clients\/[A-Za-z0-9]+\/callback$"},
665664
"nextcloud_client_id": {"type": "string", "pattern": "[A-Za-z0-9]+"},
666-
"nextcloud_client_secret": {"type": "string", "pattern": "[A-Za-z0-9]+"},
667665
"openproject_user_app_password": {"type": "string", "pattern": "[A-Za-z0-9]+"}
668666
}
669667
}

‎tests/lib/Service/OauthSeviceTest.php‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
<?php
2+
/**
3+
* @copyright Copyright (c) 2024 Sagar Gurung <sagar@jankaritech.com>
4+
*
5+
* @author Your name <sagar@jankaritech.com>
6+
*
7+
* @license GNU AGPL version 3 or any later version
8+
*
9+
* This program is free software: you can redistribute it and/or modify
10+
* it under the terms of the GNU Affero General Public License as
11+
* published by the Free Software Foundation, either version 3 of the
12+
* License, or (at your option) any later version.
13+
*
14+
* This program is distributed in the hope that it will be useful,
15+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
16+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
17+
* GNU Affero General Public License for more details.
18+
*
19+
* You should have received a copy of the GNU Affero General Public License
20+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
21+
*
22+
*/
23+
24+
namespace OCA\OpenProject\Service;
25+
26+
use OCA\OAuth2\Db\ClientMapper;
27+
use OCP\Security\ICrypto;
28+
use OCP\Security\ISecureRandom;
29+
use PHPUnit\Framework\TestCase;
30+
31+
class OauthServiceTest extends TestCase {
32+
protected function getOauthServiceMock(
33+
$clientMapperMock = null,
34+
$iSecureRandomMock = null,
35+
$iCryptoMock = null,
36+
): OauthService {
37+
38+
if ($clientMapperMock === null) {
39+
$clientMapperMock = $this->getMockBuilder(ClientMapper::class)->disableOriginalConstructor()->getMock();
40+
}
41+
if ($iSecureRandomMock === null) {
42+
$iSecureRandomMock = $this->getMockBuilder(ISecureRandom::class)->getMock();
43+
}
44+
if ($iCryptoMock === null) {
45+
$iCryptoMock = $this->getMockBuilder(ICrypto::class)->getMock();
46+
}
47+
48+
return new OauthService(
49+
$clientMapperMock,
50+
$iSecureRandomMock,
51+
$iCryptoMock
52+
);
53+
}
54+
55+
56+
/**
57+
* @return array<mixed>
58+
*/
59+
public function gethashOrEncryptSecretBasedOnNextcloudVersion(): array {
60+
return [
61+
[
62+
"30.0.0",
63+
"calculateHMAC"
64+
],
65+
[
66+
"29.0.7",
67+
"calculateHMAC"
68+
],
69+
[
70+
"29.1.0",
71+
"calculateHMAC"
72+
],
73+
[
74+
"29.0.6",
75+
"encrypt"
76+
],
77+
[
78+
"28.0.10",
79+
"calculateHMAC"
80+
],
81+
[
82+
"28.2.0",
83+
"calculateHMAC"
84+
],
85+
[
86+
"28.0.0",
87+
"encrypt"
88+
],
89+
[
90+
"29.0.0",
91+
"encrypt"
92+
],
93+
[
94+
"27.1.11.8",
95+
"calculateHMAC"
96+
],
97+
[
98+
"27.1.12.0",
99+
"calculateHMAC"
100+
],
101+
[
102+
"27.1.1.0",
103+
"encrypt"
104+
]
105+
];
106+
}
107+
108+
109+
/**
110+
* @dataProvider gethashOrEncryptSecretBasedOnNextcloudVersion
111+
* @param string $nextcloudVersion
112+
* @param string $hashOrEncryptFunction
113+
*
114+
* @return void
115+
*
116+
*/
117+
public function testGetHashedOrEncryptedClientSecretBasedOnNextcloudVersions(string $nextcloudVersion, string $hashOrEncryptFunction) {
118+
$iCryptoMock = $this->getMockBuilder(ICrypto::class)->getMock();
119+
$oAuthService = $this->getOauthServiceMock(null, null, $iCryptoMock);
120+
$iCryptoMock->expects($this->once())->method($hashOrEncryptFunction);
121+
$oAuthService->hashOrEncryptSecretBasedOnNextcloudVersion("client_secret", $nextcloudVersion);
122+
}
123+
}

0 commit comments

Comments
 (0)