Skip to content

Commit 9feeb0c

Browse files
[13.x] Fix accessing the plain secret after creating a new confidential client via deprecated ClientController::store (#1861)
* Fix ClientController::store() breaking change introduced via #1745 ```php - if (Passport::$hashesClientSecrets) { - return ['plainSecret' => $client->plainSecret] + $client->toArray(); - } + $client->secret = $client->plainSecret; return $client->makeVisible('secret'); ``` This change obviously breaks usages that previously relied on the return type array with the additional 'plainSecret' data. E.g., the old Vue components used the plainSecret to present that to the user so that he could save it, etc. Since hashing is now mandatory, I restored the previous behavior without the now obsolete `Passport::$hashesClientSecrets` check: ```php return ['plainSecret' => $client->plainSecret] + $client->toArray(); ``` I also updated the tests. I know it looks a bit fishy but I had not much choice since it's a unit test … (didn't want to make too big of a change out of this … it's deprecated anyways …) * append `plain_secret` * use `append` instead of `mergeAppend` to support L11.x --------- Co-authored-by: Martin Hettiger <[email protected]>
1 parent 2c82b1c commit 9feeb0c

File tree

3 files changed

+37
-6
lines changed

3 files changed

+37
-6
lines changed

src/Client.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,16 @@ protected function secret(): Attribute
134134
);
135135
}
136136

137+
/**
138+
* Interact with the client's plain secret.
139+
*/
140+
protected function plainSecret(): Attribute
141+
{
142+
return Attribute::make(
143+
get: fn (): ?string => $this->plainSecret
144+
);
145+
}
146+
137147
/**
138148
* Interact with the client's redirect URIs.
139149
*/

src/Http/Controllers/ClientController.php

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,11 @@ public function store(Request $request): Client
4949
$client = $this->clients->createAuthorizationCodeGrantClient(
5050
$request->name,
5151
explode(',', $request->redirect),
52-
(bool) $request->input('confidential', true),
52+
$confidential = (bool) $request->input('confidential', true),
5353
$request->user(),
5454
);
5555

56-
$client->secret = $client->plainSecret;
57-
58-
return $client->makeVisible('secret');
56+
return $confidential ? $client->append(['plain_secret']) : $client;
5957
}
6058

6159
/**

tests/Unit/ClientControllerTest.php

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use Illuminate\Contracts\Auth\Authenticatable;
66
use Illuminate\Contracts\Validation\Factory;
77
use Illuminate\Http\Request;
8+
use Illuminate\Support\Facades\Hash;
89
use Laravel\Passport\Client;
910
use Laravel\Passport\ClientRepository;
1011
use Laravel\Passport\Http\Controllers\ClientController;
@@ -41,6 +42,9 @@ public function test_all_the_clients_for_the_current_user_can_be_retrieved()
4142

4243
public function test_clients_can_be_stored()
4344
{
45+
Hash::expects('isHashed')->once()->with('secret')->andReturn(false);
46+
Hash::expects('make')->once()->with('secret')->andReturn('hashed_secret');
47+
4448
$clients = m::mock(ClientRepository::class);
4549
$user = m::mock(Authenticatable::class);
4650
$user->shouldReceive('getAuthIdentifier')->andReturn(1);
@@ -51,7 +55,11 @@ public function test_clients_can_be_stored()
5155
$clients->shouldReceive('createAuthorizationCodeGrantClient')
5256
->once()
5357
->with('client name', ['http://localhost'], true, $user)
54-
->andReturn($client = new Client);
58+
->andReturn($client = new Client([
59+
'name' => 'client name',
60+
'redirect' => 'http://localhost',
61+
'secret' => 'secret',
62+
]));
5563

5664
$redirectRule = m::mock(RedirectRule::class);
5765

@@ -71,6 +79,12 @@ public function test_clients_can_be_stored()
7179
);
7280

7381
$this->assertEquals($client, $controller->store($request));
82+
$this->assertSame('hashed_secret', $client->secret);
83+
$this->assertSame([
84+
'name' => 'client name',
85+
'redirect' => 'http://localhost',
86+
'plain_secret' => 'secret',
87+
], $client->toArray());
7488
}
7589

7690
public function test_public_clients_can_be_stored()
@@ -89,7 +103,11 @@ public function test_public_clients_can_be_stored()
89103
$clients->shouldReceive('createAuthorizationCodeGrantClient')
90104
->once()
91105
->with('client name', ['http://localhost'], false, $user)
92-
->andReturn($client = new Client);
106+
->andReturn($client = new Client([
107+
'name' => 'client name',
108+
'redirect' => 'http://localhost',
109+
'secret' => null,
110+
]));
93111

94112
$redirectRule = m::mock(RedirectRule::class);
95113

@@ -110,6 +128,11 @@ public function test_public_clients_can_be_stored()
110128
);
111129

112130
$this->assertEquals($client, $controller->store($request));
131+
$this->assertNull($client->secret);
132+
$this->assertSame([
133+
'name' => 'client name',
134+
'redirect' => 'http://localhost',
135+
], $client->toArray());
113136
}
114137

115138
public function test_clients_can_be_updated()

0 commit comments

Comments
 (0)