Skip to content

fix: modify change password API v3 endpoints #655

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.

Sign up for GitHub

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
alemar99 wants to merge 1 commit into
base: 3.8
Choose a base branch
from
Open

fix: modify change password API v3 endpoints #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions src/maasapiserver/v3/api/public/handlers/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@
from maasapiserver.v3.api.public.models.requests.query import PaginationParams
from maasapiserver.v3.api.public.models.requests.users import (
UserChangePasswordRequest,
UserChangePasswordRequestAdmin,
UserCreateRequest,
UsersFiltersParams,
UserUpdateRequest,
UserUpdateRequestAdmin,
UserUpdateRequestSelf,
)
from maasapiserver.v3.api.public.models.responses.base import (
OPENAPI_ETAG_HEADER,
Expand Down Expand Up @@ -210,6 +211,7 @@ async def complete_intro(
tags=TAGS,
responses={
204: {},
400: {"model": BadRequestBodyResponse},
401: {"model": UnauthorizedBodyResponse},
},
response_model_exclude_none=True,
Expand All @@ -225,9 +227,12 @@ async def change_password_user(
services: ServiceCollectionV3 = Depends(services), # noqa: B008
) -> Response:
assert authenticated_user is not None
await services.users.change_password(
await services.users.change_password_checks(
user_id=authenticated_user.id,
password=change_password_request.password,
current_password=change_password_request.current_password,
)
await services.users.update_by_id(
authenticated_user.id, change_password_request.to_builder()
)
return Response(status_code=status.HTTP_204_NO_CONTENT)

Expand All @@ -237,6 +242,7 @@ async def change_password_user(
tags=TAGS,
responses={
200: {"model": UserResponse},
400: {"model": BadRequestBodyResponse},
401: {"model": UnauthorizedBodyResponse},
},
response_model_exclude_none=True,
Expand All @@ -245,14 +251,20 @@ async def change_password_user(
)
async def update_user_me(
self,
user_request: UserUpdateRequest,
user_request: UserUpdateRequestSelf,
response: Response,
authenticated_user: AuthenticatedUser | None = Depends( # noqa: B008
get_authenticated_user
),
services: ServiceCollectionV3 = Depends(services), # noqa: B008
) -> UserResponse:
assert authenticated_user is not None
if user_request.new_password is not None:
await services.users.change_password_checks(
user_id=authenticated_user.id,
current_password=user_request.current_password,
)

user = await services.users.update_by_id(
authenticated_user.id, user_request.to_builder()
)
Expand Down Expand Up @@ -425,6 +437,7 @@ async def create_user(
"model": UserResponse,
"headers": {"ETag": OPENAPI_ETAG_HEADER},
},
400: {"model": BadRequestBodyResponse},
404: {"model": NotFoundBodyResponse},
},
status_code=200,
Expand All @@ -444,6 +457,10 @@ async def update_user(
response: Response,
services: ServiceCollectionV3 = Depends(services), # noqa: B008
) -> UserResponse:
if user_request.password is not None:
await services.users.change_password_checks(
user_id=user_id, current_password=None
)
user = await services.users.update_by_id(
user_id, user_request.to_builder()
)
Expand Down Expand Up @@ -544,7 +561,7 @@ async def delete_user(
tags=TAGS,
responses={
204: {},
401: {"model": UnauthorizedBodyResponse},
400: {"model": BadRequestBodyResponse},
404: {"model": NotFoundBodyResponse},
},
response_model_exclude_none=True,
Expand All @@ -560,11 +577,14 @@ async def delete_user(
async def change_password_admin(
self,
user_id: int,
change_password_request: UserChangePasswordRequest,
change_password_request: UserChangePasswordRequestAdmin,
services: ServiceCollectionV3 = Depends(services), # noqa: B008
) -> Response:
await services.users.change_password(
user_id=user_id, password=change_password_request.password
await services.users.change_password_checks(
user_id=user_id, current_password=None
)
await services.users.update_by_id(
user_id, change_password_request.to_builder()
)
return Response(status_code=status.HTTP_204_NO_CONTENT)

Expand Down
51 changes: 45 additions & 6 deletions src/maasapiserver/v3/api/public/models/requests/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import re

from fastapi import Query
from pydantic import BaseModel, Field, field_validator
from pydantic import BaseModel, Field, field_validator, model_validator

from maasservicelayer.builders.users import UserBuilder
from maasservicelayer.db.filters import Clause
Expand Down Expand Up @@ -81,13 +81,22 @@ def to_builder(self) -> UserBuilder:
)


class UserUpdateRequest(BaseUserRequest):
password: str | None = Field(min_length=1, default=None)
class UserUpdateRequestSelf(BaseUserRequest):
current_password: str | None = Field(min_length=1, default=None)
new_password: str | None = Field(min_length=1, default=None)

@model_validator(mode="after")
def check_passwords(self):
if self.new_password is not None and self.current_password is None:
raise ValueError(
"The current password must be provided when changing password."
)
return self

def to_builder(self) -> UserBuilder:
password = (
UserBuilder.hash_password(self.password)
if self.password
UserBuilder.hash_password(self.new_password)
if self.new_password is not None
else UNSET
)
return UserBuilder(
Expand All @@ -101,12 +110,42 @@ def to_builder(self) -> UserBuilder:
)


class UserUpdateRequestAdmin(UserUpdateRequest):
class UserUpdateRequestAdmin(BaseUserRequest):
password: str | None = Field(min_length=1, default=None)
groups: list[int] = Field(
default_factory=list,
description="The IDs of the groups the user will be a member of.",
)

def to_builder(self) -> UserBuilder:
password = (
UserBuilder.hash_password(self.password)
if self.password is not None
else UNSET
)
return UserBuilder(
Comment thread
alemar99 marked this conversation as resolved.
username=self.username,
password=password,
is_staff=False,
is_active=True,
first_name=self.first_name,
last_name=self.last_name,
email=self.email,
)


class UserChangePasswordRequest(BaseModel):
current_password: str = Field(..., min_length=1)
new_password: str = Field(..., min_length=1)

def to_builder(self) -> UserBuilder:
password = UserBuilder.hash_password(self.new_password)
return UserBuilder(password=password)


class UserChangePasswordRequestAdmin(BaseModel):

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Calling UserBuilder.hash_password(...) synchronously here means that whenever this request model is converted to a builder in an async route handler, the event loop will block for the duration of the PBKDF2 hash computation (often hundreds of milliseconds). To avoid degrading API performance and preventing DoS, consider moving password hashing to the service layer where it can be offloaded to a thread pool (e.g., using anyio.to_thread.run_sync).

@alemar99 alemar99 Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#659

password: str = Field(..., min_length=1)

def to_builder(self) -> UserBuilder:
password = UserBuilder.hash_password(self.password)
return UserBuilder(password=password)
20 changes: 15 additions & 5 deletions src/maasservicelayer/services/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from time import time
from typing import List

from django.contrib.auth.hashers import PBKDF2PasswordHasher
import structlog

from maascommon.constants import (
Expand Down Expand Up @@ -363,7 +364,9 @@ async def complete_intro(self, user_id: int) -> UserProfile:
builder = UserProfileBuilder(completed_intro=True)
return await self.update_profile(user_id, builder)

async def change_password(self, user_id: int, password: str) -> None:
async def change_password_checks(
self, user_id: int, current_password: str | None
) -> None:
user = await self.get_by_id(user_id)
if user is None:
raise NotFoundException()
Expand All @@ -388,10 +391,17 @@ async def change_password(self, user_id: int, password: str) -> None:
]
)

hashed_password = UserBuilder.hash_password(password)
await self._update_resource(
user, UserBuilder(password=hashed_password)
)
if current_password is not None and not PBKDF2PasswordHasher().verify(

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Running CPU-bound operations like PBKDF2PasswordHasher().verify(...) directly in an async def function will block the asyncio event loop. For security and performance reasons, offload this to a thread pool by wrapping it in await anyio.to_thread.run_sync(PBKDF2PasswordHasher().verify, current_password, user.password).

current_password, user.password
):
raise BadRequestException(
details=[
BaseExceptionDetail(
type=PRECONDITION_FAILED,
message="Wrong password.",
)
]
)

async def post_update_hook(self, old_resource, updated_resource):
if old_resource.password != updated_resource.password:
Expand Down
Loading
Loading