unify with existing fetch credential system

[wanderview]

Currently a fetch Request has a .credentials value indicating whether or not cookies should be sent with the request. These cookies are added as headers at the network layer. So the cookie headers are not available to script anywhere.

The currently proposed API in this repo, however, works completely differently. Instead of setting the credentials at the network layer they are set as the body up front in the constructor. The API then adds additional mechanisms for hiding the body from script.

As an alternative I'd like to suggest that we try to unify with the current fetch credentials systems:

1. A CredentialRequestOptons object can be provided to the Request() constructor. It gets attached as some internal value. Providing both this value and a separate body would throw synchronously in the constructor.
2. The Request functions normally to script. The body can be inspected, but simply shows an empty body. This is consistent with a Request with cookie credentials where the headers are simply not exposed.
3. At the network layer the CredentialRequestOptions from (1) is used to lookup any credentials which are then serialized to the body. (Not sure if we should fail the request or proceed without credentials if the lookup fails. This could be controlled by an option to Request().)

This would directly map to our current fetch credential system. Step (1) is like passing { credentials: 'include' } to Request(). Step (2) is mirrors how we simply hide cookie headers today. Step (3) looks up and attaches the credentials at the same time that cookies are handled today.

In addition to being more consistent with the current system, this approach would also work better with the Cache API. If we serialize the actual Credential object into Cache and then the password is changed, using that Request out of the Cache will fail. With this alternate approach, however, we would look up the new password at the network layer and everything would continue to work.

Finally, I prefer this approach because I think adding an opaque variant of Request adds unnecessary complexity. Creating an opaque Request type can break existing libraries working with Request objects. Previously they could consistently access the body without throwing, but not if a Request is marked opaque at construction time.

[mikewest]

Just to make sure I understand, I think you're suggesting something like the following, which would fold the credential lookup/mediation into the fetch API:

var options = {
  password: true
};
fetch("https://example.com/endpoint", {
  method: "POST",
  credentials: options // as an alternative to "include"?  
});

I have a few thoughts:

1. The separation between navigator.credentials.get() and fetch() allows a few use cases that this model doesn't. Consider a future FIDO extension which uses get() to obtain a FIDOCredential that has some assertion methods hanging off of it, or the existing FederatedCredential, which isn't directly submitted to a same-site endpoint, but is instead used to trigger a call to a particular identity provider's authentication SDK. These tend to require presenting the site with a Credential object of some sort, which I think this model would have a hard time serving.
2. It's unclear how we'd want to handle the unmediated option for automatic sign-in (or, for that matter, a user denying access to credentials via whatever UI a mediated request presented). How would a developer deal with failures due to credentials not being available without user mediation vs. a network error that caused Fetch to fail?

Rather than folding the lookup step into Fetch itself, what do you think about obtaining a Credential object as outlined in the document, and then passing that Credential object into fetch()?

That is, change RequestInit to:

typedef (PasswordCredential or RequestCredentials) CredentialInfo;
partial dictionary RequestInit {
  CredentialInfo credentials;
}

and call fetch() as

navigator.credentials.get(options).then((c)=>{
  if (c)
    fetch("https://example.com/", { credentials: c, method: "POST" });
});

We would still throw if a body was provided, the body would still appear empty to various bits and pieces that inspected the generated Request, and the PasswordCredential could be piped through to "HTTP-network-or-cache fetch" and injected as the body there.

Would that still address your concerns?

/cc @annevk @AxelNennker

[annevk]

This design, but with credentials being part of Request, seems nicer. That way we could also expose Request.prototype.hasCredentials() (or some such) so a service worker knows that while this Request has no observable body, one will be attached at the HTTP layer.

[mikewest]

Can you spell out what you mean in a little more detail, Anne? Doesn't adding the flag to RequestInit make it part of the Request that fetch generates? Or do you want developers to have to create a Request object themselves and pass it into fetch()?

[annevk]

I meant to make it part of RequestInit and Request. So yes, you can pass it to fetch(), but you can also create a Request first (or get one from somewhere, e.g., in service workers) and pass that to fetch().

[mikewest]

Ok. Shall I send you a PR for Fetch along the lines of what I suggested in #11 (comment)? Or would you like to take a stab at it?

[annevk]

Feel free to take a stab at it. I'm still a little worried about #12, but maybe you'll figure something out in the process.

[wanderview]

@mikewest What about the Cache API issue? If:

1. A Request is frozen on disk
2. The credential is changed in the browser password manager
3. Then the on-disk Request is used for a fetch

Should the old credential from when the Request was frozen be used? Or the new updated credential in the password manager?

Cookie credentials get the updated credential.

It seems passing the Credential object in RequestInit would require freezing the credential as-is when serializing to disk. This would be different from cookies. I'm ok with this if you guys are ok with it.

[mikewest]

The original plan was to skip ServiceWorkers entirely for requests containing credentials. I don't think there's a good use case (where "good" means a use case that retains user control and notification when credentials are being provided to a site) for allowing Service Workers to cache such a request and reuse it. I'd like to make sure that the browser notifies a user when credentials are handed over, and given that Service Workers can operate without any UI at all, they worry me in this case.

I can imagine a number of ways of obtaining that result, but before diving into any of them, would you agree with the goal of PasswordCredential-laden Request objects as being practically uncachable?

[wanderview]

I think it would be very unexpected for credential state to cause a request to skip the service worker. For example, many sites would like to be able to return an "sorry, offline so we can't login" response in that case. Bypassing service workers prevents that.

Also, bypassing service workers does not address the Cache API since its available in normal windows in addition to the ServiceWorker.

To be honest, I think getting a Request out of Cache and using it is somewhat rare. Most people only read Responses out. So its probably ok to freeze the Credential at that time. I just want to make sure we're all on the same page about it.

Edit: For example, the only way to get a Request from Cache API is with cache.keys(). The main methods used are .match() and .matchAll() which return Responses.

[mikewest]

For example, many sites would like to be able to return an "sorry, offline so we can't login"

Sure, I accept that criticism, and I think that the new approach allows folks to gracefully handle these situations. I appreciate you raising the concern.

To be honest, I think getting a Request out of Cache and using it is somewhat rare

I'd suggest that it would become less rare if doing so allowed a site to retain a persistent identifier for a user. shrug I think it's reasonable to consider Credential objects somewhat ephemeral: if I give a site my username and password, but don't do whatever dance is necessary to turn on automatic sign-in, then I'd prefer for the site not to have continual access to a stored credential. "Defrosting" a frozen Credential during the lifetime of the page is reasonable, "defrosting" it a week later to reauthenticate a user is questionable.

Does that make sense?

[wanderview]

I'd suggest that it would become less rare if doing so allowed a site to retain a persistent identifier for a user. shrug I think it's reasonable to consider Credential objects somewhat ephemeral: if I give a site my username and password, but don't do whatever dance is necessary to turn on automatic sign-in, then I'd prefer for the site not to have continual access to a stored credential. "Defrosting" a frozen Credential during the lifetime of the page is reasonable, "defrosting" it a week later to reauthenticate a user is questionable.

Well, this is why I wanted Request to contain the query parameters and the network layer to do the query of the credentials. So whatever process happens to get the Credential in the first place would have to happen again a week later. (Is the idea there is some UX shown to the user to approve the use of the password?)

We could make Cache.put() and Cache.add() reject if a Credential object is associated with the Request. We do this for other cases like POST, etc.

I still think it should go through the service worker, though. Its somewhat unprecedented for a per-Request value like this to bypass the service worker. I think it would be surprising to devs and hamper the use of this feature in offline sites.

[mikewest]

Well, this is why I wanted Request to contain the query parameters and the network layer to do the query of the credentials.

Sure. I hope I explained above why I think that would be a bad fit for this API, but I agree that it would resolve this particular tension.

Is the idea there is some UX shown to the user to approve the use of the password?

Yes, or something shown to the user when credentials are handed over in the case that they've chosen to automatically sign-in.

We could make Cache.put() and Cache.add() reject if a Credential object is associated with the Request.

That sounds good to me. WDYT, @annevk?

I still think it should go through the service worker, though.

whatwg/fetch#237

[wanderview]

I've asked @jakearchibald to comment on the Cache question as well.

[mikewest]

NO NOT JAKE! I mean. Ok. Great. Jake. cough :)

[annevk]

I agree that these Request objects should not bypass the service worker. I would be fine with making these Request objects impossible to put in the cache.

[mikewest]

Now that I read #11 (comment) again, if POST isn't cachable, then I think we already get the behavior we want without any special-casing, as you can't construct a GET request with a body. The same should apply to bound credentials.

[annevk]

Good point.

[wanderview]

Well, people have asked for bodies in GET and we've at least discussed POST requests in Cache API before. We should be careful about relying on things that might change. Still, seems reasonable for now if we can remember we have this dependency.

[wanderview]

Now that I read #11 (comment) again, if POST isn't cachable, then I think we already get the behavior we want without any special-casing, as you can't construct a GET request with a body. The same should apply to bound credentials.

I guess this means that the Request constructor should throw if the new "credential-as-body" thing is used with a method that does not allow bodies? Seems we should fast-fail that.

[mikewest]

I guess this means that the Request constructor should throw if the new "credential-as-body" thing is used with a method that does not allow bodies? Seems we should fast-fail that.

Yes. The patch in whatwg/fetch#237 fast-fails if a credential is present, just as it does for a GET request with a body. It just adds that bit to the same step, which should mean that we'll consider them both if we ever change the behavior.

I'd appreciate your feedback on that PR, if you have time.

[wanderview]

I'd appreciate your feedback on that PR, if you have time.

I'll try, but I find reviewing HTML changes in github extremely difficult.

[jakearchibald]

Finally read through this, and yeah, this works fine now because we can't store bodies in the cache.

However, I don't expect this to be true forever, either through allowing POST etc into the cache, or allowing GET to have a body. In those cases I'm happy storing the data in the request - it won't be any more stale than other request data right?