-
Notifications
You must be signed in to change notification settings - Fork 91
feat(security): add mandatory password change step to admin setup wizard #694
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
base: main
Are you sure you want to change the base?
Changes from all commits
c9c31a2
e4910f2
ef89ba6
c114425
517a088
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ | |
| QuickAccessTeam, | ||
| WelcomeConfigResponse, | ||
| ) | ||
| from app.schemas.user import UserCreate, UserInDB, UserUpdate | ||
| from app.schemas.user import ChangePasswordRequest, UserCreate, UserInDB, UserUpdate | ||
| from app.services.kind import kind_service | ||
| from app.services.user import user_service | ||
|
|
||
|
|
@@ -80,6 +80,36 @@ async def update_current_user_endpoint( | |
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) | ||
|
|
||
|
|
||
| @router.put("/me/password", response_model=UserInDB) | ||
| async def change_password( | ||
| request: ChangePasswordRequest, | ||
| db: Session = Depends(get_db), | ||
| current_user: User = Depends(security.get_current_user), | ||
| ): | ||
| """Change current user's password""" | ||
| if request.new_password != request.confirm_password: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Passwords do not match", | ||
| ) | ||
|
|
||
| # Prevent admin from "changing" to the same default password | ||
| if current_user.user_name == "admin": | ||
| from app.core.yaml_init import DEFAULT_ADMIN_Password | ||
|
|
||
| if request.new_password == DEFAULT_ADMIN_Password: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="New password cannot be the same as the default password", | ||
| ) | ||
|
|
||
| # Hash and update the password | ||
| current_user.password_hash = security.get_password_hash(request.new_password) | ||
| db.commit() | ||
| db.refresh(current_user) | ||
| return current_user | ||
|
Comment on lines
+83
to
+110
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Password change endpoint (authenticated user) — security best practices1) Require strong proof it’s really the user
2) Prevent abuse (brute force/DoS)
3) Make password selection resistant to guessing and breaches
4) Protect the endpoint itself
5) Secure lifecycle after the change
6) Avoid authorization mistakes
Sources
🏁 Script executed: # Find the ChangePasswordRequest model definition
rg "class ChangePasswordRequest" --type pyRepository: wecode-ai/Wegent Length of output: 127 🏁 Script executed: # Search for where the change_password endpoint is called or documented
rg "change_password|/me/password" --type py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 422 🏁 Script executed: # Check for rate limiting or security middleware
rg "rate_limit|RateLimiter|throttle" --type pyRepository: wecode-ai/Wegent Length of output: 8207 🏁 Script executed: # Search for session invalidation logic after password change
rg "session|Session|refresh_token|clear_cookies" backend/app/api/endpoints/users.py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 1480 🏁 Script executed: # Check for audit logging or security event tracking
rg "audit|log_event|security_log" --type pyRepository: wecode-ai/Wegent Length of output: 847 🏁 Script executed: # Look at the full users.py file to understand the context of the endpoint
wc -l backend/app/api/endpoints/users.pyRepository: wecode-ai/Wegent Length of output: 98 🏁 Script executed: # View the ChangePasswordRequest model to see what fields it requires
cat -n backend/app/schemas/user.py | grep -A 10 "class ChangePasswordRequest"Repository: wecode-ai/Wegent Length of output: 277 🏁 Script executed: # View the complete change_password endpoint implementation
sed -n '83,120p' backend/app/api/endpoints/users.pyRepository: wecode-ai/Wegent Length of output: 1397 🏁 Script executed: # Check if rate limiting is applied to the password change endpoint
sed -n '75,105p' backend/app/api/endpoints/users.pyRepository: wecode-ai/Wegent Length of output: 1028 🏁 Script executed: # Check for session/token invalidation after password change
rg "invalidate|revoke|token|session" backend/app/api/endpoints/users.py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 648 🏁 Script executed: # Look at security module to understand what get_current_user provides
fd "security" --type f backend/app | head -10Repository: wecode-ai/Wegent Length of output: 135 🏁 Script executed: # View the security module to understand authentication and token handling
cat backend/app/core/security.py | head -100Repository: wecode-ai/Wegent Length of output: 3592 🏁 Script executed: # Check if there's any token refresh or invalidation logic after password change
rg "password|token|refresh" backend/app/core/security.py -B 2 -A 2Repository: wecode-ai/Wegent Length of output: 10906 🏁 Script executed: # Look for any password change event handlers or callbacks
rg "change_password|password.*change" --type pyRepository: wecode-ai/Wegent Length of output: 1546 🏁 Script executed: # Check if rate limiting is defined anywhere for sensitive endpoints
cat backend/app/core/rate_limit.pyRepository: wecode-ai/Wegent Length of output: 3168 Add current password verification to password change endpoint. The endpoint currently allows password changes without verifying the current password, which violates OWASP ASVS security standards. This creates security risks:
Add an
If this endpoint is only for initial admin setup (where the default password is well-known), document this constraint and consider a separate, more restrictive flow for general user password changes. 🧰 Tools🪛 Ruff (0.15.4)[warning] 86-86: Do not perform function call (B008) [warning] 87-87: Do not perform function call (B008) 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| @router.delete("/me/git-token/{git_domain:path}", response_model=UserInDB) | ||
| async def delete_git_token( | ||
| git_domain: str, | ||
|
|
@@ -312,6 +342,7 @@ async def get_welcome_config( | |
|
|
||
| # Check admin setup status for admin users | ||
| admin_setup_completed = None | ||
| admin_password_changed = None | ||
| if current_user.role == "admin": | ||
| setup_config = ( | ||
| db.query(SystemConfig) | ||
|
|
@@ -323,6 +354,11 @@ async def get_welcome_config( | |
| else: | ||
| admin_setup_completed = False | ||
|
|
||
| # Check if admin password has been changed from default | ||
| from app.services.admin_utils import is_admin_password_default | ||
|
|
||
| admin_password_changed = not is_admin_password_default(db) | ||
|
|
||
| if not config: | ||
| # Return default configuration | ||
| return WelcomeConfigResponse( | ||
|
|
@@ -331,6 +367,7 @@ async def get_welcome_config( | |
| ], | ||
| tips=[ChatTipItem(**tip) for tip in DEFAULT_SLOGAN_TIPS_CONFIG["tips"]], | ||
| admin_setup_completed=admin_setup_completed, | ||
| admin_password_changed=admin_password_changed, | ||
| ) | ||
|
|
||
| config_value = config.config_value or {} | ||
|
|
@@ -344,6 +381,7 @@ async def get_welcome_config( | |
| for tip in config_value.get("tips", DEFAULT_SLOGAN_TIPS_CONFIG["tips"]) | ||
| ], | ||
| admin_setup_completed=admin_setup_completed, | ||
| admin_password_changed=admin_password_changed, | ||
| ) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,14 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Default admin password plaintext - used to verify if admin still uses default credentials | ||
| DEFAULT_ADMIN_Password = "Wegent2025!" | ||
|
|
||
| # Default admin password hash for "Wegent2025!" - used for initial user creation | ||
| DEFAULT_ADMIN_Password_HASH = ( | ||
| "$2b$12$5jQMrJGO8NMXmF90f/xnKeLtM/Deh912k4GRPx.q3nTGOg1e1IJzW" | ||
| ) | ||
|
|
||
|
|
||
| def load_yaml_documents(file_path: Path) -> List[Dict[str, Any]]: | ||
| """ | ||
|
|
@@ -64,7 +72,7 @@ def ensure_default_user(db: Session) -> tuple[int, bool]: | |
| # Default admin user (admin/Wegent2025!) | ||
| admin_user = User( | ||
| user_name="admin", | ||
| password_hash="$2b$12$5jQMrJGO8NMXmF90f/xnKeLtM/Deh912k4GRPx.q3nTGOg1e1IJzW", | ||
| password_hash=DEFAULT_ADMIN_Password_HASH, | ||
| email="[email protected]", | ||
| git_info=[], | ||
| is_active=True, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # SPDX-FileCopyrightText: 2025 Weibo, Inc. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Utility functions for admin user operations.""" | ||
|
|
||
| from sqlalchemy.orm import Session | ||
|
|
||
| from app.core.security import verify_password | ||
| from app.core.yaml_init import DEFAULT_ADMIN_Password | ||
| from app.models.user import User | ||
|
|
||
|
|
||
| def is_admin_password_default(db: Session) -> bool: | ||
| """ | ||
| Check if the admin user's password is still the default value. | ||
|
|
||
| Uses bcrypt verification to compare the known default plaintext password | ||
| against the stored hash. This correctly handles re-hashed passwords | ||
| (same plaintext with different salt) unlike direct hash comparison. | ||
|
|
||
| Args: | ||
| db: Database session | ||
|
|
||
| Returns: | ||
| True if the admin password is still the default, False if changed | ||
| or if the admin user does not exist. | ||
| """ | ||
| admin_user = db.query(User).filter(User.user_name == "admin").first() | ||
| if not admin_user: | ||
| return False | ||
| return verify_password(DEFAULT_ADMIN_Password, admin_user.password_hash) | ||
|
Comment on lines
+29
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== UserUpdate schema =="
rg -n -C3 'class UserUpdate|user_name' backend/app/schemas/user.py
echo
echo "== update_current_user implementation =="
rg -n -C5 'def update_current_user|user_name' backend/app/services/user.py
echo
echo "== User model definition =="
rg -n -C3 'user_name' backend/app/models/user.pyRepository: wecode-ai/Wegent Length of output: 5626 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Search for admin account initialization/seeding =="
rg -n "admin" backend/app/models/user.py backend/app/db/init_db.py 2>/dev/null | head -30
echo
echo "== Check User model for id/role/flags =="
rg -n -A20 'class User\(' backend/app/models/user.py | head -40
echo
echo "== Check if admin check exists elsewhere with stable identifier =="
rg -n 'admin' backend/app/api/endpoints/users.py | grep -i 'def\|==\|admin' | head -20Repository: wecode-ai/Wegent Length of output: 113 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Find User model file =="
fd -t f 'user\.py' backend/app/models/ 2>/dev/null
echo
echo "== Find all db initialization files =="
fd -t f '*.py' backend/app/db/ 2>/dev/null | head -20
echo
echo "== Search entire backend for 'admin' mentions in initialization context =="
rg -n "admin" backend/app/ -t py | grep -i "create\|seed\|init\|default" | head -20Repository: wecode-ai/Wegent Length of output: 153 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Read User model =="
cat -n backend/app/models/user.py
echo
echo
echo "== Search for admin user initialization =="
rg -n "admin" backend/app/ -t py --max-count=30Repository: wecode-ai/Wegent Length of output: 37366 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check if role field is updated in update_current_user =="
sed -n '172,220p' backend/app/services/user.py
echo
echo
echo "== Check UserUpdate schema for role field =="
rg -n 'class UserUpdate' -A15 backend/app/schemas/user.py | head -25
echo
echo
echo "== Check if role is ever set in user updates =="
rg -n 'user.role|obj_in.role' backend/app/services/user.pyRepository: wecode-ai/Wegent Length of output: 2558 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Find all locations checking user_name == 'admin' or similar =="
rg -n "user_name.*==.*admin|==.*\"admin\".*user_name" backend/app/ -t py
echo
echo
echo "== Check yaml_init for admin user creation to see if ID is stable =="
sed -n '59,90p' backend/app/core/yaml_init.pyRepository: wecode-ai/Wegent Length of output: 1457 Use the immutable The admin identity check depends on
Switch all three checks to use the immutable 🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,9 +16,23 @@ test.describe('Admin - Public Model Management', () => { | |
| await apiClient.login(ADMIN_USER.username, ADMIN_USER.password) | ||
|
|
||
| // Mark admin setup as complete via API to prevent GlobalAdminSetupWizard from showing | ||
| await apiClient.markAdminSetupComplete().catch(() => { | ||
| // Ignore errors - setup may already be complete | ||
| // First change password (required before setup can be completed) | ||
| const passwordResult = await apiClient | ||
| .changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) | ||
| .catch((error: Error) => { | ||
| console.log(`password change request failed (may already be changed): ${error.message}`) | ||
| return null | ||
| }) | ||
|
Comment on lines
+19
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't ignore setup-precondition failures in E2E This 🤖 Prompt for AI Agents |
||
| if (passwordResult && passwordResult.status >= 400) { | ||
| console.log(`password change API returned ${passwordResult.status} - may already be changed`) | ||
| } | ||
| const setupResult = await apiClient.markAdminSetupComplete().catch((error: Error) => { | ||
| console.log(`Setup complete request failed (may already be complete): ${error.message}`) | ||
| return null | ||
| }) | ||
| if (setupResult && setupResult.status >= 400) { | ||
| console.log(`Setup complete API returned ${setupResult.status} - may already be complete`) | ||
| } | ||
|
|
||
| // Navigate directly to admin page (already authenticated via global setup storageState) | ||
| await adminPage.navigateToTab('public-models') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,23 @@ test.describe('Admin - User Management', () => { | |
| await apiClient.login(ADMIN_USER.username, ADMIN_USER.password) | ||
|
|
||
| // Mark admin setup as complete via API to prevent GlobalAdminSetupWizard from showing | ||
| await apiClient.markAdminSetupComplete().catch(() => { | ||
| // Ignore errors - setup may already be complete | ||
| // First change password (required before setup can be completed) | ||
| const passwordResult = await apiClient | ||
| .changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) | ||
| .catch((error: Error) => { | ||
| console.log(`password change request failed (may already be changed): ${error.message}`) | ||
| return null | ||
| }) | ||
|
Comment on lines
+20
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't swallow the password-change precondition in If this request fails, the test still proceeds as if the admin setup gate was cleared. That makes regressions in the new mandatory password step show up as flaky UI behavior instead of a hard test failure. 🤖 Prompt for AI Agents |
||
| if (passwordResult && passwordResult.status >= 400) { | ||
| console.log(`password change API returned ${passwordResult.status} - may already be changed`) | ||
| } | ||
| const setupResult = await apiClient.markAdminSetupComplete().catch((error: Error) => { | ||
| console.log(`Setup complete request failed (may already be complete): ${error.message}`) | ||
| return null | ||
| }) | ||
| if (setupResult && setupResult.status >= 400) { | ||
| console.log(`Setup complete API returned ${setupResult.status} - may already be complete`) | ||
| } | ||
|
|
||
| // Navigate directly to admin page (already authenticated via global setup storageState) | ||
| await adminPage.navigateToTab('users') | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 355
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 3051
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 287
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 670
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 756
🏁 Script executed:
Repository: wecode-ai/Wegent
Length of output: 548
Security & code duplication: Exposing
admin_password_changedon unauthenticated endpoint with duplicated logic across endpoints.The
/healthendpoint is unauthenticated, exposingadmin_password_changedpublicly. This allows attackers to probe whether default credentials (admin/Wegent2025!) are still active. While this is an intentional design choice for the login page to auto-fill credentials (verified infrontend/src/features/login/components/LoginForm.tsx), it creates an information disclosure vector that should be explicitly documented as an approved security trade-off.Additionally, the admin password check logic is duplicated across at least three endpoints:
backend/app/api/endpoints/health.py(lines 38-47)backend/app/api/endpoints/users.py(lines 346-355)backend/app/api/endpoints/admin/system_config.py(lines 238-241)Extract this into a shared utility function to eliminate duplication and ensure consistent behavior:
Then replace duplicated logic in all three endpoints with calls to this utility.
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 49-56: Consider moving this statement to an
elseblock(TRY300)
🤖 Prompt for AI Agents