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

Handle is nil when allowCredentials is non-empty during discoverable login #171

Open
Daedaluz opened this issue Oct 18, 2023 · 3 comments
Open
Labels
status/needs-triage Issues that need to be triaged. type/documentation Improvements or additions to documentation type/feature-request Feature Requests

Comments

@Daedaluz
Copy link

Version

0.8.6

Description

Using phone as a passkey / credential seems to give a response without a handle duing a discoverable login with allowedCredentials set.

Currently there are checks that invalidates the response if the handle is empty.

Reading about user handles in w3c github;

Discoverable credentials store this identifier and MUST return it as response.userHandle in authentication ceremonies started with an empty allowCredentials argument.

This doesn't specifically say anything about what must and musnt be provided when starting authentication ceremonies with povided allowedCredentials, however, I take it that responses with empty handle are valid if the ceremony was started with given allowedCredentials?

Reproduction

Register a phone as a discoverable credential, then try login with the credential id listed in allowedCredentials.

Expectations

No response

Documentation

w3c github about user handles

@Daedaluz Daedaluz added status/needs-triage Issues that need to be triaged. type/potential-bug Potential Bugs labels Oct 18, 2023
@james-d-elliott
Copy link
Member

I think unless I'm mistaken this is purely a naming and documentation issue. If you knew the allowed credential ID's you'd know the identity of the user, thus could use the standard ValidateLogin. ValidateDiscoverableLogin would be more aptly named "ValidateUsernamelessLogin" maybe?

@Daedaluz
Copy link
Author

Daedaluz commented Oct 18, 2023

You are absolutely correct.

BeginLogin == BeginDiscoverableLogin (with allowed credentials and userid).

Not sure how i missed that.

I think it's probably best to leave the name as is.
It kind of make sense that ValidateDiscoverableLogin should validate a BeginDiscoverableLogin.

That said, is there a particular reason to why there are two distinct set of functions for this?
They both call the non-exported function beginLogin, but BeginLogin have a few checks that there are keys present for the given user.
I'm thinking a more generic function that only takes options, and have another option like WithUser or similar.
The check that's currently in BeginLogin could potentially be done in this generic function and only do the check
if a user is specified with the option.
Likewise, the validation could check the session data for a userID and use this over the one provided in the response if present.

But, perhaps doing this is just trying to make it too "generic" or introduce more confusion.
Perhaps this is a story for v2, if at all?

@james-d-elliott
Copy link
Member

james-d-elliott commented Oct 18, 2023

Those are very good points. The main reason was there was no clear way to perform a login without a user, and to validate the credential you need the hook-like function to call to load their credential(s) from the storage location, and this specific element is unique to usernameless logins.

I think we'd need two funcs to cater for this: WithUser and WithUserLookup or WithUsernameless.. maybe we can just do the WithUsernameless and allow the User to be nil though.

@james-d-elliott james-d-elliott added type/documentation Improvements or additions to documentation type/feature-request Feature Requests and removed type/potential-bug Potential Bugs labels Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/needs-triage Issues that need to be triaged. type/documentation Improvements or additions to documentation type/feature-request Feature Requests
Projects
None yet
Development

No branches or pull requests

2 participants