Bug 2034430: unlock identity.fxaccounts.remote.root in browser_relay_signup_flow_showToAllBrowsers.js for enterprise builds

[victhorlopez]

Description

Bugzilla: Bug-2034430

The pref is locked in the test MOZ_BYPASS_FELT=1 ./mach mochitest toolkit/components/passwordmgr/test/browser/browser_relay_signup_flow_showToAllBrowsers.js

---

Dependencies / Related Issues

• Depends on:

---

Screenshots

---

Testing

• Added tests
• Manual testing performed

See issue
Here with the changes, passing

[1rneh]

My first thought was that this test can be skipped because Relay is a feature coupled to Firefox Accounts which is not supported in enterprise builds, hence this test should also not pass with an unlocked preference because Relay shouldn't be available/surfaced in enterprise builds in the first place.

[lissyx]

My first thought was that this test can be skipped because Relay is a feature coupled to Firefox Accounts which is not supported in enterprise builds, hence this test should also not pass with an unlocked preference because Relay shouldn't be available/surfaced in enterprise builds in the first place.

You're raising a good point, maybe the test should run on enterprise build to assert relay is properly blocked ?

[victhorlopez]

My first thought was that this test can be skipped because Relay is a feature coupled to Firefox Accounts which is not supported in enterprise builds, hence this test should also not pass with an unlocked preference because Relay shouldn't be available/surfaced in enterprise builds in the first place.

You're raising a good point, maybe the test should run on enterprise build to assert relay is properly blocked ?

Should we disable this pref?
https://searchfox.org/firefox-main/rev/a358f48a49a4d72df7b214805c4b2d2049d34a5a/browser/app/profile/firefox.js#2770

[lissyx]

My first thought was that this test can be skipped because Relay is a feature coupled to Firefox Accounts which is not supported in enterprise builds, hence this test should also not pass with an unlocked preference because Relay shouldn't be available/surfaced in enterprise builds in the first place.

You're raising a good point, maybe the test should run on enterprise build to assert relay is properly blocked ?

Should we disable this pref? https://searchfox.org/firefox-main/rev/a358f48a49a4d72df7b214805c4b2d2049d34a5a/browser/app/profile/firefox.js#2770

At first that may be a good idea, but maybe we want --disable-relay at build time? Would it be feasible?

[1rneh]

At first that may be a good idea, but maybe we want --disable-relay at build time? Would it be feasible?

The need is not so clear to me why relay should be disabled at build time since that's not what we do for other non-enterprise functionalities. Setting the preference to signon.firefoxRelay.feature=disabled as Victor suggested seems sufficient.

That what we do with browser.profiles.enabled in firefox-enterprise.js as well. That makes me think, how we override defaults is quite scattered. We disable profiles in firefox-enterprise.js but clear a bunch of preferences next to where they are originally set e.g. in firefox.js. Should we align our approaches and always override enterprise defaults next to where the non-enterprise defaults are defined?

[1rneh]

You're raising a good point, maybe the test should run on enterprise build to assert relay is properly blocked ?

I think we can still skip this one. But also add a test that's only run in enterprise builds that asserts that the expected relay item is not surfaced? That approach seems less invasive and less prone to merge conflicts?

[victhorlopez]

Okay so this is what I will do

• Close this PR, this test will be disabled
• Create a new test (in the same .toml enterprise only) that ensures the pref is disabled and that relay is not working
• action item for us, we need to clarify longterm if we want to disable features at compile time?

[1rneh]

Okay so this is what I will do

* Close this PR, this test will be disabled

You can also simply update this PR and leave a comment on the Bugzilla item explaining why we are skipping the test in enterprise builds. This way the contents of the conversation in this PR are not becoming obsolete.

* Create a new test (in the same .toml enterprise only) that ensures the pref is disabled and that relay is not working

Yes!

* action item for us, we need to clarify longterm if we want to disable features at compile time?

Maybe @lissyx can elaborate here a bit?

[lissyx]

At first that may be a good idea, but maybe we want --disable-relay at build time? Would it be feasible?

The need is not so clear to me why relay should be disabled at build time since that's not what we do for other non-enterprise functionalities. Setting the preference to signon.firefoxRelay.feature=disabled as Victor suggested seems sufficient.

Well, any feature we absolutely do not use, I think it's better the code is just not there to reduce risks, and it's something we would have on m-c, assuming relay would never be something we care about (which may not be true, I dont know).

Setting the preference may work, but i'm just unsure how all the rest of the code will behave when reading this: https://searchfox.org/firefox-main/rev/a358f48a49a4d72df7b214805c4b2d2049d34a5a/toolkit/components/satchel/integrations/FirefoxRelayUtils.sys.mjs#16-22

Probably easier to just ask to the people working on it what is the best course of action here

That what we do with browser.profiles.enabled in firefox-enterprise.js as well. That makes me think, how we override defaults is quite scattered. We disable profiles in firefox-enterprise.js but clear a bunch of preferences next to where they are originally set e.g. in firefox.js. Should we align our approaches and always override enterprise defaults next to where the non-enterprise defaults are defined?

I dont have an opinion yet on how we should do it, but I agree the current state is hard to track

[lissyx]

Okay so this is what I will do

* Close this PR, this test will be disabled

You can also simply update this PR and leave a comment on the Bugzilla item explaining why we are skipping the test in enterprise builds. This way the contents of the conversation in this PR are not becoming obsolete.

Yep, I agree on that

* Create a new test (in the same .toml enterprise only) that ensures the pref is disabled and that relay is not working

Yes!

👍

* action item for us, we need to clarify longterm if we want to disable features at compile time?

Maybe @lissyx can elaborate here a bit?

I'm not sure what I can elaborate more, I was just sharing the idea: if the feature is completely disabled and no code is compiled, then tests would not have to run as well

[1rneh]

At first that may be a good idea, but maybe we want --disable-relay at build time? Would it be feasible?

The need is not so clear to me why relay should be disabled at build time since that's not what we do for other non-enterprise functionalities. Setting the preference to signon.firefoxRelay.feature=disabled as Victor suggested seems sufficient.

Well, any feature we absolutely do not use, I think it's better the code is just not there to reduce risks, and it's something we would have on m-c, assuming relay would never be something we care about (which may not be true, I dont know).

Generally I agree, but so far it just hasn't been anything we explored for other non-enterprise functionalities unless for simple #ifdef exclusions at build time. So it's maybe worth coming up with a strategy here.

So I think this highlights a broader question about which services we want to offer in Firefox Enterprise. What do we do about Relay, Monitor and Mozilla VPN (Note: That's not Firefox VPN) in the enterprise builds? Do we disable all, only some, none? Let's discuss this with the team.

Setting the preference may work, but i'm just unsure how all the rest of the code will behave when reading this: https://searchfox.org/firefox-main/rev/a358f48a49a4d72df7b214805c4b2d2049d34a5a/toolkit/components/satchel/integrations/FirefoxRelayUtils.sys.mjs#16-22

Probably easier to just ask to the people working on it what is the best course of action here

Yes, 100%. I can help out with that, as I've worked on parts of the Relay integration into the password manager in my previous team :)

[victhorlopez]

At first that may be a good idea, but maybe we want --disable-relay at build time? Would it be feasible?

The need is not so clear to me why relay should be disabled at build time since that's not what we do for other non-enterprise functionalities. Setting the preference to signon.firefoxRelay.feature=disabled as Victor suggested seems sufficient.

Well, any feature we absolutely do not use, I think it's better the code is just not there to reduce risks, and it's something we would have on m-c, assuming relay would never be something we care about (which may not be true, I dont know).

Generally I agree, but so far it just hasn't been anything we explored for other non-enterprise functionalities unless for simple #ifdef exclusions at build time. So it's maybe worth coming up with a strategy here.

So I think this highlights a broader question about which services we want to offer in Firefox Enterprise. What do we do about Relay, Monitor and Mozilla VPN (Note: That's not Firefox VPN) in the enterprise builds? Do we disable all, only some, none? Let's discuss this with the team.

Setting the preference may work, but i'm just unsure how all the rest of the code will behave when reading this: https://searchfox.org/firefox-main/rev/a358f48a49a4d72df7b214805c4b2d2049d34a5a/toolkit/components/satchel/integrations/FirefoxRelayUtils.sys.mjs#16-22
Probably easier to just ask to the people working on it what is the best course of action here

Yes, 100%. I can help out with that, as I've worked on parts of the Relay integration into the password manager in my previous team :)

I asked here, https://matrix.to/#/!MkUoZtetjdyykPqtIH:mozilla.org/$sCcfL-iBcwQPFgHz1Cs-FZd--QFy88o0qyK_IdOjzd4?via=mozilla.org&via=matrix.org&via=vovo.id.au

Regarding the general strategy about those features let's discuss later then

[lissyx]

At first that may be a good idea, but maybe we want --disable-relay at build time? Would it be feasible?

The need is not so clear to me why relay should be disabled at build time since that's not what we do for other non-enterprise functionalities. Setting the preference to signon.firefoxRelay.feature=disabled as Victor suggested seems sufficient.

Well, any feature we absolutely do not use, I think it's better the code is just not there to reduce risks, and it's something we would have on m-c, assuming relay would never be something we care about (which may not be true, I dont know).

Generally I agree, but so far it just hasn't been anything we explored for other non-enterprise functionalities unless for simple #ifdef exclusions at build time. So it's maybe worth coming up with a strategy here.

Not for non enterprise features, but it's not unheard off in other context, I can remember of --disable being added for B2G builds where feature were either not required or could even collide. We definitively need to think this and come up with a strategy, and if we do this, it should be on m-c and not just on our builds.

So I think this highlights a broader question about which services we want to offer in Firefox Enterprise. What do we do about Relay, Monitor and Mozilla VPN (Note: That's not Firefox VPN) in the enterprise builds? Do we disable all, only some, none? Let's discuss this with the team.

👍

Setting the preference may work, but i'm just unsure how all the rest of the code will behave when reading this: https://searchfox.org/firefox-main/rev/a358f48a49a4d72df7b214805c4b2d2049d34a5a/toolkit/components/satchel/integrations/FirefoxRelayUtils.sys.mjs#16-22
Probably easier to just ask to the people working on it what is the best course of action here

Yes, 100%. I can help out with that, as I've worked on parts of the Relay integration into the password manager in my previous team :)

Ah, cool!