Implement stable consent hash with version tracking

The old consent hash was sensitive to attribute ordering, case changes,
stray empty attributes, and sparse array indexes — causing spurious
re-consent prompts. This commit introduces a stable hashing algorithm
alongside the existing one, with a migration path that avoids
invalidating all existing consent records at once.

Key changes:

- Add `attribute_stable` VARCHAR(80) nullable column to `consent` table;
  make existing `attribute` column nullable for future cleanup
- Add ConsentVersion value object with three states: stable, unstable,
  not-given (replaces boolean return from consent-check methods)
- Retrieve old-style and new-style attribute hash in a single query
  instead of two sequential lookups
- When existing consent matches only the unstable hash, silently upgrade
  by writing the stable hash (no re-consent prompt shown)
- Newly given consent is stored with the stable hash
- Normalize NameID objects before hashing to prevent serialization errors
- Only fetch stored consent when consent is enabled (short-circuit
  optimization)

The migration strategy (from the Pivotal story):
- Existing consent hashing is left untouched
- Consent lookup checks both old and new hash: match on either means
  consent was given
- New consent is stored with the new stable hash
- Old consent that matches unstable hash is upgraded in-place

Pivotal ticket: https://www.pivotaltracker.com/story/show/176513931

Author: MKodde

config/services/controllers/api.yml

@@ -16,7 +16,7 @@ services:
             - '@security.token_storage'
             - '@security.access.decision_manager'
             - '@OpenConext\EngineBlockBundle\Configuration\FeatureConfiguration'
-            - '@OpenConext\EngineBlock\Service\ConsentService'
+            - '@OpenConext\EngineBlock\Service\Consent\ConsentService'
 
     OpenConext\EngineBlockBundle\Controller\Api\DeprovisionController:
         arguments:

config/services/services.yml

@@ -77,8 +77,10 @@ services:
     engineblock.service.consent.ConsentHashService:
         class: OpenConext\EngineBlock\Service\Consent\ConsentHashService
         public: false
+        arguments:
+            - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository'
 
-    OpenConext\EngineBlock\Service\ConsentService:
+    OpenConext\EngineBlock\Service\Consent\ConsentService:
         arguments:
             - '@OpenConext\EngineBlockBundle\Authentication\Repository\DbalConsentRepository'
             - '@OpenConext\EngineBlock\Service\MetadataService'

library/EngineBlock/Application/DiContainer.php

@@ -165,7 +165,7 @@ public function getAuthenticationLoopGuard()
      */
     public function getConsentService()
     {
-        return $this->container->get(\OpenConext\EngineBlock\Service\ConsentService::class);
+        return $this->container->get(\OpenConext\EngineBlock\Service\Consent\ConsentService::class);
     }
 
     /**

library/EngineBlock/Corto/Model/Consent.php

@@ -16,7 +16,10 @@
  * limitations under the License.
  */
 
-use Doctrine\DBAL\Statement;
+use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery;
+use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters;
+use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters;
+use OpenConext\EngineBlock\Authentication\Value\ConsentVersion;
 use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
 use OpenConext\EngineBlock\Authentication\Value\ConsentType;
 use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface;
@@ -84,14 +87,36 @@ public function __construct(
 
     public function explicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool
     {
-        return !$this->_consentEnabled ||
-            $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT);
+        if (!$this->_consentEnabled) {
+            return true;
+        }
+        $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT);
+        return $consent->given();
+    }
+
+    /**
+     * Although the user has given consent previously we want to upgrade the deprecated unstable consent
+     * to the new stable consent type.
+     * https://www.pivotaltracker.com/story/show/176513931
+     */
+    public function upgradeAttributeHashFor(ServiceProvider $serviceProvider, string $consentType): void
+    {
+        if (!$this->_consentEnabled) {
+            return;
+        }
+        $consentVersion = $this->_hasStoredConsent($serviceProvider, $consentType);
+        if ($consentVersion->isUnstable()) {
+            $this->_updateConsent($serviceProvider, $consentType);
+        }
     }
 
     public function implicitConsentWasGivenFor(ServiceProvider $serviceProvider): bool
     {
-        return !$this->_consentEnabled ||
-            $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT);
+        if (!$this->_consentEnabled) {
+            return true;
+        }
+        $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_IMPLICIT);
+        return $consent->given();
     }
 
     public function giveExplicitConsentFor(ServiceProvider $serviceProvider): bool
@@ -138,38 +163,48 @@ private function _storeConsent(ServiceProvider $serviceProvider, $consentType):
             return false;
         }
 
-        $parameters = array(
-            sha1($consentUuid),
-            $serviceProvider->entityId,
-            $this->_getStableAttributesHash($this->_responseAttributes),
-            $consentType,
+        $parameters = new ConsentStoreParameters(
+            hashedUserId: sha1($consentUuid),
+            serviceId: $serviceProvider->entityId,
+            attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes),
+            consentType: $consentType,
         );
 
         return $this->_hashService->storeConsentHash($parameters);
     }
 
-    private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): bool
+    private function _updateConsent(ServiceProvider $serviceProvider, $consentType): bool
     {
-        $parameters = array(
-            sha1($this->_getConsentUid()),
-            $serviceProvider->entityId,
-            $this->_getAttributesHash($this->_responseAttributes),
-            $consentType,
+        $consentUid = $this->_getConsentUid();
+        if (!is_string($consentUid)) {
+            return false;
+        }
+
+        $parameters = new ConsentUpdateParameters(
+            attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes),
+            attributeHash: $this->_getAttributesHash($this->_responseAttributes),
+            hashedUserId: sha1($consentUid),
+            serviceId: $serviceProvider->entityId,
+            consentType: $consentType,
         );
 
-        $hasUnstableConsentHash = $this->_hashService->retrieveConsentHash($parameters);
+        return $this->_hashService->updateConsentHash($parameters);
+    }
 
-        if ($hasUnstableConsentHash) {
-            return true;
+    private function _hasStoredConsent(ServiceProvider $serviceProvider, $consentType): ConsentVersion
+    {
+        $consentUid = $this->_getConsentUid();
+        if (!is_string($consentUid)) {
+            return ConsentVersion::notGiven();
         }
 
-        $parameters[2] = array(
-            sha1($this->_getConsentUid()),
-            $serviceProvider->entityId,
-            $this->_getStableAttributesHash($this->_responseAttributes),
-            $consentType,
+        $query = new ConsentHashQuery(
+            hashedUserId: sha1($consentUid),
+            serviceId: $serviceProvider->entityId,
+            attributeHash: $this->_getAttributesHash($this->_responseAttributes),
+            attributeStableHash: $this->_getStableAttributesHash($this->_responseAttributes),
+            consentType: $consentType,
         );
-
-        return $this->_hashService->retrieveConsentHash($parameters);
+        return $this->_hashService->retrieveConsentHash($query);
     }
 }

library/EngineBlock/Corto/Model/Consent/Factory.php

@@ -16,7 +16,7 @@
  * limitations under the License.
  */
 
-use OpenConext\EngineBlock\Service\Consent\ConsentHashService;
+use OpenConext\EngineBlock\Service\Consent\ConsentHashServiceInterface;
 
 /**
  * @todo write a test
@@ -27,7 +27,7 @@ class EngineBlock_Corto_Model_Consent_Factory
     private $_filterCommandFactory;
 
     /**
-     * @var ConsentHashService
+     * @var ConsentHashServiceInterface
      */
     private $_hashService;
 
@@ -36,7 +36,7 @@ class EngineBlock_Corto_Model_Consent_Factory
       */
     public function __construct(
         EngineBlock_Corto_Filter_Command_Factory $filterCommandFactory,
-        ConsentHashService $hashService
+        ConsentHashServiceInterface $hashService
     ) {
         $this->_filterCommandFactory = $filterCommandFactory;
         $this->_hashService = $hashService;

library/EngineBlock/Corto/Module/Service/ProcessConsent.php

@@ -16,6 +16,7 @@
  * limitations under the License.
  */
 
+use OpenConext\EngineBlock\Authentication\Value\ConsentType;
 use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface;
 use OpenConext\EngineBlock\Service\ProcessingStateHelperInterface;
 use SAML2\Constants;
@@ -101,6 +102,8 @@ public function serve($serviceName, Request $httpRequest)
 
         if (!$consentRepository->explicitConsentWasGivenFor($serviceProvider)) {
             $consentRepository->giveExplicitConsentFor($destinationMetadata);
+        } else {
+            $consentRepository->upgradeAttributeHashFor($destinationMetadata, ConsentType::TYPE_EXPLICIT);
         }
 
         $response->setConsent(Constants::CONSENT_OBTAINED);

library/EngineBlock/Corto/Module/Service/ProvideConsent.php

@@ -16,6 +16,7 @@
  * limitations under the License.
  */
 
+use OpenConext\EngineBlock\Authentication\Value\ConsentType;
 use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider;
 use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
 use OpenConext\EngineBlock\Service\AuthenticationStateHelperInterface;
@@ -145,6 +146,8 @@ public function serve($serviceName, Request $httpRequest)
         if ($this->isConsentDisabled($spMetadataChain, $identityProvider)) {
             if (!$consentRepository->implicitConsentWasGivenFor($serviceProviderMetadata)) {
                 $consentRepository->giveImplicitConsentFor($serviceProviderMetadata);
+            } else {
+                $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_IMPLICIT);
             }
 
             $response->setConsent(Constants::CONSENT_INAPPLICABLE);
@@ -163,6 +166,8 @@ public function serve($serviceName, Request $httpRequest)
 
         $priorConsent = $consentRepository->explicitConsentWasGivenFor($serviceProviderMetadata);
         if ($priorConsent) {
+            $consentRepository->upgradeAttributeHashFor($serviceProviderMetadata, ConsentType::TYPE_EXPLICIT);
+
             $response->setConsent(Constants::CONSENT_PRIOR);
 
             $response->setDestination($response->getReturn());

migrations/DoctrineMigrations/Version20220503092351.php

@@ -0,0 +1,35 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenConext\EngineBlock\Doctrine\Migrations;
+
+use Doctrine\DBAL\Schema\Schema;
+
+/**
+ * Change to the consent schema
+ * 1. Added the `attribute_stable` column, string(80), nullable
+ * 2. Changed the `attribute` column, has been made nullable
+ */
+final class Version20260315000001 extends AbstractEngineBlockMigration
+{
+    public function getDescription(): string
+    {
+        return 'Add attribute_stable column to consent table and make attribute nullable';
+    }
+
+    public function preUp(Schema $schema): void
+    {
+        parent::preUp($schema);
+    }
+
+    public function up(Schema $schema): void
+    {
+        $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) DEFAULT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL');
+    }
+
+    public function down(Schema $schema): void
+    {
+        $this->addSql('ALTER TABLE consent DROP attribute_stable, CHANGE attribute attribute VARCHAR(80) CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`');
+    }
+}

src/OpenConext/EngineBlock/Authentication/Repository/ConsentRepository.php

@@ -19,6 +19,10 @@
 namespace OpenConext\EngineBlock\Authentication\Repository;
 
 use OpenConext\EngineBlock\Authentication\Model\Consent;
+use OpenConext\EngineBlock\Authentication\Value\ConsentHashQuery;
+use OpenConext\EngineBlock\Authentication\Value\ConsentStoreParameters;
+use OpenConext\EngineBlock\Authentication\Value\ConsentUpdateParameters;
+use OpenConext\EngineBlock\Authentication\Value\ConsentVersion;
 
 interface ConsentRepository
 {
@@ -38,9 +42,21 @@ public function deleteAllFor($userId);
 
     public function deleteOneFor(string $userId, string $serviceProviderEntityId): bool;
 
-    public function hasConsentHash(array $parameters): bool;
+    /**
+     * Test if the consent row is set with an attribute hash either stable or unstable
+     */
+    public function hasConsentHash(ConsentHashQuery $query): ConsentVersion;
 
-    public function storeConsentHash(array $parameters): bool;
+    /**
+     * By default stores the stable consent hash. The legacy consent hash is left.
+     */
+    public function storeConsentHash(ConsentStoreParameters $parameters): bool;
+
+    /**
+     * When a deprecated unstable consent hash is encoutered, we upgrade it to the new format using this
+     * update consent hash method.
+     */
+    public function updateConsentHash(ConsentUpdateParameters $parameters): bool;
 
     public function countTotalConsent($consentUid): int;
 }

src/OpenConext/EngineBlock/Authentication/Value/ConsentHashQuery.php

@@ -0,0 +1,31 @@
+<?php
+
+/**
+ * Copyright 2010 SURFnet B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace OpenConext\EngineBlock\Authentication\Value;
+
+final class ConsentHashQuery
+{
+    public function __construct(
+        public readonly string $hashedUserId,
+        public readonly string $serviceId,
+        public readonly string $attributeHash,
+        public readonly string $attributeStableHash,
+        public readonly string $consentType,
+    ) {
+    }
+}