Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initiate cancel reset password step #22810

Merged
merged 9 commits into from
Dec 4, 2024

Conversation

caddoo
Copy link
Contributor

@caddoo caddoo commented Nov 29, 2024

Description:

A problem was discovered with the cancel reset password flow where email link scanners would unintentionally cancel the reset password flow during their scan.

This adds a new screen in between that action to prevent that from happening.

image
image

Review

@caddoo caddoo marked this pull request as ready for review December 2, 2024 05:20
@caddoo caddoo changed the title Dev 18691 interstitial password reset cancel Add initiate cancel reset password step Dec 2, 2024
@caddoo caddoo requested a review from a team December 2, 2024 05:21
@caddoo caddoo added the Needs Review PRs that need a code review label Dec 2, 2024
$errorMessage = $ex->getMessage();
}

$nonce = Nonce::getNonce(self::NONCE_CONFIRMCANCELRESETPASSWORD);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't really know if the nonce is necessary but it doesn't hurt

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two comments for improvements. Besides that it works as expected when tested locally.

plugins/Login/lang/en.json Outdated Show resolved Hide resolved
Comment on lines 489 to 492
$isNonceValid = Nonce::verifyNonce(self::NONCE_CONFIRMCANCELRESETPASSWORD, $request->getStringParameter('nonce'));
if ($isNonceValid === false) {
throw new Exception('Cannot verify request');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use Nonce::checkNonce instead. That should directly throw a form validation exception if the nonce does not match and will also discard the nonce.

Also you need to do the nonce check before the actually action is performed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've fixed this.

caddoo and others added 2 commits December 3, 2024 15:44
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@caddoo caddoo requested a review from sgiehl December 3, 2024 02:52
@sgiehl sgiehl added this to the 5.2.0 milestone Dec 3, 2024
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@caddoo caddoo merged commit 3fcea16 into 5.x-dev Dec 4, 2024
24 of 26 checks passed
@caddoo caddoo deleted the dev-18691-interstitial-password-reset-cancel branch December 4, 2024 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants