fix: log a failed password migration instead of swallowing it
The round-2 review noted that `_upgrade_password` catches `PyMongoError` without logging it. A failed upgrade is harmless to the login itself - the user still gets in - but it leaves the password stored in plain text, and that is exactly the kind of thing an operator needs to know about. It is now logged as a warning with the username and the traceback. A test covers the path: the migration write is forced to fail, the login still succeeds, and the warning is emitted.
This commit is contained in:
@@ -17,6 +17,7 @@ Two collections are used:
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import logging
|
||||||
import os
|
import os
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
@@ -28,6 +29,8 @@ from brewing import Run
|
|||||||
|
|
||||||
__all__ = ["BeerRepository", "get_repository"]
|
__all__ = ["BeerRepository", "get_repository"]
|
||||||
|
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
|
||||||
class BeerRepository:
|
class BeerRepository:
|
||||||
"""Thin, testable wrapper around the two Mongo collections."""
|
"""Thin, testable wrapper around the two Mongo collections."""
|
||||||
@@ -89,8 +92,15 @@ class BeerRepository:
|
|||||||
}},
|
}},
|
||||||
)
|
)
|
||||||
except PyMongoError:
|
except PyMongoError:
|
||||||
# A failed upgrade must not stop the login succeeding.
|
# A failed upgrade must not stop the login succeeding — but the
|
||||||
pass
|
# operator needs to know the password is still stored in plain
|
||||||
|
# text, so log it rather than swallowing the error silently.
|
||||||
|
logger.warning(
|
||||||
|
"Could not upgrade the password for user %r to a hashed value; "
|
||||||
|
"it is still stored in plain text",
|
||||||
|
doc.get("username"),
|
||||||
|
exc_info=True,
|
||||||
|
)
|
||||||
|
|
||||||
# -- readings ----------------------------------------------------------- #
|
# -- readings ----------------------------------------------------------- #
|
||||||
|
|
||||||
|
|||||||
@@ -215,6 +215,29 @@ def test_legacy_plaintext_password_is_accepted_and_migrated(repo):
|
|||||||
assert "password_migrated_at" in doc
|
assert "password_migrated_at" in doc
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_failed_password_migration_still_lets_the_login_succeed(repo, caplog):
|
||||||
|
"""If the upgrade write fails, the login must still work — but the
|
||||||
|
failure is logged so the operator knows the password is still plaintext."""
|
||||||
|
import logging
|
||||||
|
|
||||||
|
from pymongo.errors import PyMongoError
|
||||||
|
|
||||||
|
repo.users.insert_one({"username": "beer", "password": "beer_pass"})
|
||||||
|
|
||||||
|
def boom(*args, **kwargs):
|
||||||
|
raise PyMongoError("write failed")
|
||||||
|
|
||||||
|
original = repo.users.update_one
|
||||||
|
repo.users.update_one = boom
|
||||||
|
|
||||||
|
with caplog.at_level(logging.WARNING):
|
||||||
|
assert repo.user_check("beer", "beer_pass") is True
|
||||||
|
|
||||||
|
repo.users.update_one = original
|
||||||
|
|
||||||
|
assert any("plain text" in rec.message for rec in caplog.records)
|
||||||
|
|
||||||
|
|
||||||
def test_hashed_password_verifies(repo):
|
def test_hashed_password_verifies(repo):
|
||||||
from mongo.repository import hash_password
|
from mongo.repository import hash_password
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user