-
Notifications
You must be signed in to change notification settings - Fork 39
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
Disallow multiple in-progress credential requests #207
Conversation
index.bs
Outdated
1. [=map/For each=] |optionKey| → <var ignore>optionValue</var> of |options|: | ||
|
||
1. If |topLevelBrowsingContext|'s [=active credential types=] [=set/contains=] |optionKey|, | ||
[=reject=] |p| with null and return |p|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure what we should do (probably not reject p with null?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably an error, see https://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors
TypeError
seems to be the most generic, or else see https://webidl.spec.whatwg.org/#dfn-error-names-table
* Add permissions policy check for FedCM Companion change to w3c-fedid/FedCM#236
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to disable autoremoval of blank spaces before end of a line...
It looks like this PR needs to be rebased, there are a bunch of unrelated changes (like the introduction of permissions policy for fedcm). |
What do you mean? https://github.com/npm1/webappsec-credential-management/tree/repeated says I'm already ahead of w3c:main branch. I skimmed the diff and didn't see the introduction of permissions policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
1. For each |interface| in |options|' <a>relevant credential interface objects</a>: | ||
|
||
1. If |settings|' [=active credential types=] [=set/contains=] |interface|'s | ||
{{Credential/[[type]]}}, return [=a promise rejected with=] a "{{NotAllowedError}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the PR but can you file a crbug for this & to have a corresponding WPT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHA: 976f91b Reason: push, by @nsatragno Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #206
Preview | Diff