Require email confirmation for TOTP-based logins #18689
Conversation
| def send_unrecognized_login_email(request, user, *, ip_address, user_agent, token): | ||
| return { | ||
| "username": user.username, | ||
| "ip_address": ip_address, |
There was a problem hiding this comment.
suggestion: include geo data as well
| UserUniqueLogin.user_id == userid, | ||
| UserUniqueLogin.ip_address == request.remote_addr, |
There was a problem hiding this comment.
Since these appear to be the "couplet" of conditions used in most lookups, there should probably be a composite index for both columns.
Here's an example of how that's done:
warehouse/warehouse/packaging/models.py
Lines 581 to 583 in e9b70d0
Unless the UniqueConstraint satisfies this already?
| user_service.update_user( | ||
| userid, last_totp_value=form.totp_value.data | ||
| ) |
There was a problem hiding this comment.
question: would it make sense to also update a unique_login.last_used or something like that when we see it again?
There was a problem hiding this comment.
I'm not sure what that would give us. Do we want to "expire" trusted devices after some period of time?
| if unique_login is None and two_factor_method != "totp": | ||
| # We haven't seen this login before. Create a new one and mark it as confirmed | ||
| # if this is non-TOTP. | ||
| unique_login = UserUniqueLogin( | ||
| user_id=userid, | ||
| ip_address=request.remote_addr, | ||
| status=UniqueLoginStatus.CONFIRMED, | ||
| ) | ||
| request.db.add(unique_login) |
There was a problem hiding this comment.
question: should this logic trigger if the user used recovery codes?
| ) | ||
| user: Mapped[User] = orm.relationship(back_populates="unique_logins") | ||
|
|
||
| ip_address: Mapped[str] = mapped_column(String, nullable=False) |
There was a problem hiding this comment.
question: Should this be its own string, or a relationship to warehouse.events.models.IpAddress?
| last_login=( | ||
| datetime.datetime.now(datetime.UTC) - datetime.timedelta(days=1) | ||
| ), | ||
| ) | ||
| monkeypatch.setattr( | ||
| type(user), | ||
| "has_recovery_codes", | ||
| property(lambda u: has_recovery_codes), | ||
| ) | ||
| user.record_event = pretend.call_recorder(lambda *a, **kw: None) |
There was a problem hiding this comment.
thought: instead of monkeypatching, we could pass something like:
recovery_codes=[RecoveryCode(code="fake")] if has_recovery_codes else [],to UserFactory.create() - but since you're following an existing pattern, that's fine to defer.
There was a problem hiding this comment.
When running a manual verification, the email sent was only text, not HTML. Any clue as to why?
There was a problem hiding this comment.
Are you talking about the ConsoleAndSMTPEmailSender in local dev? I don't think it displays HTML, only text:
warehouse/warehouse/email/services.py
Line 157 in f891447
There was a problem hiding this comment.
I am - one line above it is the note that you can visualize the HTML version in MailDev, which usually displays something richer for other emails.
Fixes #18425.
This PR maintains a record of device information across logins for each user: