863417 - need b2g pref to change default persona uri

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Edwin Wong [:edwong]]

copied from: https://github.com/mozilla-b2g/gaia/issues/9254

Our goal is to test persona/browserid code on a b2g device as it moves to mainline browserid. To do this, we need a pref on a b2g device to use a specific browserid URL.

steps:
1. on b2g device, attempt to use browserid / sign in using persona.org

result: you can't it uses native.persona.org hard coded

expected: i should be able to set a pref somehwhere to test this config.

Comment 1

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

I think we should make the persona urls preffable, so that we can push prefs for testing localhost, dev, staging, production, and whatever else.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Jed Parsons [:jparsons] from comment #1)
> I think we should make the persona urls preffable, so that we can push prefs
> for testing localhost, dev, staging, production, and whatever else.

Right. A simple pref modified by pushing custom prefs would work here.

Comment 3

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

Attachment: gecko part: a pref to set the persona uri

Comment 4

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

Attachment: gaia part: a pref to set the persona uri

Comment 5

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

The attached patch will let you set toolkit.identity.uri to point to your persona service (e.g., http://127.0.0.1:10002 for local testing, or persona dev or stage, and, for now, https://login.native-persona.org for pre-merge integration testing).

I've set the default uri to be https://login.persona.org, which will "just work" when the persona b2g code makes it to persona.org (May or June).  In the interim weeks, users will have to set the pref to be "https://login.native-persona.org".  I think this is reasonable, since we ultimately want the default to be persona.org, and we don't want to have to change this again.

To set the pref, you would do this to your profile/user.js:

  pref("toolkit.identity.uri", "https://login.native-persona.org");

Ed, how does this sound?

/cc :benadida

Comment 6

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

Comment on attachment 743366 [details] [diff] [review]
gecko part: a pref to set the persona uri

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

Hi, Ben,
This patch allows you to set the persona uri via a pref
The purpose is to make it possible to test different persona deployments

Comment 7

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

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=6fff18a0e9f3

Comment 8

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

Attachment: gecko part: a pref to set the persona uri

removed two unnecessary prefs from modules/libpref
r=benadida

Comment 9

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

Attachment: gaia part: a pref to set the persona uri

r=benadida

I revised the PR so that it's against gaia master (not v1-train), which is what leo uplift rules in https://wiki.mozilla.org/Release_Management/B2G_Landing ask for

Comment 10

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

Spoke with jbonacci.  Sounds like this approach will work for QA.

Comment 11

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

This patch will also improve performance for mobile users tremendously.  If we do not merge it in, they will eventually be redirected from native-persona to persona.org, which will add at least 6 network requests.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Jed Parsons [:jparsons] from comment #11)
> This patch will also improve performance for mobile users tremendously.  If
> we do not merge it in, they will eventually be redirected from
> native-persona to persona.org, which will add at least 6 network requests.

This sounds like something that can resolved server-side to reduce the performance problems.

In terms of blocking, I don't think this blocks. It's not a partner performance requirement. The original bug as cited as well is a benefit to QA testing, not an impact to users.

We can track this though.

Comment 13

[Jason Smith [:jsmith]]

Ben's review is sufficient to land the gecko patch, but not the Gaia patch. You need a Gaia peer to give a r+ to land the gaia pieces.

Removing checkin-needed since the review isn't sufficient to land.

Comment 14

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

(In reply to Jason Smith [:jsmith] from comment #13)
> Ben's review is sufficient to land the gecko patch, but not the Gaia patch.
> You need a Gaia peer to give a r+ to land the gaia pieces.
> 
> Removing checkin-needed since the review isn't sufficient to land.

Jason thank you for that catch.

Comment 15

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

Comment on attachment 743798 [details]
gaia part: a pref to set the persona uri

Thank you for looking, Fabrice!

Comment 16

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

Comment on attachment 743784 [details] [diff] [review]
gecko part: a pref to set the persona uri

Ben, can you take another look?  I removed some needless cruft from the patch and forgot to ask for r again :(

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 743784 [details] [diff] [review]
gecko part: a pref to set the persona uri

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

::: b2g/components/SignInToWebsite.jsm
@@ +93,5 @@
> +let kPersonaUri = "https://login.persona.org";
> +try {
> +  kPersonaUri = Cc["@mozilla.org/preferences-service;1"]
> +                  .getService(Components.interfaces.nsIPrefService)
> +                  .getCharPref("toolkit.identity.uri");

You should use Services.prefs.getCharPref(...)

Comment 18

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

Attachment: gecko part: a pref to set the persona uri

Use Services.prefs; lazy getter, less typing; one less import (Cc)

Comment 19

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

(In reply to Fabrice Desré [:fabrice] from comment #17)
> Comment on attachment 743784 [details] [diff] [review]
> gecko part: a pref to set the persona uri
> 
> Review of attachment 743784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/SignInToWebsite.jsm
> @@ +93,5 @@
> > +let kPersonaUri = "https://login.persona.org";
> > +try {
> > +  kPersonaUri = Cc["@mozilla.org/preferences-service;1"]
> > +                  .getService(Components.interfaces.nsIPrefService)
> > +                  .getCharPref("toolkit.identity.uri");
> 
> You should use Services.prefs.getCharPref(...)

Thank you for pointing that out.  This is much better, and takes half the lines out of the overall patch!

Comment 20

[Daniel Coloma:dcoloma]

Based on comment 13, I don't think we should block on this

Comment 21

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

Attachment: gecko part: a pref to set the persona uri

Having worked with Lloyd and Francois and Ben, we have decided that firefoxos.persona.org is a better default pref for the url.

Comment 22

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

(In reply to Daniel Coloma:dcoloma from comment #20)
> Based on comment 13, I don't think we should block on this

Hi, Daniel,

Sorry about the mixup in the checkin protocol.  My bad for not getting a gaia peer in there right away.

So here's why we need this and why I think we should block on this:

We need to retire the native-persona.org ephemeral instance and get phones hitting the real persona.org when they go to market.

- If we simply turn native-persona into a proxy/redirect to persona.org:

  - Add 6+ network calls per transaction
    - that will undo months of speedup work
  - We would have to redeploy in localized regions
  - Way reduced reliability and visibility with ops
    - this was never a production deployment

- If we don't add the pref:

  - we can't do proper testing of our deployment tiers
  - we can't accommodate the thousands of people who are about to get geeksphones
    - because they'll need to set the pref the other way!
    - the geeksphone launch ahead of the date we were given for launch is a big deal

Adding the pref and the new default will ensure:

- High availability
- Proper speed on mobile
- Better maintenance and testing for persona and marketplace going forward

:benadida, :lloyd, please jump in if I missed anything or if you have additional concerns.

Comment 23

[Lloyd Hilaiel [:lloyd]]

yeah - bottom line is firefoxos.persona.org is going to be at least triple DC redundant and where we want traffic from the next billion smartphone users.

Further, we need a preference to toggle this url so that we can test identity changes and ensure there's no user impact.

This 9 line patch let's us support the traffic that FirefoxOS will bring without needlessly redundant infrastructure, and lets us actually test changes before we ship then.  

I feel like the late merge is warranted given the minimal risk.
This 9 line patch let's us support the traffic that FirefoxOS will bring without needlessly redundant infrastructure, and lets us actually test changes before we ship then.

Comment 24

[Jason Smith [:jsmith]]

(In reply to Jed Parsons [:jparsons] from comment #22)
> (In reply to Daniel Coloma:dcoloma from comment #20)
> > Based on comment 13, I don't think we should block on this
> 
> Hi, Daniel,
> 
> Sorry about the mixup in the checkin protocol.  My bad for not getting a
> gaia peer in there right away.
> 
> So here's why we need this and why I think we should block on this:
> 
> We need to retire the native-persona.org ephemeral instance and get phones
> hitting the real persona.org when they go to market.
> 
> - If we simply turn native-persona into a proxy/redirect to persona.org:
> 
>   - Add 6+ network calls per transaction
>     - that will undo months of speedup work

That's a good idea, but not a partner performance requirement.

>   - We would have to redeploy in localized regions
>   - Way reduced reliability and visibility with ops
>     - this was never a production deployment

There was a separate bug focusing on migrating to the main final persona server. Let's not convolute both concepts here. A decision was already made on that bug saying that we were not blocking on that because we could still function without it.

Keep in mind there's risk to switching things around last minute, especially when it hasn't been tested effectively.

> 
> - If we don't add the pref:
> 
>   - we can't do proper testing of our deployment tiers
>   - we can't accommodate the thousands of people who are about to get
> geeksphones
>     - because they'll need to set the pref the other way!
>     - the geeksphone launch ahead of the date we were given for launch is a
> big deal

None of these arguments constitute blocking. Testing-based bugs we can take into 1.1 on an approval for uplift. The geeksphone argument has no relevance either.

> 
> Adding the pref and the new default will ensure:
> 
> - High availability
> - Proper speed on mobile
> - Better maintenance and testing for persona and marketplace going forward
> 
> :benadida, :lloyd, please jump in if I missed anything or if you have
> additional concerns.

This is convoluting concepts of what actually blocks vs. not. High availability and proper speed on mobile can be resolved server side if need be. The testing piece is not necessary on a 1.01 branch at this time.

I still disagree this is a blocker as a result.

Comment 25

[[:fabrice] Fabrice Desré]

I also think this should not block, but this bug is good candidate for approval-b2g18

Comment 26

[Ben Adida [:benadida]]

(In reply to Jason Smith [:jsmith] from comment #24)
> This is convoluting concepts of what actually blocks vs. not. High
> availability and proper speed on mobile can be resolved server side if need
> be.

No, they cannot be resolved in the time we have without this change.

I understand that the general approach here is to say no to all new requests, but this is not optional. This needs to land or we cannot guarantee uptime at phone launch.

Comment 27

[[:fabrice] Fabrice Desré]

You could land a patch that just switches to the new host in Gaia. That could be a blocker. But all the mechanism that makes it a pref from gecko, no. You'll get it in 1.1 if you ask for approval.

And please stop saying that people "say no to all new requests", that's plain wrong.

Comment 28

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #27)
> You could land a patch that just switches to the new host in Gaia. That
> could be a blocker. But all the mechanism that makes it a pref from gecko,
> no. You'll get it in 1.1 if you ask for approval.
> 
> And please stop saying that people "say no to all new requests", that's
> plain wrong.

Correct. That's what bug 836928 was filed for originally.

However - there was discussion on that bug that constituted saying this might not be a blocker, however. But let's take the discussion about over to that bug.

As for this bug, this is still not a blocker.

Comment 29

[Lloyd Hilaiel [:lloyd]]

:jsmith - I ask you to reconsider the decision on the pref.  

Bottom line: We screwed up and we need help.

The full story: The way that Persona and marketplace are implemented on the phone, code is sent down from a server.  For Persona, that code will be changing every 2 weeks.  Every time we ship, we have a 2 week testing process where we will confirm on actual user devices that the new code won't break users ability to sign into websites or to buy applications at marketplace.

So this code freeze for the device software doesn't freeze persona code.

Without the ability to change a pref and vet changes, our QA team will need to create new builds every two weeks and reflash phones - this wastes time and is a real bug vector.  This introduces real risk that can translate into user impact.

So we have a choice here - We can accept nine lines of actual changes right now, and the risk that implies for the next week, OR we can incur equivalent risk every two weeks for the foreseeable future.

Did we screw up?  yes.  Should we have thought this through sooner?  yes.  Is it better for Mozilla to block this pref change than to ship it?  no.

Please give this some thought - the team is willing to work the weekend to vet the pref change or hotfix if there's any stability introduced.  We're willing to do whatever we have to to get it landed, because our QA team has said this is a requirement to ensure the ongoing stability of the authentication system on FirefoxOS.

Comment 30

[[:fabrice] Fabrice Desré]

ok.

Comment 31

[Jason Smith [:jsmith]]

Fair enough. I'll bend then so long as we're careful on this.

Comment 32

[Lloyd Hilaiel [:lloyd]]

You guys rule.  My cell is 720.209.2878, it's fully charged, and you can call me at any time if this decision causes us pain.

Comment 34

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

What Lloyd said.  Except that my cell is 510 710.5539.

Comment 35

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

tbpl:

https://tbpl.mozilla.org/?tree=Try&rev=6a3a7a87c0ca

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/d16b949edff0

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d16b949edff0

Comment 38

[Jason Smith [:jsmith]]

Reopening to land gaia piece as well

Comment 39

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

(In reply to Jason Smith [:jsmith] from comment #38)
> Reopening to land gaia piece as well

Thanks, Jason.  gaia part merged.

Comment 40

[Jason Smith [:jsmith]]

Okay. Now we can close.

Comment 41

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

1. please put the gaia commit hash here
2. the gaia commit broke the linter, please fix it now or I'll backout today.

thanks

Comment 42

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

(In reply to Julien Wajsberg [:julienw] from comment #41)
> 1. please put the gaia commit hash here

https://github.com/mozilla-b2g/gaia/commit/9dbb2d9ee919ea75d33be495ff710b7c3a3b24d9

> 2. the gaia commit broke the linter, please fix it now or I'll backout today.

here's the fix:
https://github.com/mozilla-b2g/gaia/pull/9592

Thanks, Julien.

Comment 43

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

Attachment: gaia part 2: appease linter by fixing line too long

r=julienw

Comment 44

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 746438 [details]
gaia part 2: appease linter by fixing line too long

r=me
go for it :)

Comment 45

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

gaia part 2 sha: https://github.com/jedp/gaia/commit/f48d569231583b91118f1c9c848796259e35aa7b

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ab8e87ee9693
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/68676ae6b10b

Leaving the status flags set to affected for the gaia uplifts.

Comment 47

[John Ford [:jhford] CET/CEST Berlin Time]

I squashed the commits for v1-train

v1-train: 51798e3c610f57796cd73068bace8718d6f0d00c

But this doesn't apply on v1.0.1 train.  Can you please resolve the merge conflicts there?

#	both modified:      apps/system/js/identity.js

++<<<<<<< HEAD
 +const kIdentityScreen = 'https://login.native-persona.org/sign_in#NATIVE';
 +const kIdentityFrame =
 +    'https://login.native-persona.org/communication_iframe';
++=======
+ const kIdentityScreen = '/sign_in#NATIVE';
+ const kIdentityFrame = '/communication_iframe';
++>>>>>>> 51798e3... Merge pull request #9490 from jedp/863417-prefs-for-persona-urls

To get to this point, you could do:

git checkout v1.0.1
git cherry-pick 51798e3c610f57796cd73068bace8718d6f0d00c
<resolve conflict>
git add apps/system/js/identity.js
git commit

Comment 48

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

John, thank you.
Where do you want me to push the resolved merge?

Comment 49

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

John, I opened a PR on v1.0.1 with merge resolved
https://github.com/mozilla-b2g/gaia/pull/9625

The sha is: 9ee341a81e4c3c9020f5328b68467c318f44d4cd

Is there anything more I should do?
Thanks,
j

Comment 50

[John Ford [:jhford] CET/CEST Berlin Time]

v1.0.1: 90a731c

Comment 51

[Deepa Subramanian]

Can you please provide more details to verify this fix - as Q Analysts can perform blackbox testing from the UI?