feat(security): add mandatory password change step to admin setup wizard#694
feat(security): add mandatory password change step to admin setup wizard#694
Conversation
Add security improvements to enforce password change for admin users: - Add password change API endpoint (PUT /api/users/me/password) - Add admin_password_changed field to welcome-config and health APIs - Add password validation to setup-complete endpoint - Create SetupPasswordStep component as first wizard step - Update wizard from 2-step to 3-step flow (password → Models → Skills) - Conditionally hide default credentials on login page based on password status - Extract default password hash as shared constant
📝 WalkthroughWalkthroughAdds a mandatory admin password-change flow: backend util and check for default admin password, new change-password endpoint, health flag reporting, frontend setup-wizard password step and API call, updated schemas/types/i18n, and e2e updates to perform an idempotent password change before completing setup. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant WizardUI as Wizard UI
participant SetupStep as SetupPasswordStep
participant FrontAPI as Frontend API (userApis)
participant Backend as Backend Server
participant DB as Database
Admin->>WizardUI: Open admin setup wizard
WizardUI->>SetupStep: Render password step (or skip if already changed)
Admin->>SetupStep: Submit new_password & confirm_password
SetupStep->>SetupStep: Client-side validate (min length, match)
SetupStep->>FrontAPI: PUT /users/me/password {new_password, confirm_password}
FrontAPI->>Backend: Forward change-password request
Backend->>DB: Update admin user's password_hash
DB-->>Backend: Acknowledge update
Backend-->>FrontAPI: Return updated user
FrontAPI-->>SetupStep: Success response
SetupStep->>WizardUI: onPasswordChanged()
WizardUI->>WizardUI: Advance to next step
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/app/core/yaml_init.py (1)
26-29: Constant naming should use SCREAMING_SNAKE_CASE.The constant name
DEFAULT_ADMIN_Password_HASHuses mixed case which deviates from PEP 8's convention for constants. This should beDEFAULT_ADMIN_PASSWORD_HASH.Note: The Ruff S105 hint about "hardcoded password" is a false positive here — this hash is intentionally stored to detect whether the admin has changed the default password, which is the correct security pattern for this use case.
✏️ Suggested fix for naming convention
-# Default admin password hash for "Wegent2025!" - used to detect if admin has changed default password -DEFAULT_ADMIN_Password_HASH = ( +# Default admin password hash for "Wegent2025!" - used to detect if admin has changed default password +DEFAULT_ADMIN_PASSWORD_HASH = ( "$2b$12$5jQMrJGO8NMXmF90f/xnKeLtM/Deh912k4GRPx.q3nTGOg1e1IJzW" )If you adopt this change, update all references in other files (
health.py,system_config.py,users.py) to useDEFAULT_ADMIN_PASSWORD_HASH.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/yaml_init.py` around lines 26 - 29, The constant DEFAULT_ADMIN_Password_HASH is misnamed; rename it to DEFAULT_ADMIN_PASSWORD_HASH in yaml_init.py and update all usages (references in health.py, system_config.py, users.py or any other modules) to the new symbol; ensure imports and any equality checks or comparisons that used DEFAULT_ADMIN_Password_HASH are replaced with DEFAULT_ADMIN_PASSWORD_HASH to preserve behavior and run tests/type checks to catch missed references.backend/app/api/endpoints/health.py (1)
39-40: Consider moving imports to module level.Importing
DEFAULT_ADMIN_Password_HASHandUserinside the function works but is unconventional. If there are no circular import issues, consider moving these to the top of the file for consistency.✏️ Suggested refactor
from app.api.dependencies import get_db from app.core.shutdown import shutdown_manager +from app.core.yaml_init import DEFAULT_ADMIN_Password_HASH +from app.models.user import User router = APIRouter()Then remove lines 39-40 from inside the function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/endpoints/health.py` around lines 39 - 40, The imports for DEFAULT_ADMIN_Password_HASH and User are currently inside the health check function; move the imports to the module level at the top of the file so they are imported once on module load (unless doing so creates a circular import), i.e. add "from app.core.yaml_init import DEFAULT_ADMIN_Password_HASH" and "from app.models.user import User" to the top of the file and remove the lines importing them from inside the function where they currently appear.frontend/src/features/admin/components/GlobalAdminSetupWizard.tsx (1)
281-281: Use cross-namespace format forcommontranslation key.Same issue as in SetupPasswordStep — this component uses
useTranslation('admin'), so cross-namespace keys should use the colon format.🔧 Proposed fix
- {t('common.loading')} + {t('common:loading')}As per coding guidelines: "use
t('namespace:key')format for cross-namespace keys".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/admin/components/GlobalAdminSetupWizard.tsx` at line 281, In GlobalAdminSetupWizard (which calls useTranslation('admin')) the translation key for the common namespace should use cross-namespace colon format; update the t call that currently uses t('common.loading') to t('common:loading') (same fix applied in SetupPasswordStep) so the component correctly resolves the common translation when using useTranslation('admin').frontend/src/features/admin/components/SetupPasswordStep.tsx (1)
165-165: Use cross-namespace format forcommontranslation key.The component uses
useTranslation('admin')as a single namespace. Per coding guidelines, cross-namespace keys should use thet('namespace:key')format.🔧 Proposed fix
- {t('common.loading')} + {t('common:loading')}As per coding guidelines: "use
t('namespace:key')format for cross-namespace keys".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/admin/components/SetupPasswordStep.tsx` at line 165, In SetupPasswordStep (component using useTranslation('admin')) update the cross-namespace translation call from t('common.loading') to the proper cross-namespace format t('common:loading'); locate the t usage in the render (currently t('common.loading')) and replace it with t('common:loading') while keeping the existing useTranslation('admin') hook intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/endpoints/health.py`:
- Around line 38-56: The health endpoint currently computes
admin_password_changed inline (admin_password_changed) and duplicates that same
logic in other places; extract the check into a shared utility function named
is_admin_password_default(db: Session) that returns True/False based on
comparing User.password_hash for user_name "admin" against
DEFAULT_ADMIN_Password_HASH (and returns True if no admin user exists), then
replace the inline logic in the health endpoint (admin_password_changed
assignment) and the duplicated checks in users.py and admin/system_config.py
with calls to is_admin_password_default(db) (negating as needed to preserve the
existing admin_password_changed boolean) so all three locations use the single
utility.
In `@backend/app/api/endpoints/users.py`:
- Around line 83-100: The change_password endpoint currently allows password
updates without verifying the current password; update the ChangePasswordRequest
model to include an old_password field and, inside change_password, call the
existing password verification helper (e.g., security.verify_password or
equivalent) to check request.old_password against current_user.password_hash and
raise HTTP 400/401 on mismatch before hashing the new password; add the
`@limiter`(...) decorator to the change_password route to apply rate limiting;
after successful password update (before/after db.commit()) trigger token
invalidation for the user (e.g., increment a token_version on User or call
security.invalidate_user_tokens(current_user)) and write an audit log entry for
the password change event (using the app logger/processLogger) noting user id
and timestamp; ensure db.commit() persists both the new password_hash and any
token_version change and db.refresh(current_user).
---
Nitpick comments:
In `@backend/app/api/endpoints/health.py`:
- Around line 39-40: The imports for DEFAULT_ADMIN_Password_HASH and User are
currently inside the health check function; move the imports to the module level
at the top of the file so they are imported once on module load (unless doing so
creates a circular import), i.e. add "from app.core.yaml_init import
DEFAULT_ADMIN_Password_HASH" and "from app.models.user import User" to the top
of the file and remove the lines importing them from inside the function where
they currently appear.
In `@backend/app/core/yaml_init.py`:
- Around line 26-29: The constant DEFAULT_ADMIN_Password_HASH is misnamed;
rename it to DEFAULT_ADMIN_PASSWORD_HASH in yaml_init.py and update all usages
(references in health.py, system_config.py, users.py or any other modules) to
the new symbol; ensure imports and any equality checks or comparisons that used
DEFAULT_ADMIN_Password_HASH are replaced with DEFAULT_ADMIN_PASSWORD_HASH to
preserve behavior and run tests/type checks to catch missed references.
In `@frontend/src/features/admin/components/GlobalAdminSetupWizard.tsx`:
- Line 281: In GlobalAdminSetupWizard (which calls useTranslation('admin')) the
translation key for the common namespace should use cross-namespace colon
format; update the t call that currently uses t('common.loading') to
t('common:loading') (same fix applied in SetupPasswordStep) so the component
correctly resolves the common translation when using useTranslation('admin').
In `@frontend/src/features/admin/components/SetupPasswordStep.tsx`:
- Line 165: In SetupPasswordStep (component using useTranslation('admin'))
update the cross-namespace translation call from t('common.loading') to the
proper cross-namespace format t('common:loading'); locate the t usage in the
render (currently t('common.loading')) and replace it with t('common:loading')
while keeping the existing useTranslation('admin') hook intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d97bd579-5f5c-43b6-ba85-0be94f0eeaae
📒 Files selected for processing (13)
backend/app/api/endpoints/admin/system_config.pybackend/app/api/endpoints/health.pybackend/app/api/endpoints/users.pybackend/app/core/yaml_init.pybackend/app/schemas/admin.pybackend/app/schemas/user.pyfrontend/src/apis/user.tsfrontend/src/features/admin/components/GlobalAdminSetupWizard.tsxfrontend/src/features/admin/components/SetupPasswordStep.tsxfrontend/src/features/login/components/LoginForm.tsxfrontend/src/i18n/locales/en/admin.jsonfrontend/src/i18n/locales/zh-CN/admin.jsonfrontend/src/types/api.ts
| # Check if admin password has been changed from default | ||
| from app.core.yaml_init import DEFAULT_ADMIN_Password_HASH | ||
| from app.models.user import User | ||
|
|
||
| admin_password_changed = True | ||
| admin_user = db.query(User).filter(User.user_name == "admin").first() | ||
| if admin_user: | ||
| admin_password_changed = ( | ||
| admin_user.password_hash != DEFAULT_ADMIN_Password_HASH | ||
| ) | ||
|
|
||
| return { | ||
| "status": "healthy", | ||
| "database": "connected", | ||
| "users_initialized": user_count > 0, | ||
| "user_count": user_count, | ||
| "shutting_down": shutdown_manager.is_shutting_down, | ||
| "admin_password_changed": admin_password_changed, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that /health is indeed unauthenticated by checking dependencies
rg -n 'def health_check|@router.get\("/health"\)' backend/app/api/endpoints/health.py -A 5Repository: wecode-ai/Wegent
Length of output: 355
🏁 Script executed:
#!/bin/bash
# Find the default admin password hash
rg -n 'DEFAULT_ADMIN_Password_HASH' backend/ -A 2 -B 2Repository: wecode-ai/Wegent
Length of output: 3051
🏁 Script executed:
#!/bin/bash
# Search for uses of the health endpoint in frontend code
fd -e tsx -e ts -e jsx -e js . | xargs rg -l '/health' 2>/dev/null | head -20Repository: wecode-ai/Wegent
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Check if there are other endpoints that expose similar information
rg -n '@router.get\(' backend/app/api/endpoints/health.py -A 3Repository: wecode-ai/Wegent
Length of output: 670
🏁 Script executed:
#!/bin/bash
# Check how LoginForm.tsx uses the /health endpoint
cat frontend/src/features/login/components/LoginForm.tsx | grep -n -A 10 -B 5 '/health'Repository: wecode-ai/Wegent
Length of output: 756
🏁 Script executed:
#!/bin/bash
# Check if admin_password_changed is used in LoginForm
cat frontend/src/features/login/components/LoginForm.tsx | grep -n -A 5 -B 5 'admin_password_changed'Repository: wecode-ai/Wegent
Length of output: 548
Security & code duplication: Exposing admin_password_changed on unauthenticated endpoint with duplicated logic across endpoints.
The /health endpoint is unauthenticated, exposing admin_password_changed publicly. 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 in frontend/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:
# backend/app/core/admin_password.py
def is_admin_password_default(db: Session) -> bool:
"""Check if admin user still uses default password."""
from app.core.yaml_init import DEFAULT_ADMIN_Password_HASH
from app.models.user import User
admin_user = db.query(User).filter(User.user_name == "admin").first()
if admin_user:
return admin_user.password_hash == DEFAULT_ADMIN_Password_HASH
return True # No admin user existsThen 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 else block
(TRY300)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/endpoints/health.py` around lines 38 - 56, The health
endpoint currently computes admin_password_changed inline
(admin_password_changed) and duplicates that same logic in other places; extract
the check into a shared utility function named is_admin_password_default(db:
Session) that returns True/False based on comparing User.password_hash for
user_name "admin" against DEFAULT_ADMIN_Password_HASH (and returns True if no
admin user exists), then replace the inline logic in the health endpoint
(admin_password_changed assignment) and the duplicated checks in users.py and
admin/system_config.py with calls to is_admin_password_default(db) (negating as
needed to preserve the existing admin_password_changed boolean) so all three
locations use the single utility.
| @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", | ||
| ) | ||
|
|
||
| # 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 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Best practices for password change endpoint security requirements
💡 Result:
Password change endpoint (authenticated user) — security best practices
1) Require strong proof it’s really the user
- Require the current password + new password + confirmation (and verify the current password server-side). [2], [5]
- Use step-up / re-authentication for “sensitive actions” (password change, email/MFA changes), especially if sessions can be long-lived (mitigates stolen-cookie/session hijack). [6]
- If MFA is enabled, consider requiring MFA for the change (step-up), not just for login. [6]
2) Prevent abuse (brute force/DoS)
- Apply rate limiting and lockout / throttling on the current-password verification attempts (treat like an authentication surface). [7]
- Add monitoring/alerting for repeated failures and unusual patterns (IP/device). [7]
3) Make password selection resistant to guessing and breaches
- On password set/change, check the new password against a blocklist of common/expected/compromised passwords. [1]
- Avoid outdated rules like mandatory periodic rotation; prefer event-driven changes (e.g., compromise) and encourage MFA. [3], [1]
4) Protect the endpoint itself
- Enforce TLS end-to-end (never accept password changes over plaintext).
- If cookie-based browser session: require CSRF protection (e.g., same-site cookies + CSRF token).
- Return generic error messages (don’t reveal whether “current password” was close/valid beyond a simple failure).
5) Secure lifecycle after the change
- Invalidate existing sessions/refresh tokens (at least all other sessions; often also the current one depending on risk). [6]
- Send out-of-band notification (“Your password was changed”) to help detect account takeover. [3]
- Write audit logs for password-change attempts/success (who/when/IP/device), protected from tampering.
6) Avoid authorization mistakes
- Ensure the endpoint can only change the password for the authenticated account (no user ID in the request that could enable IDOR).
- For admin-initiated resets vs. user-initiated changes: separate flows and controls; treat reset/forgot-password as an auth mechanism with equal rigor. [7]
Sources
- NIST Digital Identity Guidelines (SP 800-63-4 / 63B successor): blocklist checks and guidance around password changes. [1]
- OWASP ASVS: secure credential change requirements (include old password, confirmation). [2], [5]
- OWASP Cheat Sheets / WSTG / API Security Top 10: re-authentication for sensitive changes and treating reset flows as auth surfaces. [3], [6], [7]
🏁 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:
- Session hijacking or unattended devices could result in unauthorized password changes
- No protection against brute force attempts (endpoint lacks rate limiting)
- Existing tokens remain valid after password change, extending attack window
Add an old_password field to ChangePasswordRequest and verify it before allowing the change. Additionally:
- Apply rate limiting to this endpoint using the
@limiterdecorator (framework already exists) - Consider invalidating existing tokens after a password change
- Add audit logging for password change events
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 Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 87-87: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/endpoints/users.py` around lines 83 - 100, The
change_password endpoint currently allows password updates without verifying the
current password; update the ChangePasswordRequest model to include an
old_password field and, inside change_password, call the existing password
verification helper (e.g., security.verify_password or equivalent) to check
request.old_password against current_user.password_hash and raise HTTP 400/401
on mismatch before hashing the new password; add the `@limiter`(...) decorator to
the change_password route to apply rate limiting; after successful password
update (before/after db.commit()) trigger token invalidation for the user (e.g.,
increment a token_version on User or call
security.invalidate_user_tokens(current_user)) and write an audit log entry for
the password change event (using the app logger/processLogger) noting user id
and timestamp; ensure db.commit() persists both the new password_hash and any
token_version change and db.refresh(current_user).
…ement - Change admin password via API in global setup before marking setup complete - Add changeAdminPassword method to E2E API client - Update admin test beforeEach hooks to change password before completing setup - Use same credentials with different bcrypt hash to satisfy the requirement
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/e2e/tests/admin/admin-public-models.spec.ts`:
- Around line 19-24: Remove the silent catch so setup failures surface: replace
the current awaited call with a direct await of
apiClient.changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) (i.e.,
remove the .catch(() => {}) wrapper) inside the beforeEach so any error will
throw and fail the test; if you need idempotency, explicitly check a
precondition endpoint or assert the specific expected error instead of
swallowing it.
In `@frontend/e2e/tests/admin/admin-users.spec.ts`:
- Around line 20-25: The current beforeEach swallows all errors from
apiClient.changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) which
masks failures; modify the call in the beforeEach so that you only ignore a
known "password already changed" success condition and otherwise let the error
bubble (or explicitly fail the test). Specifically, replace the generic
.catch(() => {}) around changeAdminPassword with error inspection in the promise
rejection from apiClient.changeAdminPassword (or await in try/catch): if the
error/message/status indicates "already changed" treat it as a no-op, otherwise
rethrow or fail the test so failures in changeAdminPassword cause the test to
error; keep references to apiClient.changeAdminPassword and ADMIN_USER.password
to locate the code.
In `@frontend/e2e/tests/global-setup.ts`:
- Around line 48-54: The password-change step currently just logs a warning on
failure (checking passwordResponse.ok()), allowing global setup to continue;
change this so the global setup fails loudly: when passwordResponse.ok() is
false, throw an Error (or reject) with a descriptive message including
passwordResponse.status() and any body text to abort the setup (similarly update
the other occurrence around the 99-101 check). Reference the passwordResponse
check in global-setup.ts and ensure the thrown error replaces the console.warn
so the Playwright global setup does not save state or proceed when the
prerequisite fails.
- Around line 35-47: Replace use of the default TEST_USER.password in the
password-change flow with a dedicated E2E admin password constant (e.g.,
E2E_ADMIN_PASSWORD) in frontend/e2e/tests/global-setup.ts: update the PUT to
/api/users/me/password (the request that currently sends TEST_USER.password for
new_password/confirm_password) to send the new dedicated password, and update
any subsequent admin login calls in the same file that currently reuse
TEST_USER.password to use the dedicated E2E_ADMIN_PASSWORD so the test actually
verifies the "must change default password" requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8045115-2e06-495c-a64f-5b0bfc3a9a9f
📒 Files selected for processing (4)
frontend/e2e/tests/admin/admin-public-models.spec.tsfrontend/e2e/tests/admin/admin-users.spec.tsfrontend/e2e/tests/global-setup.tsfrontend/e2e/utils/api-client.ts
| // First change password (required before setup can be completed) | ||
| await apiClient | ||
| .changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) | ||
| .catch(() => { | ||
| // Ignore errors - password may already be changed | ||
| }) |
There was a problem hiding this comment.
Don't ignore setup-precondition failures in E2E beforeEach.
This .catch(() => {}) lets the test continue even when the admin password was not changed, which hides failures in the new wizard gate instead of surfacing them immediately. Based on learnings: "E2E tests MUST NOT silently fail: DO NOT catch errors and return early without failing the test, every assertion MUST pass for test to succeed".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/e2e/tests/admin/admin-public-models.spec.ts` around lines 19 - 24,
Remove the silent catch so setup failures surface: replace the current awaited
call with a direct await of apiClient.changeAdminPassword(ADMIN_USER.password,
ADMIN_USER.password) (i.e., remove the .catch(() => {}) wrapper) inside the
beforeEach so any error will throw and fail the test; if you need idempotency,
explicitly check a precondition endpoint or assert the specific expected error
instead of swallowing it.
| // First change password (required before setup can be completed) | ||
| await apiClient | ||
| .changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) | ||
| .catch(() => { | ||
| // Ignore errors - password may already be changed | ||
| }) |
There was a problem hiding this comment.
Don't swallow the password-change precondition in beforeEach.
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
Verify each finding against the current code and only fix it if needed.
In `@frontend/e2e/tests/admin/admin-users.spec.ts` around lines 20 - 25, The
current beforeEach swallows all errors from
apiClient.changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) which
masks failures; modify the call in the beforeEach so that you only ignore a
known "password already changed" success condition and otherwise let the error
bubble (or explicitly fail the test). Specifically, replace the generic
.catch(() => {}) around changeAdminPassword with error inspection in the promise
rejection from apiClient.changeAdminPassword (or await in try/catch): if the
error/message/status indicates "already changed" treat it as a no-op, otherwise
rethrow or fail the test so failures in changeAdminPassword cause the test to
error; keep references to apiClient.changeAdminPassword and ADMIN_USER.password
to locate the code.
- Create admin_utils.py with is_admin_password_default() to eliminate duplicated password check logic across health, users, and system_config endpoints - Improve E2E test error handling with descriptive logging for password change and setup completion API calls
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/e2e/tests/admin/admin-users.spec.ts (1)
20-35:⚠️ Potential issue | 🟠 MajorThis precondition can pass without ever clearing the new wizard gate.
changeAdminPassword()takes(newPassword, confirmPassword), so passingADMIN_USER.passwordtwice just resubmits the default secret instead of rotating it. Thecatch(... => null)branches then hide both password-change and setup-complete failures, which means this suite can still go green while the mandatory password step is broken.Use a real non-default test password or preflight
admin_password_changed, and let unexpected failures fail the test. Based on learnings: "E2E tests MUST NOT silently fail: DO NOT catch errors and return early without failing the test, every assertion MUST pass for test to succeed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/tests/admin/admin-users.spec.ts` around lines 20 - 35, The password-change precondition is incorrect and silently swallows failures: update the call to apiClient.changeAdminPassword to pass a real non-default test password for both new and confirm (do not reuse ADMIN_USER.password), or preflight check admin_password_changed before attempting rotation; remove the .catch(...) that returns null on both apiClient.changeAdminPassword and apiClient.markAdminSetupComplete so errors bubble and fail the test if unexpected, and add explicit assertions on responses (e.g., status codes or admin_password_changed) to ensure the new wizard gate is actually cleared.frontend/e2e/tests/admin/admin-public-models.spec.ts (1)
19-35:⚠️ Potential issue | 🟠 MajorThis setup helper doesn't actually rotate the admin password, and it suppresses failures.
Here too,
changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password)sends the default password as both the new and confirmed value, so it doesn't satisfy the intended setup requirement. Because both API calls degrade errors intonull, the test can continue even when the password gate or setup-complete guard is broken.Please use a real rotated password or an explicit
admin_password_changedcheck, and keep these calls fail-fast. Based on learnings: "E2E tests MUST NOT silently fail: DO NOT catch errors and return early without failing the test, every assertion MUST pass for test to succeed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/tests/admin/admin-public-models.spec.ts` around lines 19 - 35, The setup helper is silently swallowing failures and isn't rotating the password because changeAdminPassword(ADMIN_USER.password, ADMIN_USER.password) uses the same value; update the flow to generate or use a distinct NEW_ADMIN_PASSWORD (not equal to ADMIN_USER.password) and call apiClient.changeAdminPassword(ADMIN_USER.password, NEW_ADMIN_PASSWORD) without catching and returning null; after the call assert the response is successful (status < 400) or let the error propagate so the test fails fast; likewise call apiClient.markAdminSetupComplete() without degrading errors and assert success; additionally, add an explicit verification step using the admin status endpoint or apiClient.getAdminStatus()/getAdmin() to assert admin_password_changed is true and setup-complete is true before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/admin_utils.py`:
- Around line 27-30: The current check in admin_user ->
DEFAULT_ADMIN_Password_HASH compares raw hashes (admin_user.password_hash ==
DEFAULT_ADMIN_Password_HASH) which is invalid for salted bcrypt hashes; replace
it with a proper verification of the known default plaintext against the stored
hash (e.g. use the project's password verification utility or bcrypt.checkpw
with DEFAULT_ADMIN_Plaintext and admin_user.password_hash) or alternatively
switch to reading a persisted "default password rotated" flag; update the helper
that references admin_user and DEFAULT_ADMIN_Password_HASH to perform plaintext
verification (or read the rotation flag) so the default-password detection is
not bypassed by re-salting.
---
Duplicate comments:
In `@frontend/e2e/tests/admin/admin-public-models.spec.ts`:
- Around line 19-35: The setup helper is silently swallowing failures and isn't
rotating the password because changeAdminPassword(ADMIN_USER.password,
ADMIN_USER.password) uses the same value; update the flow to generate or use a
distinct NEW_ADMIN_PASSWORD (not equal to ADMIN_USER.password) and call
apiClient.changeAdminPassword(ADMIN_USER.password, NEW_ADMIN_PASSWORD) without
catching and returning null; after the call assert the response is successful
(status < 400) or let the error propagate so the test fails fast; likewise call
apiClient.markAdminSetupComplete() without degrading errors and assert success;
additionally, add an explicit verification step using the admin status endpoint
or apiClient.getAdminStatus()/getAdmin() to assert admin_password_changed is
true and setup-complete is true before proceeding.
In `@frontend/e2e/tests/admin/admin-users.spec.ts`:
- Around line 20-35: The password-change precondition is incorrect and silently
swallows failures: update the call to apiClient.changeAdminPassword to pass a
real non-default test password for both new and confirm (do not reuse
ADMIN_USER.password), or preflight check admin_password_changed before
attempting rotation; remove the .catch(...) that returns null on both
apiClient.changeAdminPassword and apiClient.markAdminSetupComplete so errors
bubble and fail the test if unexpected, and add explicit assertions on responses
(e.g., status codes or admin_password_changed) to ensure the new wizard gate is
actually cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 213e8e74-8cc3-42db-b2f2-92b844bdf654
📒 Files selected for processing (7)
backend/app/api/endpoints/admin/system_config.pybackend/app/api/endpoints/health.pybackend/app/api/endpoints/users.pybackend/app/services/admin_utils.pyfrontend/e2e/tests/admin/admin-public-models.spec.tsfrontend/e2e/tests/admin/admin-users.spec.tsfrontend/e2e/tests/global-setup.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/e2e/tests/global-setup.ts
- backend/app/api/endpoints/health.py
… prevent same-password bypass - Replace direct hash comparison with bcrypt verify_password() in admin_utils.py to correctly detect default password even after re-hashing with different salt - Add DEFAULT_ADMIN_Password plaintext constant for bcrypt verification - Reject password change to the same default value in change endpoint - Introduce dedicated E2E admin password (WegentE2E2025!) separate from default credentials, ensuring E2E tests properly validate the password change requirement - Centralize E2E password constants in test-users.ts config - Update all E2E tests and utilities to use E2E password after setup
API tests need the setup project to complete first because global-setup changes the admin password from default to the E2E password. Without this dependency, API tests start before the password is changed and fail to authenticate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/api/endpoints/users.py (1)
83-110:⚠️ Potential issue | 🟠 MajorRequire the current password before accepting a password change.
This route only validates the new password pair, so any stolen or unattended authenticated session can rotate the account password without knowing the existing one. Add a
current_password/old_passwordfield toChangePasswordRequestand verify it againstcurrent_user.password_hashbefore updating. If the setup wizard needs a first-login shortcut, keep that as a separate constrained flow instead of weakening/me/password.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/endpoints/users.py` around lines 83 - 110, The change_password endpoint currently only checks new/confirm passwords; add an old/current password check by extending ChangePasswordRequest with a current_password (or old_password) field and, inside change_password, verify this against the stored hash (use the existing password verification helper, e.g. security.verify_password or equivalent) for current_user before hashing and updating; if verification fails, raise an HTTPException (400/401) with an appropriate message; keep any special first-login/setup-wizard shortcut as a separate controlled path instead of weakening change_password.
🧹 Nitpick comments (1)
backend/app/core/yaml_init.py (1)
26-32: Keep the default admin credential in one source of truth.
ensure_default_user()seeds fromDEFAULT_ADMIN_Password_HASH, whileis_admin_password_default()verifiesDEFAULT_ADMIN_Password. That makes future default-password changes depend on two values staying in sync, and it also pulls this large init module into request-time code just to read constants. Move the default password to a small shared module and derive the seed hash when creating the user instead of hardcoding both forms.As per coding guidelines, "Manage complexity through extraction: if a module is already complex, prioritize extracting common logic into utilities or creating new modules rather than adding more complexity" and "Before implementing new functionality, search for existing implementations, extract reusable patterns, create shared utilities, and never copy-paste code or write duplicate logic."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/yaml_init.py` around lines 26 - 32, The module currently duplicates the default admin secret as DEFAULT_ADMIN_Password and DEFAULT_ADMIN_Password_HASH and mixes uses between ensure_default_user() and is_admin_password_default(); extract a single source-of-truth by creating a small shared constant (e.g., DEFAULT_ADMIN_PASSWORD) in a lightweight utility module and import that everywhere, remove DEFAULT_ADMIN_Password_HASH, and change ensure_default_user() to derive the bcrypt hash at seed-time from DEFAULT_ADMIN_PASSWORD (instead of reading a hardcoded hash); also update is_admin_password_default() to compare against DEFAULT_ADMIN_PASSWORD so no heavy init module is imported at request-time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/admin_utils.py`:
- Around line 29-32: The admin lookup currently queries by user_name ("admin")
in is_admin_password_default (admin_utils.py), which breaks if username is
changed; change the query to filter on the immutable role field (role ==
"admin") instead. Update the same logic in ensure_admin_user_exists
(yaml_init.py) and the password reset guard in users.py to use role == "admin"
for identifying the admin account, mirroring how get_admin_user (security.py)
locates the admin; ensure any returned variable names (e.g., admin_user) and
subsequent checks (like verify_password(DEFAULT_ADMIN_Password,
admin_user.password_hash)) remain valid after switching the filter.
---
Duplicate comments:
In `@backend/app/api/endpoints/users.py`:
- Around line 83-110: The change_password endpoint currently only checks
new/confirm passwords; add an old/current password check by extending
ChangePasswordRequest with a current_password (or old_password) field and,
inside change_password, verify this against the stored hash (use the existing
password verification helper, e.g. security.verify_password or equivalent) for
current_user before hashing and updating; if verification fails, raise an
HTTPException (400/401) with an appropriate message; keep any special
first-login/setup-wizard shortcut as a separate controlled path instead of
weakening change_password.
---
Nitpick comments:
In `@backend/app/core/yaml_init.py`:
- Around line 26-32: The module currently duplicates the default admin secret as
DEFAULT_ADMIN_Password and DEFAULT_ADMIN_Password_HASH and mixes uses between
ensure_default_user() and is_admin_password_default(); extract a single
source-of-truth by creating a small shared constant (e.g.,
DEFAULT_ADMIN_PASSWORD) in a lightweight utility module and import that
everywhere, remove DEFAULT_ADMIN_Password_HASH, and change ensure_default_user()
to derive the bcrypt hash at seed-time from DEFAULT_ADMIN_PASSWORD (instead of
reading a hardcoded hash); also update is_admin_password_default() to compare
against DEFAULT_ADMIN_PASSWORD so no heavy init module is imported at
request-time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a89fdd6-1e81-4f1e-b892-d319e27e6022
📒 Files selected for processing (11)
backend/app/api/endpoints/users.pybackend/app/core/yaml_init.pybackend/app/services/admin_utils.pyfrontend/e2e/config/test-users.tsfrontend/e2e/tests/auth.spec.tsfrontend/e2e/tests/global-setup.tsfrontend/e2e/utils/api-client.tsfrontend/e2e/utils/auth.tsfrontend/e2e/utils/cleanup.tsfrontend/e2e/utils/helpers.tsfrontend/playwright.config.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/e2e/utils/auth.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/e2e/utils/api-client.ts
- frontend/e2e/tests/global-setup.ts
| 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) |
There was a problem hiding this comment.
🧩 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 role field instead of username to identify the admin account.
The admin identity check depends on user_name == "admin", but this account can be renamed via update_current_user() (line 196 of user.py), which has no admin-specific protection. If renamed, three critical locations will break:
is_admin_password_default()(admin_utils.py:29) will fail to find the userensure_admin_user_exists()(yaml_init.py:68) will attempt to create a duplicate- Password reset guard (users.py:97) will become ineffective
Switch all three checks to use the immutable role == "admin" field, which already exists and is safely used in get_admin_user() (security.py:332).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/admin_utils.py` around lines 29 - 32, The admin lookup
currently queries by user_name ("admin") in is_admin_password_default
(admin_utils.py), which breaks if username is changed; change the query to
filter on the immutable role field (role == "admin") instead. Update the same
logic in ensure_admin_user_exists (yaml_init.py) and the password reset guard in
users.py to use role == "admin" for identifying the admin account, mirroring how
get_admin_user (security.py) locates the admin; ensure any returned variable
names (e.g., admin_user) and subsequent checks (like
verify_password(DEFAULT_ADMIN_Password, admin_user.password_hash)) remain valid
after switching the filter.
1 participant
Summary
Changes
Backend
backend/app/core/yaml_init.py: Extract default admin password hash asDEFAULT_ADMIN_Password_HASHconstantbackend/app/schemas/user.py: AddChangePasswordRequestschema with validationbackend/app/api/endpoints/users.py: AddPUT /api/users/me/passwordendpoint for password changebackend/app/api/endpoints/users.py: Addadmin_password_changedto welcome-config responsebackend/app/schemas/admin.py: Addadmin_password_changedfield toWelcomeConfigResponsebackend/app/api/endpoints/health.py: Addadmin_password_changedto health check (unauthenticated)backend/app/api/endpoints/admin/system_config.py: Block setup completion if password still defaultFrontend
frontend/src/types/api.ts: Addadmin_password_changedtoWelcomeConfigResponsefrontend/src/apis/user.ts: AddchangePassword()API methodfrontend/src/features/admin/components/SetupPasswordStep.tsx: New password change step componentfrontend/src/features/admin/components/GlobalAdminSetupWizard.tsx: 3-step wizard with mandatory password stepfrontend/src/features/login/components/LoginForm.tsx: Conditional default credentials based on health APIfrontend/src/i18n/locales/{en,zh-CN}/admin.json: Add translations for password stepTest plan
POST /setup-completereturns 400 if password is still defaultSummary by CodeRabbit