347954 - Running the NSIS installer as normal user (restricted group) or without enough free space available makes it silently fail

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce:
1. Fetch trunk installer build
2. Put yourself or another user in the Users group (called Restricted user under Windows XP)
3. Run the installer and install with default options

Actual results:
The installer runs through and claims to be successful and offers you to run Firefox. This of course cannot work since C:\Program Files is read-only for a restricted user.

Expected results:
Throw a warning (or maybe even error?) when launching the installer that you do not have write access (or don't run as Admin or power user) and proceeding might led to problems. And when the user then tries to install anyway and writing to C:\Program Files fails it should say that and not ignore this :).

Comment 1

[Benjamin Smedberg]

Does it work if the user manually changes the install location to, say, their desktop? What would be a sensible default location for restricted installs?

Comment 2

[Robert Strong (they/them - no direct email)]

(In reply to comment #1)
> Does it work if the user manually changes the install location to, say, their
> desktop?
Yes... we don't add the uninstall reg keys if we don't have access to HKLM... other reg keys are added to HKCU.

> What would be a sensible default location for restricted installs?
I don't think there is such a thing but we could display the Select Install dir page with the default being something like 
c:\<path to users Application Data dir>\Mozilla Firefox
e.g. %APPDATA%\Mozilla Firefox

Comment 3

[Frank Wein [:mcsmurf]]

I'm not so sure about the %APPDATA% thing since the profile would be right next to it (%APPDATA%\Mozilla\Firefox). Probably there is no "good" location, maybe Desktop would be a good compromise, not sure...

Comment 4

[Robert Strong (they/them - no direct email)]

If not appdata then I'd prefer to go to my documents rather than the desktop

Comment 5

[Benjamin Smedberg]

Hrm, I'm tempted to say we should propose %APPDATA%/Program Files

Comment 6

[Robert Strong (they/them - no direct email)]

I would very much prefer to avoid creating that type of directory structure since there is no capability for removing the Program Files dir that we create and I don't want to add that for this.

I have most of this done so it will display the install directory selection page when we don't have write access to the default and it will default to another directory - currently "my documents\BrandFullName". I have not added new strings to inform the user about what is going on since it is rather late for string changes... then again there are a couple of string changes landing so it isn't impossible to add them.

Comment 7

[Matthew (lilmatt) Willis]

(In reply to comment #1)
> What would be a sensible default location for restricted installs?

In a corporate environment, there is none. Restricted users shouldn't be able to install applications with their limited rights, but it's up to the app developer to enforce this by not installing if you have limited rights.

Just a thought...

Comment 8

[Benjamin Smedberg]

That's ridiculous. Just because someone is in a restricted environment doesn't mean that they shouldn't be allowed to install software, just that it shouldn't have any effect on other users of the system.

If users really shouldn't be able to install software, the system security policy should enforce that, not our installer.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Robert, we still have some time for string changes, so if you have some for me to look at, please just dump 'em in a text file and ask for uir ...

Comment 10

[Robert Strong (they/them - no direct email)]

I think the right choice here is to just inform the user that they need to select a directory where they have write access to and display the install directory selection page. People running on systems where they don't have privileges to install apps into the program files directory will most likely be used to having to select a different location and there is no good default location.

Comment 11

[Robert Strong (they/them - no direct email)]

beltzner, I'd like to just display a dialog with an ok button informing the person installing that they need to select a directory that they have write access to. Perhaps something along the lines of

Error writing to installation directory:\r\n\r\n<path to install directory>\r\n\r\nClick OK and select an installation directory that you can write to.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: string change

beltzner, a psuedo patch for the added string. I'll add the actual path later

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233263 [details] [diff] [review]
string change

>+WARN_WRITE_ACCESS=Error writing to installation directory:\n\n<path to install directory>\n\nClick OK and select an installation directory that you can write to.

WARN_WRITE_ACCESS=You don't have access to write to\n\n<path to install directory>\n\nClick OK to select a different installation directory.

Also: why are we using an "OK" dialog as opposed to a "Browse..." dialog? I'll accept reasonable arguments here, although I'd rather we use a button more expressive of what's going to happen.

Comment 14

[Robert Strong (they/them - no direct email)]

(In reply to comment #13)
> WARN_WRITE_ACCESS=You don't have access to write to\n\n<path to install
> directory>\n\nClick OK to select a different installation directory.
Thanks!

> Also: why are we using an "OK" dialog as opposed to a "Browse..." dialog? I'll
> accept reasonable arguments here, although I'd rather we use a button more
> expressive of what's going to happen.
There are a couple of cases where this can happen.
1) Person selects basic install and we need them to choose an install location. Basic installs typically provide the option to select an install directory but this is non-standard for us.
2) Person selects custom and they have chosen an install location they don't have write access to.

We are going to use the standard select install directory page for both instances and need to notify that we don't have write access and it is needed. In the second instance this will only occur when next is clicked.

So, this accomplishes the desired end result without having to change ui strings for the same page... this is much simpler and solves this edgecase.

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: string change 2

Since the ui under the dialog has the directory displayed I'd like to remove the directory path from the string.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233289 [details] [diff] [review]
string change 2

Looks OK to me, Rob. We can probably even get rid of the second "installation" ... just "select a different directory" works OK when you have the two phrases next to each other like that.

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch - both write access and space available

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: strings only including additional disk space error msg.

Mike, I separated the strings since I'd like to get the strings on the branch ASAP.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233558 [details] [diff] [review]
strings only including additional disk space error msg.

first-review,a=me

small nit: s/drive/location/ in WARN_DISK_SPACE for both the symmetry and the fact that it's possible to be out of space in a directory yet still have space in the drive (yay, quotas!)

You can fix that as you land it on branch and trunk.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233557 [details] [diff] [review]
patch - both write access and space available

second-review=me with the nit on the WARN_DISK_SPACE string as mentioned in the previous comment (s/drive/location/)

Comment 21

[(not reading, please use [e-mail] instead)]

Comment on attachment 233557 [details] [diff] [review]
patch - both write access and space available

r=sspizer on the nsis changes.

Comment 22

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233557 [details] [diff] [review]
patch - both write access and space available

a=drivers for the MOZILLA_1_8_BRANCH

Comment 24

[Robert Strong (they/them - no direct email)]

Checked in to MOZILLA_1_8_BRANCH