Skip to content

feat: implement user password update functionality with validation #1887

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Junjiequan
Copy link
Member

@Junjiequan Junjiequan commented May 6, 2025

Description

Add endpoints for users to change their own password and for admins to change other users’ passwords, with input validation and auth-strategy checks.

Motivation

Users need a secure way to rotate their password; admins need to manage local-only accounts. Enforces that only “local” users can have their passwords updated.

Changes:

  • Added UpdateUserPasswordDto and AdminUpdateUserPasswordDto DTOs
  • Implemented updateOwnPassword (POST /users/password) and updateUserPassword (PATCH /users/:id/password) in UsersController
  • Validate current password, enforce authStrategy === "local", and return standardized response
  • Extended UsersService to perform password hash/update logic

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

official documentation info

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements user password update functionality with validation, adding new endpoints for users to update their own password and enabling administrators to update user passwords. Key changes include additions to the user service for password updates, controller endpoints for both user and admin password updates, and corresponding test cases and DTOs.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/Users.js Adds tests for changing password scenarios, including validations for incorrect passwords and auth strategy restrictions.
src/users/users.service.ts Introduces a new updateUserPassword method to hash and update the password in the database.
src/users/users.service.spec.ts Updates user settings mocks to include required properties for testing.
src/users/users.controller.ts Adds endpoints for local users to change their own passwords and for admins to change user passwords with proper authorization.
src/users/dto/update-user-password.dto.ts Defines the DTOs for user and admin password update requests.
src/datasets/datasets.controller.ts Cleans up unused imports by removing deprecated type references.
src/common/types.ts Adds password update response type for standardizing API responses.
Comments suppressed due to low confidence (1)

src/users/users.controller.ts:291

  • [nitpick] Consider revising the error message for clarity, for example: "Only local users' passwords can be changed by admin".
"Only local users passwords can be changed by admin",

@Junjiequan Junjiequan self-assigned this May 6, 2025
Copy link
Member

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

Just a couple of question/comments

@Junjiequan Junjiequan force-pushed the SWAP-4672-scicat-be-create-new-endpoint-for-password-change branch from 6f47c69 to e08fadc Compare May 7, 2025 08:53
Copy link
Member

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

Two minor changes on tests

@Junjiequan Junjiequan requested a review from nitrosx May 7, 2025 11:17
@Junjiequan Junjiequan force-pushed the SWAP-4672-scicat-be-create-new-endpoint-for-password-change branch from 44d7be8 to ca8171d Compare May 13, 2025 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants