Skip to content

Commit 8efa510

Browse files
PothercaPotherca
authored
Merge pull request #219 from pdsinterop/fix/CLN-012
Fix CLN-012 Improper Authorization
2 parents 51c405e + d28301d commit 8efa510

File tree

5 files changed

+162
-168
lines changed

5 files changed

+162
-168
lines changed

‎init.sh‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ php console.php app:enable solid
66
php console.php config:system:set trusted_domains 1 --value=server
77
php console.php config:system:set trusted_domains 2 --value=nextcloud.local
88
php console.php config:system:set trusted_domains 3 --value=thirdparty
9-
# set 'tester' and 'https://tester' as allowed clients for the test suite to run
10-
php console.php user:setting alice solid allowedClients '["f5d1278e8109edd94e1e4197e04873b9", "2e5cddcf0f663544e98982931e6cc5a6"]'
9+
# set 'tester' and 'https://tester' as tyrusted apps for the test suite to run
10+
php console.php config:app:set solid trustedApps --value='["https://tester", "tester"]'
11+
1112
echo configured
1213
mkdir -p /var/www/html/data/files_trashbin/versions

‎solid/lib/BaseServerConfig.php‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -157,25 +157,35 @@ public function removeClientConfig($clientId) {
157157
$this->config->setAppValue('solid', 'clientScopes', $scopes);
158158
}
159159

160-
public function saveClientRegistration($origin, $clientData) {
161-
$originHash = md5($origin);
162-
$existingRegistration = $this->getClientRegistration($originHash);
163-
if ($existingRegistration && isset($existingRegistration['redirect_uris'])) {
164-
foreach ($existingRegistration['redirect_uris'] as $uri) {
165-
$clientData['redirect_uris'][] = $uri;
166-
}
167-
$clientData['redirect_uris'] = array_unique($clientData['redirect_uris']);
168-
if (isset($existingRegistration['blocked'])) {
169-
$clientData['blocked'] = $existingRegistration['blocked'];
160+
public function saveClientRegistration($clientData)
161+
{
162+
$generatedClientId = bin2hex(random_bytes(16)); // 32 chars for the client Id
163+
164+
// Avoid collision, give up after 5 tries.
165+
for ($i = 0; $i < 5; $i++) {
166+
$existingRegistration = $this->getClientRegistration($generatedClientId);
167+
if ($existingRegistration['client_id'] ?? false) {
168+
$generatedClientId = bin2hex(random_bytes(16));
169+
} else {
170+
break;
170171
}
171172
}
172173

173-
$clientData['client_id'] = $originHash;
174-
$clientData['client_name'] = $origin;
175-
$clientData['client_secret'] = md5(random_bytes(32));
176-
$this->config->setAppValue('solid', "client-" . $originHash, json_encode($clientData));
174+
if ($existingRegistration['client_id'] ?? false) {
175+
throw new \Exception("Could not generate unique client ID");
176+
}
177+
178+
$generatedClientSecret = bin2hex(random_bytes(32)); // and 64 chars for the client secret
179+
180+
$clientData['client_id'] = $generatedClientId;
181+
$clientData['client_secret'] = $generatedClientSecret;
182+
183+
if (!isset($clientData['client_name'])) {
184+
$clientData['client_name'] = "Client $generatedClientId";
185+
}
186+
187+
$this->config->setAppValue('solid', "client-" . $generatedClientId, json_encode($clientData));
177188

178-
$this->config->setAppValue('solid', "client-" . $origin, json_encode($clientData));
179189
return $clientData;
180190
}
181191

@@ -188,6 +198,13 @@ public function getClientRegistration($clientId) {
188198
return json_decode($data, true);
189199
}
190200

201+
public function getTrustedApps()
202+
{
203+
$appValue = $this->config->getAppValue('solid', 'trustedApps', '[]');
204+
205+
return json_decode($appValue, true, 512, JSON_THROW_ON_ERROR);
206+
}
207+
191208
public function getUserSubDomainsEnabled() {
192209
$value = $this->config->getAppValue('solid', 'userSubDomainsEnabled', false);
193210

‎solid/lib/Controller/ServerController.php‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Lcobucci\JWT\Configuration;
1919
use Lcobucci\JWT\Signer\Key\InMemory;
2020
use Lcobucci\JWT\Signer\Rsa\Sha256;
21+
use Pdsinterop\Solid\Auth\Enum\Authorization;
2122

2223
class ServerController extends Controller
2324
{
@@ -208,22 +209,25 @@ public function authorize() {
208209
$getVars['redirect_uri']
209210
)
210211
);
211-
$clientId = $this->config->saveClientRegistration($origin, $clientData)['client_id'];
212-
$clientId = $this->config->saveClientRegistration($getVars['client_id'], $clientData)['client_id'];
212+
213+
$clientId = $this->config->saveClientRegistration($clientData)['client_id'];
213214
$returnUrl = $getVars['redirect_uri'];
215+
$clientRegistration = $this->config->getClientRegistration($clientId);
216+
// @FIXME: Implement $clientRegistration = $this->fetchRemoteClientDocument($getVars['client_id'])
217+
// to replace this section of this `if` statement.
214218
} else {
215219
$clientId = $getVars['client_id'];
216220
$returnUrl = $_SERVER['REQUEST_URI'];
221+
$clientRegistration = $this->config->getClientRegistration($clientId);
217222
}
218223

219-
$clientRegistration = $this->config->getClientRegistration($clientId);
220224
if (isset($clientRegistration['blocked']) && ($clientRegistration['blocked'] === true)) {
221225
$result = new JSONResponse('Unauthorized client');
222226
$result->setStatus(403);
223227
return $result;
224228
}
225229

226-
$approval = $this->checkApproval($clientId);
230+
$approval = $this->checkApproval($clientId, $clientRegistration);
227231
if (!$approval) {
228232
$result = new JSONResponse('Approval required');
229233
$result->setStatus(302);
@@ -283,13 +287,23 @@ public function authorize() {
283287
return $this->respond($response); // ->addHeader('Access-Control-Allow-Origin', '*');
284288
}
285289

286-
private function checkApproval($clientId) {
290+
private function checkApproval($clientId, $clientRegistration)
291+
{
292+
$approved = Authorization::DENIED;
293+
287294
$allowedClients = $this->config->getAllowedClients($this->userId);
288295
if (in_array($clientId, $allowedClients)) {
289-
return \Pdsinterop\Solid\Auth\Enum\Authorization::APPROVED;
290-
} else {
291-
return \Pdsinterop\Solid\Auth\Enum\Authorization::DENIED;
296+
$approved = Authorization::APPROVED;
297+
} elseif (isset($clientRegistration['origin'])) {
298+
$origin = $clientRegistration['origin'];
299+
$trustedApps = $this->config->getTrustedApps();
300+
301+
if (in_array($origin, $trustedApps)) {
302+
$approved = Authorization::APPROVED;
303+
}
292304
}
305+
306+
return $approved;
293307
}
294308

295309
private function getProfilePage() {
@@ -405,20 +419,24 @@ public function register() {
405419
if (! isset($clientData['redirect_uris'])) {
406420
return new JSONResponse("Missing redirect URIs", Http::STATUS_BAD_REQUEST);
407421
}
422+
408423
$clientData['client_id_issued_at'] = time();
409-
$parsedOrigin = parse_url($clientData['redirect_uris'][0]);
424+
$parsedOrigin = parse_url($clientData['redirect_uris'][0]); // FIXME: Should we have multiple origins?
410425
$origin = $parsedOrigin['scheme'] . '://' . $parsedOrigin['host'];
411426
if (isset($parsedOrigin['port'])) {
412427
$origin .= ":" . $parsedOrigin['port'];
413428
}
429+
$clientData['origin'] = $origin;
430+
431+
$clientData = $this->config->saveClientRegistration($clientData);
414432

415-
$clientData = $this->config->saveClientRegistration($origin, $clientData);
416433
$registration = array(
417434
'client_id' => $clientData['client_id'],
418435
'client_secret' => $clientData['client_secret'],
419436
'registration_client_uri' => $this->urlGenerator->getAbsoluteURL($this->urlGenerator->linkToRoute("solid.server.registeredClient", array("clientId" => $clientData['client_id']))),
420437
'client_id_issued_at' => $clientData['client_id_issued_at'],
421438
'redirect_uris' => $clientData['redirect_uris'],
439+
'origin' => $clientData['origin'],
422440
);
423441
$registration = $this->tokenGenerator->respondToRegistration($registration, $this->config->getPrivateKey());
424442
return (new JSONResponse($registration));

‎solid/tests/Unit/BaseServerConfigTest.php‎

Lines changed: 20 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,6 @@ public function testGetClientRegistrationForNonExistingClient()
149149
$this->assertEquals($expected, $actual);
150150
}
151151

152-
/**
153-
* @testdox BaseServerConfig should complain when asked to save ClientRegistration without origin
154-
* @covers ::saveClientRegistration
155-
*/
156-
public function testSaveClientRegistrationWithoutOrigin()
157-
{
158-
$this->expectException(TypeError::class);
159-
$this->expectExceptionMessage('Too few arguments to function');
160-
161-
$configMock = $this->createMock(IConfig::class);
162-
$baseServerConfig = new BaseServerConfig($configMock);
163-
164-
$baseServerConfig->saveClientRegistration();
165-
}
166-
167152
/**
168153
* @testdox BaseServerConfig should complain when asked to save ClientRegistration without client data
169154
* @covers ::saveClientRegistration
@@ -176,7 +161,7 @@ public function testSaveClientRegistrationWithoutClientData()
176161
$configMock = $this->createMock(IConfig::class);
177162
$baseServerConfig = new BaseServerConfig($configMock);
178163

179-
$baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN);
164+
$baseServerConfig->saveClientRegistration();
180165
}
181166

182167
/**
@@ -189,156 +174,61 @@ public function testSaveClientRegistrationForNewClient()
189174

190175
$configMock->expects($this->once())
191176
->method('getAppValue')
192-
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
193177
->willReturnArgument(2);
194178

195-
$expected = [
196-
'client_id' => md5(self::MOCK_ORIGIN),
197-
'client_name' => self::MOCK_ORIGIN,
198-
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
199-
];
200-
201-
$configMock->expects($this->exactly(2))
179+
$configMock->expects($this->exactly(1))
202180
->method('setAppValue')
203181
->willReturnMap([
204182
// Using willReturnMap as withConsecutive is removed since PHPUnit 10
205-
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
206-
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
183+
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode('{}')]
207184
]);
208185

209186
$baseServerConfig = new BaseServerConfig($configMock);
210187

211-
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []);
188+
$actual = $baseServerConfig->saveClientRegistration([]);
212189

213-
$this->assertEquals($expected, $actual);
190+
$this->assertArrayHasKey('client_id', $actual);
191+
$this->assertArrayHasKey('client_secret', $actual);
192+
$this->assertArrayHasKey('client_name', $actual);
214193
}
215194

216195
/**
217-
* @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for existing client
218-
* @covers ::saveClientRegistration
196+
* @testdox BaseServerConfig should remove ClientRegistration when asked to remove ClientRegistration
197+
* @covers ::removeClientRegistration
219198
*/
220-
public function testSaveClientRegistrationForExistingClient()
199+
public function testRemoveClientRegistration()
221200
{
222201
$configMock = $this->createMock(IConfig::class);
223-
224-
$expected = [
225-
'client_id' => md5(self::MOCK_ORIGIN),
226-
'client_name' => self::MOCK_ORIGIN,
227-
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
228-
'redirect_uris' => [self::MOCK_REDIRECT_URI],
229-
];
230-
231-
$configMock->expects($this->once())
232-
->method('getAppValue')
233-
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
234-
->willReturn(json_encode($expected));
235-
236-
$configMock->expects($this->exactly(2))
237-
->method('setAppValue')
238-
->willReturnMap([
239-
// Using willReturnMap as withConsecutive is deprecated since PHPUnit 10
240-
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
241-
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
242-
]);
243-
244202
$baseServerConfig = new BaseServerConfig($configMock);
245203

246-
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []);
247-
248-
$this->assertEquals($expected, $actual);
249-
}
250-
251-
/**
252-
* @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for blocked client
253-
* @covers ::saveClientRegistration
254-
*/
255-
public function testSaveClientRegistrationForBlockedClient()
256-
{
257-
$configMock = $this->createMock(IConfig::class);
258-
259-
$expected = [
260-
'client_id' => md5(self::MOCK_ORIGIN),
261-
'client_name' => self::MOCK_ORIGIN,
262-
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
263-
'redirect_uris' => [self::MOCK_REDIRECT_URI],
264-
'blocked' => true,
265-
];
266-
267204
$configMock->expects($this->once())
268-
->method('getAppValue')
269-
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
270-
->willReturn(json_encode($expected));
271-
272-
$configMock->expects($this->exactly(2))
273-
->method('setAppValue')
274-
->willReturnMap([
275-
// Using willReturnMap as withConsecutive is deprecated since PHPUnit 10
276-
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
277-
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
278-
]);
279-
280-
$baseServerConfig = new BaseServerConfig($configMock);
281-
282-
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, $expected);
205+
->method('deleteAppValue')
206+
->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID);
283207

284-
$this->assertEquals($expected, $actual);
208+
$baseServerConfig->removeClientRegistration(self::MOCK_CLIENT_ID);
285209
}
286210

287211
/**
288-
* @testdox BaseServerConfig should always "blocked" to existing value when asked to save ClientRegistration for blocked client
289-
* @covers ::saveClientRegistration
212+
* @testdox BaseServerConfig should return decoded trusted apps when asked to GetTrustedApps
213+
* @covers ::getTrustedApps
290214
*/
291-
public function testSaveClientRegistrationSetsBlocked()
215+
public function testGetTrustedApps()
292216
{
293217
$configMock = $this->createMock(IConfig::class);
218+
$baseServerConfig = new BaseServerConfig($configMock);
294219

295-
$expected = [
296-
'client_id' => md5(self::MOCK_ORIGIN),
297-
'client_name' => self::MOCK_ORIGIN,
298-
'client_secret' => md5(self::MOCK_RANDOM_BYTES),
299-
'redirect_uris' => [self::MOCK_REDIRECT_URI],
300-
'blocked' => true,
301-
];
220+
$expected = [self::MOCK_ORIGIN];
302221

303222
$configMock->expects($this->once())
304223
->method('getAppValue')
305-
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
224+
->with(Application::APP_ID, 'trustedApps', '[]')
306225
->willReturn(json_encode($expected));
307226

308-
$clientData = $expected;
309-
$clientData['blocked'] = false;
310-
311-
$configMock->expects($this->exactly(2))
312-
->method('setAppValue')
313-
->willReturnMap([
314-
// Using willReturnMap as withConsecutive is deprecated since PHPUnit 10
315-
[Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)],
316-
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)]
317-
]);
318-
319-
$baseServerConfig = new BaseServerConfig($configMock);
320-
321-
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, $clientData);
227+
$actual = $baseServerConfig->getTrustedApps();
322228

323229
$this->assertEquals($expected, $actual);
324230
}
325231

326-
/**
327-
* @testdox BaseServerConfig should remove ClientRegistration when asked to remove ClientRegistration
328-
* @covers ::removeClientRegistration
329-
*/
330-
public function testRemoveClientRegistration()
331-
{
332-
$configMock = $this->createMock(IConfig::class);
333-
$baseServerConfig = new BaseServerConfig($configMock);
334-
335-
$configMock->expects($this->once())
336-
->method('deleteAppValue')
337-
->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID);
338-
339-
$baseServerConfig->removeClientRegistration(self::MOCK_CLIENT_ID);
340-
}
341-
342232
/////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
343233

344234
public function provideBooleans()

0 commit comments

Comments
 (0)