search

CZ-NIC / django-fido

Django application for FIDO protocol U2F
GNU General Public License v3.0
28 stars 11 forks source link

recording user handle in authenticator #142

Closed variable closed 2 years ago

variable commented 2 years ago

This adds a user_handle field to authenticator so that in passwordless login, the authenticator can be looked up by user_handle from navigator.credentials.get()

This is for https://github.com/CZ-NIC/django-fido/issues/140

variable commented 2 years ago

I don't understand when I run python manage.py test without --settings=django_fido.test.settings I get this test failed, what's special in the test settings that prevented this?

FAIL: test_no_user (django_fido.tests.test_views.TestFido2AuthenticationRequestView)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlin/projects/django-fido/django_fido/tests/test_views.py", line 210, in test_no_user
  ....
  File "/Users/jlin/projects/django-fido/django_fido/views.py", line 331, in create_fido2_request
    assert user and user.is_authenticated, "User must not be anonymous for FIDO 2 requests."
AssertionError: User must not be anonymous for FIDO 2 requests.
variable commented 2 years ago

Nevermind, found the issue, two_setp_auth in the test settings is True, but my project settings has set it to False

variable commented 2 years ago

I finally managed to get 2 yubi keys, but it seems the user ids for 2 different keys for 2 different user generate the same base64 string, it's weird, hence I am pausing this PR.

variable commented 2 years ago

Nevermind, just realised my browser was overwriting session for each user, doh!

ziima commented 2 years ago

Nevermind, found the issue, two_setp_auth in the test settings is True, but my project settings has set it to False

When something like this happens, it usually means that tests are not properly isolated. It would be better to set the setting in tests correctly, so they are independent on the settings actually used. Not in this PR though.

variable commented 2 years ago

Hmmmmmmm, I just realised this is not going to fly when we run the migration for existing users, because the user_handle field is supposed to be unique, even we work out a magical way to populate some random string for each existing user, the passwordless won't match the random string until user re-register the key. Doh....

variable commented 2 years ago

I guess one way to fix this is to make the new user_handle field nullable, I know it's against the django recommendation not to set text fields to null...

variable commented 2 years ago

Hello guys,

Please rip out this PR from master, I just came to realised the user handle can be decoded into username by using base64.b64decode(value) so this field is not needed since we can simply using the username from the userhandle provided by resident_key option to find the user.

variable commented 2 years ago

Hello guys,

Please rip out this PR from master, I just came to realised the user handle can be decoded into username by using base64.b64decode(value) so this field is not needed since we can simply using the username from the userhandle provided by resident_key option to find the user.

Please ignore this as we should not use username as user handle when we have resident_key=True

tpazderka commented 2 years ago

But we still need to fix the migration for existing users.

variable commented 2 years ago

But we still need to fix the migration for existing users.

Yes, there is another PR on top of this which removes the unique=True constraint.