Skip to content

fix: modify change password API v3 endpoints - #655

Open
alemar99 wants to merge 1 commit into
canonical:3.8from
alemar99:backport-ffcb72f-3.8
Open

alemar99 wants to merge 1 commit into
canonical:3.8from
alemar99:backport-ffcb72f-3.8

Conversation

@alemar99

Copy link
Copy Markdown
Contributor

(cherry picked from commit ffcb72f)

@alemar99 alemar99 added the 3.8 label Sep 14, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[maas-code-reviewer review]

LLM-generated review from https://github.com/canonical/maas-code-reviewer.
Intended to assist a human reviewer, not replace one — suggestions may be
incorrect, please verify before acting.

This PR successfully splits the user update requests into Self and Admin variants and correctly moves password validation into a separate check before updating. However, there are two important issues to address:

  1. Unintentional Overwrite of is_staff/is_active: Hardcoding these fields to False and True in the to_builder() methods causes any profile update to overwrite them. This means staff users will accidentally demote themselves when updating their names.
  2. Event Loop Blocking: Password hashing and verification are CPU-bound operations. Running them synchronously blocks the asyncio event loop and introduces a Denial-of-Service vulnerability.

Comment thread src/maasapiserver/v3/api/public/models/requests/users.py
return UserBuilder(password=password)


class UserChangePasswordRequestAdmin(BaseModel):

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).

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

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).

Comment thread src/tests/maasapiserver/v3/api/public/handlers/test_users.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant