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

change providers to requests #165

Merged
merged 8 commits into from
Sep 18, 2024
Merged

change providers to requests #165

merged 8 commits into from
Sep 18, 2024

Conversation

marcoscaceres
Copy link
Collaborator

@marcoscaceres marcoscaceres commented Sep 4, 2024

Closes #155

The following tasks have been completed:

Implementation commitment:

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

Documentation and checks

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

Preview | Diff

@samuelgoto
Copy link
Collaborator

samuelgoto commented Sep 4, 2024

Something that occurred to me that feels a bit off is that we'd have a requests and request in the API.

Here:

await navigator.credentials.get({
  digital: {
     requests: [{      <-----------
       protocol: "openid4vp",
       request: {       <----------- is this weird?
         nonce: "foo",
         client_id_metadata: "bar",
         // ... more stuff
       },
     }]
  }
});

It is not terrible, but felt like worth noting.

WDYT?

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Sep 5, 2024

Yeah, I spotted that too... I can live with it though.

How about query: for the internal one?

@marcoscaceres
Copy link
Collaborator Author

Some alternatives:

  • presentationRequests and presentationDetails
  • presentmentRequests and requestDetails

@bc-pi
Copy link

bc-pi commented Sep 9, 2024

To add to the a bit odd but not terrible observation, due to the layering and reuse of prior RFCs, the top level thing[1] of a request in a signed openid4vp request is also a request. See not permanent content at https://openid.github.io/OpenID4VP/openid-4-verifiable-presentations-wg-draft.html#appendix-A.3.2-3 but that would mean something like the below with requests: ... request: ... request: "..." which is pretty special.

await navigator.identity.get({
  digital: {
    requests: [{
      protocol: "openid4vp",
      request: {
        request: "eyJhbGciOiJF..."
     }
    }]
  }
});

[1] I can't even keep track of what things at what layer are called anymore

@bc-pi
Copy link

bc-pi commented Sep 9, 2024

Can someone remind me what the rational for the requests: [...] array under digital rather than a single request?

@bc-pi
Copy link

bc-pi commented Sep 9, 2024

Can someone remind me what the rational for the requests: [...] array under digital rather than a single request?

The layers and potential options for various cardinalities is making my little mind hurt. Kinda related is openid/OpenID4VP#248 not to mention that whatever query language(s) in openid4vp can themselves express asking for multiple things and/or combinatorial options.

@samuelgoto
Copy link
Collaborator

Can someone remind me what the rational for the requests: [...] array under digital rather than a single request?

Yeah, @bc-pi , that's one of the things that I think we may be worth revisiting: I'm not quite sure anymore we need the level of indirection of providers.

To answer your question, originally, providers was introduced because we thought that there would be more than 1 protocol used (e.g. the same request using OpenID4VP, VC API, something coming from ISO, etc) and that wallets would accept "one or the other". A similar level of indirection that we introduced for formats (e.g. mDocs and SD-JWTs).

At this point, I think it is not clear to me that we would run into this situation: when a verifier makes a request, it doesn't have to connect with a variety of wallets that support different protocols.

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Sep 12, 2024

I wouldn't mind dropping .requests entirely to be honest. The OS might not support requesting multiple presentment requests, so we would just pick the first known type that matches. Otherwise we get into a world of pain about prioritization - which one wins? Why? etc. That's should be a decision the developer makes.

We should then add a. static setlike.supportedProtocols... that's probably better), or some such. Otherwise developers won't know if calling .get() will immediately fail (as it requires user activation).

With the sequence, a developer can easily filter with standard Set methods.

Proposed API addition

interface DigitalCredentialsSupportedProtocols {
  readonly setlike<DOMString>; // user agent pre-populates
};

partial interface DigitalCredential {
   readonly attribute DigitalCredentialsSupportedProtocols supportedProtocols;
}

Usage:

if (DigitalCredential.supportedProtocols.has("openid4vp")) {
  // let's make an openid4vp request
}

// Or you can do...
const supported = Array.from(DigitalCredential.supportedProtocols).filter(typesTheRPSupports);

// Or whatever developers want:
for (const supported in DigitalCredential.supportedProtocols.values()) {
   // do things with supported
}

const requests = [...DigitalCredential.supportedProtocols].map(toRequestWeKnowHowToMake);

// or even... 
switch (true) {
   case DigitalCredential.supportedProtocols.has("openid4vp"):
      makeOpenIDRequest(data);
      break;
   case DigitalCredential.supportedProtocols.has("whatever"):
      makeWhateverRequest(data);
      break;
   default:
     throw TypeError("Oh noes! they don't support our favorite protocol!")
}

That gives a ton of flexibility. You can even use it will all the new fancy JavaScript set comparison operations:

// DigitalCredential.supportedProtocols is a Set
const supportedProtocols = DigitalCredential.supportedProtocols;

// Example Set of protocols to compare
const protocolsToCheck = new Set(['openid4vp', 'someOtherProtocol']);

// Union: Combining both sets
const union = supportedProtocols.union(protocolsToCheck);
console.log(union); // Set { 'openid4vp', 'someOtherProtocol', ... }

// Intersection: Getting common protocols between the sets
const intersection = supportedProtocols.intersection(protocolsToCheck);
console.log(intersection); // Set { 'openid4vp' }

// Difference: Getting protocols supported by DigitalCredential but not in protocolsToCheck
const difference = supportedProtocols.difference(protocolsToCheck);
console.log(difference); // Set { ... } (protocols supported by DigitalCredential but not in protocolsToCheck)

// Symmetric Difference: Getting protocols that are in either set, but not in both
const symmetricDifference = supportedProtocols.symmetricDifference(protocolsToCheck);
console.log(symmetricDifference); // Set { 'someOtherProtocol', ... }

@msporny
Copy link
Contributor

msporny commented Sep 12, 2024

@samuelgoto wrote:

To answer your question, originally, providers was introduced because we thought that there would be more than 1 protocol used (e.g. the same request using OpenID4VP, VC API, something coming from ISO, etc) and that wallets would accept "one or the other". A similar level of indirection that we introduced for formats (e.g. mDocs and SD-JWTs).

Hmm, no, we definitely still have that requirement that a Verifier might ask for the same sort of thing over multiple protocols (because it won't know what the wallet supports when it asks). For example, in CA DMV, we might have a future where mDoc is requested over a different protocol (or protocol version) than other VCs. We cannot presume that there will only be one protocol.

To be clear, it sounds like @marcoscaceres proposal above would work just fine.

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Sep 12, 2024

Right, @msporny, but the future format would need to be known to the browser, so "mdoc" would need to be a registered protocol (i.e., you can't sent up anything as that would expose what is installed on the device, so for obvious user privacy and security reasons you can't send "wallet-custom-format"). The developer can check and do the filtering on their most preferred standardized type there.

@marcoscaceres
Copy link
Collaborator Author

Moved the proposal here: #168

@msporny
Copy link
Contributor

msporny commented Sep 13, 2024

The developer can check and do the filtering on their most preferred standardized type there.

Yes, agreed. I wasn't making an argument against having a set of standardized protocols or standardized formats.

It sounded like @samuelgoto was saying: "We don't need to contemplate a set of standardized protocols that a verifier might use to request a credential, because there will only ever be one that a verifier will use to request a credential." ... and that's not true today and is unlikely to be true in the future (as much as we'd all like that to be true from an interoperability and implementation complexity standpoint).

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Sep 13, 2024

Ok, talked with @samuelgoto and he also convinced me that we should keep the array.

for the member names, how about we do { requests: [ { protocol: "openid4vp", data: {} }] }.

So requests and data.

@bc-pi
Copy link

bc-pi commented Sep 13, 2024

Can someone remind me what the rational for the requests: [...] array under digital rather than a single request?

Yeah, @bc-pi , that's one of the things that I think we may be worth revisiting: I'm not quite sure anymore we need the level of indirection of providers.

To answer your question, originally, providers was introduced because we thought that there would be more than 1 protocol used (e.g. the same request using OpenID4VP, VC API, something coming from ISO, etc) and that wallets would accept "one or the other". A similar level of indirection that we introduced for formats (e.g. mDocs and SD-JWTs).

At this point, I think it is not clear to me that we would run into this situation: when a verifier makes a request, it doesn't have to connect with a variety of wallets that support different protocols.

Thanks for backstory @samuelgoto. My general intuition is that there are too many levels of indirection in all the various layers of all this. But that's easy to say and not so easy to clearly say what should be removed.

@bc-pi
Copy link

bc-pi commented Sep 13, 2024

Ok, talked with @samuelgoto and he also convinced me that we should keep the array.

for the member names, how about we do { requests: [ { protocol: "openid4vp", data: {} }] }.

So requests and data.

I like requests and data, thanks @marcoscaceres!

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

This is a backwards incompatible change, so we are going to need to update the OpenID4VP spec as well as verifiers. It is still possible, but we shouldn't take this lightly.

Can we run this by the community group to make sure that we are on all board with this trade-off?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
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

This is a backwards incompatible change, but a good one in case we still have time. Lets run this by the CG before merging.

@samuelgoto
Copy link
Collaborator

Just for transparency, the other alternatives that we considered for data was payload, params and body, but data seemed best.

@timcappalli
Copy link
Member

2024-09-18 call: good to merge once we have links

@marcoscaceres
Copy link
Collaborator Author

Updated the tests #165

@marcoscaceres
Copy link
Collaborator Author

Filed issues on WebKit side...

@marcoscaceres
Copy link
Collaborator Author

Going ahead and merging, as we have tests and implementation commitments and it's mostly a cosmetic change.

@marcoscaceres marcoscaceres merged commit 1b4cfeb into main Sep 18, 2024
1 check passed
@marcoscaceres marcoscaceres deleted the providers branch September 18, 2024 23:23
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.

Rename "providers" to "presentationRequests" or just "requests"?
5 participants