1062001 - (navigator.auth) FxA-Oauth on FxOS implementation

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Sam Penrose]

A tracking bug for implementing this API for FxOS:

  https://github.com/SamPenrose/ua-augmented-auth

initially for FxA-Oauth only. Creating now as a least-bad way to track design and discussion. Background:

  http://lists.w3.org/Archives/Public/public-webapps/2014JulSep/0243.html

Work will consist of:

  RP API: new WebIDL definition and new modules in dom/identity
  login flow: Gaia work possibly modeled on Calendar App:
    apps/calendar/js/oauth_window.js

I propose that we use the existing FxA Gecko/Gaia signal channels in b2g/components and the System App, to get up and running:

  https://mxr.mozilla.org/gaia/source/apps/system/js/fxa_manager.js#23

Here is a quick sketch, certainly wrong in some details:

  Gecko sends new mozFxAccountsUnsolContentEvent:
    {detail: {eventName: 'authenticate', 'providers': [
              {authURL: 'https://oauth.accounts.firefox.com/v1',
               returnURL: 'maybe Marketplace's?',
               interactive: <bool>
               // possibly scope and state, though maybe Gaia injects them}]}

  Gaia opens an Oauth flow in the System? Settings? context.
  The flow captures the redirect event and sends a new mozFxAccountsChromeEvent to Gecko:
    {data: {method: 'oauth-change-event',
            eventName: 'signIn',
            // how should we spell "source"? "authURL"? "oauth_uri"? "authEndpoint"?
            account: {source: 'https://oauth.accounts.firefox.com/v1',
                      returnURL: 'unchanged.from.source?with=queryString,
                      // next three extracted from queryString
                      client_id:'', state:'', code:''
                      // state only if provided initially}}
  Gecko fires:

    'onSignInChanged', (account, true)

  in the RP's window.

Comment 1

[Chris Karlof [:ckarlof]]

FYI, if you in a privileged context and can generate BiD assertions for arbitrary domains, you can use this API endpoint:

https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md#post-v1authorization

to get an FxA OAuth token to access user data from the client. This token is not good for authenticating to external services, but it's useful to access the user's profile image and other user data from the client.

Comment 2

[Sam Penrose]

Thanks Chris. Jared, this may be a quicker route to getting you tokens to work with than me coding everything I mentioned in Comment 0.

(In reply to Chris Karlof [:ckarlof] from comment #1)
> FYI, if you in a privileged context and can generate BiD assertions for
> arbitrary domains, you can use this API endpoint:
> 
> https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md#post-
> v1authorization
> 
> to get an FxA OAuth token to access user data from the client. This token is
> not good for authenticating to external services, but it's useful to access
> the user's profile image and other user data from the client.

Comment 3

[Chris Karlof [:ckarlof]]

This requires a client_id with 'canGrant' permissions. It's not clear to me that one of these has made its way into our dev servers, much less production. But if not, it's coming soon.

Comment 4

[Ian Bicking (:ianbicking)]

The backdoor/BiD assertion feels like the approach we'd want to make if we needed an expedient solution to give access to the profile data.  But I don't think that requirement is urgent right now, so we have the time to explore an option that enhances authentication on the platform generally, and also makes FxA less bound to other parts of the OS.

Comment 5

[Sam Penrose]

Fernando, because navigator.auth.authorize() is a one-way message, the existing MobileIdentity and FxAccounts patterns using b2g/components/ContentRequestHelper have more complexity than it needs. From mxr it looks like SystemAppProxy is used mostly in b2g/components, with dom/audiochannel and dom/inputmethod as exceptions:

  http://mxr.mozilla.org/mozilla-central/search?string=systemappproxy&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

I think I should probably add a OneWaySystemEvent.jsm to b2g/components that doesn't store state the way ContentRequestHelper does (complexity which comes back into DOMIdentity.jsm). Other choices: use SystemAppProxy directly in dom/identity/ or use ContentRequestHelper anyway, maybe modifying it to add a "one-way" channel. What do you think? Should I ask Fabrice or someone like that?

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Sorry I couldn't get to this today, I'd like to read in detail about the proposal mentioned in the description before giving any feedback about the implementation approach. I'll try to do it tomorrow.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Sorry for the delay here.

(In reply to Sam Penrose from comment #0)
> A tracking bug for implementing this API for FxOS:
> 
>   https://github.com/SamPenrose/ua-augmented-auth
> 
> initially for FxA-Oauth only. Creating now as a least-bad way to track
> design and discussion. Background:
> 
>   http://lists.w3.org/Archives/Public/public-webapps/2014JulSep/0243.html
> 

Thanks for starting this Sam. Looks pretty promising. I'd love to see an improved login experience on web apps.

> Work will consist of:
> 
>   RP API: new WebIDL definition and new modules in dom/identity

This is not really important, but note that dom/identity is for the navigator.mozId API implementation (for both Persona and Firefox Accounts). Even if the initial target IdP is FxA-OAuth, what you are trying to build here is a new API that seems to be completely independent from the mozId API and wider than Firefox Accounts auth and so it should probably live somewhere else (dom/auth? or maybe dom/identity/auth and dom/identity/mozId?). I don't really care about the naming, but try not to mix different API implementation files in the same folder, please.

>   login flow: Gaia work possibly modeled on Calendar App:
>     apps/calendar/js/oauth_window.js
> 

I am not sure that I understood this. Do you mean that the Calendar app would be the first consumer of this API?

> I propose that we use the existing FxA Gecko/Gaia signal channels in
> b2g/components and the System App, to get up and running:
> 
>   https://mxr.mozilla.org/gaia/source/apps/system/js/fxa_manager.js#23
> 
> Here is a quick sketch, certainly wrong in some details:
> 
>   Gecko sends new mozFxAccountsUnsolContentEvent:
>     {detail: {eventName: 'authenticate', 'providers': [
>               {authURL: 'https://oauth.accounts.firefox.com/v1',
>                returnURL: 'maybe Marketplace's?',
>                interactive: <bool>
>                // possibly scope and state, though maybe Gaia injects them}]}
> 
>   Gaia opens an Oauth flow in the System? Settings? context.
>   The flow captures the redirect event and sends a new
> mozFxAccountsChromeEvent to Gecko:
>     {data: {method: 'oauth-change-event',
>             eventName: 'signIn',
>             // how should we spell "source"? "authURL"? "oauth_uri"?
> "authEndpoint"?
>             account: {source: 'https://oauth.accounts.firefox.com/v1',
>                       returnURL: 'unchanged.from.source?with=queryString,
>                       // next three extracted from queryString
>                       client_id:'', state:'', code:''
>                       // state only if provided initially}}
>   Gecko fires:
> 
>     'onSignInChanged', (account, true)
> 
>   in the RP's window.

Same as before, try not to mix API implementations, please. IMHO this new API should be completely decoupled from the mozId one. What you describe above is simply the usual B2G chrome <-> content event communication flow applied to the FxA specific case. You can have a dedicated set of custom events (mozChromeAuthEvent?) for this API that should be triggered and handled outside of the FxA/mozId implementation bits.

(In reply to Sam Penrose from comment #5)
> Fernando, because navigator.auth.authorize() is a one-way message, the
> existing MobileIdentity and FxAccounts patterns using
> b2g/components/ContentRequestHelper have more complexity than it needs. From
> mxr it looks like SystemAppProxy is used mostly in b2g/components, with
> dom/audiochannel and dom/inputmethod as exceptions:
> 
>  
> http://mxr.mozilla.org/mozilla-central/
> search?string=systemappproxy&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mo
> zilla-central
> 
> I think I should probably add a OneWaySystemEvent.jsm to b2g/components that
> doesn't store state the way ContentRequestHelper does (complexity which
> comes back into DOMIdentity.jsm). Other choices: use SystemAppProxy directly
> in dom/identity/ or use ContentRequestHelper anyway, maybe modifying it to
> add a "one-way" channel. What do you think? Should I ask Fabrice or someone
> like that?

I don't think you need to add anything like OneWaySystemEvent.jsm or modify ContentRequestHelper. If you just need to send a one-way message from chrome to content, use the SystemAppProxy for that. But do not use it outside of b2g/components, please ( even if dom/audiochannel and dom/inputmethod do it already :_( ).

Try to design the implementation of this API in a way that it can be extensible to other platforms with the minimum set of changes and exceptions as possible. For example, if you use the SystemAppProxy within the dom/* bits and then we decide to implement and enable this API for Desktop you will need to wrap the code that uses SystemAppProxy within an #ifdef MOZ_B2G and add the specific bits for the equivalent code for Desktop also within a conditional group.

There are certainly many ways to do this, but a very simple pattern that worked so far for other APIs is creating an interface abstracting the UI and product specific parts and implement it in each of the products where the API is enabled. You can see a few examples at:

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/nsIInterAppCommUIGlue.idl
http://mxr.mozilla.org/mozilla-central/source/dom/activities/interfaces/nsIActivityUIGlue.idl
http://mxr.mozilla.org/mozilla-central/source/dom/payment/interfaces/nsIPaymentUIGlue.idl
http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/interfaces/nsIFxAccountsUIGlue.idl
http://mxr.mozilla.org/mozilla-central/source/services/mobileid/interfaces/nsIMobileIdentityUIGlue.idl

Comment 8

[Sam Penrose]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #7)
> Sorry for the delay here.

Not at all! This is very helpful, thank you.
 
> (In reply to Sam Penrose from comment #0)
> > A tracking bug for implementing this API for FxOS:
> > 
> >   https://github.com/SamPenrose/ua-augmented-auth
> > 
> > initially for FxA-Oauth only. Creating now as a least-bad way to track
> > design and discussion. Background:
> > 
> >   http://lists.w3.org/Archives/Public/public-webapps/2014JulSep/0243.html
> > 
> 
> Thanks for starting this Sam. Looks pretty promising. I'd love to see an
> improved login experience on web apps.
> 
> > Work will consist of:
> > 
> >   RP API: new WebIDL definition and new modules in dom/identity
> 
> This is not really important, but note that dom/identity is for the
> navigator.mozId API implementation (for both Persona and Firefox Accounts).
> Even if the initial target IdP is FxA-OAuth, what you are trying to build
> here is a new API that seems to be completely independent from the mozId API
> and wider than Firefox Accounts auth and so it should probably live
> somewhere else (dom/auth? or maybe dom/identity/auth and
> dom/identity/mozId?). I don't really care about the naming, but try not to
> mix different API implementation files in the same folder, please.

OK, I will move it to dom/auth/.

> >   login flow: Gaia work possibly modeled on Calendar App:
> >     apps/calendar/js/oauth_window.js
> > 
> 
> I am not sure that I understood this. Do you mean that the Calendar app
> would be the first consumer of this API?

No, I simply wanted to point to an existing Gaia implementation of Oauth flows. Jared will probably own this part of the problem, so nevermind :-).

> > I propose that we use the existing FxA Gecko/Gaia signal channels in
> > b2g/components and the System App, to get up and running:
> > 
> >   https://mxr.mozilla.org/gaia/source/apps/system/js/fxa_manager.js#23
> > 
> > Here is a quick sketch, certainly wrong in some details:
> > 
> >   Gecko sends new mozFxAccountsUnsolContentEvent:
> >     {detail: {eventName: 'authenticate', 'providers': [
> >               {authURL: 'https://oauth.accounts.firefox.com/v1',
> >                returnURL: 'maybe Marketplace's?',
> >                interactive: <bool>
> >                // possibly scope and state, though maybe Gaia injects them}]}
> > 
> >   Gaia opens an Oauth flow in the System? Settings? context.
> >   The flow captures the redirect event and sends a new
> > mozFxAccountsChromeEvent to Gecko:
> >     {data: {method: 'oauth-change-event',
> >             eventName: 'signIn',
> >             // how should we spell "source"? "authURL"? "oauth_uri"?
> > "authEndpoint"?
> >             account: {source: 'https://oauth.accounts.firefox.com/v1',
> >                       returnURL: 'unchanged.from.source?with=queryString,
> >                       // next three extracted from queryString
> >                       client_id:'', state:'', code:''
> >                       // state only if provided initially}}
> >   Gecko fires:
> > 
> >     'onSignInChanged', (account, true)
> > 
> >   in the RP's window.
> 
> Same as before, try not to mix API implementations, please. IMHO this new
> API should be completely decoupled from the mozId one. What you describe
> above is simply the usual B2G chrome <-> content event communication flow
> applied to the FxA specific case. You can have a dedicated set of custom
> events (mozChromeAuthEvent?) for this API that should be triggered and
> handled outside of the FxA/mozId implementation bits.

OK. In the short term, I want to take advantage of the existing FxA Gaia code, so you may see some early patches that send mozFxAccountsChromeEvents. I'll remove them before r?

> (In reply to Sam Penrose from comment #5)
> > Fernando, because navigator.auth.authorize() is a one-way message, the
> > existing MobileIdentity and FxAccounts patterns using
> > b2g/components/ContentRequestHelper have more complexity than it needs. From
> > mxr it looks like SystemAppProxy is used mostly in b2g/components, with
> > dom/audiochannel and dom/inputmethod as exceptions:
> > 
> >  
> > http://mxr.mozilla.org/mozilla-central/
> > search?string=systemappproxy&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mo
> > zilla-central
> > 
> > I think I should probably add a OneWaySystemEvent.jsm to b2g/components that
> > doesn't store state the way ContentRequestHelper does (complexity which
> > comes back into DOMIdentity.jsm). Other choices: use SystemAppProxy directly
> > in dom/identity/ or use ContentRequestHelper anyway, maybe modifying it to
> > add a "one-way" channel. What do you think? Should I ask Fabrice or someone
> > like that?
> 
> I don't think you need to add anything like OneWaySystemEvent.jsm or modify
> ContentRequestHelper. If you just need to send a one-way message from chrome
> to content, use the SystemAppProxy for that. But do not use it outside of
> b2g/components, please ( even if dom/audiochannel and dom/inputmethod do it
> already :_( ).

OK, I will add a module to b2g/components that accesses SystemAppProxy.

> Try to design the implementation of this API in a way that it can be
> extensible to other platforms with the minimum set of changes and exceptions
> as possible. For example, if you use the SystemAppProxy within the dom/*
> bits and then we decide to implement and enable this API for Desktop you
> will need to wrap the code that uses SystemAppProxy within an #ifdef MOZ_B2G
> and add the specific bits for the equivalent code for Desktop also within a
> conditional group.
> 
> There are certainly many ways to do this, but a very simple pattern that
> worked so far for other APIs is creating an interface abstracting the UI and
> product specific parts and implement it in each of the products where the
> API is enabled. You can see a few examples at:
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/
> nsIInterAppCommUIGlue.idl
> http://mxr.mozilla.org/mozilla-central/source/dom/activities/interfaces/
> nsIActivityUIGlue.idl
> http://mxr.mozilla.org/mozilla-central/source/dom/payment/interfaces/
> nsIPaymentUIGlue.idl
> http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/interfaces/
> nsIFxAccountsUIGlue.idl
> http://mxr.mozilla.org/mozilla-central/source/services/mobileid/interfaces/
> nsIMobileIdentityUIGlue.idl

Yes, I with help from bz and others I was able to get an IDL compiling.

Let me know when you want to see patches for f? and thanks very much for the helpful feedback.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Sam Penrose from comment #8) 
> Let me know when you want to see patches for f? and thanks very much for the
> helpful feedback.

The sooner the better :)

Comment 10

[(no longer active)]

Moving to Core::DOM since it seems like we're talking about adding DOM APIs here.

Comment 11

[Sam Penrose]

Attachment: 1062001-navigator-auth.patch

Fernando, Ehsan, and others: here is very simple implementation of [1] for early feedback. Please take your time; this does not need to land before 2.2. Currently it lacks:

  - variadic args support
  - enough error and security checking for input coming from content
  - test modules

all of which I will add. Omissions and questions marked XXX. Thanks for your comments!

[1] https://github.com/SamPenrose/ua-augmented-auth/blob/master/api/navigator-auth-rp.js#L36

Comment 12

[(no longer active)]

Here's a very high level question: Why not implement <https://mikewest.github.io/credentialmanagement/spec/#dom-credentialscontainer-request> instead?  That API seems to supersede navigator.auth.authenticate, at least based <https://github.com/SamPenrose/ua-augmented-auth/blob/master/api/navigator-auth-rp.js#L7>.  I believe that it has two big differences, one is that it assumes the UA knows the OAuth return URL, and the second is that it also allows requesting other types of credentials (such as the UA's saved user names and passwords.)  It's fine if you don't want to implement support for the rest of credentials initiallly.

That API also has notifySignedIn, notifyFailedSignIn and notifySignedOut, which seem like a more complete version of onSignInChanged.  If there is more that you need to be able to handle, I think you should propose other notify* methods to that spec.

Comment 13

[Sam Penrose]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #12)
> Here's a very high level question: Why not implement
> <https://mikewest.github.io/credentialmanagement/spec/#dom-
> credentialscontainer-request> instead?  That API seems to supersede
> navigator.auth.authenticate, at least based
> <https://github.com/SamPenrose/ua-augmented-auth/blob/master/api/navigator-
> auth-rp.js#L7>.  I believe that it has two big differences, one is that it
> assumes the UA knows the OAuth return URL, and the second is that it also
> allows requesting other types of credentials (such as the UA's saved user
> names and passwords.)  It's fine if you don't want to implement support for
> the rest of credentials initiallly.

Thanks for the feedback! Mike's proposal is awesome, and I am watching it to see how best to coordinate with it. But it solves a different problem. As I note in the repo [1], cookie jar and redirect issues make OAuth-based SSO for apps different on mobile. In offline conversations Mike has made it clear to me that his proposal focuses on desktop-style browsing, not mobile or webapps. Part of his motivation is "paving the cowpath" of desktop browser password managers. That is an important but different problem.

Note that if there are namespacing or other API style areas in which ua-augmenting-auth can conform to the cred manager approach, it absolutely should. Speaking of which ...

> That API also has notifySignedIn, notifyFailedSignIn and notifySignedOut,
> which seem like a more complete version of onSignInChanged.  If there is
> more that you need to be able to handle, I think you should propose other
> notify* methods to that spec.

Thanks for pointing those out -- I will examine them with the presumption that ua-augmented-auth should include them unless a misfit reveals itself.

[1] https://github.com/SamPenrose/ua-augmented-auth#augmenting-oauth

Comment 14

[(no longer active)]

(In reply to Sam Penrose from comment #13)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO
> 11/3-11/21) from comment #12)
> > Here's a very high level question: Why not implement
> > <https://mikewest.github.io/credentialmanagement/spec/#dom-
> > credentialscontainer-request> instead?  That API seems to supersede
> > navigator.auth.authenticate, at least based
> > <https://github.com/SamPenrose/ua-augmented-auth/blob/master/api/navigator-
> > auth-rp.js#L7>.  I believe that it has two big differences, one is that it
> > assumes the UA knows the OAuth return URL, and the second is that it also
> > allows requesting other types of credentials (such as the UA's saved user
> > names and passwords.)  It's fine if you don't want to implement support for
> > the rest of credentials initiallly.
> 
> Thanks for the feedback! Mike's proposal is awesome, and I am watching it to
> see how best to coordinate with it. But it solves a different problem. As I
> note in the repo [1], cookie jar and redirect issues make OAuth-based SSO
> for apps different on mobile.

I read that link, and I'm trying to understand the difference, but I don't think it's very clear to me.  Let's say that I have a web page which wants to trigger an OAuth request from Google.  In your proposal, it should do:

var resultPromise = navigator.auth.authenticate("https://accounts.google.com");

whereas in mikewest's proposal, it should do:

var resultPromise = navigator.credentials.request({federations: ["https://accounts.google.com"]});

It seems to me that in both cases, the implementation of the API will get a chance to do stuff that a normal web page couldn't.  For example, if it needs to read cookies off of a redirect response, it can.  And in both cases, when the authentication is finished, the implementation of the API can resolve the returned promise (or reject it if the authentication fails.)

Why couldn't you just base your implementation on top of mikewest's proposal?  I'm sure there is something that I'm missing here, so please don't hesitate to point it out. :)

> In offline conversations Mike has made it
> clear to me that his proposal focuses on desktop-style browsing, not mobile
> or webapps. Part of his motivation is "paving the cowpath" of desktop
> browser password managers. That is an important but different problem.

I'm not sure if I understand what this means.  I mean, sure, that may be Mike's focus, but what needs to be different in the Web content facing API for authentication of mobile vs desktop?  (Differences in the implementation strategies do not necessarily need to bleed into the API surface.)  Can you please give me specific examples of use cases that his proposal will explicitly not address?

> Note that if there are namespacing or other API style areas in which
> ua-augmenting-auth can conform to the cred manager approach, it absolutely
> should. Speaking of which ...

To make it clear, I'm interested in us implementing one authentication API and not two.  :-)

Comment 15

[(no longer active)]

Comment on attachment 8511384 [details] [diff] [review]
1062001-navigator-auth.patch

(Clearing the flag to get this out of my queue while we're discussing on the bug)

Comment 16

[Sam Penrose]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #14)
> (In reply to Sam Penrose from comment #13)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO
> > 11/3-11/21) from comment #12)
> > > Here's a very high level question: Why not implement
> > > <https://mikewest.github.io/credentialmanagement/spec/#dom-
> > > credentialscontainer-request> instead?  That API seems to supersede
> > > navigator.auth.authenticate, at least based
> > > <https://github.com/SamPenrose/ua-augmented-auth/blob/master/api/navigator-
> > > auth-rp.js#L7>.  I believe that it has two big differences, one is that it
> > > assumes the UA knows the OAuth return URL, and the second is that it also
> > > allows requesting other types of credentials (such as the UA's saved user
> > > names and passwords.)  It's fine if you don't want to implement support for
> > > the rest of credentials initiallly.
> > 
> > Thanks for the feedback! Mike's proposal is awesome, and I am watching it to
> > see how best to coordinate with it. But it solves a different problem. As I
> > note in the repo [1], cookie jar and redirect issues make OAuth-based SSO
> > for apps different on mobile.
> 
> I read that link, and I'm trying to understand the difference, but I don't
> think it's very clear to me.  Let's say that I have a web page which wants
> to trigger an OAuth request from Google.  In your proposal, it should do:
> 
> var resultPromise =
> navigator.auth.authenticate("https://accounts.google.com");

navigator.auth.authenticate() does not return a promise (it doesn't take a single URL either). It may or may not result in a redirect back to the calling site's URI -- just like the current OAuth experience.

> whereas in mikewest's proposal, it should do:
> 
> var resultPromise = navigator.credentials.request({federations:
> ["https://accounts.google.com"]});
> It seems to me that in both cases, the implementation of the API will get a
> chance to do stuff that a normal web page couldn't.

Agreed.

> needs to read cookies off of a redirect response, it can.  And in both
> cases, when the authentication is finished, the implementation of the API
> can resolve the returned promise (or reject it if the authentication fails.)

Or not have a returned promise at all, and stick with the current HTTP-based experience. One of the lessons of implementing mozId is that promise chains introduce a state machine that is tricky to get right, tricky to understand, and can introduce race conditions. For example, sometimes a low-level Gecko HTTP library reject()'s when everything is normal, because the HTTP server returns a 4xx or 5xx as part of a standard handshake, and hey, an HTTP errno is a Javascript Error(), amirite? Maintaining code that multiplexes rich semantics onto a duplexed-because-promise-based channel is a lot of work, and it is not called for by the underlying problem, just by the way this API was written. Note that I am NOT criticizing Mike's proposal (which I will call "CM" hereafter). CM is very impressive and I do not have the contextual knowledge to suggest improvements. It is, to repeat myself, addressing a different problem than mine ("UAA"). But the extra work is only necessary if your goal is to make password-manager based control flow better ...

> Why couldn't you just base your implementation on top of mikewest's
> proposal?  I'm sure there is something that I'm missing here, so please
> don't hesitate to point it out. :)
> 
> > In offline conversations Mike has made it
> > clear to me that his proposal focuses on desktop-style browsing, not mobile
> > or webapps. Part of his motivation is "paving the cowpath" of desktop
> > browser password managers. That is an important but different problem.
> 
> I'm not sure if I understand what this means.

... Password managers are not an end in themselves, they are not essential to authentication, and while they are well-established, they are not better established than HTTP. CM's API begins by retrieving login key-value pairs from a client-side password manager using a promise, then it decides what to do. See [3], especially:

  - "Note: This API is focused on a sign-in workflow, but could potentially be useful for federated sign-up flows as well."

Sign-up is a core use case for any identity system. It's only an afterthought in CM because CM starts with existing desktop-style password managers.

> I mean, sure, that may be
> Mike's focus, but what needs to be different in the Web content facing API
> for authentication of mobile vs desktop?  (Differences in the implementation
> strategies do not necessarily need to bleed into the API surface.)  Can you
> please give me specific examples of use cases that his proposal will
> explicitly not address?

(Summarizing)
1) OAuth on user agents which do not implement password managers, such as FirefoxOS. See [1] and [2].
2) Sign up.

Sign up is kind of a big deal. It would be very strange to implement an authentication standard that allowed you to sign in, but not to sign up!

I went to a couple of the WebAPI meetings with my early proposals and I listened very hard to feedback from you and your teammates. The result was UAA, which you accepted -- or seemed to. I'd like especially to call your attention to [2]. Existing OAuth code already must know:

  - the authURL
  - the redirectURL
  - what to do with the token embedded in the redirectURL

Not coincidentally, the first two are the required keys when calling authenticate()**. So RP code looks like this on the authURL page:

  <existing code that defines authURL and redirectURL, because OAuth uses them>
  if (navigator.auth) { // or "credentials"? See below.
    navigator.auth.authenticate({authURL: authURL, redirectURL: redirectURL})
  } else {
    <existing code that calls window.open(<authURL>?redirect=<redirectURL>) or whatever>
  }

This is nearly the smallest possible hurdle to adoption: a feature test and a single statement.

> > Note that if there are namespacing or other API style areas in which
> > ua-augmenting-auth can conform to the cred manager approach, it absolutely
> > should. Speaking of which ...
> 
> To make it clear, I'm interested in us implementing one authentication API
> and not two.  :-)

Me too: that's why I sat down with Mike to see if the two proposals were solving the same problem. Turns out they aren't.

How about navigator.credentials.oauthRedirect(providerOne,...), with the semantics I have proposed for auth.authenticate(), living next to navigator.credentials.request()?

Finally, I hope it is not my job to attack CM in order to see UAA succeed. I want CM to succeed too, and I have nothing but respect for it.

[1] https://groups.google.com/forum/#!msg/mozilla.dev.gaia/rd-TDukXiKA/D18cm1uJPZEJ
[2] https://github.com/SamPenrose/ua-augmented-auth#augmenting-oauth
[3] https://mikewest.github.io/credentialmanagement/spec/#examples-federated-signin

** Whoops! Some OAuth flows have the redirectURL pre-registered with the IdP. Thanks for helping me catch that -- at a minimum, the parameter should be optional.

Comment 17

[(no longer active)]

Thanks for the explanation, Sam.  I think I'm closer to understanding the thought behind this.  But I still don't understand one big thing:

Let's say that the app calls:

navigator.auth.authenticate({authURL: "foo", redirectURL: "bar"});

I assume that the entire operation is asynchronous, so almost nothing happens when the authenticate function returns.  I also assume that there won't be an actual redirect happening, because otherwise we could just use window.open().  So, how does the app get the redirectURL when the authentication succeeds?  And how is it notified if the authentication fails?

On the comparison between these two proposals, sign up is a good point, but if I understand things correctly, your current proposal doesn't address it either, and I assume you're planning to address the sign up bit later?  Or does CM include something which makes handling sign ups with it impractical?

On the first point of comparison though, I'm still struggling a bit.  :-)  Is the fact that the redirect URL hidden in CM a big issue?  Does the federated authentication stuff in that spec even do an OAuth flow at all?  I'm beginning to doubt what I thought that spec is proposing...

> How about navigator.credentials.oauthRedirect(providerOne,...), with the 
> semantics I have proposed for auth.authenticate(), living next to
> navigator.credentials.request()?

Hmm, this may be a good idea.  I guess part of the answer depends on what contexts you're hoping to expose this API to.  If this is only intended as an API that will be available to privileged apps but not to Web content, then perhaps the current auth object is fine.  If you're hoping to expose it to all Web pages down the road, however, it may make sense to try to integrate it into the CM spec, to avoid confusing Web developers as to which object should be used for which APIs, etc.  Another thing to think about is feature detection.  I can imagine people trying to feature detect the CM API like:

if (navigator.credentials) {
  navigator.credentials.request(...);
}

In that case, if we implement the credentials object without the "request" method on it, then that will break the feature detection.  (Ideally they would detect the exact method, but I doubt that the pattern above turns to be very rare.)  Of course this won't be that big of an issue for privileged apps, since they are only expected to run on one engine.

I have some comments on the IDL too in a moment...

Comment 18

[(no longer active)]

Comment on attachment 8511384 [details] [diff] [review]
1062001-navigator-auth.patch

Review of attachment 8511384 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/Auth.webidl
@@ +5,5 @@
> + */
> +
> +dictionary AccountObject {
> +
> +};

What goes into this object?

@@ +7,5 @@
> +dictionary AccountObject {
> +
> +};
> +
> +callback AuthOnSignInChangedCallback = void (AccountObject account, optional boolean isSignedIn=false);

You don't seem to use this callback type anywhere in the IDL.

But these days we discourage the use of callback types and expect new APIs to use promises for notifying about asynchronous operations.  *Personally*, I find promise chains very confusing too, but it seems like Web developers have spoken their minds that they prefer callbacks over promises, so that's what we should do for new APIs.

@@ +16,5 @@
> +  boolean interactive=true;
> +};
> +
> +[JSImplementation="@mozilla.org/dom/auth;1",
> + NoInterfaceObject,

Why make this NoInterfaceObject?

@@ +18,5 @@
> +
> +[JSImplementation="@mozilla.org/dom/auth;1",
> + NoInterfaceObject,
> + NavigatorProperty="auth",
> + Pref="dom.auth.enabled"]

Depending on what kind of content you're trying to expose this API to, using [Pref] for that may or may not be the right thing.  See <https://wiki.mozilla.org/WebAPI/WebIDL_Review_Checklist> for some examples on what IDL attributes needs to be used to expose APIs to what contexts.

@@ +19,5 @@
> +[JSImplementation="@mozilla.org/dom/auth;1",
> + NoInterfaceObject,
> + NavigatorProperty="auth",
> + Pref="dom.auth.enabled"]
> +interface IAuth {

Right now because this interface is NoInterfaceObject, the name of the interface is invisible through the API surface, but we don't have the convention of using I prefixes.  So, I'd just call this AuthManager or some such.

@@ +20,5 @@
> + NoInterfaceObject,
> + NavigatorProperty="auth",
> + Pref="dom.auth.enabled"]
> +interface IAuth {
> +  void authenticate(IdentityProvider... provider);

Shouldn't "interactive" be a direct parameter of this method?  As things currently stand, the UA can use one IP interactively but not the next one, if you pass different values for interactive in the dictionaries.  Is that intentional?

Comment 19

[Sam Penrose]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #17)
> Thanks for the explanation, Sam.  I think I'm closer to understanding the
> thought behind this.  But I still don't understand one big thing:
> 
> Let's say that the app calls:
> 
> navigator.auth.authenticate({authURL: "foo", redirectURL: "bar"});
> 
> I assume that the entire operation is asynchronous, so almost nothing
> happens when the authenticate function returns.  I also assume that there
> won't be an actual redirect happening, because otherwise we could just use
> window.open().  So, how does the app get the redirectURL when the
> authentication succeeds?  And how is it notified if the authentication fails?

It gets the redirect from the user agent. Per previous, using notifyFailedSignIn? Right now OAuth doesn't get notifications, so providing them will be an enhancement.

> On the comparison between these two proposals, sign up is a good point, but
> if I understand things correctly, your current proposal doesn't address it
> either, and I assume you're planning to address the sign up bit later?

Yes, basically as soon as I can get cycles to do Gaia work I'll be learning what signup looks like, and that may feed back into changes to this patch. I *hope* not.

> does CM include something which makes handling sign ups with it impractical?

Don't know.

> On the first point of comparison though, I'm still struggling a bit.  :-) 
> Is the fact that the redirect URL hidden in CM a big issue?  Does the
> federated authentication stuff in that spec even do an OAuth flow at all? 
> I'm beginning to doubt what I thought that spec is proposing...

I'll let Mike (or Jonas?) speak to CM.

> > How about navigator.credentials.oauthRedirect(providerOne,...), with the 
> > semantics I have proposed for auth.authenticate(), living next to
> > navigator.credentials.request()?
> 
> Hmm, this may be a good idea.  I guess part of the answer depends on what
> contexts you're hoping to expose this API to.  If this is only intended as
> an API that will be available to privileged apps but not to Web content,
> then perhaps the current auth object is fine.  If you're hoping to expose it
> to all Web pages down the road, however, it may make sense to try to
> integrate it into the CM spec, to avoid confusing Web developers as to which
> object should be used for which APIs, etc.

I don't have any insights to offer here. We're defining a spec before we've shipped because that's how we ship. Of course we can mess up either way. In general, I want the best of both worlds, which is why UAA is so conservative and minimalist. We can certainly make it bigger/more opinionated if you like, but as you note there are drawbacks. Same goes for the next issue.

> feature detection.  I can imagine people trying to feature detect the CM API
> like:
> 
> if (navigator.credentials) {
>   navigator.credentials.request(...);
> }
> 
> In that case, if we implement the credentials object without the "request"
> method on it, then that will break the feature detection.  (Ideally they
> would detect the exact method, but I doubt that the pattern above turns to
> be very rare.)  Of course this won't be that big of an issue for privileged
> apps, since they are only expected to run on one engine.
> 
> I have some comments on the IDL too in a moment...

Comment 20

[Sam Penrose]

Setting needinfo so you see this, but it gives me enough to work on. Thanks very much for the feedback.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #18)
> Comment on attachment 8511384 [details] [diff] [review]
> 1062001-navigator-auth.patch
> 
> Review of attachment 8511384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Auth.webidl
> @@ +5,5 @@
> > + */
> > +
> > +dictionary AccountObject {
> > +
> > +};
> 
> What goes into this object?

Excellent question. It's unclear to me the extent to which we can mash major OAuth implementations into a single set of keys, or even just "token", and the extent to which each OAuth implementation wants to be its own snowflake. I have research to do, and would love guidance if anyone has knowledge.

> @@ +7,5 @@
> > +dictionary AccountObject {
> > +
> > +};
> > +
> > +callback AuthOnSignInChangedCallback = void (AccountObject account, optional boolean isSignedIn=false);
> 
> You don't seem to use this callback type anywhere in the IDL.

This may be a type mistake on my part. I'm trying to define an event for which content can provide a callback. See next response.

> But these days we discourage the use of callback types and expect new APIs
> to use promises for notifying about asynchronous operations.  *Personally*,
> I find promise chains very confusing too, but it seems like Web developers
> have spoken their minds that they prefer callbacks over promises, so that's
> what we should do for new APIs.

Per previous exchange about CM's handy notify* assortment of events, this (soon "these") isn't a callback passed to an asynchronous function. From the perspective of content these are stochastic events, not a foreseeable resolve()/reject().

> @@ +16,5 @@
> > +  boolean interactive=true;
> > +};
> > +
> > +[JSImplementation="@mozilla.org/dom/auth;1",
> > + NoInterfaceObject,
> 
> Why make this NoInterfaceObject?

No idea. I was trying to get the keyword to appear at all, and copied either mozId or mozPay.

> @@ +18,5 @@
> > +
> > +[JSImplementation="@mozilla.org/dom/auth;1",
> > + NoInterfaceObject,
> > + NavigatorProperty="auth",
> > + Pref="dom.auth.enabled"]
> 
> Depending on what kind of content you're trying to expose this API to, using
> [Pref] for that may or may not be the right thing.  See
> <https://wiki.mozilla.org/WebAPI/WebIDL_Review_Checklist> for some examples
> on what IDL attributes needs to be used to expose APIs to what contexts.

I found exposing a dom keyword to content to be difficult. Adding this pref was a necessary step in getting the code to work. Because FxA OAuth is exposed to the web now, I suspect we won't need to restrict permissions on FxOS (but maybe I'm missing something). Either way, if we need to change this I may need help in keeping the keyword working.

> @@ +19,5 @@
> > +[JSImplementation="@mozilla.org/dom/auth;1",
> > + NoInterfaceObject,
> > + NavigatorProperty="auth",
> > + Pref="dom.auth.enabled"]
> > +interface IAuth {
> 
> Right now because this interface is NoInterfaceObject, the name of the
> interface is invisible through the API surface, but we don't have the
> convention of using I prefixes.  So, I'd just call this AuthManager or some
> such.

OK.

> @@ +20,5 @@
> > + NoInterfaceObject,
> > + NavigatorProperty="auth",
> > + Pref="dom.auth.enabled"]
> > +interface IAuth {
> > +  void authenticate(IdentityProvider... provider);
> 
> Shouldn't "interactive" be a direct parameter of this method?  As things
> currently stand, the UA can use one IP interactively but not the next one,
> if you pass different values for interactive in the dictionaries.  Is that
> intentional?

Great question. From my perspective we have an atomic quantity of ugly, and I don't care whether it stays on the nose or moves to the cheek. In order to support NASCAR, we need variadic args one way or another. We could do any of:

  1) current: authenticate(providerOne, ...moreProviders)
  2) array:   authenticate([p1, p2, ...], interactive=true)
  3) works?:  authenticate(providerOne, ...moreProviders, interactive=true)

I can't remember in what contexts if any option 3, the cleanest, is valid JS/IDL. Option 1 does provide the footgun you note, as is a bit more work when interactive === false. Option 2 is the simplest/most backwards-compatible, though I happen to dislike the "always use an array even if N==1 most of the time" pattern. If there is a preferred style LMK and I'll conform to it.

Thanks very much for the close reading!

Comment 21

[(no longer active)]

(In reply to Sam Penrose from comment #20)
> > @@ +7,5 @@
> > > +dictionary AccountObject {
> > > +
> > > +};
> > > +
> > > +callback AuthOnSignInChangedCallback = void (AccountObject account, optional boolean isSignedIn=false);
> > 
> > You don't seem to use this callback type anywhere in the IDL.
> 
> This may be a type mistake on my part. I'm trying to define an event for
> which content can provide a callback. See next response.

Where is that event defined?  I don't see it in the IDL.

> > But these days we discourage the use of callback types and expect new APIs
> > to use promises for notifying about asynchronous operations.  *Personally*,
> > I find promise chains very confusing too, but it seems like Web developers
> > have spoken their minds that they prefer callbacks over promises, so that's
> > what we should do for new APIs.
> 
> Per previous exchange about CM's handy notify* assortment of events, this
> (soon "these") isn't a callback passed to an asynchronous function. From the
> perspective of content these are stochastic events, not a foreseeable
> resolve()/reject().

We have well known ways to dispatch events in the DOM.  But perhaps I'm misunderstanding what you're trying to do here.  At any rate, using callback functions is probably not the way to go.  :)

> > @@ +16,5 @@
> > > +  boolean interactive=true;
> > > +};
> > > +
> > > +[JSImplementation="@mozilla.org/dom/auth;1",
> > > + NoInterfaceObject,
> > 
> > Why make this NoInterfaceObject?
> 
> No idea. I was trying to get the keyword to appear at all, and copied either
> mozId or mozPay.

Don't follow those as examples, please.  They have gotten a lot of stuff wrong.  When not sure about these issues, please ask on #content.

> > @@ +18,5 @@
> > > +
> > > +[JSImplementation="@mozilla.org/dom/auth;1",
> > > + NoInterfaceObject,
> > > + NavigatorProperty="auth",
> > > + Pref="dom.auth.enabled"]
> > 
> > Depending on what kind of content you're trying to expose this API to, using
> > [Pref] for that may or may not be the right thing.  See
> > <https://wiki.mozilla.org/WebAPI/WebIDL_Review_Checklist> for some examples
> > on what IDL attributes needs to be used to expose APIs to what contexts.
> 
> I found exposing a dom keyword to content to be difficult. Adding this pref
> was a necessary step in getting the code to work. Because FxA OAuth is
> exposed to the web now, I suspect we won't need to restrict permissions on
> FxOS (but maybe I'm missing something). Either way, if we need to change
> this I may need help in keeping the keyword working.

We should avoid exposing Mozilla specific APIs like this to the Web, before we have an indication that other browser vendors are interested in implementing this.  Do we for this case?

(You can read our general exposure guidelines here: <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>)

> > @@ +20,5 @@
> > > + NoInterfaceObject,
> > > + NavigatorProperty="auth",
> > > + Pref="dom.auth.enabled"]
> > > +interface IAuth {
> > > +  void authenticate(IdentityProvider... provider);
> > 
> > Shouldn't "interactive" be a direct parameter of this method?  As things
> > currently stand, the UA can use one IP interactively but not the next one,
> > if you pass different values for interactive in the dictionaries.  Is that
> > intentional?
> 
> Great question. From my perspective we have an atomic quantity of ugly, and
> I don't care whether it stays on the nose or moves to the cheek. In order to
> support NASCAR, we need variadic args one way or another. We could do any of:
> 
>   1) current: authenticate(providerOne, ...moreProviders)
>   2) array:   authenticate([p1, p2, ...], interactive=true)
>   3) works?:  authenticate(providerOne, ...moreProviders, interactive=true)
> 
> I can't remember in what contexts if any option 3, the cleanest, is valid
> JS/IDL. Option 1 does provide the footgun you note, as is a bit more work
> when interactive === false. Option 2 is the simplest/most
> backwards-compatible, though I happen to dislike the "always use an array
> even if N==1 most of the time" pattern. If there is a preferred style LMK
> and I'll conform to it.

The restriction on variadic arguments in WebIDL <http://heycam.github.io/webidl/#dfn-variadic> is that they need to be the last argument to the method.  I don't know what NASCAR is, and what kinds of backwards compatibility concerns you have (this is a new API isn't it?), but #3 is not expressible in WebIDL.  And #2 is super ugly. :)  Why not do:

void authenticate(IdentityProvider... providers); // non-interactive
void interactiveAuthenticate(IdentityProvider... providers); // ugly name but you get the idea


Note that I'll be on vacation for three weeks, in the mean time you can ask other DOM folks for help.  Also, we hang around on #content if you have questions.  Good luck!

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Sorry for the late response here. I've been offline last week. I'll get to this tomorrow.

Comment 23

[Sam Penrose]

(In reply to Fernando Jiménez Moreno [:ferjm] - Offline until November 3rd from comment #22)
> Sorry for the late response here. I've been offline last week. I'll get to
> this tomorrow.

No problem -- and today I had some conversations about maybe not using this approach at all, so if you haven't started yet, please wait. I am going to clear the flag and will reset if needed.

Comment 24

[Sam Penrose]

Comment on attachment 8511384 [details] [diff] [review]
1062001-navigator-auth.patch

Clearing f? while rethinking approach.

Comment 25

[Sam Penrose]

We will handle FxA OAuth integration differently: https://bugzilla.mozilla.org/show_bug.cgi?id=1098519