Skip to content

Feat/ssad 0122/ ability to change user password - #123

Open
RomanKholod wants to merge 4 commits into
devfrom
122-enhancement-create-an-ability-to-change-user-password
Open

Feat/ssad 0122/ ability to change user password#123
RomanKholod wants to merge 4 commits into
devfrom
122-enhancement-create-an-ability-to-change-user-password

Conversation

@RomanKholod

Copy link
Copy Markdown
Contributor

dev

JIRA

Summary of change

Added an ability to change user password

@RomanKholod

Copy link
Copy Markdown
Contributor Author

Only fails Test Email API

@RomanKholod RomanKholod self-assigned this Mar 4, 2026
Serpantyn
Serpantyn previously approved these changes Mar 4, 2026
[InlineData("")]
[InlineData(null)]
public void ShouldHaveError_WhenEmailIsEmpty(string email)
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

approved but format your tests with arrange act assert comments

@DrFaust555 DrFaust555 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Problems:

  1. SECURITY: Endpoint without [Authorize]
    [HttpPost("change-password")] does not have the [Authorize] attribute. Anyone can try to change the password knowing the email and current password. This endpoint must require authentication.

  2. SECURITY: Email is taken from the body, not from the token
    Handler accepts Email from the request body and looks for the user by it. This means that authenticated user A can try to change the password of user B if he knows his email and password. Correctly — take email from HttpContext.User.Claims (from the JWT token), and do not trust the body.

  3. build.yml contains an unnecessary change — adds a Test Email API step for a project that is not on dev. This came from merge and does not apply to the password feature. Because of this, CI crashes. It needs to be removed.

@sonarqubecloud

sonarqubecloud Bot commented Mar 5, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
69.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@RomanKholod
RomanKholod requested a review from DrFaust555 March 5, 2026 20:23

@DrFaust555 DrFaust555 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

“logout from all devices” does not work for access tokens.
After changing the password, the handler calls RevokeAllAsync, but the implementation only revokes refresh tokens. The JWT middleware only checks signature, issuer, audience, and lifetime, without SecurityStamp, token version, or denylist (configuration).
So, all already issued access JWTs remain valid until their expiration date. A TokenVersion/SecurityStamp or denylist mechanism is needed and an integration test: the old access token should receive a 401 after changing the password.

The user is identified via mutable email.
The controller takes ClaimTypes.Email, and the handler calls FindByEmailAsync (controller, handler). The JWT already contains a stable ClaimTypes.NameIdentifier, so it is better to pass the user ID and use FindByIdAsync.

Partial success is possible.
First, the password is actually stored via ChangePasswordAsync, and only then are the tokens revoked separately. If RevokeAllAsync fails, the client will get an error, even though the password has already been changed, and sessions may remain active. A specific strategy for handling this situation and a test for revocation failure are needed.

Quality Gate failed.
SonarCloud shows 69.2% coverage of new code, out of the required 80%. No controller tests, claim mapping, cookie deletion, access-token invalidation, and revocation errors.

Extra Email field in DTO.
ChangePasswordRequestDTO.Email is no longer used, but remains in the public API contract. It should be removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Create an ability to change user password

3 participants