Merge pull request #540 from christianlouis/copilot/fix-local-user-login-issue
fix(auth): local user login fails after password reset
This commit is contained in:
@@ -488,9 +488,11 @@ def admin_set_password(
|
|||||||
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Local user not found.")
|
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Local user not found.")
|
||||||
|
|
||||||
user.hashed_password = hash_password(body.password)
|
user.hashed_password = hash_password(body.password)
|
||||||
# Clear any outstanding reset tokens
|
# Clear any outstanding reset tokens and activate the account so the user
|
||||||
|
# can log in immediately after an admin sets their password.
|
||||||
user.password_reset_token = None
|
user.password_reset_token = None
|
||||||
user.password_reset_sent_at = None
|
user.password_reset_sent_at = None
|
||||||
|
user.is_active = True
|
||||||
|
|
||||||
try:
|
try:
|
||||||
db.commit()
|
db.commit()
|
||||||
|
|||||||
@@ -366,6 +366,9 @@ async def reset_password(body: PasswordResetBody, db: DbSession) -> dict[str, st
|
|||||||
user.hashed_password = hash_password(body.new_password)
|
user.hashed_password = hash_password(body.new_password)
|
||||||
user.password_reset_token = None
|
user.password_reset_token = None
|
||||||
user.password_reset_sent_at = None
|
user.password_reset_sent_at = None
|
||||||
|
# Activate the account in case it was still pending email verification.
|
||||||
|
# A valid password-reset token proves control of the registered email address.
|
||||||
|
user.is_active = True
|
||||||
db.commit()
|
db.commit()
|
||||||
|
|
||||||
logger.info("[SECURITY] PASSWORD_RESET_SUCCESS user=%s", user.email)
|
logger.info("[SECURITY] PASSWORD_RESET_SUCCESS user=%s", user.email)
|
||||||
|
|||||||
+3
-3
@@ -318,15 +318,15 @@ async def auth(request: Request, db: Session = Depends(get_db)):
|
|||||||
db.query(_LocalUser).filter((_LocalUser.username == username) | (_LocalUser.email == username)).first()
|
db.query(_LocalUser).filter((_LocalUser.username == username) | (_LocalUser.email == username)).first()
|
||||||
)
|
)
|
||||||
if local_user is not None:
|
if local_user is not None:
|
||||||
if not _verify_password(password or "", local_user.hashed_password):
|
|
||||||
logger.warning("[SECURITY] LOCAL_LOGIN_FAILURE user=%s", username)
|
|
||||||
return RedirectResponse(url="/login?error=Invalid+username+or+password", status_code=302)
|
|
||||||
if not local_user.is_active:
|
if not local_user.is_active:
|
||||||
logger.warning("[SECURITY] LOCAL_LOGIN_UNVERIFIED user=%s", username)
|
logger.warning("[SECURITY] LOCAL_LOGIN_UNVERIFIED user=%s", username)
|
||||||
return RedirectResponse(
|
return RedirectResponse(
|
||||||
url="/login?error=Please+verify+your+email+address+before+logging+in",
|
url="/login?error=Please+verify+your+email+address+before+logging+in",
|
||||||
status_code=302,
|
status_code=302,
|
||||||
)
|
)
|
||||||
|
if not _verify_password(password or "", local_user.hashed_password):
|
||||||
|
logger.warning("[SECURITY] LOCAL_LOGIN_FAILURE user=%s", username)
|
||||||
|
return RedirectResponse(url="/login?error=Invalid+username+or+password", status_code=302)
|
||||||
user_data = _build_session_user(local_user)
|
user_data = _build_session_user(local_user)
|
||||||
request.session["user"] = user_data
|
request.session["user"] = user_data
|
||||||
logger.info("[SECURITY] LOCAL_LOGIN_SUCCESS user=%s", local_user.email)
|
logger.info("[SECURITY] LOCAL_LOGIN_SUCCESS user=%s", local_user.email)
|
||||||
|
|||||||
@@ -515,6 +515,41 @@ def test_reset_password_mismatch(la_client, la_session):
|
|||||||
assert resp.status_code == 422
|
assert resp.status_code == 422
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.integration
|
||||||
|
def test_reset_password_activates_inactive_user(la_client, la_session):
|
||||||
|
"""POST /api/auth/reset-password sets is_active=True for inactive accounts.
|
||||||
|
|
||||||
|
A user who registered when SMTP was configured starts out with is_active=False.
|
||||||
|
Using the password-reset flow (which proves ownership of the email address) must
|
||||||
|
also activate the account so the user can log in immediately afterwards.
|
||||||
|
"""
|
||||||
|
token = "activatetoken456"
|
||||||
|
user = LocalUser(
|
||||||
|
email="inactive_reset@example.com",
|
||||||
|
username="inactivereset",
|
||||||
|
hashed_password=hash_password("oldpassword"),
|
||||||
|
is_active=False, # account not yet verified
|
||||||
|
password_reset_token=token,
|
||||||
|
password_reset_sent_at=datetime.now(tz=timezone.utc),
|
||||||
|
)
|
||||||
|
la_session.add(user)
|
||||||
|
la_session.commit()
|
||||||
|
|
||||||
|
resp = la_client.post(
|
||||||
|
"/api/auth/reset-password",
|
||||||
|
json={
|
||||||
|
"token": token,
|
||||||
|
"new_password": "newpassword2",
|
||||||
|
"new_password_confirm": "newpassword2",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
assert resp.status_code == 200
|
||||||
|
la_session.refresh(user)
|
||||||
|
assert user.is_active is True, "reset_password must activate inactive accounts"
|
||||||
|
assert verify_password("newpassword2", user.hashed_password)
|
||||||
|
assert user.password_reset_token is None
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Integration tests: page routes
|
# Integration tests: page routes
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
@@ -646,6 +681,34 @@ async def test_local_login_unverified(la_session, pending_user):
|
|||||||
assert "verify" in result.headers["location"].lower()
|
assert "verify" in result.headers["location"].lower()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.unit
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
@patch.object(settings, "multi_user_enabled", True)
|
||||||
|
async def test_local_login_unverified_wrong_password(la_session, pending_user):
|
||||||
|
"""auth() for unverified user with wrong password still shows the verify message.
|
||||||
|
|
||||||
|
is_active is checked before password so that inactive users always receive
|
||||||
|
the email-verification prompt regardless of whether they typed the correct
|
||||||
|
password. This avoids leaking whether the password is correct for an
|
||||||
|
account that has not yet been verified.
|
||||||
|
"""
|
||||||
|
from unittest.mock import AsyncMock, MagicMock
|
||||||
|
|
||||||
|
from fastapi import Request
|
||||||
|
|
||||||
|
from app.auth import auth
|
||||||
|
|
||||||
|
mock_request = MagicMock(spec=Request)
|
||||||
|
mock_request.form = AsyncMock(return_value={"username": "pendinguser", "password": "wrongpassword"})
|
||||||
|
mock_request.session = {}
|
||||||
|
|
||||||
|
result = await auth(mock_request, db=la_session)
|
||||||
|
assert result.status_code == 302
|
||||||
|
# Must send to the *verify* page, not the generic "invalid credentials" page.
|
||||||
|
assert "verify" in result.headers["location"].lower()
|
||||||
|
assert "user" not in mock_request.session
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Single-user backward-compatibility: LocalUser table must NOT be queried
|
# Single-user backward-compatibility: LocalUser table must NOT be queried
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Reference in New Issue
Block a user