Feat/ssad 0122/ ability to change user password - #123
Conversation
|
Only fails Test Email API |
| [InlineData("")] | ||
| [InlineData(null)] | ||
| public void ShouldHaveError_WhenEmailIsEmpty(string email) | ||
| { |
There was a problem hiding this comment.
approved but format your tests with arrange act assert comments
DrFaust555
left a comment
There was a problem hiding this comment.
Problems:
-
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. -
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. -
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.
|
DrFaust555
left a comment
There was a problem hiding this comment.
“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


dev
JIRA
Summary of change
Added an ability to change user password