w3c / webappsec

Web Application Security Working Group repo
https://www.w3.org/groups/wg/webappsec/
Other
603 stars 148 forks source link

CREDENTIAL: Credential scope should not be limited to login #256

Open dlongley opened 9 years ago

dlongley commented 9 years ago

Credentials may be used for more than just login, and a credential may not represent a user's entire identity. This means that browsers can't just take a list of credentials and throw them up in a UI when someone is attempting to log into a website. It also means the API should support more complex queries for the types of credentials desired. This likely means redefining what a credential is -- and making changes to the Credential base class.

There are at least two ways to proceed:

  1. Consider some credentials to be "LoginCredentials" (or username+password legacy credentials) and others to be of a more generalized sort. The browser only bothers displaying "LoginCredentials" in a special way. Browsers can defer to IdPs for displaying non-LoginCredentials.
  2. Consider that all credentials are of a generalized sort, and browsers will have to inspect a variety of properties about them to determine how to best display them to users. Browsers could also simply defer to IdPs to handle specialized display.

In the Credentials CG work, we don't consider "login" to be a special use case. A relying party may ask for whatever credential they want to in order to authorize a user to take some action or to simply collect information about that user for later review, etc. For example, "login" can be implemented by requesting a "Verified Email Credential" from a user. It could be implemented in another way as well.

The current Credential Management API sees the "login use case" as a special first class citizen, which makes perfect sense, considering that it is scope-limited to making incremental improvements to "login" via a new imperative password manager API. I don't see any conflict with the Credentials CG in this respect.

The conflict arises from the fact that the spec aims to do more than just provide an API for password managers, it suggests there is "future work" and attempts to define an extensible API to try and cover it. Again, it makes perfect sense that you'd want such an API to support a broader range of credentials if it can. Fortunately, the Credentials CG has spent years working on designs and technologies in the "future work" space the Credentials Management API refers to. Unfortunately, the current design feels a bit inverted to those of us that have spent that time. I don't have a quick fix for this particular issue -- but it's clear to me that how we want credentials to work in the future doesn't quite mesh with the existing "login" paradigm.

Obviously, it would be nice to make a minimal number of changes to the API now to future proof it -- and then simply list these goals out in the future work section.

mikewest commented 9 years ago

Concretely, what would you like to see change in the API to make your use case less awkward?

mikewest commented 9 years ago

That is, I don't understand how this would work for a password. For that case, the things you're calling "identity" and "credential" are the same. :)

dlongley commented 9 years ago

@mikewest, I think when you ask for a LocalIdentity, it would be returned with a PasswordCredential stored in it. Then you call send() and everything functions as it does now. Other types of identities may not have "built-in" credentials.

dlongley commented 9 years ago

Other types of identities may not have "built-in" credentials, you have to go fetch the ones you want for the user's chosen identifier.

dlongley commented 9 years ago

A quick strawman:

navigator.identifiers.get({
  type: ["LocalIdentifier", "FederatedIdentifier", "FutureIdentifier"]
}).then(function(identifier) {
  if(identifier instanceof LocalIdentifier) {
    // identifier.credential is a PasswordCredential w/hidden password property
    // identifier.send() will send the identifier+password
  }
  if(identifier instanceof FederatedIdentifier) {
    // identifier.credential is a FederatedCredential or some other
    // custom/proprietary/super-provider thing
  }
  if(identifier instanceof FutureIdentifier) {
    // identifier.idp holds a URL to an IdP that speaks a standard protocol
    // identifier.get() will talk to that IdP and fetch credentials for the identifier
    // based on a query you give it
    // identifier.send() will send the retrieved credentials (if from cache)
  }
});
mikewest commented 9 years ago

navigator.credentials.get() should return a Credential, shouldn't it? With regard to the use cases the doc specifies, I don't really understand the necessity for indirection for any of those use cases. Similarly, I don't understand the necessity for anything other than a bundle of Credential objects that something like CredentialSet would supply.

mikewest commented 9 years ago

if(identifier instanceof LocalIdentifier) {

In this case, the .credential indirection is, practically, meaningless.

if(identifier instanceof FederatedIdentifier) {

In this case, we're asking developers to type .credential.federation rather than .federation, which seems unfortunate.

if(identifier instanceof FutureIdentifier) {

In this case, you're not using the .credential at all, which I don't understand. Didn't you suggest that "Identity" (or whatever) contained multiple credentials?

mikewest commented 9 years ago

Also: won't the if(identifier instanceof FutureIdentifier) { flow require multiple user interactions? If there's only one IdP, as you suggested in an earlier comment, it would seem better to do everything at once. There's no semantic distinction between navigator.identifiers.get() + identifier.get() in your strawman, and navigator.credentials.getAll() in my strawman, is there? The practical outcome seems identical.

adrianhopebailie commented 9 years ago

@dlongley if you look at the discussions on the mailing list you'll see that trying to get the concept of identity in scope here is not going to happen even though I agree that the Credential base-class does represent an identity in many respects more than it represents a credential.

With that in mind I have taken the view that this API is a mechanism for accessing locally cached credentials and nothing more.

I think everyone is on-board with the idea that the ideal model is an identity that has one or more credentials. In future one might get an instance of the identity by calling something like navigator.identies.get() or accessing a it via a Credential that has a property pointing to it's identity.

Today the API only deals with locally cached credentials. Future APIs could either extend the Credential class to have a property of type Identity or we define a new Identity Management API with methods like navigator.identities.get(). Whenever a new identity is loaded or fetched from an IdP the associated credentials are stored and accessible via the Credentials Management API.

The flow for a calling app is much the same as how the proposed FederatedCredential flow works.

  1. The RP site requires a credential so it looks for it via the API.
  2. If it finds what it needs it performs whatever functions are required based on the specifics of that credential. Examples:
    • It's a FederatedCredential that indicates the user has a login with Microsoft. So the RP site uses some custom Microsoft supplied logic to login to the Microsoft IdP.
    • It's a signed "age verification credential" (that was loaded into the cache in a parallel operation when the user visited their IdP in another session). So the RP site validates the signature on the credential and compares the issuer to a list they are happy with then proceeds.
    • It's a new custom credential type that supports fetching linked-data. The RP site uses some linked-data libraries to process the credential and fetch whatever additional data needs to be fetched online.
  3. The RP site doesn't find the credentials it needs in the cache so it kicks of a process of getting the user to login via their IdP and during that process caches the credentials it gathers for next time. (@mikewest It would be great if we could define a standard property on the Credential class that is just a function callback allowing the caller to define some logic that is performed for each credential before the set is returned. Something like an onload function).

It's not as good as a fully-fledged Identity Management API out the gates but I think it's a good compromise.

mikewest commented 9 years ago

It would be great if we could define a standard property on the Credential class that is just a function callback allowing the caller to define some logic that is performed for each credential before the set is returned.

Assuming we defined getAll(), the following should do what you want here, right?

function process(credential) {
    // do something interesting.
}

navigator.credentials.getAll(...).then(function(credentials) {
    credentials.map(process);
});

Given that there's no use case in the document for multiple credentials, I don't think there's any real need to define a callback property. It's easily bolted on.

adrianhopebailie commented 9 years ago

@mikewest I was thinking more of a hook on the Credential base class that is always called at some point during the get() flow. It gives sub-classes a stub they can implement if required.

So I could store a custom Credential like this and expect my function to get called when the Credential is being loaded from the cache:

navigator.credentials.store(
  new LinkedDataCredential({
    "id": "http://credentials.com/1234",
    "onLoad": function() { //Do some processing here... },
  })
);

Not convinced this solves @dlongley 's use case but keen to hear your thoughts

mikewest commented 9 years ago

@adrianhopebailie: I don't really understand the use case. Could you give me some detail around what you'd like to do with regard to incoming credentials?

adrianhopebailie commented 9 years ago

Call the IdP that originally issued the credential to get the linked data before it is returned to the caller

adrianhopebailie commented 9 years ago

@mikewest I am not attached to the idea, was trying to suggest something that may give @dlongley the hook he needs

mikewest commented 9 years ago

I'd suggest that the current draft has sufficiently explicit extensibility hooks to make it possible to support the use cases outlined in this issue. I've documented those at https://w3c.github.io/webappsec/specs/credentialmanagement/#implementation-extension.

Is there anything left here that would block the future work you're interested in, @dlongley and @adrianhopebailie? If not, can we close this out, and deal with more detailed questions in issues like #274?

dlongley commented 9 years ago

@mikewest, I'm not quite ready to close out the issue; I just haven't had time to write some example code that would work for us. I'd like to keep it open until we've at least seen what our implementation might look like with the current state of the spec. We'll get back to you when we've got something.

mikewest commented 8 years ago

Moving this to "future". Based on the conversation at https://lists.w3.org/Archives/Public/public-webappsec/2015Aug/0145.html (and the rest of that thread) I think we're at a point where the current spec is extensible enough for your needs.

dlongley commented 8 years ago

@mikewest,

We've got a simple polyfill that explores how our API would look on top of the Credential Management API. We've got a demo using it here: https://authorization.io

The demo is fairly fragile and doesn't do much in terms of error handling, but it demonstrates registration with a credential curator (previously known to and referred to as "identity provider" in the demo) and the main credential flows.

dlongley commented 8 years ago

@mikewest,

The link to the polyfill code is here: https://github.com/digitalbazaar/credentials-polyfill