Validate clients with an empty string secret
Closes #3322325
Merge request reports
Activity
added 8 commits
-
0224c62f...ec174da8 - 6 commits from branch
project:6.0.x
- 43871e8f - Validate clients with an empty string secret
- b7f40bdd - Refactor validation logic to prevent client_credentials for public clients...
-
0224c62f...ec174da8 - 6 commits from branch
added 1 commit
- 0d8a3378 - Check status code before error to avoid undefined array key error
166 'no_client_secret' => [ 167 'NULL', 168 'invalid_client', 169 401, 170 ], 171 ]; 172 } 173 174 /** 175 * Test public client with ClientCredentials grant. 176 * 177 * The client credentials grant cannot be used with a public client. 178 * 179 * @dataProvider publicClientCredentialsGrantProvider 180 */ 181 public function testPublicClientCredentialsGrant(string $secret, string $error, int $code): void { changed this line in version 6 of the diff
Ah I see. When we say "public" you are specifically referring to the
confidential
boolean field value. I was referring to the OAuth spec which considers a client without a secret to be "public", and did not consider theconfidential
boolean in the logic here (since it was not already included).
180 */ 181 public function testPublicClientCredentialsGrant(string $secret, string $error, int $code): void { 182 $this->client->set('secret', NULL)->save(); 183 $parameters = [ 184 'grant_type' => 'client_credentials', 185 'client_id' => $this->client->getClientId(), 186 'scope' => $this->scope, 187 ]; 188 if ($secret) { 189 $parameters['client_secret'] = $secret; 190 } 191 $request = Request::create($this->url->toString(), 'POST', $parameters); 192 $response = $this->httpKernel->handle($request); 193 $parsed_response = Json::decode((string) $response->getContent()); 194 $this->assertSame($code, $response->getStatusCode(), sprintf('Correct status code %d', $code)); 195 $this->assertSame($error, $parsed_response['error'], sprintf('Correct error code %s', $error)); Yes I agree. I can add that in. We should probably test the failing combinations of
confidental
true/false andsecret
empty/not empty, where the only valid combination forclient_credentials
should becofidential == false
+ a valid secret (already done in other tests). Do you think it would be better in this test case or another?Part of the "issue" is that the OAuth Spec considers "public" as a client without a secret. So at minimum we need to check this case regardless of the
confidential
boolean. I kind of wonder if this could be improved (as a followup), maybe makeconfidential
a computed field based on the existence of asecret
? Does that make sense? Maybe I am missing something about howconfidential
is used.@bojan_dev I've made some changes so that the logic checks both the
confidential
boolean and the configured client secret. At first I pushed these changes to this MR but the test suite was not working. I remember tests in simple oauth used to be very flaky so I just opened a new MR !156 (merged) rebased on 6.0.x.The validation logic and test structure is slightly different and I wanted to make sure to preserve the history in this MR without modifying it. Sorry for any confusion.
added 1 commit
- a4b2ded3 - Update validateClient logic to include a check for the confidential boolean
added 1 commit
- d4b812cf - Update ClientCredentialsTest tests to set configured confidential and client_secret values