Bug 2034430 - Disable Firefox Relay feature in enterprise builds

[victhorlopez]

Description

Bugzilla: Bug-2034430

This PR does the following:

• Sets the signon.firefoxRelay.feature pref default to "disabled" (instead of "available") in enterprise builds via #ifdef MOZ_ENTERPRISE in firefox.js
  • Skips the existing Relay browser tests (browser_relay_signup_flow.js, browser_relay_signup_flow_showToAllBrowsers.js, browser_relay_telemetry.js,
    browser_relay_use.js) on enterprise builds since they assume the feature is available
  • Adds a new enterprise-only test (browser_relay_disabled_enterprise.js) that verifies:
    • The default pref value is "disabled"
    • FirefoxRelayUtils.relayIsAvailableOrEnabled() returns false
    • The Relay autocomplete item does not appear in the password popup

---

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!

[1rneh]

nit: "not supported" might be misleading here. We support it but the default for enterprise builds is that it's disabled but can be enabled by a policy (which doesn't exist as of now).

[victhorlopez]

If we support it (I thought we don't based on the excel), then the tests should be enabled and set "signon.firefoxRelay.feature", "available" for those tests for enterprise. (together with the original unlock of url). Do you see it like this or have a diferent opinion?

[1rneh]

My comment was intended as a nit and solely about the phrasing of the comment. But I have to correct my previous statement. An idea was to have a policy to disable Relay. But now that I'm revisiting this, that policy won't be applicable for Firefox Enterprise builds.

Let's wait for a product decision, because if we disable the Relay feature entirely, we also need to make sure it's not surfaced and toggleable from anywhere e.g. about:preferences, ideally with test coverage.

[victhorlopez]

okay, is my understand correct now, that we do not support relay?

I wrote a test already using their utils and similar logic, but based on your comment we need to test more (i.e about preferences and so on).

[1rneh]

Yes, as of now we do not support Relay, hence the user shouldn't be able to turn it on. I checked and setting the preference signon.firefoxRelay.feature to unavailable instead of disabled, will make it not show up e.g. in about:preferences#privacy. So let's set the preference value to unavailable? Relay then still shows up on about:preferences#moreFromMozilla, but that section can be controled via the policy UserMessaging.

[victhorlopez]

Where did you get this unavailable from?

In the prefs it is not stated, and for this check they use this. Does it behave different than disabled?

link

edit: I just tried and with unavailable it disappears ... nice

[victhorlopez]

Based on this test I guess this is equivalent to undefined?
test

edit: ok I am leaving it as unavailable, reason is to get undefined we should not set the pref, but that seems quite easy that it gets broken

[1rneh]

I had a vague memory that when we integrated relay into the password manager that we made a distinction between whether the pref was set to unavailable or disabled. So I tried it out and then assumed preemptively that unavailable was the correct value. But I think you are pointing it out correctly. It can be any arbitrary value except "available", "offered", "enabled" or "disabled".

[1rneh]

I've reached out to Relay folks to make sure this doesn't cause unexpected breakage.