Closed Bug 1333789 Opened 5 years ago Closed 4 years ago

Firefox installer should not request UAC privilege when installing to a user directory

Categories

(Firefox :: Installer, defect)

51 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 54
Tracking Status
firefox52 --- verified
firefox-esr52 --- verified
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: mkaply, Assigned: mhowell)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

If you use an INI file to specify the Firefox install directory to a user directory, we should not ask for UAC admin privilege since we do not need it for the install.

See:

https://wiki.mozilla.org/Installer:Command_Line_Arguments
I think this bug is valid even if no INI file is used. The current method (cancel the UAC dialog) is not discoverable. The installer should request an elevation only after user specified the install path.
:mkaply, would you be able to elaborate on the impact of this bug? I'm not sure what priority to assign this because I don't know how it actually affects people trying to do silent/INI installs.

The interactive case that :emk brought up probably should have its own bug, because it's going to require new UI and therefore be a bigger project than the silent case. It might also get a different priority level.
Flags: needinfo?(mozilla)
Blocks: 1337218
(In reply to Matt Howell [:mhowell] from comment #2)
> The interactive case that :emk brought up probably should have its own bug,
> because it's going to require new UI and therefore be a bigger project than
> the silent case. It might also get a different priority level.

OK, filed bug 1337218.
> :mkaply, would you be able to elaborate on the impact of this bug? I'm not sure what priority to assign this because I don't know how it actually affects people trying to do silent/INI installs.

Sure. I should have given more context. We're working with some partners that install Firefox as part of other software installs. Our current installer always requests UAC privileges which means that we lose a certain amount of users the moment that popup is displayed.

The assertion is that if they could simply install Firefox into the user directory without displaying the UAC dialog, we'd be less likely to lose that install.

So I'm wondering if we can be smart in our NSIS file and do RequestExecutionLevel admin only if the install directory is Program Files. This would obviously affect registry entries as well, for control panel uninstall and our other stuff.
Flags: needinfo?(mozilla)
Okay, thank you. So it sounds like all we need to do to handle the INI install case is defer trying to elevate until we know that the target directory isn't writable otherwise. That's easy enough to do that I'll just go ahead and put a patch up. For interactive installs there's more to do because we currently use the elevation state to determine what the default directory should be, so we need some other way to deal with that (thanks to :emk for filing that separately).
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
I got this comment from the partner as well. Posting it just in case it is relevant.

Regarding the UAC install
The INI configuration also needs MaintenanceService=false, since the Maintenance Service will install into Program Files (it ignores InstallDirectoryPath). Might be useful information for the ticket: checking if elevation is required based on the values of InstallDirectoryPath and MaintenanceService.
(In reply to Mike Kaply [:mkaply] from comment #8)
> I got this comment from the partner as well. Posting it just in case it is
> relevant.
> 
> Regarding the UAC install
> The INI configuration also needs MaintenanceService=false, since the
> Maintenance Service will install into Program Files (it ignores
> InstallDirectoryPath). Might be useful information for the ticket: checking
> if elevation is required based on the values of InstallDirectoryPath and
> MaintenanceService.

Hadn't thought of that. I'll amend the patch to address this case as well.
Comment on attachment 8834541 [details]
Bug 1333789 - Try to avoid UAC prompts during automated installations using INI files.

https://reviewboard.mozilla.org/r/110426/#review112214
Attachment #8834541 - Flags: review?(agashlin) → review+
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa534d18c2d2
Try to avoid UAC prompts during automated installations using INI files. r=agashlin
https://hg.mozilla.org/mozilla-central/rev/aa534d18c2d2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Matt:

Is there any chance you'd consider this safe enough to move all the way up to Firefox 52?

We have a partner rollout scheduled and this would make them very happy.
I don't think it's high risk, since it's a small change to a relatively uncommonly used code path. But I had a chat with the reviewer and we would both feel better (and probably so would relman) if you or the partner could manually verify that the change a) does solve their need, and b) doesn't fail spectacularly in the process. Today's nightly installer should have this change in it, so you can use it to test. Thanks.
Flags: needinfo?(mozilla)
Definitely going to test once it's live. Leaving the needinfo to remind me.
Partner has verified this works for them. I'm going to request uplift.
Flags: needinfo?(mozilla)
Comment on attachment 8834541 [details]
Bug 1333789 - Try to avoid UAC prompts during automated installations using INI files.

Approval Request Comment
[Feature/Bug causing the regression]: Allow silent installer to install to User directoy
[User impact if declined]: No user impacted. Requested by partner. Partner unable to do silent install (which could affect adoption).
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Follow instruction here: https://wiki.mozilla.org/Installer:Command_Line_Arguments for a silent install. Specify a user directory and verify there is no UAC prompt.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Extremely low.
[Why is the change risky/not risky?]: Only affects silent/config.ini install which has very low usage.
[String changes made/needed]:
Attachment #8834541 - Flags: approval-mozilla-beta?
Comment on attachment 8834541 [details]
Bug 1333789 - Try to avoid UAC prompts during automated installations using INI files.

Thanks, Mike!
We should request aurora approval as well, if we need beta.

See comment 18 for approval request comment.
Attachment #8834541 - Flags: approval-mozilla-aurora?
Hi Brindusa, 
Could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Verified as fixed on latest Nightly 54.0a1, build ID 20170215030342 on Windows 10 x64. Tested with instructions and  INI configuration file specified at https://wiki.mozilla.org/Installer:Command_Line_Arguments, and make sure to have set MaintenanceService=false into the ini file.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Comment on attachment 8834541 [details]
Bug 1333789 - Try to avoid UAC prompts during automated installations using INI files.

Fix a silent install issue and was verified in nightly. Aurora53+.
Attachment #8834541 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Comment on attachment 8834541 [details]
Bug 1333789 - Try to avoid UAC prompts during automated installations using INI files.

tweak windows install script to avoid unneeded prompt, beta52+
Attachment #8834541 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on Windows 10 x64 with Release Candidate 52.0-build2 and 52.0esr-build1.

Tested with instructions from comment 18 and INI configuration file specified at https://wiki.mozilla.org/Installer:Command_Line_Arguments, and make sure to have set MaintenanceService=false into the INI file.
Removing the qe-verify flag, since this bug has been confirmed fixed on Fx54 (see Comment 21), Fx52 and Fx52esr (see Comment 27).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.