Skip to content
Snippets Groups Projects

Validate clients with an empty string secret

2 unresolved threads

Closes #3322325

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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 {
  • 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));
    • It would be nice to have also test coverage when dealing with a public client (non-confidential).

      $this->client->set('confidential', FALSE);

    • Author Contributor

      Yes I agree. I can add that in. We should probably test the failing combinations of confidental true/false and secret empty/not empty, where the only valid combination for client_credentials should be cofidential == 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 make confidential a computed field based on the existence of a secret? Does that make sense? Maybe I am missing something about how confidential is used.

    • Author Contributor

      @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.

    • Please register or sign in to reply
  • paul121 added 1 commit

    added 1 commit

    • a4b2ded3 - Update validateClient logic to include a check for the confidential boolean

    Compare with previous version

  • paul121 added 1 commit

    added 1 commit

    Compare with previous version

  • paul121 added 1 commit

    added 1 commit

    • d4b812cf - Update ClientCredentialsTest tests to set configured confidential and client_secret values

    Compare with previous version

  • Please register or sign in to reply
    Loading