LibWeb: Implement most CredentialsContainer methods
#3495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Provide the default implementation for `is_conditional_mediation_available` and `will_request_conditional_creation`.
devgianlu
force-pushed
the
credentials-cont
branch
3 times, most recently
from
795803e to
243df11
Compare
The introduction of the abstract `CredentialInterface` class serves to implement what is mentioned in the spec as "interface objects". It is not possible to implement this perfectly to spec in Ladybird so let's make use of a custom abstraction.
This is probably a spec bug. If the property has a default value it will always be populated in the `CredentialRequestOptions` dictionary making that credential type always active.
devgianlu
force-pushed
the
credentials-cont
branch
from
243df11 to
483f268
Compare
konradekk
reviewed
Feb 9, 2025
| for (auto& interface : relevant_credential_interface_objects(options)) { | ||
| // 1. Let r be the result of executing interface’s [[CollectFromCredentialStore]](origin, options, sameOriginWithAncestors) | ||
| // internal method on origin, options, and sameOriginWithAncestors. If that threw an exception, rethrow that exception. | ||
| auto maybe_r = interface->collect_from_credential_store(realm, origin, options, same_origin_with_ancestors); |
There was a problem hiding this comment.
consider using possible_result instead of maybe_r?
| if (maybe_r.is_error()) | ||
| return maybe_r.error(); | ||
|
|
||
| auto r = maybe_r.release_value(); |
There was a problem hiding this comment.
consider using result instead of r?
| // TODO: 2. Assert: r is a list of interface objects. | ||
|
|
||
| // 3. For each c in r: | ||
| for (auto& c : r) { |
There was a problem hiding this comment.
consider using credential instead of c?
| // If that threw an exception: | ||
| if (maybe_result.is_error()) { | ||
| // 1. Let e be the thrown exception. | ||
| auto e = maybe_result.error_value(); |
There was a problem hiding this comment.
consider using exception instead of e?
| // 9. React to p: | ||
| auto on_completion = GC::create_function(realm().heap(), [&settings, &credential](JS::Value) -> WebIDL::ExceptionOr<JS::Value> { | ||
| // 1. Remove credential’s [[type]] from settings’ active credential types. | ||
| settings.active_credential_types().remove_first_matching([&](auto& v) { return v == credential.type(); }); |
There was a problem hiding this comment.
consider using type instead of v?
Comment on lines
428
to
444
| // 1. Let r be the result of executing interfaces[0]'s [[Create]](origin, options, sameOriginWithAncestors) | ||
| // internal method on origin, options, and sameOriginWithAncestors. | ||
| auto maybe_r = interfaces[0]->create(realm(), origin, options, same_origin_with_ancestors); | ||
| // If that threw an exception: | ||
| if (maybe_r.is_error()) { | ||
| // 1. Let e be the thrown exception. | ||
| auto e = maybe_r.error_value(); | ||
| // 2. Queue a task on global’s DOM manipulation task source to run the following substeps: | ||
| queue_global_task(HTML::Task::Source::DOMManipulation, global, GC::create_function(document.heap(), [&] { | ||
| // 1. Reject p with e. | ||
| WebIDL::reject_promise(realm(), *promise, e); | ||
| })); |
There was a problem hiding this comment.
same as above with more descriptive names
| return; | ||
| } | ||
|
|
||
| auto r = maybe_r.release_value(); |
devgianlu
force-pushed
the
credentials-cont
branch
from
483f268 to
94ff908
Compare
Implement `get`, `store`, `create` on `CredentialsContainer`. Some pieces that we don't support yet, like permissions and custom user interactions, are left as TODO. The `rejects when aborted after the promise creation` tests are broken because the spec says to check the aborted signal synchronously.
devgianlu
force-pushed
the
credentials-cont
branch
from
94ff908 to
3274cf3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR mplements
get,store,createonCredentialsContainer.Notably, it also introduces the
CredentialInterfaceabstract class to deal with credentials internal methods and metadata. It has been implemented as a singleton for each credential type. This is the design that I managed to come up with, any suggestion is appreciated.The spec isn't the best and contains some bugs:
options.signal && options.signal->aborted()check ingetandcreateis not deferred therefore fails two WPT testsinterfaces.size() < 1check increateis custom and avoids an out of bound access in the next step -> Spec issueCredentialRequestOptionsIDL dictionary has a default value forpasswordwhich renders the credential type always active. I think we are supposed to reject it when computing the relevant credential interface objects ifpasswordisfalse, but that's not specifiedI'll create some spec issues and link them here even if the repository has been inactive for the last 6 months.
Lastly, I am unsure how to check
If settings’s relevant global object has no associated Documentso I've left two TODOs.