Closed Bug 1028398 Opened 10 years ago Closed 10 years ago

FxA will silently provide user's email to privileged apps in 2.0

Categories

(Firefox OS Graveyard :: FxA, defect)

Product:
Firefox OS Graveyard
Component:
FxA
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Status:
RESOLVED FIXED
Milestone:
2.0 S6 (18july)
Project Flags:
blocking-b2g 2.0+
Tracking Flags:
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: spenrose, Assigned: ferjm)

References

Details

(Whiteboard: ft:loop)

Attachments

(4 files, 9 obsolete files)

Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21268
46 bytes, text/x-github-pull-request
ggp
: review+
arthurcc
: review+
Details | Review
Part 1: Add "firefox-accounts" and "moz-firefox-accounts" permissions
17.50 KB, patch
fabrice
: review+
ferjm
: review+
Details | Diff | Splinter Review
Part 2: Trigger forceAuth UI when a new priviledged app tries to get an assertion
21.14 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
Part 3: Tests
19.53 KB, patch
jedp
: review+
Details | Diff | Splinter Review 
By allowing privileged apps to use FxA, we allow them access to the logged-in user's email address without notifying the user or giving them a chance to prevent the access. We can:

  1) Reverse the decision to allow privileged apps to access FxA.
  2) Whitelist the privileged apps, and mention them in login UX.
  3) Surface a dialog when the app calls navigator.mozId.watch().
  4) ?

The first option is a straightforward uplift for 2.0. The second is dicier, since we have missed string freeze. The third is probably a non-starter.
This patch adds the "firefox-accounts" permission to the permissions table and so allow us to limit the number of privileged consumers on the Marketplace side (solution 2 explained in the bug description)
Assignee: nobody → ferjmoreno
This WIP patch allows FxA to ask for explicit permission to the user to share a verified email with an specific RP. The permission is only asked once per origin and should be revocable from Settings.
This patch enables "solution 3" and requires also the patch that enables "solution 2".
Unfortunately the Gaia counter part of this patch will require new strings and so it might be too late for 2.0.
A few more details of what we discussed with the security team and the FxA folks.

We can basically target "solution 2" for 2.0. "solution 3" is out of 2.0 scope as it requires new strings and we are post string freeze.

Some more comments about this approach:

- This patch adds a "firefox-accounts" permission to the permission table.

- This permission is allowed by default to privileged and certified apps in 2.0.

- We rely on bug 1024396 to give this permission only to Loop in 2.0.

- In 2.1, we will change the permission for privileged apps from allow to prompt.

- We won't be prompting the user to give explicit permission to share her verified email to new privileged apps requesting it. This shouldn't be a problem as long as the permission is restricted by bug 1024396.

- We won't have the "firefox-accounts" permission listed in the "Apps Permissions" menu in the Settings app.

Alternative approach:

- If we don't want to rely on bug 1024396 restricting the "firefox-accounts" permission for Loop only, we can trigger the force authentication screen for every privileged app that asks for an fxa assertion. It is not exactly asking for explicit permission to share the verified email with a 3rd party app, but at least we put the user in context and make sure that the user can opt out.

- We will show this prompt only once per origin (unless the force authentication is explicitly triggered by the RP).

- We still won't have the "firefox-accounts" permission listed in the "App Permissions" in the Settings app. So the user won't be able to change her previous decision.

For 2.1, if we still want to do it, we should be able to show a more detailed permission prompt and to list the "firefox-accounts" permission in the Settings app.
Attachment #8444449 - Attachment is obsolete: true
Depends on: 1024396
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #3)
> Created attachment 8444580 [details] [diff] [review]
> Enables "solution 2" - Add "firefox-accounts" permission (WIP)
> 
> A few more details of what we discussed with the security team and the FxA
> folks.
> 
> We can basically target "solution 2" for 2.0. "solution 3" is out of 2.0
> scope as it requires new strings and we are post string freeze.
> 
> Some more comments about this approach:
> 
> - This patch adds a "firefox-accounts" permission to the permission table.
> 
> - This permission is allowed by default to privileged and certified apps in
> 2.0.
> 
> - We rely on bug 1024396 to give this permission only to Loop in 2.0.
> 
> - In 2.1, we will change the permission for privileged apps from allow to
> prompt.
> 
> - We won't be prompting the user to give explicit permission to share her
> verified email to new privileged apps requesting it. This shouldn't be a
> problem as long as the permission is restricted by bug 1024396.
> 
> - We won't have the "firefox-accounts" permission listed in the "Apps
> Permissions" menu in the Settings app.

That all sounds ok with me, given the time constraints.


> 
> Alternative approach:
> 
> - If we don't want to rely on bug 1024396 restricting the "firefox-accounts"
> permission for Loop only, we can trigger the force authentication screen for
> every privileged app that asks for an fxa assertion. It is not exactly
> asking for explicit permission to share the verified email with a 3rd party
> app, but at least we put the user in context and make sure that the user can
> opt out.
> 
> - We will show this prompt only once per origin (unless the force
> authentication is explicitly triggered by the RP).
> 
> - We still won't have the "firefox-accounts" permission listed in the "App
> Permissions" in the Settings app. So the user won't be able to change her
> previous decision.

I think this is the killer here - I worry that not providing a way for the user to revoke access in the future on a per-app basic leaves the user with a poor UX. Interested on what other people think though.

I am actually also interested in what the plan is for marketplace, which is also a privileged app and I assume also is expecting to use Firefox Accounts? I assume the authorization could be handled by 1024396 though.

> 
> For 2.1, if we still want to do it, we should be able to show a more
> detailed permission prompt and to list the "firefox-accounts" permission in
> the Settings app.
(In reply to Paul Theriault [:pauljt] from comment #4)

> I am actually also interested in what the plan is for marketplace, which is
> also a privileged app and I assume also is expecting to use Firefox
> Accounts? I assume the authorization could be handled by 1024396 though.
> 

I don't think bug 1024396 applies to the Marketplace. Bug 1024396 applies only to apps that can be installed from the Marketplace and you can't install the Marketplace from the Marketplace :). The Marketplace app is always going to be pre-installed so it should be ok just by adding the "firefox-accounts" permission to its manifest.
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> (In reply to Paul Theriault [:pauljt] from comment #4)
> 
> > I am actually also interested in what the plan is for marketplace, which is
> > also a privileged app and I assume also is expecting to use Firefox
> > Accounts? I assume the authorization could be handled by 1024396 though.
> > 
> 
> I don't think bug 1024396 applies to the Marketplace. Bug 1024396 applies
> only to apps that can be installed from the Marketplace and you can't
> install the Marketplace from the Marketplace :). The Marketplace app is
> always going to be pre-installed so it should be ok just by adding the
> "firefox-accounts" permission to its manifest.

I may be getting pedantic but technically you can install the marketplace from the marketplace (https://marketplace.firefox.com/app/marketplace) which is how you'd install the app on a platform that isn't FxOS.  I also think bug 1024396 would apply in that case though so no need for anything special.
Whiteboard: ft:loop
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #3)
> - This permission is allowed by default to privileged and certified apps in
> 2.0.
> 
> - We rely on bug 1024396 to give this permission only to Loop in 2.0.
> 
> - In 2.1, we will change the permission for privileged apps from allow to
> prompt.

if this is the route that we're going down there needs to be separate permissions created just for 2.0.  We can't use the name 'firefoxaccounts' in both 2.0 and 2.1. Otherwise, for *all time* any app granted approval for using firefoxaccounts and then installed on 2.0 will get allow.  We don't live in a world where once 2.1 is released no-one will have a 2.0 phone and install apps on it.

To put it another way, Marketplace can't give approval for only Loop to use the permission in 2.0.  Marketplace can give only Loop approval to use a specific permission forever; that specific permission only exist in 2.0; and a different, more open, permission be allowed in 2.1 (which Loop would have too).
blocking-b2g: --- → 2.0?
Could we instead control privileged access via a pref in 2.0? IE grant the permission only if:
- the app is certified or,
- the app is privileged and it's origin matches a whitelist [loop, marketplace etc]
feature-b2g: --- → 2.0
Feature work, so setting the feature-b2g flag.
blocking-b2g: 2.0? → ---
(In reply to Paul Theriault [:pauljt] from comment #8)
> Could we instead control privileged access via a pref in 2.0? IE grant the
> permission only if:
> - the app is certified or,
> - the app is privileged and it's origin matches a whitelist [loop,
> marketplace etc]

We can do that, but it means that we are tied to an origin and that really scares me.

We have already some issues with the origin for Loop client like [1] and [2], but for both changes we have the flexibility of changing origins. Loop hosted web app, Loop server and Loop client can be modified at any time. But if we set an origin in this whitelist in the platform we won't be able to change it until the next release and that might be an issue. I would really prefer to have this whitelist remotely hosted (i.e. the Marketplace).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1008990#c4
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1023296
This feels like a blocker, not a feature.  We can't ship 2.0 without this being fixed (or disabling FxA access for priv apps).
blocking-b2g: --- → 2.0?
(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #11)
> This feels like a blocker, not a feature.  We can't ship 2.0 without this
> being fixed (or disabling FxA access for priv apps).

The patch I'm seeing here reads off as missed use cases that were not taken into account during requirements elicitation for the mobile ID API. Missed use cases fall under the scope of a feature, not a bug. The fact that this is preffed on right now with this problem is actually a separate problem - in reality, this API shouldn't even be preffed on if the feature work to grant privileged access isn't complete. We shouldn't try to beat the system here by flipping the switch to get a feature on & not have the security work complete to allow privileged access for the API.
Its not about gaming the system, this is a bug that resulted from landing a feature.  Not every problem related to a feature is a feature.  Its not a missing use case, its a security bug.
Regarding comment 10, seems like DNS is a pretty effective way of making changes to servers that are equivalent to changing the origin, without requiring client updates.
blocking-b2g: 2.0? → 2.0+
(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #13)
> Its not about gaming the system, this is a bug that resulted from landing a
> feature.  Not every problem related to a feature is a feature.  Its not a
> missing use case, its a security bug.

Rel man & QA disagree. Please file a separate bug for turning off FxA in privileged apps & re-pref this on after you get this fixed.
blocking-b2g: 2.0+ → ---
Blocking.  We need to fix this issue or back it out.  Do not reset this again without *talking* to me.
blocking-b2g: --- → 2.0+
(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #16)
> Blocking.  We need to fix this issue or back it out.  Do not reset this
> again without *talking* to me.

Then you should have came to triage where we already talked about this extensively. Please don't flip the flag again.
blocking-b2g: 2.0+ → ---
The *feature* was "let privileged apps use FxA". The feature has been working since maybe April. Security found a privacy bug *after* FL. Everyone worked in good faith here.
(In reply to Sam Penrose from comment #18)
> The *feature* was "let privileged apps use FxA". The feature has been
> working since maybe April. Security found a privacy bug *after* FL. Everyone
> worked in good faith here.

Will talk to you on vidyo to get on the same page on this.
Sam & I discussed this over vidyo. There's definite agreement that this is needed for 2.0. The debate that was happening here is a semantics issue for tracking. My perspective is arguing that this was missed feature work that wasn't found during the user story specification phase, so a feature flag made sense here. The other perspective argues that we've had this feature on at this point, but detected this later during a privacy discussion that this was a privacy issue that must be fixed in the 2.0 timeframe. On my side, my main root concern is making sure that we're conscious of risk for the work involved here, so I'd like to make sure that we understand the risk of the fix here involved. If there's potential regression risk, then I'd like to make sure that we should verify the patch here on trunk before we uplift.

So on that regard, I'm fine with tracking this with the blocking flag instead if that works better for the team, but I'd like to make sure QA does verification of the patch here when it lands on trunk if the patch is risky before we uplift this to 2.0.
blocking-b2g: --- → 2.0+
feature-b2g: 2.0 → ---
An attempt to summarize:

Marketplace is listed in the UX, so we don't need new permission from the user. There are two ways to inform the user: a static string, or a dialog. A string won't work if we ever let new privileged apps into the store that can invoke FxA on 2.0. We have the refreshAuth dialog now. I believe the revocation case for Loop is dubious, and in any case the user can sign out and sign back in again or remove the app. Therefore I believe the simplest path forward is:

1) A Gecko patch that calls refreshAuth the first time it sees a given privileged audience.

2) A way to prevent non-Loop privileged apps from being approved with FxA access in the marketplace. I would frankly prefer if this was bureaucratic rather than coded for now.

I do not think we need to decide on backwards compatibility if access opens up in 2.1 now.

In any case, we need to land on trunk ASAP, per comment 20. Does anyone disagree? Fernando, do you want to revise your patch along these lines:

  // persisted to storage and cleared on signOut
  this.privilegedPassedChallenge = {} 
  if (wantIssuer && app.privileged) {
    if (!this.privilegedPassedChallenge[app.audience]) {
      let ok = refreshAuth();
    }
    if (ok) {
      this.privilegedPassedChallenge[app.audience] = true
    }
  }

If we continue to have significant disagreement about product behavior, I would like to get Adam Rogers to be our arbiter as soon as possible, given the time frame.
Flags: needinfo?(ferjmoreno)
(In reply to Sam Penrose from comment #21)
> An attempt to summarize:
> 
> Marketplace is listed in the UX, so we don't need new permission from the
> user. There are two ways to inform the user: a static string, or a dialog. A
> string won't work if we ever let new privileged apps into the store that can
> invoke FxA on 2.0. We have the refreshAuth dialog now. I believe the
> revocation case for Loop is dubious, and in any case the user can sign out
> and sign back in again or remove the app. Therefore I believe the simplest
> path forward is:
> 
> 1) A Gecko patch that calls refreshAuth the first time it sees a given
> privileged audience.
> 
> 2) A way to prevent non-Loop privileged apps from being approved with FxA
> access in the marketplace. I would frankly prefer if this was bureaucratic
> rather than coded for now.
> 
> I do not think we need to decide on backwards compatibility if access opens
> up in 2.1 now.
> 
> In any case, we need to land on trunk ASAP, per comment 20. Does anyone
> disagree? Fernando, do you want to revise your patch along these lines:
> 
>   // persisted to storage and cleared on signOut
>   this.privilegedPassedChallenge = {} 
>   if (wantIssuer && app.privileged) {
>     if (!this.privilegedPassedChallenge[app.audience]) {
>       let ok = refreshAuth();
>     }
>     if (ok) {
>       this.privilegedPassedChallenge[app.audience] = true
>     }
>   }
> 
> If we continue to have significant disagreement about product behavior, I
> would like to get Adam Rogers to be our arbiter as soon as possible, given
> the time frame.

I think this will be ok. This is in system app I assume. I would probably add a check to make sure that the privileged app doing the requesting is the current foreground app as well. (to prevent apps asking from the background). (not essentially but would be better and safer UX I think)
PS I assume if we take this route, then we don't use a permission, right?
(In reply to Sam Penrose from comment #21)
> An attempt to summarize:
> 
> Marketplace is listed in the UX, so we don't need new permission from the
> user. There are two ways to inform the user: a static string, or a dialog. A
> string won't work if we ever let new privileged apps into the store that can
> invoke FxA on 2.0. We have the refreshAuth dialog now. I believe the
> revocation case for Loop is dubious, and in any case the user can sign out
> and sign back in again or remove the app. Therefore I believe the simplest
> path forward is:
> 
> 1) A Gecko patch that calls refreshAuth the first time it sees a given
> privileged audience.
> 
> 2) A way to prevent non-Loop privileged apps from being approved with FxA
> access in the marketplace. I would frankly prefer if this was bureaucratic
> rather than coded for now.
> 
> I do not think we need to decide on backwards compatibility if access opens
> up in 2.1 now.
> 
> In any case, we need to land on trunk ASAP, per comment 20. Does anyone
> disagree? Fernando, do you want to revise your patch along these lines:
> 
>   // persisted to storage and cleared on signOut
>   this.privilegedPassedChallenge = {} 
>   if (wantIssuer && app.privileged) {
>     if (!this.privilegedPassedChallenge[app.audience]) {
>       let ok = refreshAuth();
>     }
>     if (ok) {
>       this.privilegedPassedChallenge[app.audience] = true
>     }
>   }
> 
> If we continue to have significant disagreement about product behavior, I
> would like to get Adam Rogers to be our arbiter as soon as possible, given
> the time frame.

I am ok with this approach. I'll try to find some time to modify the patches according to this today or tomorrow.
Flags: needinfo?(ferjmoreno)
(In reply to Paul Theriault [:pauljt] from comment #22)
> 
> I think this will be ok. This is in system app I assume. I would probably
> add a check to make sure that the privileged app doing the requesting is the
> current foreground app as well. (to prevent apps asking from the
> background). (not essentially but would be better and safer UX I think)

No, this would still be Gecko code and we already do a check that makes navigator.mozId require explicit user action.
(In reply to Paul Theriault [:pauljt] from comment #23)
> PS I assume if we take this route, then we don't use a permission, right?

We still need the permission to fulfill point 2. of comment 21.

It'll also make the change for point 1. of comment 21 easier to do as we will be using the PermissionManager to check if we need to show the refreshAuth dialog or not for an specific origin instead of implementing a custom piece to handle this in the fxa code itself. Also, I guess that we will still want a permission in 2.1 to be able to show it in the Settings app to allow the user to revoke it, so I think having the code ready for that makes sense.
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #26)
> (In reply to Paul Theriault [:pauljt] from comment #23)
> > PS I assume if we take this route, then we don't use a permission, right?
> 
> We still need the permission to fulfill point 2. of comment 21.
> 
> It'll also make the change for point 1. of comment 21 easier to do as we
> will be using the PermissionManager to check if we need to show the
> refreshAuth dialog or not for an specific origin instead of implementing a
> custom piece to handle this in the fxa code itself. Also, I guess that we
> will still want a permission in 2.1 to be able to show it in the Settings
> app to allow the user to revoke it, so I think having the code ready for
> that makes sense.

I am not sure I understand the reason why PermissionManager is needed for 2.0. I have sketched a work around based around signing out of device -- will it not perform as I suggest? As for 2.1, there are several possibilities:

1) We kill mozId and go with Oauth or similar.
2) We continue with mozId, and allow one or two new privileged apps to access FxA.
3) We continue with mozId and allow a lot of privileged apps to access FxA.

Only in case 3) do we need a permissions UX, and only for users who

  i. don't upgrade to 2.1
  ii. do download lots of privileged apps
  iii. do give lots of them permission to use FxA via refreshAuth
  iv. decide to revoke an apps permissions but don't want to delete it

Those users will find themselves in an awkward, but not horrible, state. They have to sign out, then sign back in in a number of apps. No one else will be affected (unless I am missing something, please let me know). I submit that this is not going to affect many people. More to the point, it is a product experience decision to decide the value here. Fernando if invoking the PermissionsManager is low cost for us, then fine, let's do it. But if it makes the patch much riskier, then I think we ought to ask product how important the use case is. (Of course, we also ought to check my reasoning about the use case).
Agree with Lucas re. blocking on this - let's get Fernando's patch (whitelist of apps) landed for 2.0 to support Loop and FMD.  We'll revisit a more robust security model in 2.1

Thanks, 

Adam
(In reply to Sam Penrose from comment #27)
> Fernando if
> invoking the PermissionsManager is low cost for us, then fine, let's do it.
> But if it makes the patch much riskier, then I think we ought to ask product
> how important the use case is. (Of course, we also ought to check my
> reasoning about the use case).

It actually makes things easier and less riskier as the PermissionManager will be the one keeping the state of origins that require the refreshAuth dialog (basically those with PROMPT_ACTION will need it and those with ALLOW_ACTION won't) and not a custom state saved in the fxa code (this.privilegedPassedChallenge). And in any case, we would already be using it to check for the "tmp-firefox-accounts" permission existence in the manifest, so why not using it also to make the decision about showing or not the refreshAuth dialog :). The PermissionManager is there for this kind of things and in the end we are considering the refreshAuth dialog as the permission prompt for privileged apps.
(In reply to Adam Rogers (:arog) from comment #28)
> Agree with Lucas re. blocking on this - let's get Fernando's patch
> (whitelist of apps) landed for 2.0 to support Loop and FMD.  We'll revisit a
> more robust security model in 2.1
> 
> Thanks, 
> 
> Adam

Thanks Adam.

Just to double check. The whitelist won't be part of these patches, but instead we will be adding a "temp-firefox-accounts" permission to allow the Marketplace to keep this whitelist. Also FMD doesn't need this permission as it is certified.
(In reply to Adam Rogers (:arog) from comment #28)
> Agree with Lucas re. blocking on this - let's get Fernando's patch
> (whitelist of apps) landed for 2.0 to support Loop and FMD.  We'll revisit a
> more robust security model in 2.1

That wasn't the point of the debate here. The debate in question was claiming this was feature work, not a bug. Both workflows that were proposed indicate this needed for 2.0, but there's a difference of how the patch is handled post landing based on the flag.
(In reply to Andrew Williamson [:eviljeff] from comment #7)
> 
> if this is the route that we're going down there needs to be separate
> permissions created just for 2.0.  We can't use the name 'firefoxaccounts'
> in both 2.0 and 2.1. Otherwise, for *all time* any app granted approval for
> using firefoxaccounts and then installed on 2.0 will get allow.  We don't
> live in a world where once 2.1 is released no-one will have a 2.0 phone and
> install apps on it.
> 
> To put it another way, Marketplace can't give approval for only Loop to use
> the permission in 2.0.  Marketplace can give only Loop approval to use a
> specific permission forever; that specific permission only exist in 2.0; and
> a different, more open, permission be allowed in 2.1 (which Loop would have
> too).

Thanks Andrew. I'll add the "temp-firefox-accounts" or "privileged-firefox-accounts" or similar permission name instead of simply "firefox-accounts" to make it clear that it only applies to these special cases.
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #29)
> (In reply to Sam Penrose from comment #27)
> > Fernando if
> > invoking the PermissionsManager is low cost for us, then fine, let's do it.
> > But if it makes the patch much riskier, then I think we ought to ask product
> > how important the use case is. (Of course, we also ought to check my
> > reasoning about the use case).
> 
> It actually makes things easier and less riskier as the PermissionManager
> will be the one keeping the state of origins that require the refreshAuth
> dialog (basically those with PROMPT_ACTION will need it and those with
> ALLOW_ACTION won't) and not a custom state saved in the fxa code
> (this.privilegedPassedChallenge). And in any case, we would already be using
> it to check for the "tmp-firefox-accounts" permission existence in the
> manifest, so why not using it also to make the decision about showing or not
> the refreshAuth dialog :). The PermissionManager is there for this kind of
> things and in the end we are considering the refreshAuth dialog as the
> permission prompt for privileged apps.

That sounds great. Thanks for the explanation, and let me know if there is anything I can do to help.
(In reply to Wil Clouser [:clouserw] from comment #6)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> > (In reply to Paul Theriault [:pauljt] from comment #4)
> > 
> > > I am actually also interested in what the plan is for marketplace, which is
> > > also a privileged app and I assume also is expecting to use Firefox
> > > Accounts? I assume the authorization could be handled by 1024396 though.
> > > 
> > 
> > I don't think bug 1024396 applies to the Marketplace. Bug 1024396 applies
> > only to apps that can be installed from the Marketplace and you can't
> > install the Marketplace from the Marketplace :). The Marketplace app is
> > always going to be pre-installed so it should be ok just by adding the
> > "firefox-accounts" permission to its manifest.
> 
> I may be getting pedantic but technically you can install the marketplace
> from the marketplace (https://marketplace.firefox.com/app/marketplace) which
> is how you'd install the app on a platform that isn't FxOS.  I also think
> bug 1024396 would apply in that case though so no need for anything special.


Ugh, I missed this one. That's true.

In this case the hosted Marketplace won't have access to Firefox Accounts as hosted apps has no access to it yet, so I guess we won't have the Marketplace using Firefox Accounts in 2.0. Is that correct? Or will we have the privileged app using FxA and the hosted app using Persona?
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #34)
> (In reply to Wil Clouser [:clouserw] from comment #6)
> > (In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> > > (In reply to Paul Theriault [:pauljt] from comment #4)
> > > 
> > > > I am actually also interested in what the plan is for marketplace, which is
> > > > also a privileged app and I assume also is expecting to use Firefox
> > > > Accounts? I assume the authorization could be handled by 1024396 though.
> > > > 
> > > 
> > > I don't think bug 1024396 applies to the Marketplace. Bug 1024396 applies
> > > only to apps that can be installed from the Marketplace and you can't
> > > install the Marketplace from the Marketplace :). The Marketplace app is
> > > always going to be pre-installed so it should be ok just by adding the
> > > "firefox-accounts" permission to its manifest.
> > 
> > I may be getting pedantic but technically you can install the marketplace
> > from the marketplace (https://marketplace.firefox.com/app/marketplace) which
> > is how you'd install the app on a platform that isn't FxOS.  I also think
> > bug 1024396 would apply in that case though so no need for anything special.
> 
> 
> Ugh, I missed this one. That's true.
> 
> In this case the hosted Marketplace won't have access to Firefox Accounts as
> hosted apps has no access to it yet, so I guess we won't have the
> Marketplace using Firefox Accounts in 2.0. Is that correct? Or will we have
> the privileged app using FxA and the hosted app using Persona?

It will use the FxA for web flow.  (Pretty much what Persona does, but for FxA).  The FxA team has recently finished work on that flow and we're already implementing it in the Marketplace.
In reply to Sam Penrose from comment #21)
> 2) A way to prevent non-Loop privileged apps from being approved with FxA
> access in the marketplace. I would frankly prefer if this was bureaucratic
> rather than coded for now.

Assuming this point hasn't been nullified by later comments/work: for this to happen Marketplace needs a unique permission to check while FxA api is limited to Loop.  E.g moz-firefox-accounts in 2.0 and changed to firefox-accounts in whatever version of FxOS (2.1?) the api is available to all privileged apps.  And something different we can feature detect too ideally.
Jed, this patch adds the "moz-firefox-accounts" permission which is ALLOW_ACTION for certified apps and PROMPT_ACTION for privileged apps. It also moves the permission check to the parent.
Attachment #8444451 - Attachment is obsolete: true
Attachment #8444580 - Attachment is obsolete: true
Attachment #8449289 - Flags: review?(jed+bmo)
Jed, this patch triggers the forceAuth UI when a new privileged app tries to get an assertion. The UI is only triggered once as the user action will be remembered by default.
Attachment #8449295 - Flags: review?(jed+bmo)
Because we added the "moz-firefox-accounts" permission, we need to move the test app to the privileged test apps with the new permission.
Attachment #8449299 - Flags: review?(jed+bmo)
Zac, I believe there is no Gaia UI-test testing the FxA flow right now. Is that correct?
Flags: needinfo?(zcampbell)
That's correct ferjm, I believe rpappo was working on some but they haven't landed yet.
Flags: needinfo?(zcampbell)
ferjm sorry for churn but see my comment here: See my comment https://bugzilla.mozilla.org/show_bug.cgi?id=1024396#c26

Basically if my proposal is sane, what we do is just add firefox-accounts as a CERTIFIED permission (not privileged), and then add a hack to the permissions installer so that privileged apps can request the pre-fixed version. And then marketplace can enforce the restriction on the pre-fixed version for 2.0.

But maybe wait until fabrice provides feedback on my proposal before changing back.
Attachment #8449289 - Flags: review?(jed+bmo)
Attachment #8449295 - Flags: review?(jed+bmo)
Attachment #8449299 - Flags: review?(jed+bmo)
(In reply to Paul Theriault [:pauljt] from comment #42)
> ferjm sorry for churn but see my comment here: See my comment
> https://bugzilla.mozilla.org/show_bug.cgi?id=1024396#c26
> 
> Basically if my proposal is sane, what we do is just add firefox-accounts as
> a CERTIFIED permission (not privileged), and then add a hack to the
> permissions installer so that privileged apps can request the pre-fixed
> version. And then marketplace can enforce the restriction on the pre-fixed
> version for 2.0.
> 
> But maybe wait until fabrice provides feedback on my proposal before
> changing back.

Thanks Paul. I've clear the r? flags until we have an agreement for bug1024396
OK we have the solution from Fabrice here so we can go with that I think (ie use the substitute option). Thanks.

        Attachment #8449289 -
        Attachment is obsolete: true

        Attachment #8450053 -
        Flags: review?(jed+bmo)

    
    

    
  


  
Patches updated with Fabrice's suggestion at https://bugzilla.mozilla.org/show_bug.cgi?id=1024396#c29

        Attachment #8449295 -
        Attachment is obsolete: true

        Attachment #8450054 -
        Flags: review?(jed+bmo)

    
    

    
  
Comment on attachment 8449299 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21268

This patch does the following:

- Moves the FxA UI test to the uitest-privileged app and adds the "priv-firefox-accounts" permission to its manifest.

- Adds the "firefox-accounts" to the Settings app and FMD manifests.

        Attachment #8449299 -
        Flags: review?(zcampbell)

        Attachment #8449299 -
        Flags: review?(ggoncalves)

        Attachment #8449299 -
        Flags: review?(arthur.chen)

    
  

        Attachment #8450053 -
        Flags: review?(fabrice)

    
    

    
  
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #47)
> adds the "priv-firefox-accounts" permission to its manifest.

not that it makes any difference to the implementation, but why the change to priv-firefox-accounts from moz-firefox-accounts?

    
    

    
  
Comment on attachment 8449299 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21268

This doens't affect any tests so nothing for me to r? here

        Attachment #8449299 -
        Flags: review?(zcampbell)

    
    

    
  
(In reply to Andrew Williamson [:eviljeff] from comment #48)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #47)
> > adds the "priv-firefox-accounts" permission to its manifest.
> 
> not that it makes any difference to the implementation, but why the change
> to priv-firefox-accounts from moz-firefox-accounts?

I just followed the suggestion at https://bugzilla.mozilla.org/show_bug.cgi?id=1024396#c29 The priv-* prefix is probably more specific and self explanatory than moz-*, which was also used to prefix APIs. But in any case, if you think that moz-* is better for any reason, I'll change it.

    
    

    
  
Jed, I am working on the dom/identity tests. I'll upload a third part as soon as I have them.

        Attachment #8449299 -
        Flags: review?(ggoncalves) → review+

    
    

    
  
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #51)
> Jed, I am working on the dom/identity tests. I'll upload a third part as
> soon as I have them.

Thank you

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3: Tests (obsolete)
        — 
          — Splinter Review
      

    


  
Jed, I had to add the permission to the tests and remove the ones that didn't apply anymore (like checking the appStatus for privileged or certified). Since we are throwing an exception when there is no permission, I couldn't add any test for that situation as the exception is not being catch in the .watch or .request context. If you think there's a way to test this, I'll be glad to follow your suggestions :). Thanks!

        Attachment #8450468 -
        Flags: review?(jed+bmo)

    
    

    
  
Comment on attachment 8450053 [details] [diff] [review]
Part 1: Add "firefox-accounts" and "priv-firefox-accounts" permissions

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

This looks good to me.  Thanks for moving the permissions checks to the parent process!  My one qualm is in the naming of the permissions.

::: dom/apps/src/PermissionsTable.jsm
@@ +340,5 @@
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           "priv-firefox-accounts": {

Would it be better to exchange the pattern on these names?  What if certified apps got "firefox-accounts-certified" permission, and privileged apps applied for "firefox-accounts"?  The name "priv-firefox-accounts" feels cramped and a little unclear to me, and I fear it may be confusing to app developers.

::: dom/identity/DOMIdentity.jsm
@@ +270,5 @@
>      let targetMM = aMessage.target;
>  
> +    if (!this.hasPermission(aMessage)) {
> +      throw new Error("PERMISSION_DENIED");
> +    }

Thank you for moving the permission test to the parent process.  This should close Bug 1028769.

        Attachment #8450053 -
        Flags: review?(jed+bmo) → review+

    
    

    
  
Comment on attachment 8450054 [details] [diff] [review]
Part 2: Trigger forceAuth UI when a new priviledged app tries to get an assertion

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

It appears that the arguments to _refreshAuthentication are not always given in the right order.

Also, while it looks like you ensure that the user can never be stuck with an eternal pendingRequest after cancel or error, don't we now need to add an observer for shutdown notifications in case we get killed?

r=me with _refreshAuth arguments fixed.  If we need to observe for shutdown notifications, I'm fine having a follow-up bug for that so that we can get this patch in front of QA as soon as possible.

Thanks, Fernando.

::: dom/identity/DOMIdentity.jsm
@@ +282,5 @@
>        case "Identity:RP:Unwatch":
>          this._unwatch(msg, targetMM);
>          break;
>        case "Identity:RP:Request":
> +        this._request(msg);

Thanks for removing the extraneous second argument from request and logout.

::: services/fxaccounts/FxAccountsManager.jsm
@@ +181,5 @@
>            if (exists) {
>              return this.getAccount().then(
>                (user) => {
> +                return this._refreshAuthentication(aAudience, aPrincipal,
> +                                                   user.email, true);

The second and third arguments are reversed here (principal and email).

@@ +534,3 @@
>              if (secondsSinceAuth > gracePeriod) {
> +              return this._refreshAuthentication(aAudience, principal,
> +                                                 user.email);

principal and email params are reversed

@@ +551,5 @@
> +          if (permission == Ci.nsIPermissionManager.PROMPT_ACTION &&
> +              !this._refreshing) {
> +            return this._refreshAuthentication(aAudience, user.email,
> +                                               false /* logoutOnFailure */,
> +                                               principal);

Here the logoutOnFailure and principal params are reversed.  (Also, you won't need to specify logoutOnFailure, though I do like the extra information it provides.)

I do think the way you've got the signature for _refreshAuth at line 233 is correct (audience, email, principal, logoutOnFailure=false)

        Attachment #8450054 -
        Flags: review?(jed+bmo) → review+

    
    

    
  
Comment on attachment 8450468 [details] [diff] [review]
Part 3: Tests

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

I'm getting two errors when running mochitest-chrome for dom/identity:

0:45.93 23 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/identity/tests/mochitest/test_declareAudience.html | Assertion count 1 is greater than expected range 0-0 assertions.
0:45.93 44 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/identity/tests/mochitest/test_syntheticEvents.html | Assertion count 1 is greater than expected range 0-0 assertions.

    
    

    
  
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #54)
> Comment on attachment 8450053 [details] [diff] [review]
> Part 1: Add "firefox-accounts" and "priv-firefox-accounts" permissions
> 
> Review of attachment 8450053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me.  Thanks for moving the permissions checks to the
> parent process!  My one qualm is in the naming of the permissions.
> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +340,5 @@
> > +                             app: DENY_ACTION,
> > +                             privileged: DENY_ACTION,
> > +                             certified: ALLOW_ACTION
> > +                           },
> > +                           "priv-firefox-accounts": {
> 
> Would it be better to exchange the pattern on these names?  What if
> certified apps got "firefox-accounts-certified" permission, and privileged
> apps applied for "firefox-accounts"?  The name "priv-firefox-accounts" feels
> cramped and a little unclear to me, and I fear it may be confusing to app
> developers.

FYI this naming is for consistency with bug 1024396. We needed a temporary prefix to allow marketplace to distinguish between this API and when we expose Firefox accounts more broadly. It doesn't have to be priv, but that is what Fabrice suggested, and it needs to be the same. Probably not worth bikeshedding since this is only a temporary measure for 2.0.

    
    

    
  
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #56)
> Comment on attachment 8450468 [details] [diff] [review]
> Part 3: Tests
> 
> Review of attachment 8450468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm getting two errors when running mochitest-chrome for dom/identity:
> 
> 0:45.93 23 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/identity/tests/mochitest/
> test_declareAudience.html | Assertion count 1 is greater than expected range
> 0-0 assertions.
> 0:45.93 44 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/identity/tests/mochitest/
> test_syntheticEvents.html | Assertion count 1 is greater than expected range
> 0-0 assertions.

Thanks Jed. This didn't happen locally or in try https://tbpl.mozilla.org/?tree=Try&rev=b26672d3b55e but I'll take a look to see if I can reproduce it.

    
    

    
  
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #55)
> Comment on attachment 8450054 [details] [diff] [review]
> Part 2: Trigger forceAuth UI when a new priviledged app tries to get an
> assertion
> 
> Review of attachment 8450054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It appears that the arguments to _refreshAuthentication are not always given
> in the right order.

Oh right. Last minute change :). I'll fix it asap.

> 
> Also, while it looks like you ensure that the user can never be stuck with
> an eternal pendingRequest after cancel or error, don't we now need to add an
> observer for shutdown notifications in case we get killed?

Good point.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3: Tests (obsolete)
        — 
          — Splinter Review
      

    


  
I managed to reproduce the assertion on a desktop build. It seems that empty default CSP [1][2] are not allowed [3].

I couldn't reproduce the assertion any more after adding a default 'inline-script' CSP.

Try build: https://tbpl.mozilla.org/?tree=Try&rev=58f1a86b1a4e

Thanks Jed!

[1] https://mxr.mozilla.org/mozilla-central/source/dom/identity/tests/mochitest/test_declareAudience.html?force=1#263
[2] https://mxr.mozilla.org/mozilla-central/source/dom/identity/tests/mochitest/test_syntheticEvents.html?force=1#197
[3] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2794

        Attachment #8450468 -
        Attachment is obsolete: true

        Attachment #8450468 -
        Flags: review?(jed+bmo)

        Attachment #8450785 -
        Flags: review?(jed+bmo)

    
    

    
  
Comment on attachment 8450053 [details] [diff] [review]
Part 1: Add "firefox-accounts" and "priv-firefox-accounts" permissions

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

I want to make sure we are doing the right think with hasPermission().

::: dom/apps/src/PermissionsTable.jsm
@@ +340,5 @@
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,
> +                             certified: ALLOW_ACTION
> +                           },
> +                           "priv-firefox-accounts": {

I know I'm the one that proposed the "priv-" prefix but I think it's a bad choice now... how using "temp-" or "moz-" instead? "priv" was meant to mean private, but it's also confused with privileged.

::: dom/identity/DOMIdentity.jsm
@@ +255,5 @@
> +      let permission =
> +        permissionManager.testPermissionFromPrincipal(principal,
> +                                                      FXA_PERMISSION);
> +      return permission != Ci.nsIPermissionManager.UNKNOWN_ACTION &&
> +             permission != Ci.nsIPermissionManager.DENY_ACTION;

what if permission == Ci.nsIPermissionManager.PROMPT_ACTION ?

        Attachment #8450053 -
        Flags: review?(fabrice) → review-

    
    

    
  
(In reply to Fabrice Desré [:fabrice] from comment #61)
> Comment on attachment 8450053 [details] [diff] [review]
> Part 1: Add "firefox-accounts" and "priv-firefox-accounts" permissions
> 
> Review of attachment 8450053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I want to make sure we are doing the right think with hasPermission().
> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +340,5 @@
> > +                             app: DENY_ACTION,
> > +                             privileged: DENY_ACTION,
> > +                             certified: ALLOW_ACTION
> > +                           },
> > +                           "priv-firefox-accounts": {
> 
> I know I'm the one that proposed the "priv-" prefix but I think it's a bad
> choice now... how using "temp-" or "moz-" instead? "priv" was meant to mean
> private, but it's also confused with privileged.

Ok, I'll fix this.

> 
> ::: dom/identity/DOMIdentity.jsm
> @@ +255,5 @@
> > +      let permission =
> > +        permissionManager.testPermissionFromPrincipal(principal,
> > +                                                      FXA_PERMISSION);
> > +      return permission != Ci.nsIPermissionManager.UNKNOWN_ACTION &&
> > +             permission != Ci.nsIPermissionManager.DENY_ACTION;
> 
> what if permission == Ci.nsIPermissionManager.PROMPT_ACTION ?

We prompt :)

Take a look at the "Part 2" patch. We show the force authentication UI if the permission is set to PROMPT_ACTION and the permission as ALLOW_ACTION if the user introduced the password successfully. At the DOM level we only check that the permission is present in the manifest (!UKNOWN_ACTION) and its value is allow or prompt (!DENY_ACTION).

    
    

    
  
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #62)
> and the permission as ALLOW_ACTION if

"and SET the permission as ALLOW_ACTION if..."

    
    

    
  
Comment on attachment 8449299 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21268

In the patch fxa.js and fxa.html have been moved from uitest app to uitest-privileged app, but I guess we should still keep the fxa tests in uitest app, right?

        Attachment #8449299 -
        Flags: review?(arthur.chen)

    
    

    
  
(In reply to Arthur Chen [:arthurcc] from comment #64)
> Comment on attachment 8449299 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21268
> 
> In the patch fxa.js and fxa.html have been moved from uitest app to
> uitest-privileged app, but I guess we should still keep the fxa tests in
> uitest app, right?

Why?

With this patches non privileged apps won't be able to use Firefox Accounts so why do you think that we should keep the fxa tests in uitest app?

    
  
Flags: needinfo?(arthur.chen)

    
    

    
  
uitest app is a certified app and we use it to ensure that Firefox Accounts runs correctly on certified apps. Not sure if I understand this correctly.
Flags: needinfo?(arthur.chen)

    
    

    
  
Comment on attachment 8451633 [details] [diff] [review]
Part 1: Add "firefox-accounts" and "moz-firefox-accounts" permissions

r=jedp at comment 54

        Attachment #8451633 -
        Flags: review+

    
    

    
  


  
r=jedp at comment 55

I've fixed the _refreshAuthentication parameters.

It turns out that we are already doing the clean up when a child process dies at [1], so no need to do anything else about that.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/FirefoxAccounts.jsm#232

        Attachment #8451637 -
        Flags: review+

    
  

        Attachment #8449299 -
        Flags: review?(arthur.chen)

    
    

    
  
(In reply to Arthur Chen [:arthurcc] from comment #66)
> uitest app is a certified app and we use it to ensure that Firefox Accounts
> runs correctly on certified apps. Not sure if I understand this correctly.

That's correct. I didn't notice that uitest is already certified, I was thinking that it was a normal installed app. The name "uitest" vs "uitest-privileged" is misleading. Thanks Arthur!

    
    

    
  
> I've fixed the _refreshAuthentication parameters.

Splendid

> It turns out that we are already doing the clean up when a child process
> dies at [1], so no need to do anything else about that.

Ah, thank you.  Sorry I missed that.

FWIW, toolkit/identity/FirefoxAccounts.jsm in this patch now needs rebasing

Thanks, Fernando

    
    

    
  
I continue to get errors with mochitest-chrome on desktop firefox.  Maybe it's not a priority that these tests pass on desktop?  Sam, what do you think?
Flags: needinfo?(spenrose)

        Attachment #8451633 -
        Flags: review?(fabrice) → review+

    
    

    
  
Comment on attachment 8449299 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21268

Looks good to me, thank you.

        Attachment #8449299 -
        Flags: review?(arthur.chen) → review+

    
    

    
  
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #72)
> I continue to get errors with mochitest-chrome on desktop firefox.  Maybe
> it's not a priority that these tests pass on desktop?  Sam, what do you
> think?

I was able to bisect the errors down to the new blocks in dom/apps/src/PermissionsTable.jsm , but I switched onto a different patch queue, updated, and now I need to rebase. I do not think it is a fatal problem if tests that pass on try and don't target desktop fail on our builds. I do think we should figure it out, though, which probably means me. Fernando, if this feels safe to you I am fine to land it. What do you think?
Flags: needinfo?(spenrose)

    
    

    
  
Thanks Sam, I'd prefer to see what's going on and fix it if possible. I'll take another look.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3: Tests
        — 
          — Splinter Review
      

    


  
I think this should fix the tests. We were not handling the events properly in the tests. Could you try again Jed, please? Thanks!

        Attachment #8450785 -
        Attachment is obsolete: true

        Attachment #8450785 -
        Flags: review?(jed+bmo)

        Attachment #8453115 -
        Flags: review?(jed+bmo)

    
    

    
  
Comment on attachment 8453115 [details] [diff] [review]
Part 3: Tests

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

I'm sorry to have taken so long reviewing this.  I have been encountering an intermittent failure on test_syntheticEvents that I have been unable to track down.  It does not appear that any of the tests themselves are failing, but that something is breaking when the tests are cleaned up.  Since I appear to be the only person experiencing this problem, I'll chalk it up to some local issue on my machine, and hope that we don't start seeing intermittent oranges.

My only question is why one of the declareAudience tests was removed.  Can you either restore the test or explain why it should go?  Thanks!

That question aside, r=me

::: dom/identity/tests/mochitest/test_declareAudience.html
@@ -70,5 @@
>  // postMessage the results of this experiment back down to us, and we will
>  // compare it with the expected results.
>  let apps = [
>    {
> -    title: "an installed app, which should neither be able to use FxA, nor change audience",

This test seems to me to be an important safety check to ensure underprivileged apps cannot access privileged APIs.  The test still passes with the current changes, as well.  Why are you removing it?

@@ +89,5 @@
>      wantAudience: "https://i-like-pie.com",
>      uri: "https://example.com/chrome/dom/identity/tests/mochitest/file_declareAudience.html",
>      expected: {
>        success: false,
> +      underprivileged: true,

good catch, thanks

::: dom/identity/tests/mochitest/test_syntheticEvents.html
@@ -80,5 @@
>        ],
>      },
>    },
>    {
> -    title: "an installed app, which must may not use firefox accounts",

It makes sense to me to remove this test from this suite.  Testing for not-authorized-to-use-fxa does not pertain to synthetic events.

        Attachment #8453115 -
        Flags: review?(jed+bmo) → review+

    
    

    
  
Thanks Jed!

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #78)
> Comment on attachment 8453115 [details] [diff] [review]
> Part 3: Tests
> 
> Review of attachment 8453115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm sorry to have taken so long reviewing this.  I have been encountering an
> intermittent failure on test_syntheticEvents that I have been unable to
> track down.  It does not appear that any of the tests themselves are
> failing, but that something is breaking when the tests are cleaned up. 
> Since I appear to be the only person experiencing this problem, I'll chalk
> it up to some local issue on my machine, and hope that we don't start seeing
> intermittent oranges.

FWIW I keep seen green locally and in Try https://tbpl.mozilla.org/?tree=Try&rev=92506177d378

> 
> My only question is why one of the declareAudience tests was removed.  Can
> you either restore the test or explain why it should go?  Thanks!
>

I restored the test. I think I removed it in a previous approach and forgot to restore it. Thanks for the catch.

    
  

        Attachment #8450054 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8fd1d22c921c
https://hg.mozilla.org/mozilla-central/rev/61d9a5a59982
https://hg.mozilla.org/mozilla-central/rev/f87018e3d067
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: