390433 - [Vista] Users that are not a member of the administrators group receive update notification

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

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

We should check if the user is a member of the administrators group on Vista (e.g. IsUserAnAdmin) and if they aren't we should disable update checks in the same way that we do for earlier versions of Windows when the user doesn't have write access to the app dir. We may need to check if the app is installed into a location where they have write access so we can provide update notification in that instance. This would require that we have a manifest for the app's executable which we don't currently on 2.0.0.x

Some may think of this as a feature but it isn't the correct way to implement notifying people of updates when their account can't be elevated by UAC.

Comment 1

[Jo Hermans]

I guess this could prevent user frustration, but this would also mean that a non-admin user is never warned that an update is available. I'm the admin of my grandmothers computer, so that one will never be updated, until I remember to stop by her house, and start a manual update. (in fact, I already switched off the automatic update to prevent this scenario, so I have to do it anyway)

Can't we warn the user that there is an update available, but it needs to be installed by the administrator ?

Comment 2

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

robert, thanks for finding and logging this bug.

this is somewhat related to bug #383518.  In that bug, for vista, I made it so we don't check if the app dir is writable.  it sounds like what I should have done is made that check for all versions of windows, but for vista, a non-writable app-dir is ok if IsUserAnAdmin() returns true.

what I'd like to propose is we:

1)  add "readonly boolean attribute isUserAnAdmin" to nsIWinAppHelper.idl and rev the uuid
2) nsAppRunner implements nsIWinAppHelper, which includes nsWindowsRestart.cpp
3) nsWindowsRestart.cpp contains the existing IsUserAnAdmin() code, so make the implementation of GetIsUserAnAdmin() use it.
4) fix nsUpdateService.js so that on vista, we check if the app dir is writable, and if not, we allow update if isUserAnAdmin is true.

I can take this bug, for trunk and 2007, if you'd like.

Comment 3

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

(In reply to comment #1)
>
> Can't we warn the user that there is an update available, but it needs to be
> installed by the administrator ?
That would be a separate bug and would need to be addressed in addition to this bug.

Seth, sounds good to me. Since we will be allowing non-admin users to update bug 370571 should also be fixed for this.

Comment 4

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

robert points out:

for trunk vs. branch, the fix may have to be different, depending on if bug #378598.  if bug #378598 lands on the branch, I can make the same fix to both places.

otherwise, for vista, I need to only check GetIsUserAnAdmin() [and continue to not check if the app dir is writable.]

targetting m9, but might be able to knock this out (on the trunk) sooner.

Comment 5

[Benjamin Smedberg]

I guess it's ok to change nsIWinAppHelper: it wasn't added until 2002, so it's not like we are modifying an interface that shipped with FF2.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Related: bug 318855

Comment 7

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

Attachment: patch, v1 (need to test)

Comment 8

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

Attachment: patch, v2

Comment 9

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

Comment on attachment 278688 [details] [diff] [review]
patch, v2

Seth, I tested the patch and it disabled updates for me even though I am an admin with UAC turned on. I haven't had a chance to look at the code but based on this I'm minusing the review.

Comment 10

[Daniel Veditz [:dveditz]]

Not a stop-ship bug for branch, but when you get a tested trunk fix we'll try to get it into a 1.8.1.x release. Request approval on a branch patch.

Comment 11

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

Seth, this also needs to check if the user's token is a split token. I'll look into what this will take as soon as I am able.

Comment 12

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

Robert, thanks for testing and finding my mistake

On Vista, if I launch firefox.exe as the administrator, so it run as elevated, Help | Check for Updates is enabled (and in my console, if I set the app.update.log.all hidden pref to true, I see "canUpdate?  on Vista, isUserAnAdmin = true")

But, if I am not elevated, I see what you see, which is that "canUpdate?  on Vista, isUserAnAdmin = false".

Thanks for the pointer about split tokens.  IsUserAnAdmin() is not what I need, I need to do that, instead. 

looking at http://blogs.msdn.com/cjacks/archive/2006/10/09/How-to-Determine-if-a-User-is-a-Member-of-the-Administrators-Group-with-UAC-Enabled-on-Windows-Vista.aspx and http://blogs.codegear.com/fhaglund/archive/2007/03/19/33207.aspx now.

Comment 13

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

Attachment: patch, testing on vista now

testing this on vista now, once I have builds from http://wiki.mozilla.org/Build:TryServer

notes:

1) this builds on windows xp, but not sure if it will build on vista
2) if this work on vista, and robert likes what he sees, I'll rename:

readonly attribute boolean isUserAnAdmin;

to

readonly attribute boolean userCanElevate;

Comment 14

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

Attachment: updated patch, renamed idl, updated comment, still testing on vista

Comment 15

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

Comment on attachment 278851 [details] [diff] [review]
updated patch, renamed idl, updated comment, still testing on vista

tested this on vista:

from an admin account can update, and from the console log:

canUpdate?  on Vista, userCanElevate = true

from a guest account can't update, and from the console log:

canUpdate?  on Vista, userCanElevate = false

but note, my build was build with the try server, and I have not built on vista, and I'm a little concerned that typedefing that enum and defining TokenElevationType might be a problem when building on vista.

Comment 16

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

(In reply to comment #15)
>...
> but note, my build was build with the try server, and I have not built on
> vista, and I'm a little concerned that typedefing that enum and defining
> TokenElevationType might be a problem when building on vista.
Specifically, with the version of WinNT.h that comes with the Vista SDK. I won't have the Vista SDK until I download it tomorrow (ewww... over a gig) so I'm going to hold off on the review until then.

In testing this patch I tested an admin account with UAC on (e.g. split token) and a non-admin account with UAC on (e.g. not a split token) and these were fine with an install in Program Files and an install outside of Program Files. Both admin and non-admin accounts need to be tested with UAC turned off and an install in Program Files and an install outside of Program Files.

Comment 17

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

Comment on attachment 278851 [details] [diff] [review]
updated patch, renamed idl, updated comment, still testing on vista

>Index: toolkit/xre/nsAppRunner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v
>retrieving revision 1.189
>diff -u -8 -p -r1.189 nsAppRunner.cpp
>--- toolkit/xre/nsAppRunner.cpp	20 Aug 2007 05:09:07 -0000	1.189
>+++ toolkit/xre/nsAppRunner.cpp	29 Aug 2007 21:45:40 -0000
>@@ -750,16 +750,45 @@ nsXULAppInfo::PostUpdate(nsILocalFile *a
>   rv = LaunchAppHelperWithArgs(upgradeArgc, upgradeArgv);
>   
>   if (pathArg)
>     PR_smprintf_free(pathArg);
> 
>   free(upgradeArgv);
>   return rv;
> }
>+
>+typedef enum 
>+{
>+  TokenElevationTypeDefault = 1,
>+  TokenElevationTypeFull,
>+  TokenElevationTypeLimited
>+} TOKEN_ELEVATION_TYPE;
>+
>+#define TokenElevationType static_cast< TOKEN_INFORMATION_CLASS >( 18 )
>+
>+NS_IMETHODIMP
>+nsXULAppInfo::GetUserCanElevate(PRBool *aUserCanElevate)
>+{
>+  HANDLE hToken;
>+
>+  TOKEN_ELEVATION_TYPE elevationType;
>+  DWORD dwSize; 
>+
>+  OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken);
>+  GetTokenInformation(hToken, TokenElevationType, &elevationType, sizeof(elevationType), &dwSize); 
>+
>+  *aUserCanElevate = (elevationType == TokenElevationTypeFull || 
>+                      elevationType == TokenElevationTypeLimited);
>+
>+  if (hToken)
>+    CloseHandle(hToken);
>+
>+  return NS_OK;
>+}
I did a little digging tonight and this won't be sufficient.

(In reply to comment #11)
> Seth, this also needs to check if the user's token is a split token. I'll look
> into what this will take as soon as I am able.
So, we *also* need to check if the user's token is split though I am leaning towards a write access check as we do on non-Vista Windows systems.

When UAC is turned off the elevation type returned should be TokenElevationTypeDefault and then I think we should check if the user has write access to the app dir as we do for non-Vista Windows and disable update if they don't. We probably could use IsUserAnAdmin but the write access check is already used elsewhere. This assumes we have a manifest specifying requestedExecutionLevel level="asInvoker" which has landed on the branch so that the write access check isn't virtualized.

We'll also need to figure out how to handle the enum when the version of WINNT.H from the Vista SDK is present.

Comment 18

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

(In reply to comment #17)
> (In reply to comment #11)
> > Seth, this also needs to check if the user's token is a split token. I'll look
> > into what this will take as soon as I am able.
> So, we *also* need to check if the user's token is split though I am leaning
> towards a write access check as we do on non-Vista Windows systems.
badly worded...

So, we *also* need to check if the user's token is split along with checking if the user is already running as admin without UAC turned on though I am leaning towards just doing a write access check to check if the user is already running as admin without UAC turned on as we do on non-Vista Windows systems.

Comment 19

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

robert, thanks for the additional details (and the vista sdk headers you sent me.)

working on a complete fix, per your comments.

Comment 20

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

update:  on vista, for an admin account, if you disable uac, we return TokenElevationTypeDefault (as rstrong points out.)

Comment 21

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

Attachment: update patch, will build with WinNT.h from the vista sdk, still need to test.

Comment 22

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

note to self, need to test:

user is admin on vista, uac enabled, user can elevate, and app dir is not under c:\program files, can they create the test file even if they don't own the director?

Comment 23

[Mike Connor [:mconnor]]

Missed freeze, moving out.

Comment 24

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

Comment on attachment 279761 [details] [diff] [review]
update patch, will build with WinNT.h from the vista sdk, still need to test.

sorry for the delay here, robert.

I've tested on vista as admin and non-admin, with uac on and off, with the build under c:\program files and not under c:\program files, and everything looks good.

If you'd like to try a build, check out https://build.mozilla.org/tryserver-builds/[e-mail]

set app.update.log.UpdateService to true to see the verbose logging.

Comment 25

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

we're still frozen for m8, so this might make it (this weekend).

this is also wanted for 1.8.1.7 (firefox 2.0.0.7)

Comment 26

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

Comment on attachment 279761 [details] [diff] [review]
update patch, will build with WinNT.h from the vista sdk, still need to test.

First run through...

>Index: toolkit/mozapps/update/src/nsUpdateService.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v
>retrieving revision 1.141
>diff -u -8 -p -r1.141 nsUpdateService.js.in
>--- toolkit/mozapps/update/src/nsUpdateService.js.in	29 Aug 2007 08:16:30 -0000	1.141
>+++ toolkit/mozapps/update/src/nsUpdateService.js.in	5 Sep 2007 18:19:03 -0000
>@@ -1494,32 +1494,75 @@ UpdateService.prototype = {
>         upDirFile.create(nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE);
>         upDirFile.remove(false);
>       }
> #ifdef XP_WIN
>       var sysInfo =
>         Components.classes["@mozilla.org/system-info;1"]
>                   .getService(Components.interfaces.nsIPropertyBag2);
> 
>+      // Example windowsVersion:  Windows XP == 5.1
>+      var windowsVersion = sysInfo.getProperty("version");
>+      LOG("UpdateService", "canUpdate?  windowsVersion = " + windowsVersion);
>+
>+      // For Vista, we can update if the appDir is under C:\Program Files and
>+      // we can elevate.
>+      //
>+      // If you can elevate, you can update an application that lives under
>+      // C:\Program Files by granting the updater.exe process access via the
>+      // UAC prompt.  
>+      //
>+      // Note, if UAC is turned off, userCanElevate will be false.
How about?

For Vista, updates can be performed to a location requiring admin privileges by requesting elevation via the UAC prompt when launching updater.exe if the appDir is under the Program Files directory (e.g. C:\Program Files\) and one of the following is true:
1) UAC is turned on and we can elevate (e.g. user has a split token)
   userCanElevate will return true
2) UAC is turned off and the user can write to the installation directory
   userCanElevate will return false

Note: this does note attempt to handle the case where UAC is turned on and the installation directory is in a restricted location that requires admin privileges to update other than Program Files.

>+      var userCanElevate = false;
>+
>+      if (parseFloat(windowsVersion) >= 6) {
>+        try {
>+          var fileLocator = 
>+            Components.classes["@mozilla.org/file/directory_service;1"]
>+                      .getService(Components.interfaces.nsIProperties);
>+          // KEY_UPDROOT will fail and throw an exception if
>+          // appDir is not under the Program Files, so we rely on that
>+          var dir = fileLocator.get(KEY_UPDROOT, Components.interfaces.nsIFile);
>+          // appDir is under Program Files, so check if the user can elevate
>+          userCanElevate = 
>+            gApp.QueryInterface(Components.interfaces.nsIWinAppHelper)
>+                .userCanElevate;
>+          LOG("UpdateService", 
>+              "canUpdate?  on Vista, userCanElevate = " + userCanElevate);
>+        }
>+        catch (ex) {
>+          // if the appDir is not under Program Files, being able to elevate
>+          // doesn't help, and so we fall to the check below 
>+          // to make sure we can update
How about something like?

When the installation directory is not under Program Files fall through to checking if write access to installation directory is available.

>+          LOG("UpdateService", 
>+              "canUpdate?  on Vista, appDir is not under the Program Files");
>+        }
>+      }
>+
>       // On Windows, we no longer store the update under the app dir
>       // if the app dir is under C:\Program Files.
>       //
>-      // If we are on Windows, but not Vista, we need to check that
>+      // If we are on Windows (including Vista, if we can't elevate)
>+      // we need to check that
This should call out including Vista when the installation directory is under Program Files if we can't elevate.

>       // we can create and remove files from the actual app directory
>-      // (like C:\Program Files\Mozilla Firefox).  If we can't,
>-      // because this user is not an adminstrator, for example
>+      // (like C:\Program Files\Mozilla Firefox).  If we can't
>+      // (because this user is not an adminstrator, for example)
>       // canUpdate() should return false (like it used to).
Could you remove "(like it used to)" or rewrite it so that a casual reader can make sense of it?

>       //
>-      // For Vista, don't perform this check because non-admin users
>-      // can update firefox (by granting the updater access via the
>-      // UAC prompt)
>-      var windowsVersion = sysInfo.getProperty("version");
>-      LOG("UpdateService", "canUpdate?  version = " + windowsVersion);
>-      // Example windowsVersion:  Windows XP == 5.1
>-      if (parseFloat(windowsVersion) < 6) {
>+      // For Vista, we still do this check to cover the cases where:
>+      // 1) the user is an admin and they have turned UAC off 
>+      // 2) the user is an admin but the app doesn't live under 
>+      //    C:\Program Files
>+      // 3) if the user is not an admin but they have write permission 
>+      //    to C:\Program Files
>+      // 
>+      // Note, if the user can elevate, we don't need to bother with this test.
How about?

For Vista, we perform this check to enable updating the application when the user has write access to the installation directory under the following scenarios:
1) the installation directory is not under Program Files (e.g. C:\Program Files)
2) UAC is turned off
3) UAC is turned on and the user is not an admin (e.g. the user does not have a split token)

Comment 27

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

(In reply to comment #26)
>
> For Vista, updates can be performed to a location requiring admin privileges by
> requesting elevation via the UAC prompt when launching updater.exe if the
> appDir is under the Program Files directory (e.g. C:\Program Files\) and one of
> the following is true:
> 1) UAC is turned on and we can elevate (e.g. user has a split token)
>    userCanElevate will return true
> 2) UAC is turned off and the user can write to the installation directory
>    userCanElevate will return false
bah... skip number two here

Comment 28

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

Comment on attachment 279761 [details] [diff] [review]
update patch, will build with WinNT.h from the vista sdk, still need to test.

>Index: toolkit/xre/nsAppRunner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v
>retrieving revision 1.191
>diff -u -8 -p -r1.191 nsAppRunner.cpp
>--- toolkit/xre/nsAppRunner.cpp	4 Sep 2007 22:10:35 -0000	1.191
>+++ toolkit/xre/nsAppRunner.cpp	5 Sep 2007 18:19:04 -0000
>@@ -750,16 +750,50 @@ nsXULAppInfo::PostUpdate(nsILocalFile *a
>   rv = LaunchAppHelperWithArgs(upgradeArgc, upgradeArgv);
>   
>   if (pathArg)
>     PR_smprintf_free(pathArg);
> 
>   free(upgradeArgv);
>   return rv;
> }
>+
>+// matches the enum in WinNT.h for the Vista SDK
>+// but renamed so that we can safely build with 
>+// the Vista SDK and without it.
Any reason not to make this two lines?

// Matches the enum in WinNT.h for the Vista SDK but renamed so that we can
// safely build with the Vista SDK and without it.

>+typedef enum 
>+{
>+  VistaTokenElevationTypeDefault = 1,
>+  VistaTokenElevationTypeFull,
>+  VistaTokenElevationTypeLimited
>+} VISTA_TOKEN_ELEVATION_TYPE;
>+
>+// avoid collision with TokeElevationType enum in WinNT.h
>+// of the Vista SDK
>+#define VistaTokenElevationType static_cast< TOKEN_INFORMATION_CLASS >( 18 )
>+
>+NS_IMETHODIMP
>+nsXULAppInfo::GetUserCanElevate(PRBool *aUserCanElevate)
>+{
>+  HANDLE hToken;
>+
>+  VISTA_TOKEN_ELEVATION_TYPE elevationType;
>+  DWORD dwSize; 
>+
>+  OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken);
>+  GetTokenInformation(hToken, VistaTokenElevationType, &elevationType, sizeof(elevationType), &dwSize); 
>+
Perhaps add a comment that if either of these two calls fail aUserCanElevate will be false?

>+  *aUserCanElevate = (elevationType == VistaTokenElevationTypeFull || 
>+                      elevationType == VistaTokenElevationTypeLimited);
>+
>+  if (hToken)
>+    CloseHandle(hToken);
>+
>+  return NS_OK;
>+}
> #endif

r=me

Comment 29

[Mike Connor [:mconnor]]

Let's get this after we reopen for M9

Comment 30

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

Attachment: updated patch, per robert's review comments

Comment 31

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

Comment on attachment 281351 [details] [diff] [review]
updated patch, per robert's review comments

Hey Seth, I haven't verified but it seems to me that when the elevation type is TokenElevationTypeFull then this should just check if the user has write access since the user is already running elevated. What do you think?

Comment 32

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

robert, checking write access in that scenario would prevent update in the scenario where:

1)  the user is running elevated (and UAC is enabled)
2)  the application directory lives under "Program Files"
3)  but the elevated user (admin) does not have write access to the application directory

Won't the admin user be always able to create files (and pass the write access test) in this scenario?

Comment 33

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

What I am suggesting for the app is under program files case is that if the user is not elevated and has a split token the best we can do is prompt to elevate since we can't verify that the user has write access (at least not without jumping through a ton of hoops). For the case where the user is already elevated the best we can do is verify that they can write to the directory.

There are cases where an elevated user won't have write access under program files but they are extreme edgecases.

Comment 34

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

to clarify comment #31 & #33, there are two scenarios:

a) the firefox process has a split token, but the user is running as standard-user (TokenElevationTypeLimited)

b) the firefox process has a split token, but the user is running as an admin user (TokenElevationTypeFull)

For scenario a, if we check if the user has write permission to the app dir (which is under Program Files, in this case), we'll throw up a UAC prompt.  This would be bad, as it would happen every time we check if we can update, right?

For scenario b, if we check if the user has write permission to the app dir (which is under Program Files, in this case), we won't throw up a UAC prompt because we are already running elevated.

Are you saying that we should add an additional check for scenario a and b, or just b?

Comment 35

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

The possible values and their meanings are:
* TokenElevationTypeDefault: The token does not have a linked token (e.g. UAC disabled or a standard user).
* TokenElevationTypeFull: The token is linked to an elevated token (e.g. UAC is enabled and the user is already elevated so they can't be elevated again).
* TokenElevationTypeLimited: The token is linked to a limited token (e.g. UAC is enabled and the user is not elevated). 

(In reply to comment #34)
> to clarify comment #31 & #33, there are two scenarios:
> 
> a) the firefox process has a split token, but the user is running as
> standard-user (TokenElevationTypeLimited)
Right, and the user can elevate and though we can't verify the user has write access the vast majority of the users will.

> b) the firefox process has a split token, but the user is running as an admin
> user (TokenElevationTypeFull)
Actually, the user is already elevated so can't elevate them and we should just check if they can write to the app dir. In other words, this should be handled in the same manner as TokenElevationTypeDefault where we fall through to the write access check.

> For scenario a, if we check if the user has write permission to the app dir
> (which is under Program Files, in this case), we'll throw up a UAC prompt. 
> This would be bad, as it would happen every time we check if we can update,
> right?
I'm not suggesting changing the implementation for scenario a.

> For scenario b, if we check if the user has write permission to the app dir
> (which is under Program Files, in this case), we won't throw up a UAC prompt
> because we are already running elevated.
Correct

> Are you saying that we should add an additional check for scenario a and b, or
> just b?
Just b.

Thanks!

Comment 36

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

robert, thanks for clarifying.  working on a new patch now.

one question I have is this:

for scenario b, the user is running firefox.exe elevated.  either the user is logged in as administrator or the user is running firefox.exe as administrator.

when logged in as administrator, when we apply the update, updater.exe will be elevated as we will be running it as administrator, too.

when not logged in as administrator, will the elevated firefox.exe process launch updater.exe as elevated?

Comment 37

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

Attachment: updated patch, per robert's comments

Comment 38

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

> when not logged in as administrator, will the elevated firefox.exe process
> launch updater.exe as elevated?

here's the scenario I'm worried about:

if the elevated firefox.exe (from "run as administrator") doesn't launch updater.exe as elevated and UAC is disabled, firefox.exe can write to the application dir, but updater.exe will not be able to.

Comment 39

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

(In reply to comment #38)
> > when not logged in as administrator, will the elevated firefox.exe process
> > launch updater.exe as elevated?
> 
> here's the scenario I'm worried about:
> 
> if the elevated firefox.exe (from "run as administrator") doesn't launch
> updater.exe as elevated and UAC is disabled, firefox.exe can write to the
> application dir, but updater.exe will not be able to.

This is different than the "run as" with UAC enabled where the user is running with a split token and this scenario is also true for Win2K / WinXP. In this scenario the user profile being used would be in the user profile of the user selected to "run as". So, if UserA selectes "run as" and types in the account info for AdminA then I believe the updates dir would be under the user's %USERPROFILE% dir under Vista though I haven't verified this.

We've never tried to handle the UAC disabled / WinXP / Win2K "run as" but if there is way now would be a good time to do so.

Comment 40

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

Comment on attachment 281495 [details] [diff] [review]
updated patch, per robert's comments

seeking review from robert.

I just tested this new patch on vista, with both UAC enabled and disabled.

for the UAC disabled case, an admin user will return false for canUpdate, and we'll fall through to the write access test.

Comment 41

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

for the changes since robert's first review, see https://bugzilla.mozilla.org/attachment.cgi?oldid=281351&action=interdiff&newid=281495&headers=1

here's my notes from robert's first set of review comments:

1)

How about?

For Vista, updates can be performed to a location requiring admin privileges by
requesting elevation via the UAC prompt when launching updater.exe if the
appDir is under the Program Files directory (e.g. C:\Program Files\) and one of
the following is true:
1) UAC is turned on and we can elevate (e.g. user has a split token)
   userCanElevate will return true
2) UAC is turned off and the user can write to the installation directory
   userCanElevate will return false

Note: this does note attempt to handle the case where UAC is turned on and the
installation directory is in a restricted location that requires admin
privileges to update other than Program Files.

fixed.

2)

How about something like?

When the installation directory is not under Program Files fall through to
checking if write access to installation directory is available.

fixed

3)

>-      // If we are on Windows, but not Vista, we need to check that
>+      // If we are on Windows (including Vista, if we can't elevate)
>+      // we need to check that

This should call out including Vista when the installation directory is under
Program Files if we can't elevate.

I'm already calling that out:  "(including Vista, if we can't elevate)"

4)

>       // we can create and remove files from the actual app directory
>-      // (like C:\Program Files\Mozilla Firefox).  If we can't,
>-      // because this user is not an adminstrator, for example
>+      // (like C:\Program Files\Mozilla Firefox).  If we can't
>+      // (because this user is not an adminstrator, for example)
>       // canUpdate() should return false (like it used to).

Could you remove "(like it used to)" or rewrite it so that a casual reader can
make sense of it?

fixed.

5)

How about?

For Vista, we perform this check to enable updating the application when the
user has write access to the installation directory under the following
scenarios:
1) the installation directory is not under Program Files (e.g. C:\Program
Files)
2) UAC is turned off
3) UAC is turned on and the user is not an admin (e.g. the user does not have a
split token)

fixed.

6)

> For Vista, updates can be performed to a location requiring admin privileges by
> requesting elevation via the UAC prompt when launching updater.exe if the
> appDir is under the Program Files directory (e.g. C:\Program Files\) and one of
> the following is true:
> 1) UAC is turned on and we can elevate (e.g. user has a split token)
>    userCanElevate will return true
> 2) UAC is turned off and the user can write to the installation directory
>    userCanElevate will return false
bah... skip number two here

skipped.

7)

>+// matches the enum in WinNT.h for the Vista SDK
>+// but renamed so that we can safely build with 
>+// the Vista SDK and without it.

Any reason not to make this two lines?

// Matches the enum in WinNT.h for the Vista SDK but renamed so that we can
// safely build with the Vista SDK and without it.

fixed, thanks.


8)

>+  OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken);
>+  GetTokenInformation(hToken, VistaTokenElevationType, &elevationType, sizeof(elevationType), &dwSize); 
>+

> Perhaps add a comment that if either of these two calls fail aUserCanElevate
> will be false?

thanks for catching that.  I've added code to check the return value of both OpenProcessToken() and GetTokenInformation()

Comment 42

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

Comment on attachment 281495 [details] [diff] [review]
updated patch, per robert's comments

Looks good and this should cover the vast majority of cases. Thanks!

>Index: toolkit/xre/nsAppRunner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v
>retrieving revision 1.191
>diff -u -8 -p -r1.191 nsAppRunner.cpp
>--- toolkit/xre/nsAppRunner.cpp	4 Sep 2007 22:10:35 -0000	1.191
>+++ toolkit/xre/nsAppRunner.cpp	19 Sep 2007 16:55:44 -0000
>@@ -750,16 +750,67 @@ nsXULAppInfo::PostUpdate(nsILocalFile *a
>...
>+NS_IMETHODIMP
>+nsXULAppInfo::GetUserCanElevate(PRBool *aUserCanElevate)
>+{
>+  HANDLE hToken;
>+
>+  VISTA_TOKEN_ELEVATION_TYPE elevationType;
>+  DWORD dwSize; 
>+
>+  BOOL ok = OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken);
>+  if (!ok) {
>+    *aUserCanElevate = PR_FALSE;
>+    return NS_OK;
>+  }
>+  ok = GetTokenInformation(hToken, VistaTokenElevationType, &elevationType, sizeof(elevationType), &dwSize); 
nit: this line is a tad long ;)

I also think the checks could be simpler... something like this?

if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken) ||
    !GetTokenInformation(hToken, VistaTokenElevationType, &elevationType,
                         sizeof(elevationType), &dwSize)) {
  *aUserCanElevate = PR_FALSE;
} else {
  // The possible values returned for elevationType and their meanings are:
  //   TokenElevationTypeDefault: The token does not have a linked token 
  //     (e.g. UAC disabled or a standard user, so they can't be elevated)
  //   TokenElevationTypeFull: The token is linked to an elevated token 
  //     (e.g. UAC is enabled and the user is already elevated so they can't
  //      be elevated again)
  //   TokenElevationTypeLimited: The token is linked to a limited token 
  //     (e.g. UAC is enabled and the user is not elevated, so they can be
  //      elevated)
  *aUserCanElevate = (elevationType == VistaTokenElevationTypeLimited);
}

if (hToken)
  CloseHandle(hToken);

return NS_OK;

r=me

Comment 43

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

Attachment: patch, as checked in

Comment 44

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

fixed.

Checking in mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpda
teService.js.in
new revision: 1.142; previous revision: 1.141
done
Checking in xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.192; previous revision: 1.191
done
Checking in xre/nsIWinAppHelper.idl;
/cvsroot/mozilla/toolkit/xre/nsIWinAppHelper.idl,v  <--  nsIWinAppHelper.idl
new revision: 1.2; previous revision: 1.1
done

Comment 45

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007092204 Minefield/3.0a9pre and a limited test user.

The test user (not member of the administrator group and limited user) does not get any update offer.

-> changing to verified

Comment 46

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

carsten, thanks for verifying.

When testing my fix on Vista, I here's the 8 scenario matrix that I used:

when UAC was enabled vs disabled
x
admin group vs limited user
x
app dir is under C:\program files vs somewhere else

Is that what you tested as well?  perhaps we can turn that into a litmus test case?

If everything looks good to you, I can't work on porting this to the branch (which will require a special interface to ensure binary compatibility with previous version of fx/tb 2.0.0.x, per shaver / dveditz)

Comment 47

[Carsten Book [:Tomcat]]

Hi Seth, 

tested the scenarios your mentioned as admin user and non-member of the administrator group. 

All Scenarios pass and looks good for me.

I have also written the test case for litmus and will sent you the test case for review.