Skip to content

Commit bc92929

Browse files
cicnavicicnavi
committed
WIP
1 parent 1364817 commit bc92929

4 files changed

Lines changed: 126 additions & 13 deletions

File tree

‎docs/9-oidc-dcr-client-metadata.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,18 @@ method nor `native` is present (e.g. a federation/manual client), the explicit
9696
`is_confidential` value stands. (Consequence: to make a `native` client confidential,
9797
give it a real authentication method.)
9898

99+
## RFC 7592 update semantics (full replace)
100+
101+
A client update at the Client Configuration Endpoint (HTTP `PUT`) is a **full
102+
replace**, not a merge, per RFC 7592 §2.2: client-settable metadata that the update
103+
request omits is reset to its OP default (or removed), so the client must send the
104+
complete intended metadata set on every update. Server-managed and admin-only
105+
properties are preserved across the update — the client identifier and secret,
106+
`created_at`, the registration type, the registration access token, and in
107+
particular any administrator-set `authproc` (which a registering client can never
108+
set). This applies to Dynamic (DCR) registrations; manual (admin UI) and OpenID
109+
Federation registrations are unaffected.
110+
99111
## `redirect_uris` constraints by `application_type`
100112

101113
Per OpenID Connect Dynamic Client Registration 1.0 (Section 2, `application_type`),

‎src/Controllers/Admin/ClientController.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ public function edit(Request $request): Response
304304
}
305305

306306
/**
307-
* TODO v7 mivanci Move to ClientEntityFactory::fromRegistrationData on dynamic client registration implementation.
307+
* TODO v8 mivanci Move to ClientEntityFactory::fromRegistrationData on dynamic client registration implementation.
308308
* @throws \SimpleSAML\Module\oidc\Exceptions\OidcException
309309
*/
310310
protected function buildClientEntityFromFormData(

‎src/Factories/Entities/ClientEntityFactory.php‎

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -158,12 +158,22 @@ public function fromRegistrationData(
158158
unset($metadata[$adminOnlyMetadataKey]);
159159
}
160160

161+
// RFC 7592 client update is a full REPLACE, not a merge: on a DCR update, client-settable metadata the
162+
// request omits must be reset to its OP default (or removed), while server-managed and admin-only
163+
// properties are still carried over from the existing client. We model that with a separate "metadata
164+
// fallback" client that is null on a DCR update, so the per-field `?? $metadataFallbackClient?->...`
165+
// expressions below fall back to the default rather than the previously-registered value. Manual and
166+
// OpenID Federation registrations keep their existing merge behaviour (the entity statement / admin form
167+
// carries the full intended state anyway).
168+
$isDcrUpdate = $existingClient !== null && $registrationType === RegistrationTypeEnum::Dynamic;
169+
$metadataFallbackClient = $isDcrUpdate ? null : $existingClient;
170+
161171
$id = $clientIdentifier ?? $existingClient?->getIdentifier() ??
162172
$this->sspBridge->utils()->random()->generateID();
163173

164174
$secret = $existingClient?->getSecret() ?? $this->sspBridge->utils()->random()->generateID();
165175

166-
$name = (string)($metadata[ClaimsEnum::ClientName->value] ?? $existingClient?->getName() ?? $id);
176+
$name = (string)($metadata[ClaimsEnum::ClientName->value] ?? $metadataFallbackClient?->getName() ?? $id);
167177

168178
$description = $existingClient?->getDescription() ?? '';
169179

@@ -175,7 +185,7 @@ public function fromRegistrationData(
175185

176186
// Resolve the requested scopes: from this request's metadata, falling back to an existing client's scopes
177187
// (e.g. on a DCR update that omits `scope`). null here means scopes were genuinely not specified.
178-
$requestedScopes = $metadata[ClaimsEnum::Scope->value] ?? $existingClient?->getScopes();
188+
$requestedScopes = $metadata[ClaimsEnum::Scope->value] ?? $metadataFallbackClient?->getScopes();
179189
if ($requestedScopes === null) {
180190
// No scope was specified. For Dynamic Client Registration, assign the configured default scope set
181191
// (OIDC DCR 1.0 lets the OP assign a default set). Manual and OpenID Federation automatic registrations
@@ -205,7 +215,7 @@ public function fromRegistrationData(
205215
? $this->moduleConfig->getDcrRegisteredClientsEnabled()
206216
: true);
207217

208-
$isConfidential = $existingClient?->isConfidential() ?? $this->determineIsConfidential(
218+
$isConfidential = $metadataFallbackClient?->isConfidential() ?? $this->determineIsConfidential(
209219
$metadata,
210220
);
211221

@@ -214,39 +224,39 @@ public function fromRegistrationData(
214224
$postLogoutRedirectUris = isset($metadata[ClaimsEnum::PostLogoutRedirectUris->value]) &&
215225
is_array($metadata[ClaimsEnum::PostLogoutRedirectUris->value]) ?
216226
$this->helpers->arr()->ensureStringValues($metadata[ClaimsEnum::PostLogoutRedirectUris->value]) :
217-
$existingClient?->getPostLogoutRedirectUri() ?? [];
227+
$metadataFallbackClient?->getPostLogoutRedirectUri() ?? [];
218228

219229
$backChannelLogoutUri = isset($metadata[ClaimsEnum::BackChannelLogoutUri->value]) &&
220230
is_string($metadata[ClaimsEnum::BackChannelLogoutUri->value]) ?
221231
$metadata[ClaimsEnum::BackChannelLogoutUri->value] :
222-
$existingClient?->getBackChannelLogoutUri();
232+
$metadataFallbackClient?->getBackChannelLogoutUri();
223233

224234
$entityIdentifier = $clientIdentifier ?? $existingClient?->getEntityIdentifier();
225235

226236
$clientRegistrationTypes = isset($metadata[ClaimsEnum::ClientRegistrationTypes->value]) &&
227237
is_array($metadata[ClaimsEnum::ClientRegistrationTypes->value]) ?
228238
$this->helpers->arr()->ensureStringValues($metadata[ClaimsEnum::ClientRegistrationTypes->value]) :
229-
$existingClient?->getClientRegistrationTypes();
239+
$metadataFallbackClient?->getClientRegistrationTypes();
230240

231-
$federationJwks = $federationJwks ?? $existingClient?->getFederationJwks();
241+
$federationJwks = $federationJwks ?? $metadataFallbackClient?->getFederationJwks();
232242

233243
/** @var ?array[] $jwks */
234244
$jwks = isset($metadata[ClaimsEnum::Jwks->value]) &&
235245
is_array($metadata[ClaimsEnum::Jwks->value]) &&
236246
array_key_exists(ClaimsEnum::Keys->value, $metadata[ClaimsEnum::Jwks->value]) &&
237247
(!empty($metadata[ClaimsEnum::Jwks->value][ClaimsEnum::Keys->value])) ?
238248
$metadata[ClaimsEnum::Jwks->value] :
239-
$existingClient?->getJwks();
249+
$metadataFallbackClient?->getJwks();
240250

241251
$jwksUri = isset($metadata[ClaimsEnum::JwksUri->value]) &&
242252
is_string($metadata[ClaimsEnum::JwksUri->value]) ?
243253
$metadata[ClaimsEnum::JwksUri->value] :
244-
$existingClient?->getJwksUri();
254+
$metadataFallbackClient?->getJwksUri();
245255

246256
$signedJwksUri = isset($metadata[ClaimsEnum::SignedJwksUri->value]) &&
247257
is_string($metadata[ClaimsEnum::SignedJwksUri->value]) ?
248258
$metadata[ClaimsEnum::SignedJwksUri->value] :
249-
$existingClient?->getSignedJwksUri();
259+
$metadataFallbackClient?->getSignedJwksUri();
250260

251261
// $registrationType = $registrationType;
252262

@@ -262,14 +272,27 @@ public function fromRegistrationData(
262272
// is null here; the registration controller generates and assigns the token after building the entity.
263273
$registrationAccessToken = $existingClient?->getRegistrationAccessTokenHash();
264274

265-
$extraMetadata = $existingClient?->getExtraMetadata() ?? [];
275+
// On a DCR update this starts empty (replace semantics); on create/manual/federation it carries the existing
276+
// extra metadata. Admin-only extra metadata (e.g. authproc) is never client-settable and is re-injected from
277+
// the real existing client below so a DCR update cannot drop it.
278+
$extraMetadata = $metadataFallbackClient?->getExtraMetadata() ?? [];
279+
if ($isDcrUpdate) {
280+
// $isDcrUpdate implies $existingClient is non-null (see its definition above).
281+
$existingExtraMetadata = $existingClient->getExtraMetadata();
282+
foreach (ClientEntity::ADMIN_ONLY_METADATA_KEYS as $adminOnlyMetadataKey) {
283+
if (array_key_exists($adminOnlyMetadataKey, $existingExtraMetadata)) {
284+
/** @psalm-suppress MixedAssignment */
285+
$extraMetadata[$adminOnlyMetadataKey] = $existingExtraMetadata[$adminOnlyMetadataKey];
286+
}
287+
}
288+
}
266289

267290
// Handle any other supported client metadata as extra metadata.
268291
// id_token_signed_response_alg
269292
$idTokenSignedResponseAlg = isset($metadata[ClaimsEnum::IdTokenSignedResponseAlg->value]) &&
270293
is_string($metadata[ClaimsEnum::IdTokenSignedResponseAlg->value]) ?
271294
$metadata[ClaimsEnum::IdTokenSignedResponseAlg->value] :
272-
$existingClient?->getIdTokenSignedResponseAlg();
295+
$metadataFallbackClient?->getIdTokenSignedResponseAlg();
273296

274297
// Make sure the requested id_token_signed_response_alg is one of the OP
275298
// can actually sign ID Tokens with, i.e. one for which a protocol

‎tests/unit/src/Factories/Entities/ClientEntityFactoryTest.php‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,84 @@ public function testFromRegistrationDataUpdatePreservesEnabledStateUnderReviewSe
509509
$this->assertTrue($updatedClient->isEnabled());
510510
}
511511

512+
/**
513+
* RFC 7592 update is a full replace: client-settable metadata omitted from the update request is reset to its
514+
* default (or removed), not retained from the previous registration.
515+
*
516+
* @throws \SimpleSAML\Error\ConfigurationError
517+
* @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException
518+
*/
519+
public function testFromRegistrationDataUpdateReplacesOmittedClientMetadata(): void
520+
{
521+
$this->moduleConfigMock->method('getDcrDefaultScopes')->willReturn(['openid']);
522+
523+
$original = $this->sut()->fromRegistrationData(
524+
[
525+
ClaimsEnum::RedirectUris->value => ['https://example.org/cb'],
526+
ClaimsEnum::ClientName->value => 'Original Name',
527+
ClaimsEnum::Scope->value => 'openid profile',
528+
ClaimsEnum::GrantTypes->value => ['authorization_code', 'refresh_token'],
529+
ClaimsEnum::LogoUri->value => 'https://example.org/logo.png',
530+
ClaimsEnum::PostLogoutRedirectUris->value => ['https://example.org/post'],
531+
],
532+
RegistrationTypeEnum::Dynamic,
533+
);
534+
$this->assertSame('Original Name', $original->getName());
535+
$this->assertContains('refresh_token', $original->getGrantTypes());
536+
$this->assertSame('https://example.org/logo.png', $original->getLogoUri());
537+
538+
// Update with redirect_uris only: every other client-settable field must be reset.
539+
$updated = $this->sut()->fromRegistrationData(
540+
[ClaimsEnum::RedirectUris->value => ['https://example.org/cb2']],
541+
RegistrationTypeEnum::Dynamic,
542+
existingClient: $original,
543+
);
544+
545+
$this->assertSame(['https://example.org/cb2'], $updated->getRedirectUris());
546+
$this->assertSame($updated->getIdentifier(), $updated->getName()); // client_name reset to client_id
547+
$this->assertSame(['authorization_code'], $updated->getGrantTypes()); // reset to DCR default
548+
$this->assertSame(['openid'], $updated->getScopes()); // reset to default scope set
549+
$this->assertNull($updated->getLogoUri()); // removed
550+
$this->assertSame([], $updated->getPostLogoutRedirectUri()); // removed
551+
552+
// Server-managed identity is preserved across the update.
553+
$this->assertSame($original->getIdentifier(), $updated->getIdentifier());
554+
$this->assertSame($original->getSecret(), $updated->getSecret());
555+
}
556+
557+
/**
558+
* Admin-only metadata (e.g. authproc, which a registering client can never set) survives an RFC 7592 update,
559+
* even though the update otherwise replaces client-settable metadata.
560+
*
561+
* @throws \SimpleSAML\Error\ConfigurationError
562+
* @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException
563+
*/
564+
public function testFromRegistrationDataUpdateRetainsAdminOnlyMetadata(): void
565+
{
566+
$authProcFilters = [60 => ['class' => 'core:AttributeAdd']];
567+
$existing = $this->sut()->fromData(
568+
'client-1',
569+
'secret-1',
570+
'Name',
571+
'',
572+
['https://example.org/cb'],
573+
['openid'],
574+
true,
575+
true,
576+
registrationType: RegistrationTypeEnum::Dynamic,
577+
extraMetadata: [ClientEntity::KEY_AUTH_PROC_FILTERS => $authProcFilters],
578+
);
579+
$this->assertSame($authProcFilters, $existing->getAuthProcFilters());
580+
581+
$updated = $this->sut()->fromRegistrationData(
582+
[ClaimsEnum::RedirectUris->value => ['https://example.org/cb2']],
583+
RegistrationTypeEnum::Dynamic,
584+
existingClient: $existing,
585+
);
586+
587+
$this->assertSame($authProcFilters, $updated->getAuthProcFilters());
588+
}
589+
512590
/**
513591
* Federation automatic registrations are not forced to the Dynamic defaults: nothing is persisted for these
514592
* three fields unless the federation metadata provides them.

0 commit comments

Comments
 (0)