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.

Sign up for GitHub

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

Jump to bottom

Introduce DiscoverFromExternalSource activation + visibility requirements #129

Merged
merged 6 commits into from
Jul 16, 2024
Merged

Introduce DiscoverFromExternalSource activation + visibility requirements #129

merged 6 commits into from
Jul 16, 2024

Conversation

marcoscaceres
Copy link
Collaborator

@marcoscaceres marcoscaceres commented Jun 21, 2024 •
edited by pr-preview bot
Loading

Closes #91

The following tasks have been completed:

  • Modified Web platform tests (link)

Implementation commitment:

  • WebKit - patch
  • Chromium (link to issue)
  • Gecko (link to issue)

Documentation and checks

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

Preview | Diff

marcoscaceres
marcoscaceres commented Jun 21, 2024
View reviewed changes
index.html Outdated Show resolved Hide resolved
samuelgoto
samuelgoto reviewed Jun 21, 2024
View reviewed changes
index.html Outdated Show resolved Hide resolved
marcoscaceres
marcoscaceres commented Jun 22, 2024
View reviewed changes
index.html Outdated Show resolved Hide resolved
marcoscaceres
marcoscaceres commented Jun 22, 2024
View reviewed changes
index.html Outdated Show resolved Hide resolved
@marcoscaceres marcoscaceres mentioned this pull request Jun 22, 2024
Merged
8 tasks
@marcoscaceres
Copy link
Collaborator Author

WebKit tracking bug https://bugs.webkit.org/show_bug.cgi?id=275782

samuelgoto
samuelgoto approved these changes Jun 24, 2024
View reviewed changes
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

explainer.md
async function requestLicense() {
const oid4pv = {
// Protocol extensibility:
protocol: "oid4vp", // An example of an OpenID4VP request to wallets. // Based on https://github.com/openid/OpenID4VP/issues/125
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
protocol: "oid4vp", // An example of an OpenID4VP request to wallets. // Based on https://github.com/openid/OpenID4VP/issues/125
protocol: "urn:openid.net:oid4vp", // An example of an OpenID4VP request to wallets. // Based on https://github.com/openid/OpenID4VP/issues/125

That's the value they are using here: https://github.com/openid/OpenID4VP/pull/155/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll see if I can get them to drop that.... using a URN doesn't seem appropriate as otherwise we should parse these as URLs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought that we were deliberate in making protocol a DOMString, so that we wouldn't have to form an opinion on what these were.

Should we care if this is a URN or not? Or are you suggesting that we reject any DOMString that isn't a URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

def should care if it's an URN... because URNs are URLs, and hence it implies URL parsing. We just don't want to get into a world where people are expecting a URL where we are using a DOMString.

@TallTed
Copy link
Contributor

TallTed commented Jul 1, 2024

I suggest correcting the typo in the title, from visility to visibility

marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Jul 4, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
bac780d 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by NOBODY (OOPS!).

Adds visibility and focus checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
@marcoscaceres marcoscaceres mentioned this pull request Jul 4, 2024
Merged
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Jul 4, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
3d8cd34 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by NOBODY (OOPS!).

Adds visibility, focus, and user activation checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
@marcoscaceres
Copy link
Collaborator Author

Just sent a PR to WebKit - WebKit/WebKit#30499

I might send separate tests to WPT for this... The focus and visibility requirements can be tricky to tests.

@marcoscaceres
Copy link
Collaborator Author

Just noting that this is blocked on two things right now...

  1. tests
  2. The steps we are hooking into run in parallel, which makes what we have specified not quite correct (as written, this assumes we are running in the main thread). I've spun up an issue for the on Cred Man: Consider not running internal methods in parallel or add "consumes user activation" to registry? w3c/webappsec-credential-management#243

@marcoscaceres
Copy link
Collaborator Author

We can monkey patch for now though.

marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Jul 11, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
648fb1e 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by NOBODY (OOPS!).

Adds visibility, focus, and user activation checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:
(WebCore::CredentialsContainer::scopeAndCrossOriginParent const):
(WebCore::CredentialsContainer::get):
(WebCore::CredentialsContainer::isCreate):
(WebCore::CredentialsContainer::preventSilentAccess const):
(WebCore::CredentialsContainer::performCommonChecks):
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
(WebCore::CredentialsContainer::document const):
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Jul 15, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
427fd57 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by NOBODY (OOPS!).

Adds visibility, focus, and user activation checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:
(WebCore::CredentialsContainer::scopeAndCrossOriginParent const):
(WebCore::CredentialsContainer::get):
(WebCore::CredentialsContainer::isCreate):
(WebCore::CredentialsContainer::preventSilentAccess const):
(WebCore::CredentialsContainer::performCommonChecks):
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
(WebCore::CredentialsContainer::document const):
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
@marcoscaceres marcoscaceres changed the title Introduce DiscoverFromExternalSource activation + visility requirements Introduce DiscoverFromExternalSource activation + visibility requirements Jul 15, 2024
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Jul 15, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
437862d 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by NOBODY (OOPS!).

Adds visibility, focus, and user activation checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:
(WebCore::CredentialsContainer::scopeAndCrossOriginParent const):
(WebCore::CredentialsContainer::get):
(WebCore::CredentialsContainer::isCreate):
(WebCore::CredentialsContainer::preventSilentAccess const):
(WebCore::CredentialsContainer::performCommonChecks):
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
(WebCore::CredentialsContainer::document const):
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Jul 15, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
8f799de 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by NOBODY (OOPS!).

Adds visibility, focus, and user activation checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:
(WebCore::CredentialsContainer::scopeAndCrossOriginParent const):
(WebCore::CredentialsContainer::get):
(WebCore::CredentialsContainer::isCreate):
(WebCore::CredentialsContainer::preventSilentAccess const):
(WebCore::CredentialsContainer::performCommonChecks):
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
(WebCore::CredentialsContainer::document const):
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
marcoscaceres added a commit to marcoscaceres/WebKit that referenced this pull request Jul 16, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
bba3f1a 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by NOBODY (OOPS!).

Adds visibility, focus, and user activation checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:
(WebCore::CredentialsContainer::scopeAndCrossOriginParent const):
(WebCore::CredentialsContainer::get):
(WebCore::CredentialsContainer::isCreate):
(WebCore::CredentialsContainer::preventSilentAccess const):
(WebCore::CredentialsContainer::performCommonChecks):
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
(WebCore::CredentialsContainer::document const):
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):
webkit-commit-queue pushed a commit to marcoscaceres/WebKit that referenced this pull request Jul 16, 2024
@marcoscaceres
Digital Credentials: implement visibility and focus requirements
aadfa72 
https://bugs.webkit.org/show_bug.cgi?id=275782
rdar://130821648

Reviewed by Andy Estes.

Adds visibility, focus, and user activation checks as per:
See WICG/digital-credentials#129

* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-basics.https.html:
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https-expected.txt: Added.
* LayoutTests/http/wpt/identity/identitycredentialscontainer-get-hidden.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/digital-credentials/get-user-activation.https.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:
(WebCore::CredentialsContainer::scopeAndCrossOriginParent const):
(WebCore::CredentialsContainer::get):
(WebCore::CredentialsContainer::isCreate):
(WebCore::CredentialsContainer::preventSilentAccess const):
(WebCore::CredentialsContainer::performCommonChecks):
* Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:
(WebCore::CredentialsContainer::document const):
* Source/WebCore/Modules/identity/IdentityCredentialsContainer.cpp:
(WebCore::IdentityCredentialsContainer::get):

Canonical link: https://commits.webkit.org/281003@main
@marcoscaceres marcoscaceres merged commit e0f3696 into main Jul 16, 2024
1 check passed
@marcoscaceres marcoscaceres deleted the user_activation branch July 16, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelgoto samuelgoto samuelgoto approved these changes

@bvandersloot-mozilla bvandersloot-mozilla bvandersloot-mozilla approved these changes

@RByers RByers RByers approved these changes

@timcappalli timcappalli Awaiting requested review from timcappalli timcappalli is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consume transient activation
5 participants
@marcoscaceres @TallTed @samuelgoto @RByers @bvandersloot-mozilla