Skip to content

Commit edba7be

Browse files
cicnavicicnavi
committed
Remove requirement for client_id on token endpoint
1 parent fb3cbd5 commit edba7be

4 files changed

Lines changed: 281 additions & 10 deletions

File tree

‎docs/6-oidc-upgrade.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,15 @@ notes). These were the old routes still reachable at URLs ending in `.php`:
181181
Medium impact changes:
182182

183183
Low-impact changes:
184+
- The token endpoint no longer requires the `client_id` request parameter when
185+
the client identity is conveyed by the client authentication method itself, in
186+
line with the specifications. For example, with `private_key_jwt` the client is
187+
identified by the assertion's `iss`/`sub` claims, and with `client_secret_basic`
188+
by the `Authorization` header. Requests that still send `client_id` are
189+
unaffected, and the authenticated client is always validated against the client
190+
the authorization code was issued to. Note that for non-registered (generic VCI)
191+
clients the `client_id` parameter is still required, as their identity cannot be
192+
derived from a credential.
184193
- Client property `is_federated` has been removed, as the OP implementation
185194
can now only be a leaf entity in the federation context, and not a federation
186195
operator or intermediary entity. Previously, this property was used to

‎src/Server/Grants/AuthCodeGrant.php‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -458,8 +458,15 @@ public function respondToAccessTokenRequest(
458458

459459
if (! $authorizationClientEntity->isGeneric()) {
460460
$this->loggerService->debug('Executing standard rules for non-generic clients.');
461+
// The client is already authoritatively known from the stored authorization code, so predefine it as the
462+
// ClientRule result instead of re-resolving it from the request. This avoids mandating the client_id
463+
// request parameter at the token endpoint, which is optional for some client authentication methods (e.g.
464+
// private_key_jwt and client_secret_basic, where the client identity is conveyed by the assertion or the
465+
// Authorization header respectively). ClientAuthenticationRule still authenticates the caller against this
466+
// client, and the resolver cross-checks that the authenticated identity matches it, so the binding between
467+
// the caller and the client the code was issued to is preserved (and strengthened).
468+
$this->requestRulesManager->predefineResult(new Result(ClientRule::class, $authorizationClientEntity));
461469
$rulesToExecute = [
462-
ClientRule::class,
463470
ClientRedirectUriRule::class,
464471
ClientAuthenticationRule::class,
465472
...$rulesToExecute,
@@ -474,8 +481,12 @@ public function respondToAccessTokenRequest(
474481
$this->allowedTokenHttpMethods,
475482
);
476483

477-
// For now, we require client_id, however, in the future this will have to be resolved based on used
478-
// client authentication...
484+
// For generic (e.g. non-registered VCI) clients there is no registered credential to authenticate
485+
// against, so the client_id parameter remains REQUIRED here: it is the only thing binding this token
486+
// request to the client the authorization code was issued to. Unlike registered clients, the identity
487+
// cannot be derived from a secret or a private_key_jwt assertion. It is matched against the bound
488+
// client_id below. If an authentication scheme for non-registered clients is introduced later (e.g.
489+
// attestation), this can be relaxed the same way it was for registered clients.
479490
if (! $clientId) {
480491
throw OidcServerException::invalidRequest('client_id');
481492
}
@@ -510,10 +521,9 @@ public function respondToAccessTokenRequest(
510521
$this->allowedTokenHttpMethods,
511522
);
512523

513-
/** @var \SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $client */
514-
$client = $authorizationClientEntity->isGeneric() ?
515-
$authorizationClientEntity :
516-
$resultBag->getOrFail(ClientRule::class)->getValue();
524+
// The client the authorization code was issued to is authoritative in both branches: for non-generic clients
525+
// it is predefined as the ClientRule result and authenticated against by ClientAuthenticationRule above.
526+
$client = $authorizationClientEntity;
517527

518528
/** @var ?ResolvedClientAuthenticationMethod $resolvedClientAuthenticationMethod */
519529
$resolvedClientAuthenticationMethod = $authorizationClientEntity->isGeneric() ?

‎src/Server/RequestRules/Rules/ClientAuthenticationRule.php‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
namespace SimpleSAML\Module\oidc\Server\RequestRules\Rules;
66

77
use Psr\Http\Message\ServerRequestInterface;
8+
use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface;
89
use SimpleSAML\Module\oidc\Helpers;
910
use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException;
1011
use SimpleSAML\Module\oidc\Server\RequestRules\Interfaces\ResultBagInterface;
@@ -16,6 +17,7 @@
1617
use SimpleSAML\Module\oidc\Utils\AuthenticatedOAuth2ClientResolver;
1718
use SimpleSAML\Module\oidc\Utils\RequestParamsResolver;
1819
use SimpleSAML\OpenID\Codebooks\HttpMethodsEnum;
20+
use SimpleSAML\OpenID\Codebooks\ParamsEnum;
1921

2022
class ClientAuthenticationRule extends AbstractRule
2123
{
@@ -32,6 +34,7 @@ public function __construct(
3234
* @throws \Throwable
3335
*
3436
* @param ResponseModeInterface $responseMode
37+
* @param HttpMethodsEnum[] $allowedServerRequestMethods
3538
*/
3639
public function checkRule(
3740
ServerRequestInterface $request,
@@ -44,12 +47,27 @@ public function checkRule(
4447

4548
$loggerService->debug('ClientAuthenticationRule::checkRule');
4649

47-
// TODO mivanci Instead of ClientRule which mandates client, this should
48-
// be refactored to use optional client_id parameter and then
49-
// fetch client if present.
50+
// Use a client that an upstream rule may have already resolved (e.g. ClientRule). If none is available,
51+
// optionally pre-fetch it using the client_id request parameter. The client_id parameter is intentionally
52+
// NOT mandatory here: some client authentication methods convey the client identity by other means (e.g.
53+
// private_key_jwt via the assertion issuer, client_secret_basic via the Authorization header). When no client
54+
// is pre-fetched, the resolver derives and authenticates the client purely from the presented authentication
55+
// material, and cross-checks any client_id it does find against that material.
5056
/** @var ?\SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface $preFetchedClient */
5157
$preFetchedClient = $currentResultBag->get(ClientRule::class)?->getValue();
5258

59+
if (!$preFetchedClient instanceof ClientEntityInterface) {
60+
$clientId = $this->requestParamsResolver->getAsStringBasedOnAllowedMethods(
61+
ParamsEnum::ClientId->value,
62+
$request,
63+
$allowedServerRequestMethods,
64+
);
65+
66+
$preFetchedClient = is_string($clientId) ?
67+
$this->authenticatedOAuth2ClientResolver->findActiveClient($clientId) :
68+
null;
69+
}
70+
5371
$resolvedClientAuthenticationMethod = $this->authenticatedOAuth2ClientResolver->forAnySupportedMethod(
5472
request: $request,
5573
preFetchedClient: $preFetchedClient,

‎tests/unit/src/Server/RequestRules/Rules/ClientAuthenticationRuleTest.php‎

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SimpleSAML\Test\Module\oidc\unit\Server\RequestRules\Rules;
6+
7+
use PHPUnit\Framework\MockObject\MockObject;
8+
use PHPUnit\Framework\MockObject\Stub;
9+
use PHPUnit\Framework\TestCase;
10+
use Psr\Http\Message\ServerRequestInterface;
11+
use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface;
12+
use SimpleSAML\Module\oidc\Helpers;
13+
use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException;
14+
use SimpleSAML\Module\oidc\Server\RequestRules\Interfaces\ResultInterface;
15+
use SimpleSAML\Module\oidc\Server\RequestRules\Result;
16+
use SimpleSAML\Module\oidc\Server\RequestRules\ResultBag;
17+
use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientAuthenticationRule;
18+
use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientRule;
19+
use SimpleSAML\Module\oidc\Server\ResponseModes\ResponseModeInterface;
20+
use SimpleSAML\Module\oidc\Services\LoggerService;
21+
use SimpleSAML\Module\oidc\Utils\AuthenticatedOAuth2ClientResolver;
22+
use SimpleSAML\Module\oidc\Utils\RequestParamsResolver;
23+
use SimpleSAML\Module\oidc\ValueAbstracts\ResolvedClientAuthenticationMethod;
24+
use SimpleSAML\OpenID\Codebooks\ClientAuthenticationMethodsEnum;
25+
26+
/**
27+
* @covers \SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientAuthenticationRule
28+
*/
29+
class ClientAuthenticationRuleTest extends TestCase
30+
{
31+
protected ResultBag $resultBag;
32+
protected Stub $clientStub;
33+
protected Stub $requestStub;
34+
protected Stub $loggerServiceStub;
35+
protected MockObject $requestParamsResolverMock;
36+
protected Helpers $helpers;
37+
protected MockObject $authenticatedOAuth2ClientResolverMock;
38+
protected Stub $responseModeStub;
39+
40+
protected function setUp(): void
41+
{
42+
$this->resultBag = new ResultBag();
43+
$this->clientStub = $this->createStub(ClientEntityInterface::class);
44+
$this->requestStub = $this->createStub(ServerRequestInterface::class);
45+
$this->loggerServiceStub = $this->createStub(LoggerService::class);
46+
$this->requestParamsResolverMock = $this->createMock(RequestParamsResolver::class);
47+
$this->helpers = new Helpers();
48+
$this->authenticatedOAuth2ClientResolverMock = $this->createMock(AuthenticatedOAuth2ClientResolver::class);
49+
$this->responseModeStub = $this->createStub(ResponseModeInterface::class);
50+
}
51+
52+
protected function sut(
53+
?RequestParamsResolver $requestParamsResolver = null,
54+
?Helpers $helpers = null,
55+
?AuthenticatedOAuth2ClientResolver $authenticatedOAuth2ClientResolver = null,
56+
): ClientAuthenticationRule {
57+
$requestParamsResolver ??= $this->requestParamsResolverMock;
58+
$helpers ??= $this->helpers;
59+
$authenticatedOAuth2ClientResolver ??= $this->authenticatedOAuth2ClientResolverMock;
60+
61+
return new ClientAuthenticationRule(
62+
$requestParamsResolver,
63+
$helpers,
64+
$authenticatedOAuth2ClientResolver,
65+
);
66+
}
67+
68+
/**
69+
* A client already resolved by an upstream rule (ClientRule) is used as the pre-fetched client, without
70+
* touching the client_id request parameter.
71+
*
72+
* @throws \Throwable
73+
*/
74+
public function testUsesPreFetchedClientFromResultBag(): void
75+
{
76+
$this->resultBag->add(new Result(ClientRule::class, $this->clientStub));
77+
78+
$resolved = new ResolvedClientAuthenticationMethod(
79+
$this->clientStub,
80+
ClientAuthenticationMethodsEnum::ClientSecretBasic,
81+
);
82+
83+
// The client_id param fallback must not be consulted when a client is already available.
84+
$this->requestParamsResolverMock->expects($this->never())
85+
->method('getAsStringBasedOnAllowedMethods');
86+
$this->authenticatedOAuth2ClientResolverMock->expects($this->never())
87+
->method('findActiveClient');
88+
89+
$this->authenticatedOAuth2ClientResolverMock->expects($this->once())
90+
->method('forAnySupportedMethod')
91+
->with($this->identicalTo($this->requestStub), $this->identicalTo($this->clientStub))
92+
->willReturn($resolved);
93+
94+
$result = $this->sut()->checkRule(
95+
$this->requestStub,
96+
$this->resultBag,
97+
$this->loggerServiceStub,
98+
[],
99+
$this->responseModeStub,
100+
);
101+
102+
$this->assertInstanceOf(ResultInterface::class, $result);
103+
$this->assertSame($resolved, $result->getValue());
104+
}
105+
106+
/**
107+
* When no upstream client is available but a client_id param is present, it is used to pre-fetch the client.
108+
*
109+
* @throws \Throwable
110+
*/
111+
public function testFallsBackToClientIdParameterWhenPresent(): void
112+
{
113+
$this->requestParamsResolverMock->method('getAsStringBasedOnAllowedMethods')
114+
->willReturn('client123');
115+
116+
$this->authenticatedOAuth2ClientResolverMock->expects($this->once())
117+
->method('findActiveClient')
118+
->with('client123')
119+
->willReturn($this->clientStub);
120+
121+
$resolved = new ResolvedClientAuthenticationMethod(
122+
$this->clientStub,
123+
ClientAuthenticationMethodsEnum::ClientSecretPost,
124+
);
125+
126+
$this->authenticatedOAuth2ClientResolverMock->expects($this->once())
127+
->method('forAnySupportedMethod')
128+
->with($this->identicalTo($this->requestStub), $this->identicalTo($this->clientStub))
129+
->willReturn($resolved);
130+
131+
$result = $this->sut()->checkRule(
132+
$this->requestStub,
133+
$this->resultBag,
134+
$this->loggerServiceStub,
135+
[],
136+
$this->responseModeStub,
137+
);
138+
139+
$this->assertInstanceOf(ResultInterface::class, $result);
140+
$this->assertSame($resolved, $result->getValue());
141+
}
142+
143+
/**
144+
* The core of the fix: with no upstream client and no client_id parameter (e.g. private_key_jwt, where the
145+
* identity is conveyed by the assertion), the rule must still authenticate by letting the resolver derive the
146+
* client from the presented authentication material - it must NOT reject the request for a missing client_id.
147+
*
148+
* @throws \Throwable
149+
*/
150+
public function testDoesNotRequireClientIdParameter(): void
151+
{
152+
$this->requestParamsResolverMock->method('getAsStringBasedOnAllowedMethods')
153+
->willReturn(null);
154+
155+
// With no client_id param, no pre-fetch lookup must happen...
156+
$this->authenticatedOAuth2ClientResolverMock->expects($this->never())
157+
->method('findActiveClient');
158+
159+
$resolved = new ResolvedClientAuthenticationMethod(
160+
$this->clientStub,
161+
ClientAuthenticationMethodsEnum::PrivateKeyJwt,
162+
);
163+
164+
// ...and the resolver is invoked with a null pre-fetched client.
165+
$this->authenticatedOAuth2ClientResolverMock->expects($this->once())
166+
->method('forAnySupportedMethod')
167+
->with($this->identicalTo($this->requestStub), null)
168+
->willReturn($resolved);
169+
170+
$result = $this->sut()->checkRule(
171+
$this->requestStub,
172+
$this->resultBag,
173+
$this->loggerServiceStub,
174+
[],
175+
$this->responseModeStub,
176+
);
177+
178+
$this->assertInstanceOf(ResultInterface::class, $result);
179+
$this->assertSame($resolved, $result->getValue());
180+
}
181+
182+
/**
183+
* If the resolver can not authenticate the client by any supported method, the request is denied.
184+
*
185+
* @throws \Throwable
186+
*/
187+
public function testThrowsWhenNoAuthenticationMethodResolved(): void
188+
{
189+
$this->requestParamsResolverMock->method('getAsStringBasedOnAllowedMethods')
190+
->willReturn(null);
191+
192+
$this->authenticatedOAuth2ClientResolverMock->method('forAnySupportedMethod')
193+
->willReturn(null);
194+
195+
$this->expectException(OidcServerException::class);
196+
197+
$this->sut()->checkRule(
198+
$this->requestStub,
199+
$this->resultBag,
200+
$this->loggerServiceStub,
201+
[],
202+
$this->responseModeStub,
203+
);
204+
}
205+
206+
/**
207+
* A confidential client must not authenticate using the 'none' method.
208+
*
209+
* @throws \Throwable
210+
*/
211+
public function testThrowsWhenConfidentialClientUsesNone(): void
212+
{
213+
$this->resultBag->add(new Result(ClientRule::class, $this->clientStub));
214+
$this->clientStub->method('isConfidential')->willReturn(true);
215+
216+
$resolved = new ResolvedClientAuthenticationMethod(
217+
$this->clientStub,
218+
ClientAuthenticationMethodsEnum::None,
219+
);
220+
221+
$this->authenticatedOAuth2ClientResolverMock->method('forAnySupportedMethod')
222+
->willReturn($resolved);
223+
224+
$this->expectException(OidcServerException::class);
225+
226+
$this->sut()->checkRule(
227+
$this->requestStub,
228+
$this->resultBag,
229+
$this->loggerServiceStub,
230+
[],
231+
$this->responseModeStub,
232+
);
233+
}
234+
}

0 commit comments

Comments
 (0)