Skip to content

Commit

Permalink
Record more events (pypi#12491)
Browse files Browse the repository at this point in the history
* Record event on basic auth login success

* Record event when password is disabled

* Update translations
  • Loading branch information
di authored Nov 7, 2022
1 parent fb3909a commit 7843798
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 15 deletions.
21 changes: 18 additions & 3 deletions tests/unit/accounts/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,11 @@ def test_with_disabled_user_frozen(self, pyramid_request, pyramid_services):
assert service.is_disabled.calls == [pretend.call(1)]

def test_with_valid_password(self, monkeypatch, pyramid_request, pyramid_services):
user = pretend.stub(id=2, has_two_factor=False)
user = pretend.stub(
id=2,
has_two_factor=False,
record_event=pretend.call_recorder(lambda *a, **kw: None),
)
service = pretend.stub(
get_user=pretend.call_recorder(lambda user_id: user),
find_userid=pretend.call_recorder(lambda username: 2),
Expand Down Expand Up @@ -234,6 +238,13 @@ def test_with_valid_password(self, monkeypatch, pyramid_request, pyramid_service
pretend.call("mypass", tags=["method:auth", "auth_method:basic"])
]
assert service.update_user.calls == [pretend.call(2, last_login=now)]
assert user.record_event.calls == [
pretend.call(
ip_address="1.2.3.4",
tag=EventTag.Account.LoginSuccess,
additional={"auth_method": "basic"},
)
]

def test_via_basic_auth_compromised(
self, monkeypatch, pyramid_request, pyramid_services
Expand All @@ -251,7 +262,9 @@ def test_via_basic_auth_compromised(
lambda userid, password, tags=None: True
),
is_disabled=pretend.call_recorder(lambda user_id: (False, None)),
disable_password=pretend.call_recorder(lambda user_id, reason=None: None),
disable_password=pretend.call_recorder(
lambda user_id, reason=None, ip_address="127.0.0.1": None
),
)
breach_service = pretend.stub(
check_password=pretend.call_recorder(lambda pw, tags=None: True),
Expand Down Expand Up @@ -283,7 +296,9 @@ def test_via_basic_auth_compromised(
pretend.call("mypass", tags=["method:auth", "auth_method:basic"])
]
assert service.disable_password.calls == [
pretend.call(2, reason=DisableReason.CompromisedPassword)
pretend.call(
2, reason=DisableReason.CompromisedPassword, ip_address="1.2.3.4"
)
]
assert send_email.calls == [pretend.call(pyramid_request, user)]

Expand Down
8 changes: 6 additions & 2 deletions tests/unit/accounts/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,9 @@ def test_password_breached(self, monkeypatch):
find_userid=lambda _: 1,
get_user=lambda _: user,
check_password=lambda userid, pw, tags=None: True,
disable_password=pretend.call_recorder(lambda user_id, reason=None: None),
disable_password=pretend.call_recorder(
lambda user_id, reason=None, ip_address="127.0.0.1": None
),
is_disabled=lambda userid: (False, None),
)
breach_service = pretend.stub(
Expand All @@ -239,7 +241,9 @@ def test_password_breached(self, monkeypatch):
assert not form.validate()
assert form.password.errors.pop() == "Bad Password!"
assert user_service.disable_password.calls == [
pretend.call(1, reason=DisableReason.CompromisedPassword)
pretend.call(
1, reason=DisableReason.CompromisedPassword, ip_address="1.2.3.4"
)
]
assert send_email.calls == [pretend.call(request, user)]

Expand Down
24 changes: 22 additions & 2 deletions tests/unit/accounts/test_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
TooManyFailedLogins,
)
from warehouse.accounts.models import DisableReason, ProhibitedUserName
from warehouse.events.tags import EventTag
from warehouse.metrics import IMetricsService, NullMetrics
from warehouse.rate_limiting.interfaces import IRateLimiter

Expand Down Expand Up @@ -430,17 +431,36 @@ def test_get_admins(self, user_service):
assert admin in admins
assert user not in admins

def test_disable_password(self, user_service):
@pytest.mark.parametrize(
("reason", "expected"),
[
(None, None),
(
DisableReason.CompromisedPassword,
DisableReason.CompromisedPassword.value,
),
],
)
def test_disable_password(self, user_service, reason, expected):
user = UserFactory.create()
user.record_event = pretend.call_recorder(lambda *a, **kw: None)

# Need to give the user a good password first.
user_service.update_user(user.id, password="foo")
assert user.password != "!"

# Now we'll actually test our disable function.
user_service.disable_password(user.id)
user_service.disable_password(user.id, reason=reason)
assert user.password == "!"

assert user.record_event.calls == [
pretend.call(
tag=EventTag.Account.PasswordDisabled,
ip_address="127.0.0.1",
additional={"reason": expected},
)
]

@pytest.mark.parametrize(
("disabled", "reason"),
[(True, None), (True, DisableReason.CompromisedPassword), (False, None)],
Expand Down
4 changes: 3 additions & 1 deletion warehouse/accounts/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,9 @@ def validate_password(self, field):
user = self.user_service.get_user(userid)
send_password_compromised_email_hibp(self.request, user)
self.user_service.disable_password(
user.id, reason=DisableReason.CompromisedPassword
user.id,
reason=DisableReason.CompromisedPassword,
ip_address=self.request.remote_addr,
)
raise wtforms.validators.ValidationError(
markupsafe.Markup(self.breach_service.failure_message)
Expand Down
9 changes: 8 additions & 1 deletion warehouse/accounts/security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,20 @@ def _basic_auth_check(username, password, request):
):
send_password_compromised_email_hibp(request, user)
login_service.disable_password(
user.id, reason=DisableReason.CompromisedPassword
user.id,
reason=DisableReason.CompromisedPassword,
ip_address=request.remote_addr,
)
raise _format_exc_status(
BasicAuthBreachedPassword(), breach_service.failure_message_plain
)

login_service.update_user(user.id, last_login=datetime.datetime.utcnow())
user.record_event(
tag=EventTag.Account.LoginSuccess,
ip_address=request.remote_addr,
additional={"auth_method": "basic"},
)
return True
else:
user.record_event(
Expand Down
8 changes: 7 additions & 1 deletion warehouse/accounts/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
User,
WebAuthn,
)
from warehouse.events.tags import EventTag
from warehouse.metrics import IMetricsService
from warehouse.rate_limiting import DummyRateLimiter, IRateLimiter
from warehouse.utils.crypto import BadData, SignatureExpired, URLSafeTimedSerializer
Expand Down Expand Up @@ -294,10 +295,15 @@ def update_user(self, user_id, **changes):

return user

def disable_password(self, user_id, reason=None):
def disable_password(self, user_id, reason=None, ip_address="127.0.0.1"):
user = self.get_user(user_id)
user.password = self.hasher.disable()
user.disabled_for = reason
user.record_event(
tag=EventTag.Account.PasswordDisabled,
ip_address=ip_address,
additional={"reason": reason.value if reason else None},
)

def is_disabled(self, user_id):
user = self.get_user(user_id)
Expand Down
1 change: 1 addition & 0 deletions warehouse/events/tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class Account(EventTagEnum):
OrganizationRoleRemove = "account:organization_role:remove"
OrganizationRoleRevokeInvite = "account:organization_role:revoke_invite"
PasswordChange = "account:password:change"
PasswordDisabled = "account:password:disabled"
PasswordReset = "account:password:reset"
PasswordResetAttempt = "account:password:reset:attempt"
PasswordResetRequest = "account:password:reset:request"
Expand Down
10 changes: 5 additions & 5 deletions warehouse/locale/messages.pot
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,23 @@ msgstr ""
msgid "The name is too long. Choose a name with 100 characters or less."
msgstr ""

#: warehouse/accounts/forms.py:360
#: warehouse/accounts/forms.py:362
msgid "Invalid TOTP code."
msgstr ""

#: warehouse/accounts/forms.py:377
#: warehouse/accounts/forms.py:379
msgid "Invalid WebAuthn assertion: Bad payload"
msgstr ""

#: warehouse/accounts/forms.py:435
#: warehouse/accounts/forms.py:437
msgid "Invalid recovery code."
msgstr ""

#: warehouse/accounts/forms.py:443
#: warehouse/accounts/forms.py:445
msgid "Recovery code has been previously used."
msgstr ""

#: warehouse/accounts/forms.py:462
#: warehouse/accounts/forms.py:464
msgid "No user found with that username or email"
msgstr ""

Expand Down

0 comments on commit 7843798

Please sign in to comment.