[1.x] use `UriSigner::checkRequest()` to validate signatures using a `Request` object #157

jrushlow · 2024-02-27T18:55:12Z

add's a new validateEmailConfirmationFromRequest() to VerifyEmailHelperInterface & it's implementation.
deprecates validateEmailConfirmation() in VerifyEmailHelperInterface & it's implementation.
~~deprecates VerifyEmailQueryUtility as this is no longer needed in the new "fromRequest" helper method.~~
does not deprecates the 3rd $queryUtility argument in the VerifyEmailHelper::__construct() method as this argument is not nullable`
deprecates passing anything other than an instance of UriSigner as the $uriSigner argument in the VerifyEmailHelper::__construct() method.
silences self deprecations (introduced by this PR) in tests using @group legacy annotations. This is basically our entire test suite. Work is already started in WIP ! 2.x ! - remove deprecations from 157 #159 that will remove these annotations and deprecations in 2.0.

Doc's will be updated in #143 after maker-bundle #1464 is updated / merged to reflect the changes in this PR.

fixes #155

jrushlow · 2024-03-06T12:28:33Z

src/VerifyEmailHelper.php

@@ -63,21 +64,60 @@ public function generateSignature(string $routeName, string $userId, string $use
        return new VerifyEmailSignatureComponents(\DateTimeImmutable::createFromFormat('U', (string) $expiryTimestamp), $signature, $generatedAt);
    }

-    public function validateEmailConfirmation(string $signedUrl, string $userId, string $userEmail): void
+    public function validateEmailConfirmation(string $signedUrl, string $userId, string $userEmail, ?Request $request = null): void


I think we'll be better off leaving this method alone and adding a deprecation @trigger_deprecation('Use validateEmailConfirmationFromRequest instead...)

theres alot of if/else action happening in here

we don't have a clean path forward to reorder the arguments where Request is the 1st argument.

I think so too. Leave this method alone, but deprecate it.

src/VerifyEmailHelper.php

weaverryan

Thanks for getting this going!

weaverryan · 2024-03-06T14:19:57Z

src/VerifyEmailHelper.php

@@ -63,21 +64,60 @@ public function generateSignature(string $routeName, string $userId, string $use
        return new VerifyEmailSignatureComponents(\DateTimeImmutable::createFromFormat('U', (string) $expiryTimestamp), $signature, $generatedAt);
    }

-    public function validateEmailConfirmation(string $signedUrl, string $userId, string $userEmail): void
+    public function validateEmailConfirmation(string $signedUrl, string $userId, string $userEmail, ?Request $request = null): void


I think so too. Leave this method alone, but deprecate it.

src/VerifyEmailHelper.php

jrushlow · 2024-03-06T15:01:59Z

src/VerifyEmailHelperInterface.php

@@ -33,6 +36,8 @@ interface VerifyEmailHelperInterface
    public function generateSignature(string $routeName, string $userId, string $userEmail, array $extraParams = []): VerifyEmailSignatureComponents;

    /**
+     * @deprecated since v1.17.0, use validateEmailConfirmationFromRequest instead.


Is this even the right way to deprecate an interface method?

To the best of my knowledge, yes - but not sure.

jrushlow · 2024-03-07T13:18:12Z

src/VerifyEmailHelper.php

+        if (!$uriSigner instanceof UriSigner) {
+            /** @psalm-suppress UndefinedFunction */
+            @trigger_deprecation('symfonycasts/verify-email-bundle', '1.17.0', 'Not providing an instance of %s is deprecated. It will be required in v2.0', UriSigner::class);
+        }


We should also deprecated "passing $queryUtility as 3rd argument is deprecated. The arg will be removed in the future..." but usually when we do this - the param is = nullable. We can't do that here because the last 2 arg's are not nullable.

Do we let this ride as is and document this "non-deprecated" change in Upgrade.md or add the deprecation anyway without providing a way for the user to "silence" it?

Hmm, maybe we lave the argument as is & not deprecated for now. Then we deprecate it in v2, once it's fully unused. There may be a better way to handle this, but I can't see it at the moment.

weaverryan · 2024-03-07T15:12:53Z

src/Util/VerifyEmailQueryUtility.php

@@ -23,13 +25,19 @@ class VerifyEmailQueryUtility
 {
    public function getTokenFromQuery(string $uri): string
    {
+        /** @psalm-suppress UndefinedFunction */
+        @trigger_deprecation('symfonycasts/verify-email-bundle', '1.17.0', 'This method is deprecated and will be removed in 2.0.');


Curious that this needs the pslam-suppress... since the library does require the deprecations-contract

Ya, I don't get it either - locally I don't get the error. But on CI, we do. I think it has something to do with psalm not picking up the autoload.

I'll check into this outside of this PR.

weaverryan · 2024-03-07T15:17:40Z

src/VerifyEmailHelper.php

+        if (!$uriSigner instanceof UriSigner) {
+            /** @psalm-suppress UndefinedFunction */
+            @trigger_deprecation('symfonycasts/verify-email-bundle', '1.17.0', 'Not providing an instance of %s is deprecated. It will be required in v2.0', UriSigner::class);
+        }


Hmm, maybe we lave the argument as is & not deprecated for now. Then we deprecate it in v2, once it's fully unused. There may be a better way to handle this, but I can't see it at the moment.

weaverryan · 2024-03-07T15:27:52Z

src/VerifyEmailHelperInterface.php

@@ -33,6 +36,8 @@ interface VerifyEmailHelperInterface
    public function generateSignature(string $routeName, string $userId, string $userEmail, array $extraParams = []): VerifyEmailSignatureComponents;

    /**
+     * @deprecated since v1.17.0, use validateEmailConfirmationFromRequest instead.


To the best of my knowledge, yes - but not sure.

src/Util/VerifyEmailQueryUtility.php

This reverts commit 82cd0e1.

validate using checkRequest()

1753bef

jrushlow added Status: Needs Work Additional work is needed Feature New Feature labels Feb 27, 2024

jrushlow mentioned this pull request Feb 27, 2024

[make:registration-form] use validateEmailConfirmationFromRequest() when verifying emails symfony/maker-bundle#1464

Merged

2 tasks

different path

6a7a324

jrushlow commented Mar 6, 2024

View reviewed changes

src/VerifyEmailHelper.php Show resolved Hide resolved

weaverryan reviewed Mar 6, 2024

View reviewed changes

create new verify w/ request method

f15d4b8

jrushlow commented Mar 6, 2024

View reviewed changes

deprecate query utility

d58e241

jrushlow mentioned this pull request Mar 6, 2024

Updating docs for latest generated code from MakerBundle #143

Merged

jrushlow added 2 commits March 6, 2024 10:33

psalm doesnt know about trigger_deprecation

1f467a0

skip test if using < http-foundation 6.4

331c5e0

jrushlow mentioned this pull request Mar 6, 2024

[2.x] version bumps #160

Merged

1 task

deprecate legacy urisigner

fb37569

jrushlow changed the title ~~WIP _ validate using checkRequest()~~ [1.x] use UriSigner::checkRequest() to validate signatures using a Request object Mar 7, 2024

skip check request on <6.4

d822623

jrushlow added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Mar 7, 2024

fix 8.1 tests

e52e2af

jrushlow marked this pull request as ready for review March 7, 2024 13:14

jrushlow commented Mar 7, 2024

View reviewed changes

weaverryan requested changes Mar 7, 2024

View reviewed changes

jrushlow added 3 commits March 12, 2024 10:08

remove query util deprecation

395b47d

remove undefined function for psalm

82cd0e1

Revert "remove undefined function for psalm"

8b6f2d7

This reverts commit 82cd0e1.

jrushlow added the Status: Reviewed Has been reviewed by a maintainer label Mar 12, 2024

jrushlow merged commit d60e9e8 into SymfonyCasts:main Mar 12, 2024
8 checks passed

jrushlow deleted the feature/uri-signer branch March 12, 2024 14:25

jrushlow mentioned this pull request Mar 16, 2024

[release] v1.17.0 #169

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.x] use `UriSigner::checkRequest()` to validate signatures using a `Request` object #157

[1.x] use `UriSigner::checkRequest()` to validate signatures using a `Request` object #157

jrushlow commented Feb 27, 2024 •

edited

Loading

jrushlow Mar 6, 2024

weaverryan Mar 6, 2024

weaverryan left a comment

weaverryan Mar 6, 2024

jrushlow Mar 6, 2024

weaverryan Mar 7, 2024

jrushlow Mar 7, 2024

weaverryan Mar 7, 2024

weaverryan Mar 7, 2024

jrushlow Mar 12, 2024

jrushlow Mar 12, 2024

weaverryan Mar 7, 2024

weaverryan Mar 7, 2024

[1.x] use UriSigner::checkRequest() to validate signatures using a Request object #157

[1.x] use UriSigner::checkRequest() to validate signatures using a Request object #157

Conversation

jrushlow commented Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a 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

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

[1.x] use `UriSigner::checkRequest()` to validate signatures using a `Request` object #157

[1.x] use `UriSigner::checkRequest()` to validate signatures using a `Request` object #157

jrushlow commented Feb 27, 2024 •

edited

Loading