1639613 - Uninstaller should not unconditionally request UAC elevation in silent mode

Status: VERIFIED FIXED

(Firefox :: Installer, enhancement, P3)

Description

[Molly Howell (she/her) (no longer active)]

When the full installer is supplied a custom installation path on its command line, it checks whether it already has permission to write to that path before it requests elevation. This is convenient because it prevents UAC prompts from appearing unnecessarily in automated deployment scenarios.

However, the uninstaller does not have this feature when it is run in silent mode, meaning it always shows a UAC prompt whether it would end up needing one or not. We should move the elevation request in the uninstaller behind a write check, at least in silent mode if not always.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1639613 - Make Windows uninstaller conditionally request UAC elevation when silent. r?mhowell

This essentially cribs the equivalent installer conditional elevation.
I elected to guard this behind /S (silent) because it's not obvious to
me that unelevated users will see identical behaviour when not
elevating.

Comment 2

[Mike Kaply [:mkaply]]

If someone installs to their user profile, do we show a UAC prompt on install? If not, we shouldn't show on uninstall. We should only show the UAC prompt in cases where we need the permission.

Comment 3

[Molly Howell (she/her) (no longer active)]

(In reply to Mike Kaply [:mkaply] from comment #2)

If someone installs to their user profile, do we show a UAC prompt on install?

We would prompt, unless that install is run in silent mode, and then we wouldn't. The current patch here gives the uninstaller that same behavior.

Comment 4

[Mike Kaply [:mkaply]]

Any reason this didn't get checked in?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Kaply [:mkaply] from comment #4)

Any reason this didn't get checked in?

Yes -- I never got back to https://phabricator.services.mozilla.com/D76254#2323390 and moved on. I'll try to get back to this but I have two things in my queue first. Thanks for the nudge!

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander [he/him] from comment #5)

(In reply to Mike Kaply [:mkaply] from comment #4)

Any reason this didn't get checked in?

Yes -- I never got back to https://phabricator.services.mozilla.com/D76254#2323390 and moved on. I'll try to get back to this but I have two things in my queue first. Thanks for the nudge!

Status update: this is at the top of my queue. We've found a few wrinkles around uninstalling the maintenance service and requiring elevation if the installer was elevated, which I'll try to get worked out next week.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1639613 - Pre: Fix potential typo: lastUsed -> lastused. r?mhowell

It's not 100% clear that this has any effect -- it might be that the
string is case insensitive -- but in any case let's keep our source
code uniform.

Comment 8

[Nick Alexander :nalexander [he/him]]

Try build percolating at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d18690aff8fcb410f6c98b5aacf9095225c93d6&selectedTaskRun=eUoYuG94TfeI1z0vhUF3rA.0. (It's a debug task but that should be enough for testing.)

I tested this in three scenarios (all on Windows 10, although nothing here feels OS specific, other than UAC stuff itself):

1. Install, allow elevation, use C:\Program Files\.... Uninstall with silent (/S) should prompt to elevate, since the user can't write and can't remove privileged HKLM registry keys.

2. Install, allow elevation, use C:\Users\.... Uninstall with silent (/S) should prompt to elevate, since the user can't remove privileged HKLM registry keys.

3. Install, don't allow elevation, use C:\Users.... Uninstall with silent (/S) should not prompt to elevate, since the user can remove all files and there are no privileged HKLM registry keys to remove.

And I suppose it's worth verifying that uninstall without silent (/S) still prompts to elevate.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2b70e61fd94
Pre: Fix potential typo: lastUsed -> lastused. r=mhowell
https://hg.mozilla.org/integration/autoland/rev/96aa647b7ea4
Make Windows uninstaller conditionally request UAC elevation when silent. r=mhowell

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f2b70e61fd94
https://hg.mozilla.org/mozilla-central/rev/96aa647b7ea4

Comment 11

[Mike Kaply [:mkaply]]

Nick, what are your thoughts on uplifting this to beta and/or ESR?

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Kaply [:mkaply] from comment #11)

Nick, what are your thoughts on uplifting this to beta and/or ESR?

Beta seems totally reasonable. ESR is higher-stakes, maybe we should make sure there's no fallout on Beta?

mhowell: do you have an opinion?

Comment 13

[Molly Howell (she/her) (no longer active)]

This patch is of particular interest to the ESR audience, I think uplifting it there makes sense. I'm not sure that the risk from it is either of a degree or of a type that would lead us to get much value out of letting it sit on beta first.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9183830 [details]
Bug 1639613 - Pre: Fix potential typo: lastUsed -> lastused. r?mhowell

Beta/Release Uplift Approval Request

• User impact if declined: Users who use the uninstaller with the /S (silent) flag will always see a UAC elevation prompt, even when they could uninstall Firefox without elevating privileges. This impacts large organizations who manage multiple machines, since an automated process now requires manual intervention.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: The steps at https://bugzilla.mozilla.org/show_bug.cgi?id=1639613#c8 are reasonable.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): When run without the /S (silent) flag, we run code that already was run in different stages of the uninstall process. Therefore I expect failures to have already occurred. This is, sadly, hard to verify in the wild.
• String changes made/needed:

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9150636 [details]
Bug 1639613 - Make Windows uninstaller conditionally request UAC elevation when silent. r?mhowell

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This impacts large organizations who manage multiple machines, since an automated process now requires manual intervention. My colleague Molly reminds me that this fix is particularly of interest to the ESR population.
• User impact if declined: Users who use the uninstaller with the /S (silent) flag will always see a UAC elevation prompt, even when they could uninstall Firefox without elevating privileges.
• Fix Landed on Version: 84
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): When run without the /S (silent) flag, we run code that already was run in different stages of the uninstall process. Therefore I expect failures we see earlier to have occurred, just later in the process. This is, sadly, hard to verify in the wild.
• String or UUID changes made by this patch:

Comment 16

[Alin Ilea, Desktop QA]

Verified - Fixed in latest Nightly 84.0a1 (build id: 20201102214151) using Windows 10, following the steps from comment 8. Also uninstalling without silent parameter (/S) still prompts to elevate.

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9150636 [details]
Bug 1639613 - Make Windows uninstaller conditionally request UAC elevation when silent. r?mhowell

This is a P3/S3, this is not a regression and it recently landed on nightly, I think it can ride the trains.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9150636 [details]
Bug 1639613 - Make Windows uninstaller conditionally request UAC elevation when silent. r?mhowell

Approved for 78.6esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/d6cfe618fe67
https://hg.mozilla.org/releases/mozilla-esr78/rev/f683c43589b0

Comment 20

[Alin Ilea, Desktop QA]

Verified - Fixed in 78.6.0ESR (build id: 20201201001008) on Windows 10.