Added first user setup mode

[thestumonkey]

Removed default user details from defaults, you can use ./go and it will ask you for your user.

[thestumonkey]

@claude review

[claude]

Claude finished @thestumonkey's task in 1m 54s —— View job

---

Code Review Complete

I've reviewed the "first user setup mode" PR. This is a solid implementation that removes the need for default admin credentials and introduces a web-based setup flow. Here's my comprehensive feedback:

---

Overall Assessment

✅ Strengths:

• Well-structured separation of concerns (backend setup routes, frontend setup page, shell scripts)
• Good security practices (password validation, proper error handling, race condition protection)
• Clean user experience with auto-login after setup
• Excellent shell script automation with go.sh and clear.sh

⚠️ Areas for Improvement:
Several important issues that should be addressed before merging.

---

Critical Issues

1. Race Condition in Admin Creation (backends/advanced/src/advanced_omi_backend/routers/modules/setup_routes.py:84-91)

# Current code
admin_exists = await check_admin_exists()
if admin_exists:
    raise HTTPException(...)

# ... later ...
admin_user = await user_manager.create(admin_create)

Problem: There's a time window between checking if admin exists and creating the admin. Two concurrent requests could both pass the check and create duplicate admins.

Solution: Use an atomic operation or database-level constraint. Consider:

• Add a unique constraint on is_superuser=True in the database, OR
• Use a database transaction with a lock, OR
• Implement a mutex/lock around this critical section

2. Generator Cleanup (setup_routes.py:94-97)

user_db_gen = get_user_db()
user_db = await user_db_gen.__anext__()
user_manager_gen = get_user_manager(user_db)
user_manager = await user_manager_gen.__anext__()

Problem: Async generators are not properly closed, potentially leaking resources. The generators have cleanup code that won't execute.

Solution: Use async with or try/finally:

user_db_gen = get_user_db()
try:
    user_db = await user_db_gen.__anext__()
    user_manager_gen = get_user_manager(user_db)
    try:
        user_manager = await user_manager_gen.__anext__()
        # ... create admin ...
    finally:
        await user_manager_gen.aclose()
finally:
    await user_db_gen.aclose()

Or refactor to use dependency injection properly with FastAPI's Depends().

---

Security Concerns

3. Password Strength Validation (SetupPage.tsx:73-74)

} else if (password.length < 8) {
  errors.password = 'Password must be at least 8 characters'
}

Issue: Minimum 8 characters is weak by modern standards. No complexity requirements.

Recommendation:

• Increase minimum to 12 characters
• Consider adding complexity checks (uppercase, lowercase, numbers, symbols)
• Or use a password strength library like zxcvbn

4. Unvalidated Email Format (setup_routes.py:29)

Backend relies only on Pydantic's EmailStr validation, which is basic.

Recommendation: Consider additional validation:

• Email domain existence check (DNS MX record)
• Disposable email detection (if appropriate for your use case)
• Email confirmation flow for production deployments

5. Missing Rate Limiting (setup_routes.py:66)

The /setup/create-admin endpoint has no rate limiting.

Risk: Brute force attacks, denial of service attempts.

Solution: Add rate limiting:

from slowapi import Limiter
from slowapi.util import get_remote_address

limiter = Limiter(key_func=get_remote_address)

@router.post("/create-admin", ...)
@limiter.limit("5/minute")  # 5 attempts per minute
async def create_admin(request: AdminCreateRequest):

---

Code Quality Issues

6. Error Handling Too Broad (setup_routes.py:123-128)

except Exception as e:
    logger.error(f"Failed to create admin user: {e}", exc_info=True)
    raise HTTPException(
        status_code=500,
        detail="Failed to create admin user. Please try again."
    )

Issue: Generic exception handling masks the real problem and provides no actionable feedback to users.

Better approach:

except ValueError as e:
    raise HTTPException(status_code=400, detail=str(e))
except DatabaseError as e:
    logger.error(f"Database error: {e}", exc_info=True)
    raise HTTPException(status_code=500, detail="Database error. Please try again.")
except Exception as e:
    logger.error(f"Unexpected error: {e}", exc_info=True)
    raise HTTPException(status_code=500, detail="Unexpected error. Please try again.")

7. Inconsistent Password Validation (SetupPage.tsx vs setup_routes.py)

Frontend checks password.length < 8, but backend Pydantic model also has min_length=8.

Issue: Duplicate validation logic. If one changes, they get out of sync.

Recommendation:

• Document the password policy in a single location
• Consider moving validation rules to a shared constant or API endpoint

8. Docker Detection Assumption (clear.sh:55-61)

if ! docker ps | grep -q "mongo"; then
    echo "⚠️  MongoDB container is not running"
    echo "   Starting MongoDB..."
    docker compose -f docker-compose.infra.yml up -d mongo

Issue: docker ps | grep is fragile. Container might exist but not be named exactly "mongo".

Better approach:

if ! docker compose -f docker-compose.infra.yml ps mongo | grep -q "Up"; then

---

Documentation & UX Issues

9. Missing Router Registration (app_factory.py)

The new setup router is registered in api_router.py, but there's no visible confirmation in the diff that it's actually imported and included.

Recommendation: Verify that backends/advanced/src/advanced_omi_backend/routers/api_router.py properly imports and includes the setup router.

10. Unclear Setup Status Checking (ProtectedRoute.tsx:21-24)

// Redirect to setup if required
if (setupRequired === true) {
  return <Navigate to="/setup" replace />
}

Issue: This redirects authenticated users to setup if it's required. But setup should only be accessible when NOT authenticated.

Potential Bug: If a user is logged in and setupRequired becomes true (shouldn't happen, but edge case), they get redirected to setup while authenticated.

Recommendation: Add logic to prevent this:

if (setupRequired === true && !user) {
  return <Navigate to="/setup" replace />
}

11. No Logging for Setup Status Check (app_factory.py:104-119)

The startup logs admin creation but doesn't clearly indicate the web UI setup flow.

Recommendation: Add clearer logging:

if not admin_exists:
    application_logger.info("⚠️  No admin user found")
    application_logger.info("📝 Setup required - visit http://localhost:{port}/setup to create admin account")

---

Testing Recommendations

12. Missing Test Coverage

No tests included for the new setup flow.

Critical test scenarios:

1. ✅ Setup status returns requires_setup=true when no admin exists
2. ✅ Setup status returns requires_setup=false when admin exists
3. ✅ Creating admin succeeds with valid data
4. ✅ Creating admin fails when admin already exists (409 Conflict)
5. ✅ Creating admin fails with weak password (400 Bad Request)
6. ✅ Concurrent admin creation attempts (race condition test)
7. ✅ Auto-login after admin creation
8. ✅ Redirect to setup when no admin exists

Recommendation: Add Robot Framework tests or Python unit tests for these scenarios.

---

Shell Script Issues

13. Hardcoded Directory Name (go.sh:12-14, clear.sh:12-14)

if [ ! -f "docker-compose.yml" ] || [ ! -f "docker-compose.infra.yml" ]; then
    echo "❌ Error: Must be run from the GOLD directory"

Issue: Error message says "GOLD directory" but the actual directory name might be different.

Fix:

echo "❌ Error: Must be run from the repository root directory (containing docker-compose.yml)"

14. macOS/Linux sed Incompatibility (go.sh:45-49)

if [[ "$OSTYPE" == "darwin"* ]]; then
    sed -i '' "s|^AUTH_SECRET_KEY=.*|AUTH_SECRET_KEY=${SECRET_KEY}|" backends/advanced/.env
else
    sed -i "s|^AUTH_SECRET_KEY=.*|AUTH_SECRET_KEY=${SECRET_KEY}|" backends/advanced/.env
fi

Good: Already handles macOS vs Linux sed differences ✅

---

Minor Issues

15. Duplicate JWT_LIFETIME_SECONDS (auth.py:26-29)

JWT_LIFETIME_SECONDS = int(os.getenv("JWT_LIFETIME_SECONDS", "86400"))

# JWT configuration
JWT_LIFETIME_SECONDS = 86400  # 24 hours

Issue: Variable defined twice. The environment variable version is immediately overwritten.

Fix: Remove the duplicate:

# JWT configuration
JWT_LIFETIME_SECONDS = int(os.getenv("JWT_LIFETIME_SECONDS", "86400"))  # Default: 24 hours

16. Inconsistent Error Message Casing (SetupPage.tsx:61-62, 66-67)

errors.displayName = 'Name is required'
errors.email = 'Email is required'

Some error messages use sentence case, others don't. Consider standardizing.

---

Recommendations Summary

Before merging:

1. ✅ Fix the race condition in admin creation (Critical)
2. ✅ Fix generator cleanup (Critical)
3. ✅ Add rate limiting to setup endpoint (Security)
4. ✅ Strengthen password requirements (Security)
5. ✅ Fix duplicate JWT_LIFETIME_SECONDS (Bug)
6. ✅ Verify setup router is properly registered
7. ✅ Add test coverage for setup flow

Nice to have:

• Improve error handling specificity
• Add email validation beyond basic format checking
• Improve shell script robustness
• Add clearer startup logging for setup mode

---

Alignment with CLAUDE.md

✅ Good:

• Follows authentication patterns (email-based, JWT)
• Uses MongoDB ObjectId for user IDs
• Proper async/await patterns
• Good separation of concerns

⚠️ Could improve:

• Add integration tests (see CLAUDE.md testing section)
• Ensure backward compatibility is handled (existing deployments with ADMIN_PASSWORD set)

---

Files Reviewed

Backend:

• backends/advanced/src/advanced_omi_backend/routers/modules/setup_routes.py ⭐ Core logic
• backends/advanced/src/advanced_omi_backend/auth.py ✅ Good integration
• backends/advanced/src/advanced_omi_backend/app_factory.py ✅ Clean startup flow

Frontend:

• backends/advanced/webui/src/pages/SetupPage.tsx ⭐ Well-designed UI
• backends/advanced/webui/src/contexts/AuthContext.tsx ✅ Good setup status tracking
• backends/advanced/webui/src/components/auth/ProtectedRoute.tsx ⚠️ Minor edge case
• backends/advanced/webui/src/services/api.ts ✅ Clean API integration

Scripts:

• go.sh ✅ Excellent automation
• clear.sh ✅ Useful for development

Config:

• .env.default ✅ Good documentation
• backends/advanced/config/config.defaults.yaml ✅ Clean removal of defaults

---

Great work on this feature! The setup flow is well-designed and the user experience is clean. Address the critical issues (race condition, generator cleanup) and security concerns (rate limiting, password strength) before merging.

--- | View PR branch: login