Psre 2138/feature/pre hash string utils PSRE-2229 #70

shtrom · 2024-02-27T07:06:23Z

This PR:

introduces a separate PreHashString service concept, and moves the current implementation into a LegacyPrehashStringService
removes the ability to inject signature factories into the Init object, as using the latest version is a deliberate choice
reintroduces v1 signature tests
moves the tests params to a separate fixture provider
ensures that things we want to test for backward compatibility are hardcoded strings, rather than generated on the fly
refreshes the phpunit config
adds linting tools
lints the lot for good measure
add a github workflow to run those tests on PR

It may help to review this PR commit by commit, as they build on top of each other.

Checklist

src/Services/PreHashStrings/LegacyPreHashString.php

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] ParamsFixture/items: add user_id in security [TEST] ParamsFixture lexical ordering of method per API name [TEST] ParamsFixture: move data timestamp to fixture [TEST] ParamsFixture: reuse apiQuestionActivity in Assess init options [TEST] Explicitely disable telemetry in constructor tests Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

Only leave hardcoded signatures in test for bespoke modifications. Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

…ward compatibility Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

…d current schemes Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

klauste · 2024-04-03T04:53:58Z

src/Services/Signatures/HashSignature.php

@@ -6,17 +6,17 @@

 class HashSignature implements SignatureInterface
 {
-    const ALGORITHM = 'sha256';
+    public const ALGORITHM = 'sha256';


Are these really public? I understand that the default of these is public, but I think these should only be used in this class, so should be private

klauste · 2024-04-03T04:54:13Z

src/Services/Signatures/HmacSignature.php

@@ -8,17 +8,17 @@

 class HmacSignature implements SignatureInterface
 {
-    const ALGORITHM = 'sha256';
+    public const ALGORITHM = 'sha256';


Visibility should be private I'd say

klauste · 2024-04-03T04:55:24Z

src/Services/PreHashStrings/LegacyPreHashString.php

+
+use LearnositySdk\Exceptions\ValidationException;
+
+class LegacyPreHashString implements PreHashStringInterface


I assume there will be another class implementing this in the future, otherwise the interface makes no sense

Yep, the idea is to make each signature pattern independent, rather than the current pattern we have which is really hard to audit because it jumps around and has too many ifs.

The LegacyPreHashString implementation is different from what we had in the SDK before, as it takes the approach of the implementation we have in LP instead, which is a lot simpler to follow.

klauste · 2024-04-03T04:58:41Z

src/Services/PreHashStringFactory.php

+use LearnositySdk\Services\PreHashStrings\PreHashStringInterface;
+use LearnositySdk\Services\PreHashStrings\LegacyPreHashString;
+
+class PreHashStringFactory


I'm not sure if PreHashString is the best name for these classes. Maybe PlainStringManipulator ? Not sure if that's better though

It's not a great name, but that's the one that has been used historically, so I went with that.

https://github.com/Learnosity/learnosity-sdk-php/blob/master/src/Request/Init.php#L297-L315

It's a very specific Service that should only do PHS generation.

Prior to the signature upgrade in most cases the pre hash string variable was called $signature in many places which was so confusing. Changing it to prehash string was a revelation in terms of readability but I now realise that probably only was the case for myself and others who had struggled to understand the code when it was called $signature.

Has a look each implementation of of hash hmac in each of the SDK languages we support just for naming convention ideas and it seems most languages refer to the data to be hashed as message or simply data which in both cases is probably too ambiguous.

markkellyeire · 2024-04-03T08:47:20Z

src/Request/Init.php

+            $this->requestPacket,
+            $this->action
+        );
+        /* $preHashString  = $this->generatePreHashString(); */


@shtrom Looks like this commented out line can go.

markkellyeire · 2024-04-05T12:09:26Z

src/Services/PreHashStringFactory.php

+use LearnositySdk\Services\PreHashStrings\PreHashStringInterface;
+use LearnositySdk\Services\PreHashStrings\LegacyPreHashString;
+
+class PreHashStringFactory


Prior to the signature upgrade in most cases the pre hash string variable was called $signature in many places which was so confusing. Changing it to prehash string was a revelation in terms of readability but I now realise that probably only was the case for myself and others who had struggled to understand the code when it was called $signature.

Has a look each implementation of of hash hmac in each of the SDK languages we support just for naming convention ideas and it seems most languages refer to the data to be hashed as message or simply data which in both cases is probably too ambiguous.

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

Comments addressed.

shtrom marked this pull request as draft February 27, 2024 07:06

shtrom force-pushed the PSRE-2138/feature/pre-hash-string-utils branch 2 times, most recently from 1a9608b to df747fb Compare March 5, 2024 07:08

shtrom force-pushed the PSRE-2138/feature/pre-hash-string-utils branch 8 times, most recently from c252e2a to 0e9329b Compare March 13, 2024 06:57

shtrom requested a review from markkellyeire March 13, 2024 06:58

shtrom marked this pull request as ready for review March 13, 2024 07:01

shtrom force-pushed the PSRE-2138/feature/pre-hash-string-utils branch 8 times, most recently from 4937087 to 76aa9ba Compare March 18, 2024 05:09

shtrom requested a review from markus-liang-lrn March 19, 2024 03:54

markus-liang-lrn reviewed Mar 19, 2024

View reviewed changes

src/Services/PreHashStrings/LegacyPreHashString.php Outdated Show resolved Hide resolved

david-scarratt-lrn and others added 7 commits April 3, 2024 10:21

[FEATURE] Containerise dev env LRN-34270

f52a0a1

[TEST] Allow passing test arguments via ARGS_PHPUNIT

970aee5

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] phpunit --migrate-configuration

af5a1d1

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[VENDOR] require --dev squizlabs/php_codesniffer=*

2816b5d

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[LINT] Add phpcs config

b79be8f

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[BUILD] Add lint target

a9cfd06

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[LINT] Run phpcbf on codebase

5a8465e

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

shtrom added 6 commits April 3, 2024 10:21

[LINT] Apply manual fixes

e4e8ebc

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] Move instanciation of Init to test rather than data providers

b2c94b7

[TEST] Move getWorking*Params to separate ParamsFixture

a07f237

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] ParamsFixture: provide expected signature for params from Fixture

c4daed9

Only leave hardcoded signatures in test for bespoke modifications. Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] InitTest: Use valid action for dataApiPost -> Set

26fdfa8

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

shtrom force-pushed the PSRE-2138/feature/pre-hash-string-utils branch 2 times, most recently from d20163e to a1ee325 Compare April 2, 2024 23:41

shtrom added 6 commits April 3, 2024 10:59

[FEATURE] Introduce PreHashString services

7c02fc5

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[REFACTOR] Init: Use PreHashString services

fdeab94

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] ParamsFixture: reintroduce v1 signatures to actively test back…

ffed4f2

…ward compatibility Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] Signing services: test both v1 and v2 signatures

cc60b06

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] LegacyPreHashStringTest: hardcode expected PHS for v1Compat an…

ea380b8

…d current schemes Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] ParamsFixture: add service-agnostic entrypoint methods

3b00efc

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

shtrom force-pushed the PSRE-2138/feature/pre-hash-string-utils branch from a1ee325 to 595b8b1 Compare April 2, 2024 23:59

klauste reviewed Apr 3, 2024

View reviewed changes

shtrom requested a review from klauste April 5, 2024 03:00

shtrom changed the title ~~Psre 2138/feature/pre hash string utils~~ Psre 2138/feature/pre hash string utils PSRE-2229 Apr 5, 2024

klauste approved these changes Apr 5, 2024

View reviewed changes

markkellyeire previously requested changes Apr 5, 2024

View reviewed changes

shtrom requested a review from markkellyeire April 8, 2024 07:20

shtrom added 4 commits April 8, 2024 17:21

[REFACTOR] Remove legacy PHS+Signing code

cdcf4c8

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[TEST] Add github workflow

3f22736

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[BUG] Request can be empty (or null)

bcc5b5c

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

[REFACTOR] Signatures: reduce visibility of internal consts

49c0fc7

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>

shtrom force-pushed the PSRE-2138/feature/pre-hash-string-utils branch from 40d291e to 49c0fc7 Compare April 8, 2024 07:21

shtrom merged commit 2c05853 into master Apr 10, 2024
2 checks passed

shtrom deleted the PSRE-2138/feature/pre-hash-string-utils branch April 10, 2024 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Psre 2138/feature/pre hash string utils PSRE-2229 #70

Psre 2138/feature/pre hash string utils PSRE-2229 #70

shtrom commented Feb 27, 2024 •

edited

Loading

klauste Apr 3, 2024

klauste Apr 3, 2024

klauste Apr 3, 2024

shtrom Apr 5, 2024 •

edited

Loading

klauste Apr 3, 2024

shtrom Apr 5, 2024

markkellyeire Apr 5, 2024

markkellyeire Apr 3, 2024

markkellyeire Apr 5, 2024


		use LearnositySdk\Exceptions\ValidationException;

		class LegacyPreHashString implements PreHashStringInterface

Psre 2138/feature/pre hash string utils PSRE-2229 #70

Psre 2138/feature/pre hash string utils PSRE-2229 #70

Conversation

shtrom commented Feb 27, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shtrom Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shtrom commented Feb 27, 2024 •

edited

Loading

shtrom Apr 5, 2024 •

edited

Loading