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

[API] reseting password with validation #12391

Merged
merged 10 commits into from
Mar 4, 2021

Conversation

arti0090
Copy link
Contributor

@arti0090 arti0090 commented Mar 1, 2021

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

@arti0090 arti0090 requested a review from a team as a code owner March 1, 2021 09:23
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Mar 1, 2021
@arti0090 arti0090 changed the title [WIP][API] reseting password with validation [API] reseting password with validation Mar 2, 2021
@arti0090
Copy link
Contributor Author

arti0090 commented Mar 2, 2021

I have intentionally left comments with some changes (that tested on apiplatform v2.6) that will work when endpoint will be PATCH /request-reset-password/{token} At the moment it does not work. We first need support for that version. There should be also no issue with removing these comments as my last commit is pushed 2149e87

@arti0090 arti0090 force-pushed the api-resetting-password-part-2 branch 2 times, most recently from bb0a61d to 50384fb Compare March 3, 2021 09:56
@arti0090 arti0090 force-pushed the api-resetting-password-part-2 branch from 50384fb to c0a15e0 Compare March 3, 2021 10:00
Co-authored-by: Adam Kasperczak <adamkasperczak123@gmail.com>
use Webmozart\Assert\Assert;

/** @experimental */
final class ConfirmResetPasswordValidator extends ConstraintValidator
Copy link
Member

Choose a reason for hiding this comment

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

Missing spec for this class

$userRepository->findOneBy(['passwordResetToken' => 'TOKEN'])->willReturn($shopUser);
$metadata->getParameter('resetting')->willReturn(['token' => ['ttl' => 'P5D']]);

$shopUser->isPasswordRequestNonExpired(Argument::that(function(\DateInterval $dateInterval ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$shopUser->isPasswordRequestNonExpired(Argument::that(function(\DateInterval $dateInterval ) {
$shopUser->isPasswordRequestNonExpired(Argument::that(function(\DateInterval $dateInterval) {

Same below

/**
* @When /^I follow link on (my) email to reset my password$/
*/
public function iFollowLinkOnMyEmailToResetPassword(UserInterface $user): void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function iFollowLinkOnMyEmailToResetPassword(UserInterface $user): void
public function iFollowLinkOnMyEmailToResetPassword(ShopUserInterface $user): void

?

Comment on lines +33 to +41
public function getResetPasswordToken(): string
{
return $this->resetPasswordToken;
}

public function setResetPasswordToken(string $token): void
{
$this->resetPasswordToken = $token;
}
Copy link
Member

Choose a reason for hiding this comment

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

Are these methods needed as $resetPasswordToken has public access?

}

if ($command->getResetPasswordToken() !== $user->getPasswordResetToken()) {
throw new \InvalidArgumentException('Password reset token do not match.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new \InvalidArgumentException('Password reset token do not match.');
throw new \InvalidArgumentException('Password reset token does not match.');

throw new \InvalidArgumentException('Password reset token do not match.');
}

$user->setPlainPassword($command->newPassword);
Copy link
Member

Choose a reason for hiding this comment

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

Do you check somewhere the new password with the password confirmation?

<attribute name="messenger">input</attribute>

<collectionOperations>
<collectionOperation name="shop_password_reset_request">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<collectionOperation name="shop_password_reset_request">
<collectionOperation name="shop_request_reset_password">

</collectionOperations>

<itemOperations>
<itemOperation name="shop_password_reset">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<itemOperation name="shop_password_reset">
<itemOperation name="shop_reset_password">

<attribute name="groups">shop:reset_password:update</attribute>
</attribute>
<attribute name="openapi_context">
<attribute name="summary">Password reset</attribute>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<attribute name="summary">Password reset</attribute>
<attribute name="summary">Resets password</attribute>

to be consistent with other endpoints

<attribute name="groups">shop:reset_password:create</attribute>
</attribute>
<attribute name="openapi_context">
<attribute name="summary">Request password reset</attribute>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<attribute name="summary">Request password reset</attribute>
<attribute name="summary">Requests password reset</attribute>

@GSadee GSadee merged commit 4cfede9 into Sylius:master Mar 4, 2021
@GSadee
Copy link
Member

GSadee commented Mar 4, 2021

Thank you, @arti0090! 🎉

GSadee added a commit that referenced this pull request Mar 4, 2021
…ord PR (arti0090)

This PR was merged into the 1.10-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| License         | MIT

Continuation of #12391

Commits
-------

08adcf3 add missing specs and make fixes to reset password PR
GSadee added a commit that referenced this pull request Mar 5, 2021
This PR was merged into the 1.10-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | 
| License         | MIT

Continuation of #12391

<!--
 - Bug fixes must be submitted against the 1.7 or 1.8 branch (the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

2fdd995 [API] Adjust reset password requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants