947374 - [B2G] Certified and packaged apps should be able to declare assertion audience

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

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

Currently, the audience for a Firefox Accounts assertion is the origin of the calling window.  This is annoying for certified and packaged apps, because their origins will be the app://{guid} url, which is not a valid audience (because it cannot be verified in any way).  As a workaround, apps can host sign-in in an iframe sourced from their service's url.  This gives them an https://{url} audience, which is what we want.

Since certified apps are completely trusted (they are the system), and packaged apps are can declare an origin in their manifest files to match their service's origin, we should make life easier on these two kinds of apps by allowing them to specify the assertion audience when calling request().  (Certified apps could specify any audience; packaged apps could specify the origin in their manifest.)

Comment 1

[Sam Penrose]

This is a must-have for WMF in 1.4. WMF team wants to know if they can also have this for BrowserID during the development window. They are planning to shift to Firefox Accounts in early February. Ideally we would get them a patch that worked with both BrowserID and Firefox Accounts before that.

Comment 2

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

I can take this.  I have a preliminary patch.  Adding tests on the basis of the patches in bug 805602 and bug 929386, both of which are currently in progress.

Comment 3

[Sam Penrose]

Attachment: 947374-certified-origin.patch

Here's Jed's work. I tested it by hacking on the UITest app, which is certified, and it worked. I'll pursue getting that test added to Gaia, and also a test that uses a non-privileged app and attempts to set audience (which should fail).

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8368814 [details] [diff] [review]
947374-certified-origin.patch

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

I talked to Jed on Friday about this and I thought there was a way to simplify this?

Comment 5

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

Yes, as MattN pointed out, for packaged apps we just need to take the origin from their manifest files.  There's no point in them requesting the same origin; that's just busy work.

Comment 6

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

... and presumably the origin in their manifest file is the one FirefoxOS is going to report for them.  If so, we don't need to do anything for packaged apps.

Comment 7

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

Attachment: declare audience in request() for certified apps - WIP

:ggp this is pure work in progress, but it should unblock you

Comment 8

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

I'm stuck trying to figure out how to test this with chrome mochitests.  (I want to be able to change the appStatus of an iframe spawned by the chrome test app.)

I will focus for now on trying to do this in b2g in a way that can be driven by marionette tests.

Comment 9

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

I spoke with :fabrice on IRC.  He told me there'd be no way to adjust an iframe's appStatus from a chrome mochitest, but that it might be possible to install the same app code with different manifests to get it a different appStatus each time.

Comment 10

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

Modifying Comment 0, I think it's going to be necessary to declare the audience in watch(), not request(), so that the preferred audience can become part of the permanent state of the RP.  This will be necessary for the issuing of silent assertions when watch() is called.

This will allow the automatic login of users when an fxa app is opened.  It will also make it possible for an app that is re-awakened to know what its login state should be (receiving either onlogin or onlogout when watch is called, depending on the user's signed-in status).

Automatic assertions or logout are part of Bug 945363.

Comment 11

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

Attachment: declare audience in watch() for certified apps - WIP

Rebased

So that SSO works, we'll need to declare the desired audience in watch(), not request().  So for example:

  watch({wantIssuer: "firefox-accounts",
         audience: "https://youre.aweso.me"
         // etc.
  });

Comment 12

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

Attachment: declare audience in watch() for certified apps

Was not merging well with master.  Rebased.

Comment 13

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

O frabjous day! Callooh! Callay!

I've figured out how to alter the appStatus of the iframe in chrome mochitests.   O beamish mochitests.

Comment 14

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

Attachment: declare audience in watch() for certified apps

Now, with tests!

- Added check to nsDOMIdentity to prevent underprivileged apps from using fxa
  (We'll need to refine this later; but for now, I think it's right?)
- Explain policy violations via RP's onerror(), if available
- Added mochitests to check:
  - installed apps can't play at all
  - privileged apps can only declare audience matching origin (just checking)
  - certified apps can declare any audience

spenrose, does the logic seem ok to you?

To run the tests: mach mochitest-chrome dom/identity

Comment 15

[Sam Penrose]

Comment on attachment 8383328 [details] [diff] [review]
declare audience in watch() for certified apps

The logic looks good. I'm not able to run mochitest for some reason, but the tests are not subtle (that's a good thing) so if they work for you I think that's enough.

Comment 16

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

Thanks, Sam!

(It may be necessary to 'mach build dom/identity browser/app' to get all the manifest files updated?)

Comment 17

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

Try push:

https://tbpl.mozilla.org/?tree=Try&rev=01fc152544e5

Comment 18

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

Attachment: declare audience in watch() for certified apps

rebased

Comment 19

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

Comment on attachment 8384776 [details] [diff] [review]
declare audience in watch() for certified apps

Hi, smaug,

This patch makes it possible for certified apps to declare an audience for Firefox Accounts and Persona assertions that is different from their actual origin.  This makes life better for services like Marketplace and Wheres-My-Fox.  (Formerly they would have to accept all gaiamobile.org assertions, or they would have to perform the sign-in flow in an iframe hosted from their remote services in order to ensure that identity assertions would contain their "real" origins.)

Thanks for your review!
j

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8384776 [details] [diff] [review]
declare audience in watch() for certified apps

Hmm, errors aren't localizable. They should be, I think.
It is not clear to me whether Error exceptions should be
localizeable too (they are used in the existing code).

Comment 22

[Axel Hecht [:Pike]]

Oh, this is b2g, there's extra funky:

We don't ship localized gecko's on b2g in general, so we're trying to get all user-visible strings localized on the gaia side. You basically encode the message on gecko, catch it in gaia, and then translate it there.

No idea how that maps to this patch and the ecosystem it lives in.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8384776 [details] [diff] [review]
declare audience in watch() for certified apps

I don't feel comfortable reviewing this, since I'm not familiar with the
different types of b2g apps.

Comment 24

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

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8384776 [details] [diff] [review]
> declare audience in watch() for certified apps
> 
> I don't feel comfortable reviewing this, since I'm not familiar with the
> different types of b2g apps.

Ok.  Thanks for taking a look at it anyway, smaug.

Comment 25

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

Comment on attachment 8384776 [details] [diff] [review]
declare audience in watch() for certified apps

Hi, Fernando, do you mind giving this a review?  Many thanks,
j

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8384776 [details] [diff] [review]
declare audience in watch() for certified apps

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

Nice job Jed! I'd like to see a final version. Thanks!

::: dom/identity/DOMIdentity.jsm
@@ +193,5 @@
>  
>      let context = this._serviceContexts.get(message.id);
>      if (context.wantIssuer == "firefox-accounts") {
> +      if (Services.prefs.getPrefType(PREF_FXA_ENABLED) === Ci.nsIPrefBranch.PREF_BOOL
> +          && Services.prefs.getBoolPref(PREF_FXA_ENABLED)) {

Can this throw an exception?

::: dom/identity/nsDOMIdentity.js
@@ +70,5 @@
>      return true;
>    },
>  
> +  reportError: function(errorStr) {
> +    errorStr = "ERROR: " + errorStr;

These errors are exposed to content.

Can we pass key like errors to the '.onerror' callback? As in https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsCommon.js#79

You can keep the full error message to be shown in the error console via 'Cu.reportError'.

@@ +154,5 @@
> +      // We don't delete the rpWatcher object, because we don't want the
> +      // broken client to be able to call watch() any more.  It's broken.
> +    } else {
> +      this._identityInternal._mm.sendAsyncMessage("Identity:RP:Watch", message);
> +    }

nit:

if (message.error) {
  ...;
  return;
}

this._identityInternal. ... ;

@@ +576,5 @@
>  
>    /**
>     * Helper to create messages to send using a message manager.
>     * Pass through user options if they are not functions.  Always
> +   * overwrite id, origin, audience, and appStatus.  The caller

nit: extra comma (audience and ...)

@@ +603,5 @@
> +    // Firefox Accounts.
> +    if (this._appStatus !== principal.APP_STATUS_PRIVILEGED &&
> +        this._appStatus !== principal.APP_STATUS_CERTIFIED) {
> +      let errStr = "Must be a privileged or certified app to use Firefox Accounts";
> +      Cu.reportError(errStr);

It seems that this error will be reported later.

@@ +624,5 @@
> +        _audience = message.audience;
> +        this._log("Certified app setting assertion audience: " + _audience);
> +      } else {
> +        let errStr = "Assertion audience may not differ from origin";
> +        Cu.reportError(errStr);

Same as this one.

::: dom/identity/tests/mochitest/test_declareAudience.html
@@ +73,5 @@
> +    uri: "https://example.com/chrome/dom/identity/tests/mochitest/file_declareAudience.html",
> +    expected: {
> +      success: false,
> +      underprivileged: true,
> +    },

nit: extra comma, here and in the rest of apps.

Comment 27

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

(In reply to Fernando Jiménez Moreno [:ferjm] (Work week from 10/3 to 14/3. Low bugmail activity). from comment #26)
> Comment on attachment 8384776 [details] [diff] [review]
> declare audience in watch() for certified apps
> 
> Review of attachment 8384776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice job Jed! I'd like to see a final version. Thanks!

Thanks for your review, Fernando!  I will attach a final version.

> ::: dom/identity/DOMIdentity.jsm
> @@ +193,5 @@
> >  
> >      let context = this._serviceContexts.get(message.id);
> >      if (context.wantIssuer == "firefox-accounts") {
> > +      if (Services.prefs.getPrefType(PREF_FXA_ENABLED) === Ci.nsIPrefBranch.PREF_BOOL
> > +          && Services.prefs.getBoolPref(PREF_FXA_ENABLED)) {
> 
> Can this throw an exception?

I don't think so.  If you getPrefType() for a nonexistent pref, it returns 0.  So we should be able to getBoolPref once we know the type is PREF_BOOL.

You did however make me realize that I wasn't checking the pref type for PREF_SYNTHETIC_EVENTS_OK.  It seems unlikely that someone would set that to a non-bool value, especially as the pref is really just for testing, but I've added the check anyway.

> ::: dom/identity/nsDOMIdentity.js
> @@ +70,5 @@
> >      return true;
> >    },
> >  
> > +  reportError: function(errorStr) {
> > +    errorStr = "ERROR: " + errorStr;
> 
> These errors are exposed to content.
> 
> Can we pass key like errors to the '.onerror' callback? As in
> https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/
> FxAccountsCommon.js#79
> 
> You can keep the full error message to be shown in the error console via
> 'Cu.reportError'.

Sounds good.

As it's possible for a caller to trigger more than one error, I will now conceivably be calling onerror() more than once.  (One of the tests does this to show that an underprivileged app cannot set assertion values and cannot even use fxa to begin with.)  Is that ok?

Also, thanks for talking on IRC this morning about DOM string localizations, and confirming that we do not ship gecko localizations with b2g.  It would be nice to be able to localize these strings but oh well.

> @@ +154,5 @@
> > +      // We don't delete the rpWatcher object, because we don't want the
> > +      // broken client to be able to call watch() any more.  It's broken.
> > +    } else {
> > +      this._identityInternal._mm.sendAsyncMessage("Identity:RP:Watch", message);
> > +    }
> 
> nit:
> 
> if (message.error) {
>   ...;
>   return;
> }
>
> this._identityInternal. ... ;

Thanks.  Fixed.
 
> @@ +576,5 @@
> >  
> >    /**
> >     * Helper to create messages to send using a message manager.
> >     * Pass through user options if they are not functions.  Always
> > +   * overwrite id, origin, audience, and appStatus.  The caller
> 
> nit: extra comma (audience and ...)

I am sorry, but I must protest.  I feel strongly about the use of the Oxford Comma (aka the serial comma).  I beg you to reconsider.

First, on behalf of the lowly comma, consider that countless lives have been saved, merely by its proper use in vocative phrases ("Let's eat, children!" vs "Let's eat children!").  Or conversely, consider the dread we may feel if we detect its pause in the voice of another in certain situations (such as driving in a car: "What's that up the road ahead?" vs "What's that up in the road, a head?").

In serial lists, it can make the difference between an intended news headline ("Top stories: World leaders at Mandela tribute, Obama-Castro handshake, and same-sex marriage date set...") and something entirely different ("Top stories: World leaders at Mandela tribute, Obama-Castro handshake and same-sex marriage date set...") [1].

In a world without the Oxford Comma, we might read about Mozilla publicity events such as this: "Historic meeting of Firefox DOM peers, Miley Cyrus and Cher."

Perhaps it is to avoid such confusion that the MDN style guide prescribes the use of the serial comma [2].

You may take my vim or my emacs.  You may take my coffee, even.  But please, sir, do not deprive me of my dear, dear Oxford Comma.

[1] http://www.slate.com/blogs/lexicon_valley/2013/12/10/oxford_comma_sky_news_tweet_suggests_that_obama_and_castro_have_set_a_date.html
[2] https://developer.mozilla.org/en-US/docs/Project:MDC_style_guide#Serial_comma

> @@ +603,5 @@
> > +    // Firefox Accounts.
> > +    if (this._appStatus !== principal.APP_STATUS_PRIVILEGED &&
> > +        this._appStatus !== principal.APP_STATUS_CERTIFIED) {
> > +      let errStr = "Must be a privileged or certified app to use Firefox Accounts";
> > +      Cu.reportError(errStr);
> 
> It seems that this error will be reported later.

True.  Removed.

> @@ +624,5 @@
> > +        _audience = message.audience;
> > +        this._log("Certified app setting assertion audience: " + _audience);
> > +      } else {
> > +        let errStr = "Assertion audience may not differ from origin";
> > +        Cu.reportError(errStr);
> 
> Same as this one.

Ditto.

> ::: dom/identity/tests/mochitest/test_declareAudience.html
> @@ +73,5 @@
> > +    uri: "https://example.com/chrome/dom/identity/tests/mochitest/file_declareAudience.html",
> > +    expected: {
> > +      success: false,
> > +      underprivileged: true,
> > +    },
> 
> nit: extra comma, here and in the rest of apps.

You must think me a comma fanatic.

In this case, I was advised by a toolkit peer always to put a comma after the last item in a series so that somebody coming along at a later date would not break the blame history of the code by adding another line to the list (as they would by adding a comma to the preceding line).  It seems like good advice to me.

As far as I know, a terminal comma does not have a name, but it should.  The Mozilla Comma?  The Blame Comma?

As always, thanks for your review!,
Cheers,
j,

Comment 28

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

Attachment: 947374-certified-origin.patch

Thanks for your review, Fernando!

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8387751 [details] [diff] [review]
947374-certified-origin.patch

Thanks Jed!

Comment 30

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

Thank you, Fernando!

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8387751 [details] [diff] [review]
947374-certified-origin.patch

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

::: dom/identity/tests/mochitest/test_declareAudience.html
@@ +50,5 @@
> +    return deferred.promise;
> +  }
> +};
> +
> +let originalManager = FirefoxAccounts.fxAccountsManager;

I missed this. You need to restore the original manager

Comment 32

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

Good catch.  Fix coming.

Comment 33

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

Attachment: declare audience in watch() for certified apps

restores original fxa manager on mochitest finish
r=ferjm

Comment 34

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

Attachment: declare audience in watch() for certified apps

I found one more problem that the tests were not catching: We were always setting the rp watcher's audience to the message.audience, but we were not giving the message.audience a default value.  Thus normal assertions had a null audience.  Not good!  I've fixed the one line that needed fixing and added an additional test to catch this case.

Fernando, since this doesn't alter anything from the original intent, or make changes beyond what I just described, I'm going to trust that it still has your imprimatur and check it in.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/876ba986830c

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/876ba986830c