Skip to content

Commit 99f1b10

Browse files
PothercaPotherca
committed
Change ServerController::autherize() call to disallow updating existing clients.
1 parent 51c405e commit 99f1b10

4 files changed

Lines changed: 54 additions & 170 deletions

File tree

‎solid/lib/BaseServerConfig.php‎

Lines changed: 25 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

‎solid/lib/Controller/ServerController.php‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -208,15 +208,18 @@ public function authorize() {
208208
$getVars['redirect_uri']
209209
)
210210
);
211-
$clientId = $this->config->saveClientRegistration($origin, $clientData)['client_id'];
212-
$clientId = $this->config->saveClientRegistration($getVars['client_id'], $clientData)['client_id'];
211+
212+
$clientId = $this->config->saveClientRegistration($clientData)['client_id'];
213213
$returnUrl = $getVars['redirect_uri'];
214+
$clientRegistration = $this->config->getClientRegistration($clientId);
215+
// @FIXME: Implement $clientRegistration = $this->fetchRemoteClientDocument($getVars['client_id'])
216+
// to replace this section of this `if` statement.
214217
} else {
215218
$clientId = $getVars['client_id'];
216219
$returnUrl = $_SERVER['REQUEST_URI'];
220+
$clientRegistration = $this->config->getClientRegistration($clientId);
217221
}
218222

219-
$clientRegistration = $this->config->getClientRegistration($clientId);
220223
if (isset($clientRegistration['blocked']) && ($clientRegistration['blocked'] === true)) {
221224
$result = new JSONResponse('Unauthorized client');
222225
$result->setStatus(403);
@@ -405,14 +408,11 @@ public function register() {
405408
if (! isset($clientData['redirect_uris'])) {
406409
return new JSONResponse("Missing redirect URIs", Http::STATUS_BAD_REQUEST);
407410
}
411+
408412
$clientData['client_id_issued_at'] = time();
409-
$parsedOrigin = parse_url($clientData['redirect_uris'][0]);
410-
$origin = $parsedOrigin['scheme'] . '://' . $parsedOrigin['host'];
411-
if (isset($parsedOrigin['port'])) {
412-
$origin .= ":" . $parsedOrigin['port'];
413-
}
414413

415-
$clientData = $this->config->saveClientRegistration($origin, $clientData);
414+
$clientData = $this->config->saveClientRegistration($clientData);
415+
416416
$registration = array(
417417
'client_id' => $clientData['client_id'],
418418
'client_secret' => $clientData['client_secret'],

‎solid/tests/Unit/BaseServerConfigTest.php‎

Lines changed: 7 additions & 138 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,138 +174,22 @@ 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)]
207-
]);
208-
209-
$baseServerConfig = new BaseServerConfig($configMock);
210-
211-
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []);
212-
213-
$this->assertEquals($expected, $actual);
214-
}
215-
216-
/**
217-
* @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for existing client
218-
* @covers ::saveClientRegistration
219-
*/
220-
public function testSaveClientRegistrationForExistingClient()
221-
{
222-
$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-
244-
$baseServerConfig = new BaseServerConfig($configMock);
245-
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-
267-
$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);
283-
284-
$this->assertEquals($expected, $actual);
285-
}
286-
287-
/**
288-
* @testdox BaseServerConfig should always "blocked" to existing value when asked to save ClientRegistration for blocked client
289-
* @covers ::saveClientRegistration
290-
*/
291-
public function testSaveClientRegistrationSetsBlocked()
292-
{
293-
$configMock = $this->createMock(IConfig::class);
294-
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-
];
302-
303-
$configMock->expects($this->once())
304-
->method('getAppValue')
305-
->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN))
306-
->willReturn(json_encode($expected));
307-
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)]
183+
[Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode('{}')]
317184
]);
318185

319186
$baseServerConfig = new BaseServerConfig($configMock);
320187

321-
$actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, $clientData);
188+
$actual = $baseServerConfig->saveClientRegistration([]);
322189

323-
$this->assertEquals($expected, $actual);
190+
$this->assertArrayHasKey('client_id', $actual);
191+
$this->assertArrayHasKey('client_secret', $actual);
192+
$this->assertArrayHasKey('client_name', $actual);
324193
}
325194

326195
/**

‎solid/tests/Unit/Controller/ServerControllerTest.php‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -352,20 +352,28 @@ public function testRegisterWithRedirectUris()
352352

353353
$data = [
354354
'application_type' => 'web',
355-
'client_id' => 'f4a2d00f7602948a97ff409d7a581ec2',
356-
'client_secret' => '3b5798fddd49e23662ee6fe801085100',
357355
'grant_types' => ['implicit'],
358356
'id_token_signed_response_alg' => 'RS256',
359357
'redirect_uris' => ['https://mock.client/redirect'],
360-
'registration_access_token' => 'eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9.eyJpc3MiOiJodHRwczovL21vY2suc2VydmVyIiwiYXVkIjoiZjRhMmQwMGY3NjAyOTQ4YTk3ZmY0MDlkN2E1ODFlYzIiLCJzdWIiOiJmNGEyZDAwZjc2MDI5NDhhOTdmZjQwOWQ3YTU4MWVjMiJ9.AfOi9YW70rL0EKn4_dvhkyu02iI4yGYV-Xh8hQ9RbHBUnvcXROFfQzn-OL-R3kV3nn8tknmpG-r_8Ouoo7O_Sjo8Hx1QSFfeqjJGOgB8HbXV7WN2spOMicSB-68EyftqfTGH0ksyPyJaNSTbkdIqtawsDaSKUVqTmziEo4IrE5anwDLZrtSUcS0A4KVrOAkJmgYGiC4MC0NMYXeBRxgkr1_h7GN4hekAXs9-5XwRH1mwswUVRL-6prx0IYpPNURFNqkS2NU83xNf-vONThOdLVkADVy-l3PCHT3E1sRdkklCHLjhWiZo7NcMlB0WdS-APnZYCi5hLEr5-jwNI2sxoA',
361358
'registration_client_uri' => '',
362359
'response_types' => ['id_token token'],
363360
'token_endpoint_auth_method' => 'client_secret_basic',
364361
];
365362
$expected = $this->createExpectedResponse(Http::STATUS_OK, $data);
366363

364+
$actualData = $response->getData();
365+
$this->assertArrayHasKey('client_id', $actualData);
366+
$this->assertArrayHasKey('client_secret', $actualData);
367+
$this->assertArrayHasKey('registration_access_token', $actualData);
368+
369+
unset(
370+
$actualData['client_id'],
371+
$actualData['client_secret'],
372+
$actualData['registration_access_token'],
373+
);
374+
367375
$actual = [
368-
'data' => $response->getData(),
376+
'data' => $actualData,
369377
'headers' => $response->getHeaders(),
370378
'status' => $response->getStatus(),
371379
];
@@ -458,13 +466,10 @@ public function createMockConfig($clientData): IConfig|MockObject
458466

459467
$this->mockConfig->method('getAppValue')->willReturnMap([
460468
[Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID, '{}', 'return' => $clientData],
461-
[Application::APP_ID, 'client-d6d7896757f61ac4c397d914053180ff', '{}', 'return' => $clientData],
469+
[Application::APP_ID, 'client-6d6f636b2072616e646f6d206279746573', '{}', 'return' => $clientData],
462470
[Application::APP_ID, 'client-', '{}', 'return' => $clientData],
463-
[Application::APP_ID, 'profileData', '', 'return' => ''],
464471
[Application::APP_ID, 'encryptionKey', '', 'return' => 'mock encryption key'],
465472
[Application::APP_ID, 'privateKey', '', 'return' => self::$privateKey],
466-
// Client ID from register() with https://mock.client
467-
[Application::APP_ID, 'client-f4a2d00f7602948a97ff409d7a581ec2', '{}', 'return' => $clientData],
468473
]);
469474

470475
return $this->mockConfig;

0 commit comments

Comments
 (0)