989442 - Add getAssertionForSignedInUser() to fxa_client API

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: FxA, defect)

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Add a method to the fxa_client API in gaia that will get an assertion for a given audience on behalf of the currently-signed-in user.

This will enable us to silently provision an identity with a third-party service using a valid fxa assertion for the user.

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

how many times have i done that ...

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: The gaia part (wip)

The gaia part (wip)

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: the gecko part (wip)

The gecko part

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: the gecko part (wip)

Oops.  There we go.  Now it works :)

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #0)
> Add a method to the fxa_client API in gaia that will get an assertion for a
> given audience on behalf of the currently-signed-in user.
> 
> This will enable us to silently provision an identity with a third-party
> service using a valid fxa assertion for the user.

Take into account that the signUp/signIn UI will be automatically triggered if the user is not already logged in the device and an assertion is requested. A FxA login dialog popping up suddenly without a clear login intention by the user might be a confusing UX.

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> Take into account that the signUp/signIn UI will be automatically triggered
> if the user is not already logged in the device and an assertion is
> requested. A FxA login dialog popping up suddenly without a clear login
> intention by the user might be a confusing UX.

Yes, that would not be fun.  I will add the {silent: true} option as here:

https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/FirefoxAccounts.jsm#96

Thanks for your comment, ferjm.

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: the gecko part (wip)

Ensure getAssertion() doesn't surface any UI

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Rebased.  Added method and tests for IAC client.

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: The gaia part (wip)

Exposes getAssertion in the fxa_client and the fxa_iac_client; adds tests.

Jared, do you mind giving me some feedback?
Thanks
j

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8411321 [details]
The gaia part (wip)

Hey Jed!

Looks fine to me, only (minor) concern is that it'd be nice to ensure the options object is documented somewhere in gaia (and perhaps it already is).

Of course, as I said in the PR itself, I am not an owner/peer, so I may be overlooking some deeper concerns :-)

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Thanks for your comments, Jared!

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: the gecko part (wip)

Sam, could you give me your thoughts on this?

Basically, I'm ensuring that:
1. getAssertion is exposed, and
2. you can pass options down to jwcrypto from all APIs

Since the getAssertion calls at the bottom of the pile can pass options to jwcrypto, I thought it only fair that this capability were available at every level.

I admit, there's limited value presently for this functionality, but I would argue for it on the grounds that:
1. It normalized the api for every getAssertion call, and
2. It paves the way for passing additional payloads to embed in the assertion

Regarding item 2, such a payload could be an RTC key fingerprint, a session token, etc.  The capability for signing custom payloads has always been in jwcrypto, but we have not thus far exposed it.

Thanks for your feedback

Comment 13

[Sam Penrose]

Comment on attachment 8412313 [details] [diff] [review]
the gecko part (wip)

I think this is a good direction to take the overall system in: well done and thank you. Reading it was not as easy as I'd like, which probably boils down to the need to refactor the jsm troika.

Comment 14

[Sam Penrose]

One more thought: I worry that we've under-designed the relationship between the System ("IAC") and DOM ("RP") APIs to FxA. The latter feels pinched and un-device-like to me, per

  https://bugzilla.mozilla.org/show_bug.cgi?id=1000323#c4 **

The former does not look to me like an RP API. Gut feeling: our APIs are about the state machines forced on us by implementation details of our user agents, and not about a sensible set of verbs we want to expose. This feeling could be mistaken, but I want to resolve it one way or another in the medium term.

** to expand on that point:

refreshAuth (of which forceAuth is a special case) turns out to be somewhat tricky to implement on the RP side. The root of the problem is that you register your callbacks for request() when you call watch(), and the parameters sent to those callbacks don't say whether request() was done with refreshAuth set or not, so the RP has to manage that state manually on each request() and each callback execution. I have a feeling that providing a mozId.refreshAuthentication() convenience method would cause the Persona team to converge on my house with weapons, but from a FxA-on-FxOS help-RPs adoption standpoint it seems like a no-brainer. Also, wouldn't refreshAuthentication() ideally block and return a result rather than be non-blocking with multiple callbacks?

Comment 15

[Sam Penrose]

My approach to bug 1004319 is going to require some substantial rebasing either there or here. I propose that we mitigate the conflict by dedicating the function signature

  FxAccountsManager.getAssertion()

to the use case here (we could rename it getAssertionForSignedInUser() if we're feeling Javaish/Germanic), and renaming the current .getAssertion() to:

  .request()

(or maybe even relyingPartyRequest()) and moving most of the current code to it. That would leave getAssertion() looking something like:

  return this._getAssertion(aAudience).then(
    (assertion) => {
      if (!assertion) { // _getAssertion tests for these issues
        return Promise.reject("No signed in, verified user for getAssertion");
      }
      return assertion;
    }
  );

Modulo the options.duration of course. I confess when OO-centric me sees several intermediate functions gain a parameter to handle a simple use case, that says to me that the code suffers from poor separation of concerns. But I'm not sure if that is fixable given the iron services/ vs toolkit/ vs dom/ vs b2g/ structure we have. At any rate, let me know what you think of getAssertion[ForSignedInUser]() / r[elyingPartyR]equest() . Thanks Jed!

Comment 16

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Sam Penrose from comment #15)
> My approach to bug 1004319 is going to require some substantial rebasing
> either there or here. I propose that we mitigate the conflict by dedicating
> the function signature
> 
>   FxAccountsManager.getAssertion()
> 
> to the use case here (we could rename it getAssertionForSignedInUser() if
> we're feeling Javaish/Germanic), and renaming the current .getAssertion() to:
> 
>   .request()

I see your point, although I feel weird about renaming getAssertion() to request() for the same reason that I think you dislike getAssertion(), namely that, in the browserid RP API at least, request() can have two different behaviors depending on whether UI needs to be surfaced, which means it behaves very differently if there isn't a signed-in user.

Those considerations make me cotton to your getAssertonForSignedInUser(), javatastic as it is, because it's precise.

> (or maybe even relyingPartyRequest()) and moving most of the current code to
> it. That would leave getAssertion() looking something like:
> 
>   return this._getAssertion(aAudience).then(
>     (assertion) => {
>       if (!assertion) { // _getAssertion tests for these issues
>         return Promise.reject("No signed in, verified user for
> getAssertion");
>       }
>       return assertion;
>     }
>   );
> 
> Modulo the options.duration of course. I confess when OO-centric me sees
> several intermediate functions gain a parameter to handle a simple use case,
> that says to me that the code suffers from poor separation of concerns.  But
> I'm not sure if that is fixable given the iron services/ vs toolkit/ vs dom/
> vs b2g/ structure we have. At any rate, let me know what you think of
> getAssertion[ForSignedInUser]() / r[elyingPartyR]equest() . Thanks Jed!

No doubt there is a lot of shuffling of papers going on here.  I agree that much of it stems from being splayed out across four modules, but we could probably do better to consolidate.

In sum, I'm leaning towards getAssertionForSignedInUser().  I don't think it's a bad thing for us to have really verbose names when there are so many permutations of state, and we want to be specific.

Thanks, Sam!

Comment 17

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Resuming work on this with revisions according to Comment 16

Comment 18

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Rebasing over patch for bug 1036198

Comment 19

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8411321 [details]
The gaia part (wip)

Updated after Comment 15, and rebased over today's master.

:ferjm, do you have a moment to take a look?

Thanks, j

Comment 20

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: the gecko part

Hi, Sam, this adds getAssertionForSignedInUser.  Do you mind taking a look?
Thanks, j

Comment 21

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8454619 [details] [diff] [review]
the gecko part

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

::: services/fxaccounts/FxAccounts.jsm
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +"use strict";

Adding this exposed an error on line 66, where we had an inadvertent global assignment ( AccountState = function(fxaInternal) ...).  I failed to get that fix into this patch; I will add it.  (And the tbpl will prove it, since that line will throw with "use strict".)

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8454619 [details] [diff] [review]
the gecko part

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

::: b2g/components/FxAccountsMgmtService.jsm
@@ +105,5 @@
>            }
>          ).then(null, Components.utils.reportError);
>          break;
> +      case "getAssertionForSignedInUser":
> +        var options = data.options || {};

s/var/let

@@ +111,5 @@
> +        // The 'silent' flag prevents us from surfacing any UI when trying to
> +        // get an assertion.
> +        options.silent = true;
> +
> +        FxAccountsManager.getAssertion(data.audience, options).then(

.getAssertion now also takes the principal as the second parameter. In this case, I think you can just pass null and make sure that FxAccountsManager handles this case properly (it currently doesn't) or just pass the system principal.

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8411321 [details]
The gaia part (wip)

Hi Jed, thanks for the patch and the tests!

The code looks good, but there are a few things that I'd like to understand first. The title doesn't correspond to what I see in the code. You are certainly adding a new function to fxa_client, but you are also trying to expose it via IAC (fxa_iac_client).

* What's the requirement here?
* Who's going to use this new method?
* Do we want an app different to System to use this new method? (i.e. Settings or FTU, like for the other methods exposed via IAC).

If we want this new function to be exposed via IAC to non-System certified apps, you are missing one piece at [1]. If we just want to add it to fxa_client so other System modules can use it, then the addition to fxa_iac_client at [2] is not needed.

Thanks!

[1] https://github.com/jedp/gaia/blob/989442-getAssertion-api/apps/system/js/fxa_manager.js#L79
[2] https://github.com/jedp/gaia/blob/989442-getAssertion-api/shared/js/fxa_iac_client.js#L195

Comment 24

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #23)
> Comment on attachment 8411321 [details]
> The gaia part (wip)
> 
> Hi Jed, thanks for the patch and the tests!

And thank you for taking a look!

> The code looks good, but there are a few things that I'd like to understand
> first. The title doesn't correspond to what I see in the code. You are
> certainly adding a new function to fxa_client, but you are also trying to
> expose it via IAC (fxa_iac_client).
>
> * What's the requirement here?

The purpose behind this patch is to make it possible for an app to get an assertion from straight javascript, without using the RP API.  When experimenting with attaching to third-party services via FxA, François and I found that we lacked a simple getAssertion method.

> * Who's going to use this new method?

That's a good question, and now that you are pushing back, I'm beginning to think that this shouldn't land in any way until we resolve the question of how a privileged app could use such a method.  I think it's probably fine, since they could use the RP API, but it might be worth getting user permission anyway.

> * Do we want an app different to System to use this new method? (i.e.
> Settings or FTU, like for the other methods exposed via IAC).

That was the idea, yes.  The Contacts sync app, for which I first put this together, is only Privileged.

> If we want this new function to be exposed via IAC to non-System certified
> apps, you are missing one piece at [1]. If we just want to add it to
> fxa_client so other System modules can use it, then the addition to
> fxa_iac_client at [2] is not needed.

Ah!  Thank you.  Ok.

> Thanks!
> 
> [1]
> https://github.com/jedp/gaia/blob/989442-getAssertion-api/apps/system/js/
> fxa_manager.js#L79
> [2]
> https://github.com/jedp/gaia/blob/989442-getAssertion-api/shared/js/
> fxa_iac_client.js#L195

I'm on my way out today.  Having spoken with spenrose, we think it makes sense to back-burner this while other bugs get attention.  I would propose that :carlosdp and :6a68 eventually weigh in.  If they end up not needing this for Contacts sync, then it can just be dropped.  Otherwise they can help bring it home.

Cheers!
j

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #24)
> > The code looks good, but there are a few things that I'd like to understand
> > first. The title doesn't correspond to what I see in the code. You are
> > certainly adding a new function to fxa_client, but you are also trying to
> > expose it via IAC (fxa_iac_client).
> >
> > * What's the requirement here?
> 
> The purpose behind this patch is to make it possible for an app to get an
> assertion from straight javascript, without using the RP API.  When
> experimenting with attaching to third-party services via FxA, François and I
> found that we lacked a simple getAssertion method.
> 
> > * Who's going to use this new method?
> 
> That's a good question, and now that you are pushing back, I'm beginning to
> think that this shouldn't land in any way until we resolve the question of
> how a privileged app could use such a method.  I think it's probably fine,
> since they could use the RP API, but it might be worth getting user
> permission anyway.
> 
> > * Do we want an app different to System to use this new method? (i.e.
> > Settings or FTU, like for the other methods exposed via IAC).
> 
> That was the idea, yes.  The Contacts sync app, for which I first put this
> together, is only Privileged.
> 

Note that the IAC API is exposed to certified apps only (and it doesn't look like something that's going to change in the near future) and the FxA IAC based API is intended for FxA manager apps only (Settings and FTU). So this solution won't work for the contacts sync app if it's privileged.

> I'm on my way out today. 

:__) We'll miss you!

Comment 26

[Sam Penrose]

Comment on attachment 8454619 [details] [diff] [review]
the gecko part

Unsetting r? for now.

Comment 27

[BMO Automation]

Firefox OS is not being worked on