feat: passkeys, myaccount with dpop support#116
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
rmad17
changed the title
| raise IssuerValidationError( | ||
| "ID token issuer mismatch. Ensure your Auth0 domain is configured correctly." | ||
| ) | ||
| user_claims = UserClaims.model_validate(claims) |
There was a problem hiding this comment.
We forward organization to the passkey grant above, but here we decode the ID token and never validate the org claim. The complete_interactive_login path in this same PR calls validate_org_claims(claims, expected_org) for exactly this reason, and the README says org claim validation is enforced automatically at callback.
As it stands, a passkey login pinned to an organization does not actually verify the returned token belongs to that org. In a multi org tenant that is an authorization gap. Can we add a validate_org_claims call here when organization was passed, and a test that asserts a mismatch raises OrganizationTokenValidationError and no session is stored ?
| if connection: | ||
| body["realm"] = connection | ||
| if organization: | ||
| body["organization"] = organization |
There was a problem hiding this comment.
The constructor stores self._organization and start_interactive_login uses options.organization or self._organization. The passkey methods only read the local organization argument, so the client level default is silently ignored here.
This means a client configured with a default org gets it applied to interactive logins but not to passkey logins, which is surprising. Can we apply the self._organization fallback here too so the behavior is consistent ?
| ): | ||
| nonce = response.headers["DPoP-Nonce"] | ||
| request.headers["DPoP"] = self._make_proof(request.method, str(request.url), nonce=nonce) | ||
| yield request |
There was a problem hiding this comment.
The nonce retry re-yields the same request object after it was already sent once. For the MyAccount calls that POST or PATCH a JSON body, I want to make sure the body is actually re-sent on the retry and not consumed.
The token endpoint path in signin_with_passkey sidesteps this by rebuilding the request with a fresh client.post(...), but here we reuse the instance. On tenants that require a nonce the first call always 401s and retries, so this path runs every session. Could we add a test that drives a real bodied POST through DPoPAuth against a mock that returns 401 then 200, and asserts the second request still carries the original body ? If httpx does not re-stream it, we should rebuild the request instead.
| audience=audience or self.DEFAULT_AUDIENCE_STATE_KEY, | ||
| access_token=token_response.access_token, | ||
| scope=token_response.scope or scope or "", | ||
| expires_at=token_response.expires_at if token_response.expires_at is not None else int(time.time()) + token_response.expires_in, |
There was a problem hiding this comment.
Small one. expires_in is a required int on PasskeyTokenResponse, so by the time we get here token_response.expires_in is always an int and the earlier block already backfilled expires_at. The else int(time.time()) + token_response.expires_in branch cannot really be reached with a None, so this reads as dead defensive code.
If a server that returns only expires_at without expires_in is a real case, the model would reject it before this line anyway. Can we either make expires_in optional and keep one fallback, or drop the redundant branch ?
| authorization_params: Additional OAuth parameters (optional) | ||
| """ | ||
|
|
||
| subject_token: str |
There was a problem hiding this comment.
This drops the field_validator and model_validator (Bearer prefix, whitespace, actor_token_type) and the min_length=1 constraint, and the checks were moved inline into custom_token_exchange. That means anyone building this model directly no longer gets validation at construction, and it only fires inside that one method.
Can we confirm login_with_custom_token_exchange still enforces the same rules on its own path ? If there was a specific reason for moving these out of the model (for example a Pydantic v2 change), it would help to note it in the PR description. Otherwise keeping them on the model is entry point agnostic and avoids duplicating the logic.
There was a problem hiding this comment.
Will raise a seperate PR for CTE related fix.
There was a problem hiding this comment.
Can we confirm login_with_custom_token_exchange still enforces the same rules on its own path ?
@nandan-bhat Yes, it does.
| PasskeyLoginChallengeResponse with auth_session and authn_params_public_key. | ||
|
|
||
| Raises: | ||
| ApiError: If the challenge request fails. |
There was a problem hiding this comment.
The Raises section says ApiError, but the method body raises PasskeyError on a failed challenge. passkey_signup_challenge documents PasskeyError correctly. Can we fix this to PasskeyError ?
|
|
||
| For the full enroll/verify ceremony, listing, updating, deleting, and error handling, see [examples/MyAccountAuthenticationMethods.md](examples/MyAccountAuthenticationMethods.md). | ||
|
|
||
| ### 8. DPoP — Sender-Constrained Tokens |
There was a problem hiding this comment.
We should explicitly mention that this is with passkeys in the heading so that the users aren't confused or exclusively keep this section in Passkeys.md?
| 2. **Browser** — your front end passes those options to `navigator.credentials.create()` (signup) or `navigator.credentials.get()` (login). The authenticator produces a signed credential. | ||
| 3. **Verify / sign-in** — the SDK exchanges the signed credential for tokens (`signin_with_passkey`) and **creates a server-side session**, exactly like every other login path. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
This diagram can be dropped.
| This is step 2 of 2: call passkey_signup_challenge or passkey_login_challenge | ||
| first to obtain auth_session and the WebAuthn challenge options. | ||
|
|
||
| Uses Content-Type: application/json (required for nested authn_response). | ||
| Persists the session to the state store (same as complete_interactive_login). |
There was a problem hiding this comment.
We can drop this section of the docstring to make it consistent with other functions
| mock_response.json = MagicMock(return_value=_PASSKEY_TOKEN_RESPONSE) | ||
| mock_post.return_value = mock_response | ||
|
|
||
| with pytest.raises(Exception) as exc: |
There was a problem hiding this comment.
This catches broad Exception and only checks that "issuer" shows up in the message, though the docstring says it must raise IssuerValidationError. It would pass on an unrelated KeyError/AttributeError whose message happens to mention issuer. pytest.raises(IssuerValidationError) would pin the actual contract.
A couple of the passkey error tests nearby also lean on assert X in str(exc) or Y in str(exc), asserting on exc.value.code instead would keep them from passing under the wrong failure mode.
| data["expires_at"] = int(time.time()) + int(data["expires_in"]) | ||
| return data | ||
|
|
||
| def __repr__(self) -> str: |
There was a problem hiding this comment.
Redaction was added here on the new models, but TokenSet and MfaVerifyResponse in this same file hold access_token / refresh_token / id_token with no __repr__ redaction. Having it on only the new models leaves the surface half-covered — worth deciding this one way project-wide.
There was a problem hiding this comment.
We cannot achieve it project wide in one go. Isn;t it a good idea to start and improve the other models too as we go forward. Its not like these are interdependent.
| request.headers["DPoP"] = self._make_proof(request.method, str(request.url), nonce=nonce) | ||
| yield request | ||
|
|
||
| def _make_proof(self, method: str, url: str, nonce: str = None) -> str: |
There was a problem hiding this comment.
nonce: str = None — the annotation says str but the default is None; Optional[str] matches the style used elsewhere in this file. Same on make_dpop_proof_for_token_endpoint above.
| authorization_params: Additional OAuth parameters (optional) | ||
| """ | ||
|
|
||
| subject_token: str |
There was a problem hiding this comment.
Can we confirm login_with_custom_token_exchange still enforces the same rules on its own path ?
@nandan-bhat Yes, it does.
| # A genuinely absent/optional act claim or a benign decode | ||
| # gap leaves act None. Anything outside these types (an | ||
| # unexpected verify failure) surfaces rather than being masked. |
There was a problem hiding this comment.
We can trim down this comment to a crisp version.
| scope=merged_scope or "", | ||
| default_description="MFA required", | ||
| ) | ||
| error.mfa_token = error_data.get("mfa_token") |
There was a problem hiding this comment.
This seems to be like a major strip down. What is the intention or alternate flow to achieve this?
| # Check for mfa_required error from token refresh | ||
| if isinstance(e, ApiError) and e.code == "mfa_required": | ||
| raw_mfa_token = getattr(e, "mfa_token", None) | ||
| mfa_requirements = getattr(e, "mfa_requirements", None) | ||
|
|
||
| if raw_mfa_token: | ||
| encrypted_token = self._mfa_client._encrypt_mfa_token( | ||
| raw_mfa_token=raw_mfa_token, | ||
| audience=audience or self.DEFAULT_AUDIENCE_STATE_KEY, | ||
| scope=merged_scope or "", | ||
| mfa_requirements=mfa_requirements | ||
| ) | ||
| raise MfaRequiredError( | ||
| "Multifactor authentication required", | ||
| mfa_token=encrypted_token, | ||
| mfa_requirements=mfa_requirements | ||
| ) | ||
|
|
There was a problem hiding this comment.
This seems to be like a major strip down. What is the intention or alternate flow to achieve this?
| ```python | ||
| from auth0_server_python.auth_server.my_account_client import MyAccountClient | ||
| from auth0_server_python.auth_types import EnrollAuthenticationMethodRequest | ||
|
|
||
| # Obtain a My Account-scoped token for the current session (MRRT) | ||
| access_token = await auth0.get_access_token( | ||
| store_options={"request": request, "response": response}, | ||
| audience=f"https://{YOUR_CUSTOM_DOMAIN}/me/", | ||
| scope="create:me:authentication-methods read:me:authentication-methods", | ||
| ) | ||
|
|
||
| my_account = MyAccountClient(domain=YOUR_CUSTOM_DOMAIN) | ||
|
|
||
| # Start enrolling a passkey (then sign it in the browser and verify) | ||
| challenge = await my_account.enroll_authentication_method( | ||
| access_token=access_token, | ||
| request=EnrollAuthenticationMethodRequest(type="passkey"), | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Is the same code snippet which is already there in the .md file. If yes, please omit it from here as it will be a dupllication
|
|
||
| ```python | ||
| from auth0_server_python.auth_types import PasskeyUserProfile, PasskeyAuthResponse | ||
|
|
||
| # Step 1 — request a challenge | ||
| challenge = await auth0.passkey_login_challenge( | ||
| store_options={"request": request, "response": response} | ||
| ) | ||
|
|
||
| # Step 2 — browser signs: navigator.credentials.get(challenge.authn_params_public_key) | ||
|
|
||
| # Step 3 — complete sign-in and establish the session | ||
| result = await auth0.signin_with_passkey( | ||
| auth_session=challenge.auth_session, | ||
| authn_response=PasskeyAuthResponse(**credential), | ||
| store_options={"request": request, "response": response} | ||
| ) | ||
|
|
||
| user = result.state_data["user"] | ||
| ``` |
|
|
||
| For more details and examples, see [examples/MultipleCustomDomains.md](examples/MultipleCustomDomains.md). | ||
|
|
||
| ### 6. Session Expiry from the Upstream IdP |
There was a problem hiding this comment.
Usually, we follow the sequence of releases to append the features to Readme. It's fine for now (beacuse session expiry is not a big feature as compared to Passkeys) but it's chronological and sync with the releases.
| user_verification: Optional[str] = Field(None, alias="userVerification") | ||
|
|
||
|
|
||
| EnrollmentType = Literal["passkey", "email", "phone", "totp", "push-notification", "recovery-code", "password"] |
There was a problem hiding this comment.
Why are we creating these variables here? It's used only once and can easily be integrated to the parameter?
There was a problem hiding this comment.
This is a cleaner approach. Any reuse of this in future will not require rework.
Will move it to the top of the file.
| auth_session: str | ||
| authn_params_public_key: PasskeyPublicKeyOptions | ||
|
|
||
| def __repr__(self) -> str: |
There was a problem hiding this comment.
Redaction code added which needs to be removed
There was a problem hiding this comment.
Will perform the redaction via Field param.
| auth_session: str | ||
| authn_params_public_key: Optional[PasskeyPublicKeyOptions] = None | ||
|
|
||
| def __repr__(self) -> str: |
There was a problem hiding this comment.
Another redaction code added, which need to be removed
There was a problem hiding this comment.
Redaction is a good security practice and we should use it. Do you see any problem with redaction?
The issue I see here is str(enrollmentChallengeResponse()) would still leak the secret as it will fallback to str of BaseModel. For that I will add Field(repr=False) in the attribute of the Schema.
| id_token: Optional[str] = None | ||
| refresh_token: Optional[str] = None | ||
|
|
||
| @model_validator(mode="before") |
There was a problem hiding this comment.
Every other token path in the SDK computes expires_at in the client method right after the HTTP response (see the int(time.time()) + expires_in spots in server_client.py). This is the only place that backfills it inside the model via a mode="before" validator. We should be consistent.
|
|
||
| ```python | ||
| from auth0_server_python.auth_types import PasskeyUserProfile, PasskeyAuthResponse | ||
|
|
||
| # Step 1 — request a challenge | ||
| challenge = await auth0.passkey_login_challenge( | ||
| store_options={"request": request, "response": response} | ||
| ) | ||
|
|
||
| # Step 2 — browser signs: navigator.credentials.get(challenge.authn_params_public_key) | ||
|
|
||
| # Step 3 — complete sign-in and establish the session | ||
| result = await auth0.signin_with_passkey( | ||
| auth_session=challenge.auth_session, | ||
| authn_response=PasskeyAuthResponse(**credential), | ||
| store_options={"request": request, "response": response} | ||
| ) | ||
|
|
||
| user = result.state_data["user"] | ||
| ``` |
| def __repr__(self) -> str: | ||
| return "DPoPAuth(token=[REDACTED], key=[REDACTED])" | ||
|
|
||
| def __str__(self) -> str: | ||
| return "DPoPAuth(token=[REDACTED], key=[REDACTED])" |
There was a problem hiding this comment.
We should drop this.
| # MFA STATE | ||
| # ============================================================================ | ||
|
|
||
| async def store_pending_mfa( |
There was a problem hiding this comment.
I can see this function used only once. Can we not include the function's logic there itself? Do we anticipate need of this helper function in future?
| options=store_options, | ||
| ) | ||
|
|
||
| async def get_pending_mfa( |
There was a problem hiding this comment.
I don't see this function called anywhere.Am I missing something?
| return data.get("mfa_token") | ||
| return None | ||
|
|
||
| async def _clear_pending_mfa( |
There was a problem hiding this comment.
I can see this function used only once. Can we not include the function's logic there itself? Do we anticipate need of this helper function in future?
| Encrypt the server-issued mfa_token and raise MfaRequiredError. | ||
|
|
||
| Shared by every site that handles an `mfa_required` response so the | ||
| encrypt-then-raise behaviour cannot drift between entry points. Returns | ||
| only when the response carries no mfa_token (caller then falls through | ||
| to its own typed error). | ||
|
|
||
| store_pending controls whether the encrypted token is persisted to the | ||
| state store before raising. It is an explicit argument so the difference | ||
| between entry points is visible: the passkey grant persists it here, |
There was a problem hiding this comment.
The docstring is verbose and doesn't follow the standard format of
- Crisp Description of the fucntion (what it does)
- Args
- Returns/Raises
| MfaEnrollmentError: When enrollment fails. | ||
| """ | ||
| mfa_token = options["mfa_token"] | ||
| context = self.decrypt_mfa_token(self._resolve_encrypted_token(options)) |
| MfaChallengeError: When the challenge fails. | ||
| """ | ||
| mfa_token = options["mfa_token"] | ||
| context = self.decrypt_mfa_token(self._resolve_encrypted_token(options)) |
| MfaRequiredError: When chained MFA is required. | ||
| """ | ||
| mfa_token = options["mfa_token"] | ||
| context = self.decrypt_mfa_token(self._resolve_encrypted_token(options)) |
| raise MfaEnrollmentError( | ||
| f"Unexpected error enrolling authenticator: {str(e)}" | ||
| ) | ||
| "Unexpected error enrolling authenticator" |
There was a problem hiding this comment.
Are we loosing the exact error message store in e here? Please check once
| return public_jwk | ||
|
|
||
|
|
||
| def make_dpop_proof_for_token_endpoint( |
There was a problem hiding this comment.
This is nearly a line-for-line duplicate of DPoPAuth._make_proof below
4 participants
Changes
Passkey Authentication (ServerClient)
MyAccount Credential Management (MyAccountClient)
References