Verify user registering a device #57
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 13 13
Lines 683 690 +7
Branches 56 57 +1
=======================================
+ Hits 683 690 +7 ☔ View full report in Codecov by Sentry. |
src/django_otp_webauthn/views.py
Outdated
| @@ -132,6 +132,7 @@ def post(self, *args, **kwargs): | |||
| logger = _get_pywebauthn_logger() | |||
| with rewrite_exceptions(logger=logger): | |||
| device = helper.register_complete(user=user, state=state, data=data) | |||
| otp_login(self.request, device) | |||
There was a problem hiding this comment.
Issue: this would override whatever MFA device was used to verify the current session (if the session was verified at all) which I think would be unexpected.
We should only do this if the session is not currently verified (if not request.user.is_verified())
There was a problem hiding this comment.
To push back a bit, personally I'd expect the override. The override also happens in django-two-factor-auth. In my opinion, it also results in better UX when adding a new passkey and then deleting the old one that you logged in with in the first place. (If there's no override you need to be logged out or disallowed to delete it).
That said, I'll leave it up to you to decide. Just let me know if you are sure about this and I will implement.
There was a problem hiding this comment.
Why do you think the override should happen? Aside from it being the default behavior of django-two-factor-auth
There was a problem hiding this comment.
I feel the flow should not be dependent on how or if you are 2fa authenticated at that point. Makes it more predictable for users.
Also what I said about removing an active auth method above.
But most important is that you get 2fa authenticated when you are not, so I can live with either solution.
|
I've opted to stay on the cautious side and only mark the session as MFA verified when a user is not MFA verified yet. I've added a commit to that effect. Thanks for PR @AlmerCarbonEquity, it should make it into a release soon. Gearing up to prepare a new release soon. |
Disclaimer: very new to this, so please let me know what you'd like me to add.
fixes #51
This PR makes sure
otp_loginis called when registering a new device, which means the user is 2fa verified afterwards. Currentlyotp_loginis only called on login. This behaviour is common across 2fa packages.