-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WebAuthn authentication flow #6792
Changes from 2 commits
b38d60e
eddebaa
52999d2
1a63e08
fc34ac4
40d6362
3dd2036
1431063
644e869
3f12556
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -284,8 +284,17 @@ async def create_authentication_options_for_email( | |
cached_globally=True, | ||
) | ||
result_json = json.loads(result.decode()) | ||
if len(result_json) == 0: | ||
raise errors.WebAuthnAuthenticationFailed( | ||
"No WebAuthn credentials found for the email." | ||
) | ||
|
||
user_handles: set[str] = {x["user_handle"] for x in result_json} | ||
assert len(user_handles) == 1 | ||
if len(user_handles) > 1: | ||
raise errors.WebAuthnAuthenticationFailed( | ||
"Multiple user handles found for the email." | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could still be an assert. It's really more of an ISE if there's multiple user handles, since the db should enforce that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that's a good point. I'd want to still log a useful error message in the ISE, so I'll throw something together. |
||
|
||
user_handle = base64.b64decode(result_json[0]["user_handle"]) | ||
|
||
credential_ids = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ | |
from typing import Any, Callable | ||
from jwcrypto import jwt, jwk | ||
|
||
from edgedb import QueryAssertionError | ||
from edb.testbase import http as tb | ||
from edb.common import assert_data_shape | ||
|
||
|
@@ -3639,6 +3640,55 @@ async def test_http_auth_ext_webauthn_register_options_existing_user(self): | |
|
||
self.assertEqual(user_id_decoded, existing_user_handle) | ||
|
||
async def test_http_auth_ext_webauthn_emails_share_user_handle(self): | ||
email = "[email protected]" | ||
|
||
user_handle_one = uuid.uuid4().bytes | ||
credential_id_one = uuid.uuid4().bytes | ||
public_key_one = uuid.uuid4().bytes | ||
|
||
user_handle_two = uuid.uuid4().bytes | ||
credential_id_two = uuid.uuid4().bytes | ||
public_key_two = uuid.uuid4().bytes | ||
|
||
with self.assertRaisesRegex( | ||
QueryAssertionError, | ||
"user_handle must be the same for a given email", | ||
): | ||
await self.con.execute( | ||
""" | ||
with | ||
factor_one := (insert ext::auth::WebAuthnFactor { | ||
email := <str>$email, | ||
user_handle := <bytes>$user_handle_one, | ||
credential_id := <bytes>$credential_id_one, | ||
public_key := <bytes>$public_key_one, | ||
identity := (insert ext::auth::LocalIdentity { | ||
issuer := "local", | ||
subject := "", | ||
}), | ||
}), | ||
factor_two := (insert ext::auth::WebAuthnFactor { | ||
email := <str>$email, | ||
user_handle := <bytes>$user_handle_two, | ||
credential_id := <bytes>$credential_id_two, | ||
public_key := <bytes>$public_key_two, | ||
identity := (insert ext::auth::LocalIdentity { | ||
issuer := "local", | ||
subject := "", | ||
}), | ||
}) | ||
select true; | ||
""", | ||
email=email, | ||
user_handle_one=user_handle_one, | ||
credential_id_one=credential_id_one, | ||
public_key_one=public_key_one, | ||
user_handle_two=user_handle_two, | ||
credential_id_two=credential_id_two, | ||
public_key_two=public_key_two, | ||
) | ||
|
||
async def test_http_auth_ext_webauthn_authenticate_options(self): | ||
with self.http_con() as http_con: | ||
email = "[email protected]" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing, but I think
?? true
probably isn't needed. If the result of the select is an empty set, the assertion just won't run anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about the empty set behavior of
assert
. I'll add an example to the documentation forassert
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it just follows the normal set behaviour, where if the function arg isn't
set of
(like incount
, for example) then it's expanded out. Soassert({true, false})
is executed as{assert(true), assert(false)}
, and similarlyassert({})
becomes{}
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. The documentation hints at this to since it says:
I think it'd just be helpful to show some examples of calling this function with a sets to show that. I'll follow up.