WICG / web-otp

phone number verification
https://wicg.github.io/web-otp/
Other
94 stars 35 forks source link

OTPCredentialRequestOptions's transport definition is weird #31

Open domenic opened 4 years ago

domenic commented 4 years ago

This OPTIONAL member contains a hint as to how the server might receive the OTP.

It's unclear why a sequence of strings, which can only be "sms", would provide a hint. Maybe something more like "This member contains a list of hints as to how the server might receive the OTP"? But I don't know, it's still pretty confusing.

The values SHOULD be members of OTPCredentialTransportType but client platforms MUST ignore unknown values.

First, I don't know what a "client platform" is. Is it a user agent?

Second, this isn't how Web IDL works. Web IDL will automatically throw an exception if a different type is in the sequence.

I think this second sentence should just be deleted, leaving the type system stuff to Web IDL.

domenic commented 4 years ago

On the other hand, if the goal is to ignore unknown values to allow future extensibility, then probably you want to switch from an enum to a DOMString. Then the definition of the list of transport types would be just some kind of <ul> or <table>, instead of an IDL enumeration.

rmondello commented 4 years ago

I suspect you do want to switch from an enum to a DOMString, as suggested, for future extensibility.

majido commented 4 years ago

FWIW, I came across this as well and filed a Chromium crbug to fix this in Chromium.

As suggested earlier using enum will not achieve the intention. So I will be sending a PR to update the spec as well to switch the type to DOMString.

Dealing with invalid transport values

For what to do when given an unknown transport, the wording suggests we ignore them. So basically remove them from the array. So:

This seems like a good approach as it would allow authors to specify their preferred transports in order and the first one that is supported is used. Then an interesting question is whether we need a way for authors to figure out which transport was actually used?

Dealing with no transport value

Another related question is is what should happens if there is no transport provided i.e., []? The spec suggests transport is just a hint. So should User Agent:

A ) fall back to using its preferred transport B) do nothing and just resolve the promise immediately C) throw

C goes against the idea that transport is just a hint. B seems reasonable and simple but then again it suggests transport is more than just hint since without it we don't make any otp request. A seems like a good idea. But again it seems like we need to have a way fo authors to find out which transport was actually used.

majido commented 4 years ago

/cc @samuelgoto

samuelgoto commented 4 years ago

I have an intuition that you want to go with (c). What would a browser do if it go no transports to work with? Seems like API usage error to me.

majido commented 4 years ago

Throwing (i.e. rejecting the promise) is fine with me but then it is no longer a hint. I am assuming the future extensibility is still desired.

To validate my understanding here is that changes that will achieve this:

Here is the WebIDL:

dictionary OTPCredentialRequestOptions {
 // It is not "required" but uses DOMString
  required sequence<DOMString> transport;
};

Here is how it will work for authors:

try {
 let {code, type} = await navigator.credentials.get({
   otp: {
     transport: ["flux_capacitor", "sms"]
   }
 });

} catch(e) {
  // None of my transport worked, use fallback option to manually request. 
}

Given that the current implementation in Chrome (which is the only implementation at the moment) throws for empty or non-sms value, the proposed change above is backward-compatible as well.

samuelgoto commented 4 years ago

This approach looks generally good to me.

I just have a few clarification question on the extensibility model here. You say:

For extensibility we will allow transport to be any string (i.e., a DOMString not an enum).

And then use as an example:

let {code, type} = await navigator.credentials.get({
   otp: {
     transport: ["flux_capacitor", "sms"]
   }
 });

Question: what happens WHEN we add flux_capacitor to the enum? Wouldn't that make things backwards incompatible?