From d7a17c788f1eac314ce3fbafa8c62025f1052bc8 Mon Sep 17 00:00:00 2001 From: Scott Trinh Date: Thu, 29 Feb 2024 14:15:27 -0500 Subject: [PATCH 1/5] Handle WebAuthn challenge having multiple factors We made some big changes late in the game for allowing multiple WebAuthn factors for a given (email, user_handle) pair, but didn't update this in the authentication challenge flow. Since we cannot remove the old link, this adds an additional link and ignores the old one. --- edb/lib/ext/auth.edgeql | 3 +++ edb/server/protocol/auth_ext/data.py | 9 ++++--- edb/server/protocol/auth_ext/webauthn.py | 33 +++++++++++++++--------- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/edb/lib/ext/auth.edgeql b/edb/lib/ext/auth.edgeql index fcdb69dcc50..826ea8da3cb 100644 --- a/edb/lib/ext/auth.edgeql +++ b/edb/lib/ext/auth.edgeql @@ -114,6 +114,9 @@ CREATE EXTENSION PACKAGE auth VERSION '1.0' { create required link factor: ext::auth::WebAuthnFactor { create constraint exclusive; }; + create required multi link factors: ext::auth::WebAuthnFactor { + create constraint exclusive; + }; }; create type ext::auth::PKCEChallenge extending ext::auth::Auditable { diff --git a/edb/server/protocol/auth_ext/data.py b/edb/server/protocol/auth_ext/data.py index df0805b0fe8..a3fed0ec029 100644 --- a/edb/server/protocol/auth_ext/data.py +++ b/edb/server/protocol/auth_ext/data.py @@ -190,13 +190,14 @@ class WebAuthnAuthenticationChallenge: created_at: datetime.datetime modified_at: datetime.datetime challenge: bytes - factor: WebAuthnFactor + factors: list[WebAuthnFactor] - def __init__(self, *, id, created_at, modified_at, challenge, factor): + def __init__(self, *, id, created_at, modified_at, challenge, factors): self.id = id self.created_at = created_at self.modified_at = modified_at self.challenge = base64.b64decode(challenge) - self.factor = ( + self.factors = [ WebAuthnFactor(**factor) if isinstance(factor, dict) else factor - ) + for factor in factors + ] diff --git a/edb/server/protocol/auth_ext/webauthn.py b/edb/server/protocol/auth_ext/webauthn.py index 578fb5379e3..a50bd69a675 100644 --- a/edb/server/protocol/auth_ext/webauthn.py +++ b/edb/server/protocol/auth_ext/webauthn.py @@ -315,18 +315,19 @@ async def create_authentication_options_for_email( challenge := $challenge, user_handle := $user_handle, email := $email, - factor := ( - assert_exists(assert_single(( + factors := ( + assert_exists(( select ext::auth::WebAuthnFactor filter .user_handle = user_handle and .email = email - ))) + )) ) insert ext::auth::WebAuthnAuthenticationChallenge { challenge := challenge, - factor := factor, + factor := assert_exists(assert_single((select factors limit 1))), + factors := factors, } -unless conflict on .factor +unless conflict on .factors else ( update ext::auth::WebAuthnAuthenticationChallenge set { @@ -389,7 +390,7 @@ async def _get_authentication_challenge( created_at, modified_at, challenge, - factor: { + factors: { id, created_at, modified_at, @@ -407,7 +408,7 @@ async def _get_authentication_challenge( } }, } -filter .factor.email = email and .factor.credential_id = credential_id;""", +filter .factors.email = email and .factors.credential_id = credential_id;""", variables={ "email": email, "credential_id": credential_id, @@ -437,7 +438,7 @@ async def _delete_authentication_challenges( email := $email, credential_id := $credential_id, delete ext::auth::WebAuthnAuthenticationChallenge -filter .factor.email = email and .factor.credential_id = credential_id;""", +filter .factors.email = email and .factors.credential_id = credential_id;""", variables={ "email": email, "credential_id": credential_id, @@ -461,13 +462,21 @@ async def authenticate( credential_id=credential.raw_id, ) + factor = next( + ( + f + for f in authentication_challenge.factors + if f.credential_id == credential.raw_id + ), + None, + ) + assert factor is not None, "Missing factor for the given credential." + try: webauthn.verify_authentication_response( credential=credential, expected_challenge=authentication_challenge.challenge, - credential_public_key=( - authentication_challenge.factor.public_key - ), + credential_public_key=factor.public_key, credential_current_sign_count=0, expected_rp_id=self.provider.relying_party_id, expected_origin=self.provider.relying_party_origin, @@ -477,4 +486,4 @@ async def authenticate( "Invalid authentication response. Please retry authentication." ) - return authentication_challenge.factor.identity + return factor.identity From 2af19e9ec68c8e01ada3f9fbe760f533c02e5cf2 Mon Sep 17 00:00:00 2001 From: Scott Trinh Date: Fri, 1 Mar 2024 08:58:29 -0500 Subject: [PATCH 2/5] Union `.factor` property into `.factors` --- edb/server/protocol/auth_ext/webauthn.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/edb/server/protocol/auth_ext/webauthn.py b/edb/server/protocol/auth_ext/webauthn.py index a50bd69a675..4d0f873b4f9 100644 --- a/edb/server/protocol/auth_ext/webauthn.py +++ b/edb/server/protocol/auth_ext/webauthn.py @@ -390,7 +390,7 @@ async def _get_authentication_challenge( created_at, modified_at, challenge, - factors: { + factors := (distinct (.factors union .factor)) { id, created_at, modified_at, From 8fe06a2d61660f25e7ea7c6881666dfcf54d7a54 Mon Sep 17 00:00:00 2001 From: Scott Trinh Date: Fri, 1 Mar 2024 14:10:08 -0500 Subject: [PATCH 3/5] Just remove the vestigial link --- edb/lib/ext/auth.edgeql | 3 --- edb/server/protocol/auth_ext/webauthn.py | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/edb/lib/ext/auth.edgeql b/edb/lib/ext/auth.edgeql index 826ea8da3cb..8d69ce6f1d9 100644 --- a/edb/lib/ext/auth.edgeql +++ b/edb/lib/ext/auth.edgeql @@ -111,9 +111,6 @@ CREATE EXTENSION PACKAGE auth VERSION '1.0' { create required property challenge: std::bytes { create constraint exclusive; }; - create required link factor: ext::auth::WebAuthnFactor { - create constraint exclusive; - }; create required multi link factors: ext::auth::WebAuthnFactor { create constraint exclusive; }; diff --git a/edb/server/protocol/auth_ext/webauthn.py b/edb/server/protocol/auth_ext/webauthn.py index 4d0f873b4f9..a50bd69a675 100644 --- a/edb/server/protocol/auth_ext/webauthn.py +++ b/edb/server/protocol/auth_ext/webauthn.py @@ -390,7 +390,7 @@ async def _get_authentication_challenge( created_at, modified_at, challenge, - factors := (distinct (.factors union .factor)) { + factors: { id, created_at, modified_at, From bb83facdb5e86d9919abbb98a2d7e25ffd2539ab Mon Sep 17 00:00:00 2001 From: Scott Trinh Date: Fri, 1 Mar 2024 14:14:54 -0500 Subject: [PATCH 4/5] Also remove the `factor` when inserting Challenge Co-authored-by: James Clarke --- edb/server/protocol/auth_ext/webauthn.py | 1 - 1 file changed, 1 deletion(-) diff --git a/edb/server/protocol/auth_ext/webauthn.py b/edb/server/protocol/auth_ext/webauthn.py index a50bd69a675..78737f4a3ee 100644 --- a/edb/server/protocol/auth_ext/webauthn.py +++ b/edb/server/protocol/auth_ext/webauthn.py @@ -324,7 +324,6 @@ async def create_authentication_options_for_email( ) insert ext::auth::WebAuthnAuthenticationChallenge { challenge := challenge, - factor := assert_exists(assert_single((select factors limit 1))), factors := factors, } unless conflict on .factors From 11867d640a967dfb5523d54145651e3c221f165e Mon Sep 17 00:00:00 2001 From: Scott Trinh Date: Fri, 1 Mar 2024 15:52:29 -0500 Subject: [PATCH 5/5] Update test query for removing `.factor` --- tests/test_http_ext_auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_http_ext_auth.py b/tests/test_http_ext_auth.py index defe9a81707..929e6460f99 100644 --- a/tests/test_http_ext_auth.py +++ b/tests/test_http_ext_auth.py @@ -3807,8 +3807,8 @@ async def test_http_auth_ext_webauthn_authenticate_options(self): SELECT EXISTS ( SELECT ext::auth::WebAuthnAuthenticationChallenge filter .challenge = $challenge - AND .factor.email = $email - AND .factor.user_handle = $user_handle + AND .factors.email = $email + AND .factors.user_handle = $user_handle ) ''', challenge=challenge_bytes,