From 6112df4bd32967743f59d255532a1bcbdce0cd37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Fri, 19 Jun 2026 19:37:01 +0200 Subject: [PATCH 1/4] Add client authproc property --- docs/3-oidc-configuration.md | 54 ++++++++++- docs/6-oidc-upgrade.md | 13 +++ src/Controllers/Admin/ClientController.php | 9 ++ src/Entities/ClientEntity.php | 40 ++++++++ .../Interfaces/ClientEntityInterface.php | 5 + .../Entities/ClientEntityFactory.php | 26 +++++ src/Factories/ProcessingChainFactory.php | 17 ++-- src/Forms/ClientForm.php | 96 +++++++++++++++++++ src/Services/AuthenticationService.php | 47 ++++++--- templates/clients/includes/form.twig | 9 ++ tests/unit/src/Entities/ClientEntityTest.php | 45 +++++++++ .../Entities/ClientEntityFactoryTest.php | 59 ++++++++++++ .../Factories/ProcessingChainFactoryTest.php | 24 ++--- tests/unit/src/Forms/ClientFormTest.php | 68 +++++++++++++ .../Services/AuthenticationServiceTest.php | 49 ++++++++++ 15 files changed, 525 insertions(+), 36 deletions(-) diff --git a/docs/3-oidc-configuration.md b/docs/3-oidc-configuration.md index 6142ea12..9f450c1b 100644 --- a/docs/3-oidc-configuration.md +++ b/docs/3-oidc-configuration.md @@ -237,7 +237,7 @@ $config = [ ]; ``` -## Auth Proc filters (OIDC) +## Authentication Processing filters (OIDC) Standard SAML Auth Proc Filters do not run during OIDC authN because not all SAML entities are present (like a Service Provider). Instead, use the @@ -277,6 +277,58 @@ $config = [ ]; ``` +### Per-client Auth Proc filters + +In addition to the global filters above, you can configure Auth Proc filters +for a **specific client (Relying Party)**. This mimics the way SAML allows +filters to be defined in Service Provider metadata. + +Client filters are stored together with the client in the database (as part of +the client's extra metadata) and are managed from the client administration UI: + +- OIDC > Client Registry > (edit a client) > **Authentication Processing Filters** + +The value is entered as a JSON object using the same structure as the global +`authproc.oidc` option (keyed by priority; each entry is a class string or an +object with a `class` property), for example: + +```json +{ + "60": { + "class": "core:AttributeAdd", + "groups": ["members"] + } +} +``` + +During authentication for that client, its filters are merged with the global +filters by priority (the global filters run as the "IdP-side" list and the +client filters as the "SP-side" list), exactly as SAML merges IdP and SP +`authproc` filters. + +> **Security note:** Auth Proc filters name a PHP class that is instantiated and +> executed on the OP during authentication. For this reason, per-client filters +> can only be set by a trusted administrator through the admin UI / API. They are +> **deliberately never accepted from client-supplied registration metadata** +> (OIDC Dynamic Client Registration or OpenID Federation registration); any such +> value present in registration metadata is ignored. This deny-list of +> administrator-only client properties is defined in +> `\SimpleSAML\Module\oidc\Entities\ClientEntity::ADMIN_ONLY_METADATA_KEYS` and +> enforced in `ClientEntityFactory::fromRegistrationData()`. + +Alternatively, if you only need a global filter to run for selected clients, you +can keep using the global `authproc.oidc` option together with a +[preconditional filter](https://simplesamlphp.org/docs/stable/simplesamlphp-authproc.html#preconditional-filters), +inspecting the client ID via `$state['Destination']['entityid']`: + +```php +50 => [ + 'class' => 'core:AttributeAdd', + 'groups' => ['members'], + '%precondition' => 'return $state["Destination"]["entityid"] === "https://rp.example.org/";', +], +``` + ## Client registration permissions You can allow users to register their own clients. Control this via the diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md index 44a084dd..c04106a5 100644 --- a/docs/6-oidc-upgrade.md +++ b/docs/6-oidc-upgrade.md @@ -61,6 +61,19 @@ in the OP discovery metadata via the `response_modes_supported` claim. (`query`, `fragment`, `form_post`) are allowed, so existing clients are unaffected. It can be narrowed, for example to `form_post` only, to protect against browser-swapping attacks (if supported by the client). +- Authentication Processing Filters can now be configured per client (Relying +Party), in addition to the global filters defined under `authproc.oidc`. This +mimics defining authproc filters in SAML Service Provider metadata. During +authentication the global (IdP-side) and per-client (SP-side) filters are merged +by priority. The filters are stored together with the client (inside its extra +metadata) and are managed from the client administration UI as a JSON object, +using the same structure as the global filters. For security reasons, per-client +filters can only be set by an administrator (via the admin UI / API) and are +deliberately never accepted from client-supplied dynamic / OpenID Federation +registration metadata (a filter names a PHP class executed on the OP, so +honoring it from registration would be a remote code execution vector). + - Clients can now be configured with a new property related to the above: + - Authentication Processing Filters (`authproc`) - The encryption key (used to encrypt / decrypt artifacts like authorization codes and refresh tokens) can now optionally be set to a strong, pre-generated `\Defuse\Crypto\Key`, instead of always deriving it from the SimpleSAMLphp diff --git a/src/Controllers/Admin/ClientController.php b/src/Controllers/Admin/ClientController.php index 1ca4490b..f644dea3 100644 --- a/src/Controllers/Admin/ClientController.php +++ b/src/Controllers/Admin/ClientController.php @@ -370,6 +370,15 @@ protected function buildClientEntityFromFormData( $data[ClientEntity::KEY_ALLOWED_RESPONSE_MODES] : []; $extraMetadata[ClientEntity::KEY_ALLOWED_RESPONSE_MODES] = $allowedResponseModes; + // Per-client authproc filters. These are administrator-only (settable + // here, via the admin UI), and are deliberately never accepted from + // client-supplied registration metadata. See + // ClientEntityFactory::fromRegistrationData() and + // ClientEntity::ADMIN_ONLY_METADATA_KEYS. + $authProcFilters = is_array($data[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? null) ? + $data[ClientEntity::KEY_AUTH_PROC_FILTERS] : []; + $extraMetadata[ClientEntity::KEY_AUTH_PROC_FILTERS] = $authProcFilters; + return $this->clientEntityFactory->fromData( $identifier, $secret, diff --git a/src/Entities/ClientEntity.php b/src/Entities/ClientEntity.php index a3ad4749..acbd1343 100644 --- a/src/Entities/ClientEntity.php +++ b/src/Entities/ClientEntity.php @@ -56,6 +56,26 @@ class ClientEntity implements ClientEntityInterface public const string KEY_IS_GENERIC = 'is_generic'; public const string KEY_EXTRA_METADATA = 'extra_metadata'; public const string KEY_ALLOWED_RESPONSE_MODES = 'allowed_response_modes'; + /** + * Per-client Authentication Processing Filters. Stored as an entry inside + * the extra metadata JSON blob. + */ + public const string KEY_AUTH_PROC_FILTERS = 'authproc'; + + /** + * Client properties (metadata keys) which are "administrator-only": + * they may only be set by a trusted administrator (via the admin UI / API, + * i.e. ClientEntityFactory::fromData()), and MUST NOT be honored when they + * arrive in client-supplied registration metadata (OIDC Dynamic Client + * Registration or OpenID Federation). See the deny-list handling and the + * accompanying explanation in + * \SimpleSAML\Module\oidc\Factories\Entities\ClientEntityFactory::fromRegistrationData(). + * + * @var string[] + */ + public const array ADMIN_ONLY_METADATA_KEYS = [ + self::KEY_AUTH_PROC_FILTERS, + ]; private string $secret; @@ -236,6 +256,7 @@ public function toArray(): array ClaimsEnum::RequirePushedAuthorizationRequests->value => $this->getRequirePushedAuthorizationRequests(), ClaimsEnum::RequireSignedRequestObject->value => $this->getRequireSignedRequestObject(), ClaimsEnum::RequestUris->value => $this->getRequestUris(), + self::KEY_AUTH_PROC_FILTERS => $this->getAuthProcFilters(), ]; } @@ -429,6 +450,25 @@ public function getRequireSignedRequestObject(): bool return (bool)($this->extraMetadata[ClaimsEnum::RequireSignedRequestObject->value] ?? false); } + /** + * Per-client Authentication Processing Filters, in the same format as the + * global ModuleConfig::OPTION_AUTH_PROCESSING_FILTERS option. These run, in + * addition to the global filters, during authentication for this client + * (the SimpleSAMLphp ProcessingChain merges them as the "SP" side filters). + * + * @return array + */ + public function getAuthProcFilters(): array + { + if (!is_array($this->extraMetadata)) { + return []; + } + + $authProcFilters = $this->extraMetadata[self::KEY_AUTH_PROC_FILTERS] ?? null; + + return is_array($authProcFilters) ? $authProcFilters : []; + } + /** * @return string[] */ diff --git a/src/Entities/Interfaces/ClientEntityInterface.php b/src/Entities/Interfaces/ClientEntityInterface.php index 9ca01687..6d66c544 100644 --- a/src/Entities/Interfaces/ClientEntityInterface.php +++ b/src/Entities/Interfaces/ClientEntityInterface.php @@ -89,4 +89,9 @@ public function getRequireSignedRequestObject(): bool; * @return string[] */ public function getRequestUris(): array; + + /** + * @return array + */ + public function getAuthProcFilters(): array; } diff --git a/src/Factories/Entities/ClientEntityFactory.php b/src/Factories/Entities/ClientEntityFactory.php index 4f03ca6a..79224cd4 100644 --- a/src/Factories/Entities/ClientEntityFactory.php +++ b/src/Factories/Entities/ClientEntityFactory.php @@ -106,6 +106,32 @@ public function fromRegistrationData( ?string $clientIdentifier = null, ?array $federationJwks = null, ): ClientEntityInterface { + // Security: scrub administrator-only properties from client-supplied + // registration metadata. + // + // This method builds clients from metadata provided by a remote party, + // i.e. through OIDC Dynamic Client Registration (RFC 7591) or OpenID + // Federation (explicit / automatic) registration. Some client properties + // must never be controllable by the registering party, because honoring + // them would let an untrusted client influence server-side behavior. The + // prime example is `authproc` (per-client Authentication Processing + // Filters): a filter entry names a PHP class that is instantiated and + // executed on the OP during authentication, so accepting it from + // registration metadata would be a remote code execution vector. + // + // Such properties are settable ONLY by a trusted administrator, via the + // admin UI / API (ClientEntityFactory::fromData()). We strip every + // deny-listed key from the incoming metadata here, so it can neither be + // read below nor leak into a future code path. Any value an administrator + // has already set on an existing client is preserved because it is + // carried over from $existingClient->getExtraMetadata() further down (it + // does not come from $metadata). + // + // The deny-list itself lives next to the property definitions, in + // ClientEntity::ADMIN_ONLY_METADATA_KEYS. + foreach (ClientEntity::ADMIN_ONLY_METADATA_KEYS as $adminOnlyMetadataKey) { + unset($metadata[$adminOnlyMetadataKey]); + } $id = $clientIdentifier ?? $existingClient?->getIdentifier() ?? $this->sspBridge->utils()->random()->generateID(); diff --git a/src/Factories/ProcessingChainFactory.php b/src/Factories/ProcessingChainFactory.php index 6a6afd21..3ca0e4f1 100644 --- a/src/Factories/ProcessingChainFactory.php +++ b/src/Factories/ProcessingChainFactory.php @@ -12,28 +12,29 @@ namespace SimpleSAML\Module\oidc\Factories; use SimpleSAML\Auth\ProcessingChain; -use SimpleSAML\Module\oidc\ModuleConfig; class ProcessingChainFactory { - public function __construct( - private readonly ModuleConfig $moduleConfig, - ) { - } - /** * @codeCoverageIgnore * @throws \Exception */ public function build(array $state): ProcessingChain { + // The IdP- and SP-side metadata (entityid + authproc filter lists) is + // the single source of truth prepared in + // AuthenticationService::runAuthProcs() and stored in the state. + // Here we only consume it; + // The IdP side carries the global authproc filters, the SP side the + // per-client ones, and the SimpleSAMLphp ProcessingChain merges them + // by priority. $idpMetadata = [ 'entityid' => $state['Source']['entityid'] ?? '', - // ProcessChain needs to know the list of authproc filters we defined in module_oidc configuration - 'authproc' => $this->moduleConfig->getAuthProcFilters(), + 'authproc' => $state['Source']['authproc'] ?? [], ]; $spMetadata = [ 'entityid' => $state['Destination']['entityid'] ?? '', + 'authproc' => $state['Destination']['authproc'] ?? [], ]; return new ProcessingChain( diff --git a/src/Forms/ClientForm.php b/src/Forms/ClientForm.php index 0f444fc1..114fb3b6 100644 --- a/src/Forms/ClientForm.php +++ b/src/Forms/ClientForm.php @@ -202,6 +202,71 @@ public function validateRequestUris(Form $form): void } } + /** + * Validate the per-client Authentication Processing Filters. The value is + * expected to be a JSON object/array in the same shape as the global + * ModuleConfig::OPTION_AUTH_PROCESSING_FILTERS option, i.e. a list keyed by + * priority where each filter is either a class string or an array with a + * 'class' key. JSON syntax itself is validated in getValues(). + * + * @throws \Exception + */ + public function validateAuthProcFilters(Form $form): void + { + $values = $form->getValues(self::TYPE_ARRAY); + + $authProcFilters = $values[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? []; + + if (!is_array($authProcFilters)) { + $this->addError('Authentication Processing Filters must be a JSON object.'); + return; + } + + /** + * @var mixed $filter + */ + foreach ($authProcFilters as $filter) { + if (is_string($filter)) { + continue; + } + + if (!is_array($filter)) { + $this->addError( + 'Each Authentication Processing Filter must be a class string or an object: ' . + var_export($filter, true), + ); + continue; + } + + if (!isset($filter['class']) || !is_string($filter['class'])) { + $this->addError( + "Each Authentication Processing Filter object must have a string 'class' property: " . + var_export($filter, true), + ); + } + } + } + + /** + * Cast integer-like string array keys to int, leaving all other keys (and + * the values) untouched. Only the top level is processed, which is where + * authproc filter priority keys live. Used to normalize priorities coming + * from the JSON-encoded authproc filters field. + */ + protected function castNumericKeysToInt(array $array): array + { + $result = []; + /** @var mixed $value */ + foreach ($array as $key => $value) { + if (is_string($key) && preg_match('/^-?\d+$/', $key) === 1) { + $key = (int) $key; + } + $result[$key] = $value; + } + + return $result; + } + public function validateJwks(mixed $jwks): void { if (is_null($jwks)) { @@ -319,6 +384,24 @@ public function getValues(string|object|bool|null $returnType = null, ?array $co array_keys($this->getAllowedResponseModesValues()), ); + $authProcFilters = trim((string)($values[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? '')); + try { + /** @psalm-suppress MixedAssignment */ + $decodedAuthProcFilters = $authProcFilters === '' ? + [] : + json_decode($authProcFilters, true, 512, JSON_THROW_ON_ERROR); + // Normalize numeric priority keys to integers. PHP already casts + // canonical integer string keys (e.g. "60") to int, but not forms + // like "08", so we make the priority type predictable for the + // SimpleSAMLphp ProcessingChain. + $values[ClientEntity::KEY_AUTH_PROC_FILTERS] = is_array($decodedAuthProcFilters) ? + $this->castNumericKeysToInt($decodedAuthProcFilters) : + $decodedAuthProcFilters; + } catch (\JsonException $e) { + $this->addError('Authentication Processing Filters JSON error: ' . $e->getMessage()); + $values[ClientEntity::KEY_AUTH_PROC_FILTERS] = []; + } + return $values; } @@ -390,6 +473,11 @@ public function setDefaults(object|array $values, bool $erase = false): static $values[ClientEntity::KEY_ALLOWED_RESPONSE_MODES], ) ? $values[ClientEntity::KEY_ALLOWED_RESPONSE_MODES] : []; + $authProcFilters = $values[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? null; + $values[ClientEntity::KEY_AUTH_PROC_FILTERS] = (is_array($authProcFilters) && $authProcFilters !== []) ? + (string)json_encode($authProcFilters, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) : + ''; + parent::setDefaults($values, $erase); return $this; @@ -413,6 +501,7 @@ protected function buildForm(): void $this->onValidate[] = $this->validateProtocolJwks(...); $this->onValidate[] = $this->validateJwksUri(...); $this->onValidate[] = $this->validateRequestUris(...); + $this->onValidate[] = $this->validateAuthProcFilters(...); $this->setMethod('POST'); $this->addComponent($this->csrfProtection, Form::ProtectorId); @@ -494,6 +583,13 @@ protected function buildForm(): void $this->addCheckbox(ClaimsEnum::RequireSignedRequestObject->value, 'Require Signed Request Object'); $this->addTextArea(ClaimsEnum::RequestUris->value, 'Request URIs (OIDC Core / JAR, one per line)', null, 5) ->setHtmlAttribute('class', 'full-width'); + + $this->addTextArea( + ClientEntity::KEY_AUTH_PROC_FILTERS, + Translate::noop('Authentication Processing Filters'), + null, + 5, + )->setHtmlAttribute('class', 'full-width'); } /** diff --git a/src/Services/AuthenticationService.php b/src/Services/AuthenticationService.php index 407b30af..7101ff01 100644 --- a/src/Services/AuthenticationService.php +++ b/src/Services/AuthenticationService.php @@ -27,6 +27,7 @@ use SimpleSAML\Error\NoState; use SimpleSAML\Module\oidc\Codebooks\RoutesEnum; use SimpleSAML\Module\oidc\Controllers\EndSessionController; +use SimpleSAML\Module\oidc\Entities\ClientEntity; use SimpleSAML\Module\oidc\Entities\Interfaces\ClientEntityInterface; use SimpleSAML\Module\oidc\Entities\UserEntity; use SimpleSAML\Module\oidc\Exceptions\OidcException; @@ -346,10 +347,17 @@ public function manageState(array $queryParameters): ?array } /** - * Run authproc filters with the processing chain - * Creating the ProcessingChain require metadata. - * - For the idp metadata use the OIDC issuer as the entityId (and the authprocs from the main config file) - * - For the sp metadata use the client id as the entityId (and don’t set authprocs). + * Run authproc filters with the processing chain. + * + * This is the single source of truth for the metadata that drives the + * ProcessingChain. We build it once here and store it back into the state; + * the ProcessingChainFactory then only consumes the prepared state: + * - IdP metadata uses the OIDC issuer as the entityId and carries the global + * authproc filters (from the module configuration). + * - SP metadata uses the client ID as the entityId and carries the + * per-client authproc filters (from the client's metadata). + * The SimpleSAMLphp ProcessingChain then merges both filter lists by + * priority, mimicking SAML IdP + SP authproc filters. * * @param array $state * @@ -360,19 +368,36 @@ public function manageState(array $queryParameters): ?array */ protected function runAuthProcs(array &$state): void { - $idpMetadata = [ + $state['ReturnURL'] = $this->routes->getModuleUrl(RoutesEnum::Authorization->value); + + // Note: we only augment the existing Source / Destination entries (which + // already carry the 'entityid' set in prepareStateArray()) with their + // respective authproc filter lists. No state keys are removed. + $state['Source'] = [ 'entityid' => $state['Source']['entityid'] ?? '', - // ProcessChain needs to know the list of authproc filters we defined in module_oidc configuration 'authproc' => $this->moduleConfig->getAuthProcFilters(), ]; - $spMetadata = [ + $state['Destination'] = [ 'entityid' => $state['Destination']['entityid'] ?? '', + 'authproc' => $this->resolveClientAuthProcFilters($state), ]; - $state['ReturnURL'] = $this->routes->getModuleUrl(RoutesEnum::Authorization->value); - $state['Destination'] = $spMetadata; - $state['Source'] = $idpMetadata; - $this->processingChainFactory->build($state)->processState($state); } + + /** + * Resolve per-client authproc filters from the OIDC relying party metadata + * present in the authentication state (exposed there by prepareStateArray()). + */ + protected function resolveClientAuthProcFilters(array $state): array + { + $relyingPartyMetadata = $state['Oidc']['RelyingPartyMetadata'] ?? null; + if (!is_array($relyingPartyMetadata)) { + return []; + } + + $authProcFilters = $relyingPartyMetadata[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? null; + + return is_array($authProcFilters) ? $authProcFilters : []; + } } diff --git a/templates/clients/includes/form.twig b/templates/clients/includes/form.twig index ac648687..99a50724 100644 --- a/templates/clients/includes/form.twig +++ b/templates/clients/includes/form.twig @@ -206,6 +206,15 @@ {% trans %}Require this client to always sign a request object (even in case of OpenID Connect flavor which does not require signing). Make sure the client has a JWKS key configured if this option is enabled.{% endtrans %} + + {{ form.authproc.control | raw }} + + {% trans %}JSON object (string) defining Authentication Processing Filters specific to this client, in the same format as the global authproc filters in the module configuration. These run in addition to the global filters. The object is keyed by priority; each filter is a class string or an object with a 'class' property. For security reasons, these can only be set here (by an administrator) and are never accepted from dynamic / federation client registration. Example: {"60": {"class": "core:AttributeAdd", "groups": ["members"]}}{% endtrans %} + + {% if form.authproc.hasErrors %} + {{ form.authproc.getError }} + {% endif %} +

{{ 'OpenID Federation Related Properties'|trans }}

diff --git a/tests/unit/src/Entities/ClientEntityTest.php b/tests/unit/src/Entities/ClientEntityTest.php index acd1019c..94b52c37 100644 --- a/tests/unit/src/Entities/ClientEntityTest.php +++ b/tests/unit/src/Entities/ClientEntityTest.php @@ -229,7 +229,52 @@ public function testCanExportAsArray(): void 'require_pushed_authorization_requests' => false, 'require_signed_request_object' => false, 'request_uris' => [], + 'authproc' => [], ], ); } + + /** + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + * @throws \JsonException + */ + public function testCanGetAuthProcFilters(): void + { + // No extra metadata -> empty list. + $this->assertSame([], $this->mock()->getAuthProcFilters()); + + $authProcFilters = [ + 60 => ['class' => 'core:AttributeAdd', 'groups' => ['members']], + ]; + + $clientEntity = new ClientEntity( + $this->id, + $this->secret, + $this->name, + $this->description, + $this->redirectUri, + $this->scopes, + $this->isEnabled, + $this->isConfidential, + $this->authSource, + $this->owner, + $this->postLogoutRedirectUri, + $this->backChannelLogoutUri, + $this->entityIdentifier, + $this->clientRegistrationTypes, + $this->federationJwks, + $this->jwks, + $this->jwksUri, + $this->signedJwksUri, + $this->registrationType, + $this->updatedAt, + $this->createdAt, + $this->expiresAt, + $this->isGeneric, + [ClientEntity::KEY_AUTH_PROC_FILTERS => $authProcFilters], + ); + + $this->assertSame($authProcFilters, $clientEntity->getAuthProcFilters()); + $this->assertSame($authProcFilters, $clientEntity->toArray()[ClientEntity::KEY_AUTH_PROC_FILTERS]); + } } diff --git a/tests/unit/src/Factories/Entities/ClientEntityFactoryTest.php b/tests/unit/src/Factories/Entities/ClientEntityFactoryTest.php index 05f6f6a5..06e0d2b4 100644 --- a/tests/unit/src/Factories/Entities/ClientEntityFactoryTest.php +++ b/tests/unit/src/Factories/Entities/ClientEntityFactoryTest.php @@ -128,4 +128,63 @@ public function testFromRegistrationDataThrowsWhenRedirectUrisMissing(): void $this->sut()->fromRegistrationData([], RegistrationTypeEnum::FederatedAutomatic); } + + /** + * Admin-only client properties (e.g. authproc filters) must NEVER be honored + * when supplied through client registration metadata, since an authproc + * filter names a PHP class executed server-side (remote code execution + * vector). They can only be set by an administrator via the admin UI / API. + * + * @throws \SimpleSAML\Error\ConfigurationError + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + */ + public function testFromRegistrationDataIgnoresAdminOnlyAuthProcFilters(): void + { + $client = $this->sut()->fromRegistrationData( + [ + ClaimsEnum::RedirectUris->value => ['https://example.org/cb'], + // Malicious client tries to inject an authproc filter. + ClientEntity::KEY_AUTH_PROC_FILTERS => [ + 60 => ['class' => 'core:PHP', 'code' => 'system("id");'], + ], + ], + RegistrationTypeEnum::FederatedAutomatic, + ); + + $this->assertSame([], $client->getAuthProcFilters()); + } + + /** + * An administrator-set authproc filter on an existing client must be + * preserved across re-registration, and must not be overridable by the + * (untrusted) registration metadata. + * + * @throws \SimpleSAML\Error\ConfigurationError + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + */ + public function testFromRegistrationDataPreservesAdminSetAuthProcFiltersAndIgnoresSuppliedOnes(): void + { + $adminSetFilters = [ + 50 => ['class' => 'core:AttributeAdd', 'groups' => ['members']], + ]; + + $existingClient = $this->createMock(ClientEntity::class); + $existingClient->method('getExtraMetadata')->willReturn( + [ClientEntity::KEY_AUTH_PROC_FILTERS => $adminSetFilters], + ); + + $client = $this->sut()->fromRegistrationData( + [ + ClaimsEnum::RedirectUris->value => ['https://example.org/cb'], + // Attempt to overwrite the admin-set filters via registration metadata. + ClientEntity::KEY_AUTH_PROC_FILTERS => [ + 99 => ['class' => 'core:PHP', 'code' => 'system("id");'], + ], + ], + RegistrationTypeEnum::FederatedAutomatic, + existingClient: $existingClient, + ); + + $this->assertSame($adminSetFilters, $client->getAuthProcFilters()); + } } diff --git a/tests/unit/src/Factories/ProcessingChainFactoryTest.php b/tests/unit/src/Factories/ProcessingChainFactoryTest.php index cf159289..a998fa41 100644 --- a/tests/unit/src/Factories/ProcessingChainFactoryTest.php +++ b/tests/unit/src/Factories/ProcessingChainFactoryTest.php @@ -4,11 +4,9 @@ namespace SimpleSAML\Test\Module\oidc\unit\Factories; -use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use SimpleSAML\Auth\ProcessingChain; use SimpleSAML\Module\oidc\Factories\ProcessingChainFactory; -use SimpleSAML\Module\oidc\ModuleConfig; /** * @covers \SimpleSAML\Module\oidc\Factories\ProcessingChainFactory @@ -27,6 +25,11 @@ class ProcessingChainFactoryTest extends TestCase final public const AUTH_DATA = ['Attributes' => self::USER_ENTITY_ATTRIBUTES]; final public const CLIENT_ENTITY = ['id' => 'clientid', 'redirect_uri' => 'https://rp.example.org']; final public const AUTHZ_REQUEST_PARAMS = ['client_id' => 'clientid', 'redirect_uri' => 'https://rp.example.org']; + + /** + * The factory consumes the IdP / SP metadata (entityid + authproc) that + * AuthenticationService::runAuthProcs() has already prepared in the state. + */ final public const STATE = [ 'Attributes' => self::AUTH_DATA['Attributes'], 'Oidc' => [ @@ -34,27 +37,16 @@ class ProcessingChainFactoryTest extends TestCase 'RelyingPartyMetadata' => self::CLIENT_ENTITY, 'AuthorizationRequestParameters' => self::AUTHZ_REQUEST_PARAMS, ], + 'Source' => ['entityid' => 'https://idp.example.org', 'authproc' => []], + 'Destination' => ['entityid' => 'clientid', 'authproc' => []], ]; - /** - * @var MockObject|(object&MockObject)|ModuleConfig|(ModuleConfig&object&MockObject)|(ModuleConfig&MockObject) - */ - protected MockObject $moduleConfigMock; - - /** - * @throws \Exception - */ - protected function setUp(): void - { - $this->moduleConfigMock = $this->createMock(ModuleConfig::class); - } - /** * @return ProcessingChainFactory */ protected function prepareMockedInstance(): ProcessingChainFactory { - return new ProcessingChainFactory($this->moduleConfigMock); + return new ProcessingChainFactory(); } /** diff --git a/tests/unit/src/Forms/ClientFormTest.php b/tests/unit/src/Forms/ClientFormTest.php index 0b778a71..de58f8d0 100644 --- a/tests/unit/src/Forms/ClientFormTest.php +++ b/tests/unit/src/Forms/ClientFormTest.php @@ -224,4 +224,72 @@ public function testIdTokenSignedResponseAlgSelectIsLimitedToSupportedAlgs(): vo $this->expectException(\Nette\InvalidArgumentException::class); $this->sut()->setValues([ClaimsEnum::IdTokenSignedResponseAlg->value => 'HS256']); } + + public function testAcceptsValidAuthProcFilters(): void + { + $clientForm = $this->sut(); + $clientForm->setValues([ + ClientEntity::KEY_AUTH_PROC_FILTERS => + '{"60": {"class": "core:AttributeAdd", "groups": ["members"]}}', + ]); + $clientForm->validateAuthProcFilters($clientForm); + + $this->assertFalse($clientForm->hasErrors()); + $this->assertSame( + ['60' => ['class' => 'core:AttributeAdd', 'groups' => ['members']]], + $clientForm->getValues()[ClientEntity::KEY_AUTH_PROC_FILTERS], + ); + } + + public function testCastsNumericAuthProcFilterPrioritiesToInt(): void + { + $clientForm = $this->sut(); + // "08" is NOT auto-cast to int by PHP (leading zero), so this verifies + // the explicit normalization actually does something. + $clientForm->setValues([ + ClientEntity::KEY_AUTH_PROC_FILTERS => '{"08": {"class": "core:AttributeAdd"}}', + ]); + + $filters = $clientForm->getValues()[ClientEntity::KEY_AUTH_PROC_FILTERS]; + + $this->assertSame([8 => ['class' => 'core:AttributeAdd']], $filters); + $this->assertIsInt(array_key_first($filters)); + } + + public function testRejectsAuthProcFiltersWithInvalidJson(): void + { + $clientForm = $this->sut(); + $clientForm->setValues([ClientEntity::KEY_AUTH_PROC_FILTERS => '{not-valid-json']); + // getValues() (invoked by the validator) records the JSON decoding error. + $clientForm->validateAuthProcFilters($clientForm); + + $this->assertTrue($clientForm->hasErrors()); + } + + public function testRejectsAuthProcFilterWithoutClass(): void + { + $clientForm = $this->sut(); + $clientForm->setValues([ClientEntity::KEY_AUTH_PROC_FILTERS => '{"60": {"groups": ["members"]}}']); + $clientForm->validateAuthProcFilters($clientForm); + + $this->assertTrue($clientForm->hasErrors()); + } + + public function testSetDefaultsAndGetValuesRoundTripAuthProcFilters(): void + { + $this->sspBridgeAuthSourceMock->method('getSources')->willReturn(['default-sp']); + + $filters = [60 => ['class' => 'core:AttributeAdd', 'groups' => ['members']]]; + $data = $this->clientDataSample; + $data[ClientEntity::KEY_AUTH_PROC_FILTERS] = $filters; + + $sut = $this->sut()->setDefaults($data); + + // setDefaults() encodes the array to a JSON string for the textarea, and + // getValues() decodes it back to the original structure. + $this->assertSame( + ['60' => ['class' => 'core:AttributeAdd', 'groups' => ['members']]], + $sut->getValues()[ClientEntity::KEY_AUTH_PROC_FILTERS], + ); + } } diff --git a/tests/unit/src/Services/AuthenticationServiceTest.php b/tests/unit/src/Services/AuthenticationServiceTest.php index 5e326a86..e22e7ee1 100644 --- a/tests/unit/src/Services/AuthenticationServiceTest.php +++ b/tests/unit/src/Services/AuthenticationServiceTest.php @@ -508,6 +508,55 @@ public function runAuthProcsPublic(array &$state): void $this->assertEquals($state['ReturnURL'], $returnUrl); $this->assertEquals($state['Source']['authproc'], $authProcFilters); + // The client in STATE has no per-client filters, so the SP side is empty. + $this->assertSame([], $state['Destination']['authproc']); + // Existing state keys must be preserved (nothing dropped). + $this->assertArrayHasKey('Oidc', $state); + $this->assertArrayHasKey('Attributes', $state); + } + + /** + * Per-client authproc filters (from the relying party metadata) must be + * applied on the SP (Destination) side, while the global filters remain on + * the IdP (Source) side. The two are merged by the ProcessingChain. + * + * @return void + */ + public function testRunAuthProcsAppliesPerClientAuthProcFilters(): void + { + $globalFilters = [25 => ['class' => 'core:AttributeMap', 'oid2name']]; + $clientFilters = [60 => ['class' => 'core:AttributeAdd', 'groups' => ['members']]]; + $returnUrl = 'http://example.com/authorization'; + $this->moduleConfigMock->method('getAuthProcFilters')->willReturn($globalFilters); + $this->routesMock->method('getModuleUrl')->willReturn($returnUrl); + $mockedInstance = new class ( + $this->userRepositoryMock, + $this->authSimpleFactoryMock, + $this->clientRepositoryMock, + $this->opMetadataService, + $this->sessionServiceMock, + $this->claimTranslatorExtractorMock, + $this->moduleConfigMock, + $this->processingChainFactoryMock, + $this->stateServiceMock, + $this->requestParamsResolverMock, + $this->userEntityFactoryMock, + $this->routesMock, + ) extends AuthenticationService { + public function runAuthProcsPublic(array &$state): void + { + $this->runAuthProcs($state); + } + }; + + $state = self::STATE; + // Per-client authproc filters live in the relying party metadata. + $state['Oidc']['RelyingPartyMetadata'][ClientEntity::KEY_AUTH_PROC_FILTERS] = $clientFilters; + + $mockedInstance->runAuthProcsPublic($state); + + $this->assertSame($globalFilters, $state['Source']['authproc']); + $this->assertSame($clientFilters, $state['Destination']['authproc']); } From bef88f3fa6e420d636d8671201540bdd2d88df02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Fri, 19 Jun 2026 19:57:19 +0200 Subject: [PATCH 2/4] Fix psalm --- src/Controllers/Admin/ClientController.php | 7 ++++--- src/Entities/ClientEntity.php | 1 + src/Forms/ClientForm.php | 3 +++ src/Services/AuthenticationService.php | 1 + 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Controllers/Admin/ClientController.php b/src/Controllers/Admin/ClientController.php index f644dea3..0a2a4bde 100644 --- a/src/Controllers/Admin/ClientController.php +++ b/src/Controllers/Admin/ClientController.php @@ -375,9 +375,10 @@ protected function buildClientEntityFromFormData( // client-supplied registration metadata. See // ClientEntityFactory::fromRegistrationData() and // ClientEntity::ADMIN_ONLY_METADATA_KEYS. - $authProcFilters = is_array($data[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? null) ? - $data[ClientEntity::KEY_AUTH_PROC_FILTERS] : []; - $extraMetadata[ClientEntity::KEY_AUTH_PROC_FILTERS] = $authProcFilters; + /** @var mixed $rawAuthProcFilters */ + $rawAuthProcFilters = $data[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? null; + $extraMetadata[ClientEntity::KEY_AUTH_PROC_FILTERS] = is_array($rawAuthProcFilters) ? + $rawAuthProcFilters : []; return $this->clientEntityFactory->fromData( $identifier, diff --git a/src/Entities/ClientEntity.php b/src/Entities/ClientEntity.php index acbd1343..72ed1354 100644 --- a/src/Entities/ClientEntity.php +++ b/src/Entities/ClientEntity.php @@ -464,6 +464,7 @@ public function getAuthProcFilters(): array return []; } + /** @var mixed $authProcFilters */ $authProcFilters = $this->extraMetadata[self::KEY_AUTH_PROC_FILTERS] ?? null; return is_array($authProcFilters) ? $authProcFilters : []; diff --git a/src/Forms/ClientForm.php b/src/Forms/ClientForm.php index 114fb3b6..c7bc5bd0 100644 --- a/src/Forms/ClientForm.php +++ b/src/Forms/ClientForm.php @@ -261,6 +261,7 @@ protected function castNumericKeysToInt(array $array): array if (is_string($key) && preg_match('/^-?\d+$/', $key) === 1) { $key = (int) $key; } + /** @psalm-suppress MixedAssignment */ $result[$key] = $value; } @@ -394,6 +395,7 @@ public function getValues(string|object|bool|null $returnType = null, ?array $co // canonical integer string keys (e.g. "60") to int, but not forms // like "08", so we make the priority type predictable for the // SimpleSAMLphp ProcessingChain. + /** @psalm-suppress MixedAssignment */ $values[ClientEntity::KEY_AUTH_PROC_FILTERS] = is_array($decodedAuthProcFilters) ? $this->castNumericKeysToInt($decodedAuthProcFilters) : $decodedAuthProcFilters; @@ -473,6 +475,7 @@ public function setDefaults(object|array $values, bool $erase = false): static $values[ClientEntity::KEY_ALLOWED_RESPONSE_MODES], ) ? $values[ClientEntity::KEY_ALLOWED_RESPONSE_MODES] : []; + /** @var mixed $authProcFilters */ $authProcFilters = $values[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? null; $values[ClientEntity::KEY_AUTH_PROC_FILTERS] = (is_array($authProcFilters) && $authProcFilters !== []) ? (string)json_encode($authProcFilters, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) : diff --git a/src/Services/AuthenticationService.php b/src/Services/AuthenticationService.php index 7101ff01..73322b7b 100644 --- a/src/Services/AuthenticationService.php +++ b/src/Services/AuthenticationService.php @@ -396,6 +396,7 @@ protected function resolveClientAuthProcFilters(array $state): array return []; } + /** @var mixed $authProcFilters */ $authProcFilters = $relyingPartyMetadata[ClientEntity::KEY_AUTH_PROC_FILTERS] ?? null; return is_array($authProcFilters) ? $authProcFilters : []; From 945fb112d7e59fd9c89eaea686800355756db75f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Sat, 20 Jun 2026 09:48:04 +0200 Subject: [PATCH 3/4] Show authprocs for client --- templates/clients/show.twig | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/templates/clients/show.twig b/templates/clients/show.twig index dba1aca3..9a5c6cd3 100644 --- a/templates/clients/show.twig +++ b/templates/clients/show.twig @@ -257,6 +257,20 @@ {{ client.requireSignedRequestObject ? 'Yes' : 'No' }} + + + {{ 'Authentication Processing Filters'|trans }} + + + {% if client.authProcFilters is not empty %} + + {{- client.authProcFilters|json_encode(constant('JSON_PRETTY_PRINT') b-or constant('JSON_UNESCAPED_SLASHES')) -}} + + {% else %} + {{ 'N/A'|trans }} + {% endif %} + + {{ 'Owner'|trans }} From 33f83b95cd332ea6bac5a73adb547c24186243f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Sat, 20 Jun 2026 09:48:16 +0200 Subject: [PATCH 4/4] Unify json_encode calls --- src/Forms/ClientForm.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Forms/ClientForm.php b/src/Forms/ClientForm.php index c7bc5bd0..e85685ce 100644 --- a/src/Forms/ClientForm.php +++ b/src/Forms/ClientForm.php @@ -446,9 +446,10 @@ public function setDefaults(object|array $values, bool $erase = false): static [ClientRegistrationTypesEnum::Automatic->value]; $values['federation_jwks'] = is_array($values['federation_jwks']) ? - json_encode($values['federation_jwks']) : null; + json_encode($values['federation_jwks'], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) : null; - $values['jwks'] = is_array($values['jwks']) ? json_encode($values['jwks']) : null; + $values['jwks'] = is_array($values['jwks']) ? + json_encode($values['jwks'], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) : null; if ( $values['auth_source'] !== null &&