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

How to define OtpCredential.id #11

Closed
ayuishii opened this issue Jan 31, 2020 · 7 comments
Closed

How to define OtpCredential.id #11

ayuishii opened this issue Jan 31, 2020 · 7 comments

Comments

@ayuishii
Copy link

This issue is to talk about how we should define credential.id. There has been a few opinions that have come up which I've listed below but other options/opinions are welcome as well.

Option 1

Defining credential.id as OTP. This defines id as the user identifier from the the code distributor perspective and also removes the need to have a redundant otp field within credentials.

navigator.credentials.get({otp: true}).then((credential) => {  
   // credential.type == "otp" 
   // credential.transport = "sms"
   // credential.id == "ABCD893443"
});

Option 2

Defining credential.id as undefined. However leaving the opportunity to let it be set to other user identifiers such as email addresses if in the future we add support to other transports like email OTP retrieval.

navigator.credentials.get({otp: true}).then((credential) => {  
   // credential.type == "otp" 
   // credential.transport = "sms"
   // credential.otp == "ABCD893443"  
   // credential.id == undefined | empty
});

/cc @samuelgoto @sso-google @mikewest
Also please correct me if I've misstated something.

@mikewest
Copy link
Member

I think I prefer option 1, wherein OTPCredential is an empty subclass of Credential, and the credential's identifier is the OTP code. An empty id feels strange. (That said, I grant that accessing an id attribute to get the code you want is also strange. :) )

If you wish to provide other identifiers to a developer, you're hopping right out of the realm of OTP and into a broader set of questions and challenges. I'd suggest that keeping this API conceptually scoped to OTP is going to be both simpler to understand for developers, and simpler to reason about for privacy and security reviewers. :)

I'm curious about the value of informing the developer about the transport over which the OTP was delivered. Won't they know how they went about sending the value to the user? Is there additional benefit to encoding that in the OTPCredential object as well?

@samuelgoto
Copy link
Collaborator

I'm curious about the value of informing the developer about the transport over which the OTP was delivered. Won't they know how they went about sending the value to the user?

I was thinking that, in the future, we want to allow this API to allow multiple transports to be used at once (e.g. "fetch an OTP via either of these transport mechanisms"), and return just one (e.g. via email), so that the client can know where it came from. Something along the lines of:

let {otp, transport} = navigator.credentials.get({otp: {transport: ["sms", "email"]}});

// transport == "email"

WDYT?

Is there additional benefit to encoding that in the OTPCredential object as well?

I was thinking feature detection. Can you help me understand how else it could be done otherwise?

if (window.OTPCredential) {
  // ah, i can call navigator.credentials.get({otp: ...})
} else {
  // ah, nevermind, i can't.
}

@mikewest
Copy link
Member

mikewest commented Feb 1, 2020

I was thinking that, in the future, we want to allow this API to allow multiple transports to be used at once (e.g. "fetch an OTP via either of these transport mechanisms"), and return just one (e.g. via email), so that the client can know where it came from

Why does knowing where it came from matter? This might be obvious, but I'm missing it. :) If I send an OTP to you via 18 mechanisms, and you return whichever of those shows up first to me correctly, do I need to know which one you're giving me? It seems like the developer only cares that the OTP showed up, not that it showed up via mechanism 17 vs 13.

let {otp, transport} = navigator.credentials.get({otp: {transport: ["sms", "email"]}});

For this destructuring usage, otp makes more sense than id. But I still don't think an empty ID makes sense. Perhaps we can have both, and simply alias one to the other?

I was thinking feature detection. Can you help me understand how else it could be done otherwise?

I think we're talking past each other. :) I absolutely think you want to have an OTPCredential interface. As you note (and as the spec notes in https://w3c.github.io/webappsec-credential-management/#implementation-authors), checking for the presence of that interface is a great way to feature-detect capabiilty.

I was questioning the value of the transport member on OTPCredential, as discussed briefly above.

ayuishii added a commit to ayuishii/sms-receiver that referenced this issue Feb 6, 2020
This updates includes the following changes to the API shape. 
- `credential.id` is now the OTP value (WICG#11)
- `otp` in navigator.credentials.get() now takes in an object instead of a boolean (WICG#12)
@rmondello
Copy link

Earlier, Mike wrote:

I think I prefer option 1, wherein OTPCredential is an empty subclass of Credential, and the credential's identifier is the OTP code. An empty id feels strange. (That said, I grant that accessing an id attribute to get the code you want is also strange. :) )

I think the weirdness of possibly teaching folks to think of a “one-time code” as an “id” of some kind is a mistake. Imagine how confusing some of the resulting conversations could be.

I think I would prefer OTPCredential to be a nearly-empty subclass of Credential, with tacks on a String code property. The ergonomics and cognitive dissonance in the API is greatly reduced by this. If we did this, presumably the id of such an OTPCredential would be undefined.

@mikewest
Copy link
Member

Hey, Ricky, thanks for the feedback.

I think the weirdness of possibly teaching folks to think of a “one-time code” as an “id” of some kind is a mistake. Imagine how confusing some of the resulting conversations could be.

I imagine most conversations would go something like this:

A: Hey, B. Do you know what I need to do to verify a user via SMS?
B: Yeah, copy/paste this snippet from StackOverflow.
A: Thanks! I'll do that! Hooray!

:)

I think I would prefer OTPCredential to be a nearly-empty subclass of Credential, with tacks on a String code property.

If I'm the only one who thinks that reusing id is sane, so be it. It feels pretty reasonable to me to consider the code delivered from the server as an identifier, but if y'all would prefer to call it a "code" instead, I'm totally fine with something like:

interface OTPCredential : Credential {
  readonly attribute DOMString code;
}

In this case, leaving id as undefined is probably reasonable (though I'd reiterate my claim from earlier that I'm not terribly excited about potentially filling the id with some other user identifier in the future).

@samuelgoto
Copy link
Collaborator

samuelgoto commented Feb 14, 2020

That works for me too and I think was along the same lines others have suggested when I brought this up outside of this thread.

I'm going to mark this as closed and resolve with the resolution to make OTPCredential use a string code and leave id undefined.

interface OTPCredential : Credential {
  readonly attribute DOMString code;
}

I'll move the transport discussion here, and leave this issue to discuss specifically between id vs code to contain the OTP value.

@samuelgoto
Copy link
Collaborator

Just FYI, but tracking implementation on chrome here.

ayuishii added a commit to ayuishii/sms-receiver that referenced this issue Feb 25, 2020
Per conversation WICG#11
The spec has been updated to have a `code` field that will have the one-time-code and an undefined `id` field.
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

No branches or pull requests

4 participants