Add a stable consent hash #1137

Badlapje · 2021-05-19T15:51:05Z

Prior to this commit, there was no stable hashing algorithm used for consent attributes. Casing, order of attributes, ... could all change the hash used to store consent attributes. As such, it couldn't be relied upon.

This commit adds a stable hashing algorithm. It stores all new consent attributes with the new hash, but when retrieving does so checking first the old hash and then the new one.

Edit 4-5-2022 MKodde
After @Badlapje s work was finished on the project, I inherited his work and have finished the work remaining on this ticket.

The consent stabilization feature was implemented in the consent workflows of EB
A database migration was added to allow storing both the unstable and the stable hash.
Tests where added and updated

The QA tests fail on a security check on the JS dependencies. This will be fixed on another PR.

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

MKodde

A first review looks very promising! See some improvement suggestions below.

library/EngineBlock/Corto/Model/Consent.php

src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php

tests/unit/OpenConext/EngineBlock/Service/Consent/ConsentHashServiceTest.php

src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php

thijskh · 2022-05-04T13:36:06Z

library/EngineBlock/Corto/Model/Consent.php

-        return !$this->_consentEnabled ||
-            $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT);
+        $consent = $this->_hasStoredConsent($serviceProvider, ConsentType::TYPE_EXPLICIT);
+        return !$this->_consentEnabled || $consent->given();


This means now queries (and hash calculations) will be done even if the consent feature toggle is off. This also goes for the other public methods that use this construct.

A very good point! Will change this to only search for previous consent when consent is enabled.

thijskh · 2022-05-04T13:47:18Z

src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php

+    public function storeConsentHash(array $parameters): bool
+    {
+        $query = "INSERT INTO consent (hashed_user_id, service_id, attribute_stable, consent_type, consent_date, deleted_at)
+                  VALUES (?, ?, ?, ?, NOW(), '0000-00-00 00:00:00')


So it's stored as being deleted? Because a non-null value is being set for deleted_at?

That has to do with the following.

The deleted_at column became part of the primary key, because we can now have multiple rows for a given user for a given service. Where one of those rows is an active consent row, and the rest are soft deleted versions of previously given consent.

Having a dattime as part of a key requires it to be not nullable. But we do not want the column to be empty. Thats why a null-datetime is set for those values. DBAL will translate this to 'null'. But for the DBMS is satisfied. At least that's my understanding.

thijskh · 2022-05-04T13:53:44Z

library/EngineBlock/Corto/Model/Consent.php

+            $consentType,
+        ];
+
+        $hasUnstableConsentHash = $this->_hashService->retrieveConsentHash($parameters);


I am wondering about the approach. This currently queries first for the new hash. If not found then for the old hash. And if also not found will present consent screen.

Why is this done over the following approach

oldHash = _getAttributesHash()

newHash = _getStableAttributesHash()

SELECT * FROM consent where attributes_stable = :newHash OR attributes = :oldHash
so you always know in one query whether consent was given?

Also, a good point! Having this in one query is much better. Will update this.

thijskh · 2022-05-04T13:59:59Z

database/DoctrineMigrations/Version20220503092351.php

+        // this up() migration is auto-generated, please modify it to your needs
+        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');
+
+        $this->addSql('ALTER TABLE consent ADD attribute_stable VARCHAR(80) NOT NULL, CHANGE attribute attribute VARCHAR(80) DEFAULT NULL');


I'm not yet decided on the pros and cons of adding a new hash field next to the old field.

I believe the problem is solvable with just one database field. Not sure yet if that is preferable though.

I'm open for other suggestions! Having it in one single column will work too in my opinion. The 'OR' based query solution would work just as well on a single column. But would make it harder to decide when we can stop supporting the old style attribute hash method.

Prior to this commit, there was no service specifically for hashing attribute arrays for consent. This commit adds such a service with both the old hashing algorithm and a stable hashing algorithm. Pivotal ticket: https://www.pivotaltracker.com/story/show/176513931

The consent queries added to the ConsentRepository.php file (deleted) should have been added to the existing DbalConsentRepository. While at it, naming conventions have been applied to the query names. And the service config was updated.

Extracting this interface from the existing service is allowing us to more easily test the service. As mocking a final class is not possible.

This might prove usefull down the road

This represents the consent type at hand (implicit or explicit consent). A third option is that no consent has been given.

The requirements are stated in the story: https://www.pivotaltracker.com/story/show/176513931 and specifically these points: A challenge is that we do not want to invalidate all given consents with the current algorithm. So we implement it as follows: * We do not touch the existing consent hashing method at all * We create a new hashing method that is more stable. * We cover this new method with an abundance of unit tests to verify the stability given all sorts of inputs. * We change the consent query from (pseudocode): SELECT * FROM consent WHERE user = me AND consenthash = hashfromoldmethod OR consenthash = hashfromnewmethod * Newly given consent will be stored with the new hash. * When old consent matched, still generate new consent hash (without showing consent screen

The NameID objects where causing issues when creating a stabilized consent hash. The objects can not be serialized/unserialized. By normalizing them before starting to perform other normalization tasks on the attribute array, this issue can be prevented.

Only get the stored consent when consent is enabled. The other methods need no adjusting as the short-circuiting by the first condition prevents those expressions from being executed. Consider: `(true || $this->expensiveMethodCall())` The expensiveMethodCall is never called as the first condition already decided the rest of the condition. https://www.php.net/manual/en/language.operators.logical.php

Previously when testing if previous consent was given was based on testing for old-style attribute hash calculation. And if that one did not exist, the new (stable) attribute hash was tested. Having that in two queries made little sense as it can easily be performed in one action. The interface of the repo and service changed slightly, as now a value object is returned instead of a boolean value. The value object reflects the version (type of attribute hash) of consent that was given. That can either be: unstable, stable or not given at all.

Badlapje added enhancement consent 6.7 labels May 19, 2021

Badlapje requested a review from MKodde May 19, 2021 15:51

Badlapje changed the title ~~Add a consentHashService~~ Add a stable consent hash May 19, 2021

MKodde requested changes May 20, 2021

View reviewed changes

Badlapje force-pushed the feature/stabilize-consent-hash branch 6 times, most recently from 4e3a85e to dff4b53 Compare May 20, 2021 16:46

Badlapje requested a review from MKodde June 1, 2021 17:07

Badlapje force-pushed the feature/stabilize-consent-hash branch from dff4b53 to 595c767 Compare June 2, 2021 12:38

BackEndTea reviewed Jun 7, 2021

View reviewed changes

src/OpenConext/EngineBlock/Service/Consent/ConsentHashService.php Show resolved Hide resolved

MKodde force-pushed the feature/stabilize-consent-hash branch 8 times, most recently from c716bf8 to a5dd163 Compare May 4, 2022 10:09

MKodde requested review from thijskh and VadimSchmitz May 4, 2022 10:28

thijskh reviewed May 4, 2022

View reviewed changes

Koen Cornelis and others added 4 commits May 5, 2022 13:24

Move DB logic to ConsentHashRepository

e4a0b2f

Move consent queries to existing repository

b0a67c2

The consent queries added to the ConsentRepository.php file (deleted) should have been added to the existing DbalConsentRepository. While at it, naming conventions have been applied to the query names. And the service config was updated.

Extract ConsentHashServiceInterface

499ac7e

Extracting this interface from the existing service is allowing us to more easily test the service. As mocking a final class is not possible.

MKodde added 8 commits May 5, 2022 13:24

Integrate upstream consent changes

6a5bcd3

Add comment to getUnstableAttributesHash

109e068

This might prove usefull down the road

Add the attribute_stable column to consent

745243c

Add ConsentVersion value object

d796dfd

This represents the consent type at hand (implicit or explicit consent). A third option is that no consent has been given.

MKodde force-pushed the feature/stabilize-consent-hash branch from 26e8ac1 to 145ebd9 Compare May 5, 2022 11:24

MKodde added 6.8 and removed 6.7 labels May 5, 2022

thijskh removed the 6.8 label Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a stable consent hash #1137

Add a stable consent hash #1137

Badlapje commented May 19, 2021 •

edited by MKodde

Loading

MKodde left a comment

thijskh May 4, 2022

MKodde May 5, 2022

thijskh May 4, 2022

MKodde May 5, 2022

thijskh May 4, 2022

MKodde May 5, 2022

thijskh May 4, 2022

MKodde May 5, 2022 •

edited

Loading

Add a stable consent hash #1137

Are you sure you want to change the base?

Add a stable consent hash #1137

Conversation

Badlapje commented May 19, 2021 • edited by MKodde Loading

MKodde left a comment

Choose a reason for hiding this comment

thijskh May 4, 2022

Choose a reason for hiding this comment

MKodde May 5, 2022

Choose a reason for hiding this comment

thijskh May 4, 2022

Choose a reason for hiding this comment

MKodde May 5, 2022

Choose a reason for hiding this comment

thijskh May 4, 2022

Choose a reason for hiding this comment

MKodde May 5, 2022

Choose a reason for hiding this comment

thijskh May 4, 2022

Choose a reason for hiding this comment

MKodde May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Badlapje commented May 19, 2021 •

edited by MKodde

Loading

MKodde May 5, 2022 •

edited

Loading