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

Add required mediation #149

Merged
merged 5 commits into from
Aug 27, 2024
Merged

Add required mediation #149

merged 5 commits into from
Aug 27, 2024

Conversation

marcoscaceres
Copy link
Collaborator

@marcoscaceres marcoscaceres commented Jul 30, 2024

Closes #147

The following tasks have been completed:

Implementation commitment:

Documentation and checks

  • Affects privacy
  • Affects security
  • Pinged MDN
  • Updated Explainer

Preview | Diff

Copy link

@bvandersloot-mozilla bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that "optional" would also be acceptable here. That would be the page telling the browser that it may show UI, which is all that is being validated, correct?

@marcoscaceres
Copy link
Collaborator Author

@bvandersloot-mozilla, right. I guess it depends on how you read the definition of "optional" from Cred Man:

optional: If credentials can be handed over for a given operation without user mediation, they will be. If user mediation is required, then the user agent will involve the user in the decision.

My reading from the above, and what I'm worried about, is that leaving it as "optional" invites optionality in presenting UI (which presuppose is always "required").

For completeness, here is how required is defined:

required: The user agent will not hand over credentials without user mediation, even if the prevent silent access flag is unset for an origin.

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Aug 5, 2024

So, I guess we could do things differently... we could state in the spec that "mediation is always required" for this kind of credential, but we need to fix w3c/webappsec-credential-management#248 (issue: user mediation is currently origin bound instead of type bound)... that could allow "optional", but enforce showing UI in prose and in Cred Man (possibly as part of the registry).

@bvandersloot-mozilla
Copy link

Yeah, that will work I think. It is just a little confusing because the mediation parameter prescribes the set of allowable behaviors for the manager and the credential types to take, rather than describing what will happen depending on state.

@marcoscaceres
Copy link
Collaborator Author

@samuelgoto, wouldn't mind your thoughts on this.

Copy link
Member

@RByers RByers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I like this. Our privacy stance requires user mediation for every presentment, so this seems like a good way to reinforce that.

@marcoscaceres
Copy link
Collaborator Author

Ok, so, yeah... per step 8.1 of of Cred Man:

If options.mediation is conditional and interface does not support conditional user mediation, return a promise rejected with a "TypeError" DOMException.

So, we actually just need to state that mediation is required but not as part of the algorithm... will fix.

@marcoscaceres
Copy link
Collaborator Author

@TallTed 🦅 👀 🙏? ... 😊

marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Aug 21, 2024
https://bugs.webkit.org/show_bug.cgi?id=277322
rdar://133266859

Reviewed by NOBODY (OOPS!).

Make sure mediation is alway required when getting digital credentials
as required by the spec:
WICG/digital-credentials#149

* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https.html: Added.
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, human-facing.

index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM++

I think that "optional" would also be acceptable here. That would be the page telling the browser that it may show UI, which is all that is being validated, correct?

I share the intuition that we may want to support optional too at some point, but I think we can cross the bridge of optional when we get there without cornering ourselves.

@marcoscaceres
Copy link
Collaborator Author

For "required" and "optional"... I think I have a better solution for all this:
w3c/webappsec-credential-management#256

We need fix the Cred Man API to allow us to set our own default.

@marcoscaceres
Copy link
Collaborator Author

@samuelgoto, @bvandersloot-mozilla, here's a first stab at fixing this in Cred Man:
w3c/webappsec-credential-management#258

Co-authored-by: Ted Thibodeau Jr <[email protected]>
@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Aug 23, 2024

For those watching at home, this would mean that the following would not throw:

navigator.identity.get({digital: ...}); // this throws today, which is annoying. 
navigator.identity.get({digital: ..., mediation: "required"});

This will continue to throw:

navigator.identity.get({digital: ..., mediation: "conditional"});
navigator.identity.get({digital: ..., mediation: "silent"});

@marcoscaceres
Copy link
Collaborator Author

(also, "optional" mentioned by a few folks above is meant to be "conditional"... yay, naming things if fun 😜)

marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Aug 23, 2024
https://bugs.webkit.org/show_bug.cgi?id=277322
rdar://133266859

Reviewed by NOBODY (OOPS!).

Make sure mediation is alway required when getting digital credentials
as required by the spec:
WICG/digital-credentials#149

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/allow-attribute.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/dc-types.ts:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/default-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/disabled-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/enabled-on-self-origin-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/support/helper.js:
(export.makeGetOptions):
* LayoutTests/imported/w3c/web-platform-tests/permissions-policy/resources/digital-credentials-get.html:
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
@samuelgoto
Copy link
Collaborator

(also, "optional" mentioned by a few folks above is meant to be "conditional"... yay, naming things if fun 😜)

I'm not sure I follow this reference, but I was actually discussion optional rather than conditional as:

https://w3c.github.io/webappsec-credential-management/#dom-credentialmediationrequirement-optional

@marcoscaceres
Copy link
Collaborator Author

Sorry @samuelgoto, I might have confused myself 🙈

Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM++

Just a few minor clarifications.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@samuelgoto
Copy link
Collaborator

(added chromium bug for implementation)

index.html Outdated Show resolved Hide resolved
@marcoscaceres marcoscaceres merged commit 84cbf75 into main Aug 27, 2024
1 check passed
@marcoscaceres marcoscaceres deleted the mediaton branch August 27, 2024 01:31
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Aug 29, 2024
https://bugs.webkit.org/show_bug.cgi?id=277322
rdar://133266859

Reviewed by NOBODY (OOPS!).

Make sure mediation is alway required when getting digital credentials
as required by the spec:
WICG/digital-credentials#149

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/allow-attribute.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/dc-types.ts:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/default-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/disabled-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/enabled-on-self-origin-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/support/helper.js:
(export.makeGetOptions):
* LayoutTests/imported/w3c/web-platform-tests/permissions-policy/resources/digital-credentials-get.html:
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Aug 29, 2024
https://bugs.webkit.org/show_bug.cgi?id=277322
rdar://133266859

Reviewed by NOBODY (OOPS!).

Make sure mediation is alway required when getting digital credentials
as required by the spec:
WICG/digital-credentials#149

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/allow-attribute.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/dc-types.ts:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/default-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/disabled-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/enabled-on-self-origin-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/support/helper.js:
(export.makeGetOptions):
* LayoutTests/imported/w3c/web-platform-tests/permissions-policy/resources/digital-credentials-get.html:
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Sep 4, 2024
https://bugs.webkit.org/show_bug.cgi?id=277322
rdar://133266859

Reviewed by NOBODY (OOPS!).

Make sure mediation is alway required when getting digital credentials
as required by the spec:
WICG/digital-credentials#149

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/allow-attribute.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/dc-types.ts:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/default-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/disabled-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/enabled-on-self-origin-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/support/helper.js:
(export.makeGetOptions):
* LayoutTests/imported/w3c/web-platform-tests/permissions-policy/resources/digital-credentials-get.html:
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
webkit-commit-queue pushed a commit to marcoscaceres/WebKit that referenced this pull request Sep 4, 2024
https://bugs.webkit.org/show_bug.cgi?id=277322
rdar://133266859

Reviewed by Anne van Kesteren.

Make sure mediation is alway required when getting digital credentials
as required by the spec:
WICG/digital-credentials#149

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/allow-attribute.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/dc-types.ts:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/default-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/disabled-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/enabled-on-self-origin-by-permissions-policy.https.sub.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https.html:
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/support/helper.js:
(export.makeGetOptions):
* LayoutTests/imported/w3c/web-platform-tests/permissions-policy/resources/digital-credentials-get.html:
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/digital-credentials/identity-get.tentative.https-expected.txt:
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):

Canonical link: https://commits.webkit.org/283148@main
@samuelgoto
Copy link
Collaborator

samuelgoto commented Sep 19, 2024

As we were implementing this PR here, something occurred to me while reviewing it:

(a) first, this is a backwards incompatible change for us: mediation is an optional field and it defaults to optional, so some developers have been using the default value (validly so).
(b) second, is excluding optional really required? meaning, isn't the website leaving the choice to the user agent when they choose optional? meaning, wouldn't a valid implementation of optional be to threat it as required?

If this is correct, then can we make this more flexible and also allow the default value of optional to be used (and allow each implementation to implement it in whichever way it see fit)?

@samuelgoto
Copy link
Collaborator

As we were implementing this PR here, something occurred to me while reviewing it:

FWIW, we found a resolution to this here:

#175 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Credential request options mediation requirement
7 participants