Skip to content
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

Merged
merged 10 commits into from
Feb 21, 2024
10 changes: 10 additions & 0 deletions edb/lib/ext/auth.edgeql
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ CREATE EXTENSION PACKAGE auth VERSION '1.0' {
create constraint exclusive;
};

create trigger email_shares_user_handle after insert for each do (
std::assert(
(__new__.user_handle = (
select detached ext::auth::WebAuthnFactor
filter .email = __new__.email
and not .id = __new__.id
).user_handle) ?? true,
Copy link
Member

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.

Copy link
Contributor Author

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 for assert.

Copy link
Member

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 in count, for example) then it's expanded out. So assert({true, false}) is executed as {assert(true), assert(false)}, and similarly assert({}) becomes {}.

Copy link
Contributor Author

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:

If the input bool is false, assert raises a QueryAssertionError. Otherwise, this function returns true. (emphasis mine)

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.

message := "user_handle must be the same for a given email"
)
);
create constraint exclusive on ((.email, .credential_id));
};

Expand Down
11 changes: 10 additions & 1 deletion edb/server/protocol/auth_ext/webauthn.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."
)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 = [
Expand Down
50 changes: 50 additions & 0 deletions tests/test_http_ext_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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]"
Expand Down