394984 - Unable to update on mac if admin user is not the same admin user as the person who installed firefox

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

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

software update disabled on the mac if admin user is not the same admin user as the person who installed firefox

rhian had a machine that was stuck on 1.5.0.6, and the problem was that software update was disabled because she could not write to the application directory.

/Applications/Firefox.app was owned by "501", which was probably the previous owner for the laptop (mrz?)

The perms for that directory are:

drwxr-xr-x    3 501  admin   102 Jul 28  2006 . 

as the "rhian" user, even though she is an admin user, she can't create the update.test file under there, so update was disabled.

note, I think the fix for bug #393782 will help address this sort of thing, as we can make it part of the "why can't I update faq" for mac.

Comment 1

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

this was on Mac OS X 10.4.x

Comment 2

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

Same as Bug 318855 except for Mac specifically. I am most likely going to be doing something similar for Windows so I'd like to leave this open for the Mac specific work that needs to be done.

Comment 4

[Dave Miller [:justdave]]

From bug 518513, Mac OS X 10.6 introduces a behavior change that makes this much more critical to solve:

When you drag an application into the /Applications/ folder in Finder, it now automatically chowns it to root instead of to the user who put it there.  This effectively disables Software Update on any new install on 10.6.

Comment 5

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

Dave: are you saying as STR:

 - get a fresh copy of 10.6 (no profile, no installed Firefox.app)
 - install Firefox.app
 - try to get an update

expected: it updates
actual: it fails?

Comment 6

[Dave Miller [:justdave]]

What actually happened to me:

Shiretoko build on 20090915 had a broken NSS in it.  This hosed software update because SSL didn't work.  I manually downloaded the nightly that fixed it, and dragged it to my Applications folder, okaying it to replace the Shiretoko.app that was already there (which was owned by my user).  Finder prompted me for an admin user/pass in order to write to /Applications and then copied in the new Shiretoko.app with it owned by root instead of by me. (Under 10.5 when following this procedure, it would leave it owned by the user who was installing it).  Result: software update still didn't work, although everything else SSL now did.

And actually... I just tried this out on my wife's iMac, where I'm actually an admin user, and it does still leave the ownership alone if you're already an admin user, which makes this a bit of an edge case.  I run as a non-admin user on my MacBook Pro.  Having it owned by root after doing a sudo to install it makes perfect sense, so they must have been doing something special in 10.4 and 10.5 which they quit doing in 10.6 when Finder does an admin auth to copy into /Applications.

Which effectively puts this back to the original problem -- if you're not an admin user you can't update.  The only change here is prior to 10.6 you could still update if you weren't an admin as long as you were the user that installed it, and now you can't update, period, if you're not an admin (unless you manually chown the app to be owned by you, which was my workaround for this).

While it would still be nice to fix, this probably makes it no more of a blocker than it was before.

I should probably point out that several other Mac applications that have automatic update *can* update if you're not an admin without the app needing to be owned by you (Adium, NetNewsWire, iTerm, and Telephone come to mind).  Those apps all prompt for an admin password when it gets to the install part of the update process.

Comment 8

[Sierk Bornemann]

Concerning Firefox on MacOSX:

Why not simply use the popular Sparkle framework from http://sparkle.andymatuschak.org/ for updating Firefox on the MacOSX platform even from a non-privileged user account (elevating permissions via system's prompt for an admin password when writing into /Applications), like many other applications on MacOSX incl. Camino Browser already do?

Or, alternatively to the Sparkle framework, using Google's Update engine framework from http://code.google.com/p/update-engine/ ?

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #8)
> Why not simply use the popular Sparkle framework from
> http://sparkle.andymatuschak.org/ for updating Firefox on the MacOSX platform
> even from a non-privileged user account (elevating permissions via system's
> prompt for an admin password when writing into /Applications), like many other
> applications on MacOSX incl. Camino Browser already do?

I'll second this.  I'm pretty sure that's what all the apps I named at the bottom of comment 6 are using, too.

Although the built-in updater in Firefox does a lot of things Sparkle doesn't...  so it may make better sense to just copy Sparkle's behavior about getting an admin auth before performing the actual update.

Comment 10

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

Josh pointed me to AuthorizationCreate awhile back and it looks simple enough to implement but a) I don't have a Mac b) I doubt I'll have time to do this myself for 3.7. Added url to AuthorizationCreate docs

Comment 14

[Ray Kiddy]

It would be good if a search for 'Unable to Update', the title of the dialog
box one gets, would return this bug. It might now. We'll see.

cheers - ray

Comment 16

[Sierk Bornemann]

Access privileged functionality (for instance: application update) from a non-privileged application on Mac OS X:

Apple Developer Mac OS X Reference Library: BetterAuthorizationSample from http://developer.apple.com/library/mac/#samplecode/BetterAuthorizationSample/Introduction/Intro.html

Or use Sparkle from http://sparkle.andymatuschak.org/ (also used by Camino and many, many other third party applications on the Mac to allow a privileged application update from the role of a non-privileged user).

Comment 17

[artemis]

I have no idea why a particular user has continued to misinterpret my bug, but I only realized mine had been marked as a duplicate of this when I got an email that this bug had been updated.  Mine is NOT a duplicate - I am the only person who uses my computer - in other words, the same person administrates my computer, runs it, and adds ALL software to it - including Firefox.  Since said user is the only one who has answered my continued-to-be-a-bug in several months, I guess I will just opt to stop receiving updates to this bug, since this one is not the same bug as mine, no matter what anyone else says.  I have found this entire attempt to get my bug fixed to be very frustrating, and I guess it is a testament to how much I like Firefox that I continue to use it despite both the bug and the lack of helpful solutions to it.

Comment 18

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

comment #17 is in regards to bug 556766. In this instance the system was sent out for repair and after that the issue started... the rest of the details can be read in the bug.

(In reply to comment #17)
Regretfully, there is only so much time I can spend helping diagnose a problem since it takes time away from working on the code. For assistance, http://support.mozilla.com/ is available.

Comment 19

[Ray Kiddy]

I think that [e-mail] might be unsure of what we mean by user. 

My understanding of this problem is that if a person (of flesh and blood, who breathes, types and so on) logs on to a particular mac using one account (a thing that the system has a home directory for, and which requires a password to access) that has administrator privileges and installs Firefox, then if that same person or any other person uses another account, even if that account also has administration privileges, then there is a problem updating that copy of Firefox.

Rather than use the word "user" and use three or more meanings of that word at the same time, we could use other words that may be more clear. Just a thought.

Comment 21

[Sean]

Does Firefox 4.0 resolve this issue?

Thanks

Comment 22

[artemis]

I'm not sure if Sean is responding to my bug (which Robert Strong marked as a duplicate of this even though it is not a duplicate), but I don't know whether Firefox 4 resolves it or not, because the option to manually update isn't under the "Help" drop-down menu any more and I can't locate it elsewhere.  In previous versions when I tried to update it manually it would say I wasn't the administrator and thus was not granted "permission," so that I would know the problem was still present.  Despite what Robert Strong has repeatedly said in response to my comments (including marking my bug as a duplicate of this one), I am literally the ONLY user of my computer - by which I mean that it is not just me that installed Firefox, but that no one else EVER uses it, and I only have a single account on my computer as a result - so there is no reason why Firefox should be claiming I am not the administrator, and Firefox is, again literally, the ONLY program I have installed on my computer that makes the claim that I am not the administrator.  

I hope that the new version makes this problem go away, because as Firefox's  troubleshooting section of its website points out, the only way to get around this problem is to download the entire browser again after every update, which has taken up a heap of space on my laptop.  It continues to be a testament to how much I like Firefox that I have continued to download it despite both this space issue and getting some unhelpful comments on here when I tried to find an alternate solution.

Comment 24

[allan marcus]

may I suggest a couple of ways to resolve this bug.

1) do not write the updates to a directory in the /Applications tree. Write now the updates are written to /Applications/Firefox.app/Contents/MacOS/updates. The "mac Way" would be to write the update to /Library/Application Support/Firefox/updates. If the /Library/Application Support/Firefox directory doesn't exists, create it, set to 774, root:admin. This will allow the updates to download if any admin is running FF. 

2) When it's time to perform the update, always have the user authenticate using admin credentials, even if the user has admin rights, then run the update with sudo. that way ownership of the FF application doesn't matter.

As a corporate FF user we prefer that applications only be updated after authentication.

Alternatively, if the running user owns the FF app, then just let the update run. That should allow for the average home user to "auto update".

Thanks,

Allan

Comment 25

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

(In reply to comment #24)
> may I suggest a couple of ways to resolve this bug.
> 
> 1) do not write the updates to a directory in the /Applications tree. Write
> now the updates are written to
> /Applications/Firefox.app/Contents/MacOS/updates. The "mac Way" would be to
> write the update to /Library/Application Support/Firefox/updates. If the
> /Library/Application Support/Firefox directory doesn't exists, create it,
> set to 774, root:admin. This will allow the updates to download if any admin
> is running FF. 
Already known as a requirement

> 2) When it's time to perform the update, always have the user authenticate
> using admin credentials, even if the user has admin rights, then run the
> update with sudo. that way ownership of the FF application doesn't matter.
There are Mac API's specifically for this (see this bug's url field).

> As a corporate FF user we prefer that applications only be updated after
> authentication.
>
> Alternatively, if the running user owns the FF app, then just let the update
> run. That should allow for the average home user to "auto update".
The plan is to only request if the user requires authentication.

Comment 26

[allan marcus]

awesome, much thanks.

Comment 28

[Sierk Bornemann]

Folks, I recall again comment #8 and comment #16.
Implement Sparkle or use Apples sample code (URL in comment #16). For convenience, make use of Sparkle, like many other software vendors with _exactly_ the same challenges also make use of it! Why elaborate a so long period of time on such an issue and trying to reinvent the wheel, when the whole problem could be solved so easily with integration of an existing open source update framework that is so widely used for exactly the same purpose?

A good and best practise solution:
http://sparkle.andymatuschak.org/
https://github.com/andymatuschak/Sparkle
http://en.wikipedia.org/wiki/Sparkle_%28software%29

Just integrate the Sparkle framework into Firefox! This will make a lot of people happy and will save you a lot of work and hassle!

Comment 29

[Sierk Bornemann]

See also this relating Camino bug:

Bug 445629 - Land Sparkle 1.5 once it's done

Comment 30

[(not currently active) Ted Mielczarek]

Firefox already has a cross-platform update system. Replacing that with a platform-specific solution on one platform will be much more work than simply fixing this bug.

Comment 31

[Sierk Bornemann]

(In reply to Ted Mielczarek [:ted, :luser] from comment #30)
> Firefox already has a cross-platform update system. Replacing that with a
> platform-specific solution on one platform will be much more work than
> simply fixing this bug.

We see, how reliable and satisfying your preferred cross-platform solution works... :-(

Comment 32

[(not currently active) Ted Mielczarek]

I am not saying it has no bugs. Integrating an entirely different solution would be a huge amount of work, which would almost certainly result in a different set of bugs. It might fix this particular problem, but there's no guarantee that it would be a net improvement. Firefox's update system has been around for a long time at this point, and many other things have been fixed in it.

Comment 33

[Sierk Bornemann]

(In reply to Ted Mielczarek [:ted, :luser] from comment #32)
> I am not saying it has no bugs. Integrating an entirely different solution
> would be a huge amount of work, which would almost certainly result in a
> different set of bugs. It might fix this particular problem, but there's no
> guarantee that it would be a net improvement. 

> Firefox's update system has
> been around for a long time at this point, and many other things have been
> fixed in it.

And it never has worked flawlessly, and you still are elaborating on _fundamental_ issues, that should just _work_ since long ago. On all platforms! Not even on the Windows platform it works satisfying enough.

And relating the Mac platform: Sparkle works!! It proves that daily as a part of more than hundred applications. Then provide a best-practise-working solution at least for Firefox on the Mac platform and give the Mac users a solution, that just works. And that is Sparkle. Sparkle works! It solves precisely what the update mechanism should do, it is convenient for the users and for the developer. Not being cross-platform is the aim, the aim is to provide solutions to the user that function in daily practise. And if this means to provide a platform-specific solution to provide the _best_ result in favor to the _user_ (the user and his needs and his chosen platform are the focus!), then a platform-specific solution shouldn't be no longer a taboo -- even more, when there does exist a widely practise-proven solution like Sparkle that just works and works very superb.

Why keeping at least the Mac users from a solution (Sparkle) that works? Why being bullheaded in favouring a cross platform solution, which after years of elaborating still doesn't work as it _should_ work? Neither on the Windows platform nor on the mac Platform nor on the Linux platform?
Being cross plattform is a good aim to follow, but it has its limits, and one should acknowledge these limits. And foremost: it shouldn't end in itself.

Comment 34

[Dave Miller [:justdave]]

OK, so I'm not a Firefox developer, and I come into this with zero first-hand knowledge of how the app team wants to handle this, but from my outsider's point of view, here's the issues I see with using Sparkle, based on my quick look at the docs just now, and what I do know about Firefox's updates.

1) In order to make update downloads smaller, Firefox's update system distributes updates as patches, not a new application download.  Either a new standalone installer would have to be created which could be launched by Sparkle to apply the patch, which would have to be shipped in a .pkg file with the patch in order for Sparkle to run it (might make the doanload bigger), or the build system would have be changed to generate a different type of patch file which is compatible with Sparkle's built-in patching system. (see https://github.com/andymatuschak/Sparkle/wiki/Delta-Updates )

2) This would require server-side changes to Firefox's update service to provide an update feed which is compatible with Sparkle.  Granted, these changes don't look particularly hard.  In fact, the XML snippet fed to Firefox by Firefox's update service looks remarkably similar to what Sparkle wants, it just uses a different XML schema.

I think the hard part is going to be #1, getting the build system to package Firefox in a way that Sparkle can actually install it.

Comment 35

[Sierk Bornemann]

(In reply to Dave Miller [:justdave] from comment #34)

> I think the hard part is going to be #1, getting the build system to package
> Firefox in a way that Sparkle can actually install it.

How did the Camino developers solve it? Their code is available on your Mozilla servers, you can build on their know-how, on their experiences, on their code, even maybe re-use their code.

Comment 38

[Sierk Bornemann]

Any progress on this bug, especially taking to account comment #10, comment #24 and comment #25 (as direct reply to the latter)?

Comment 39

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

spohl (already cc'd to this bug) has done some of the initial investigation work and will likely be the one to fix this bug after he has finished with fixing the regressions from bug 636564 and finishes up the remainder of bug 678392 and bug 836456

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #39)
> spohl (already cc'd to this bug) has done some of the initial investigation
> work and will likely be the one to fix this bug after he has finished with
> fixing the regressions from bug 636564 and finishes up the remainder of bug
> 678392 and bug 836456

Having just experienced this and being concerned about user updates, I wonder if the solution for this would be able to reach people's user accounts that are already not able to update or if this will require an update first to be on track for future updates across multiple admin accounts. The former would be preferred obviously.

Comment 41

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Move MainMenu.nib to a location that allows for signing of the firefox binary

This moves the MainMenu.nib from its current location to allow for signing of the firefox binary via codesign. I could not identify any side-effects from this change.

Comment 42

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Main logic that allows for elevated updates on OSX.

There is a lot going on here. What follows is a high-level overview:

1. If we fail to write to the location that is usually used for updates, we attempt to run an elevated update.
2. A first (non-elevated) org.mozilla.updater process is launched by Firefox. It acts as a server for the elevated updater. Once the elevated update completes, it will also relaunch Firefox as the same user (with the same environment) as the original Firefox process.
3. A second (elevated) org.mozilla.updater process is launched by Firefox. This occurs via the SMJobBless API. After launching the elevated updater, Firefox will shut down. The elevated updater will connect to the first (non-elevated) updater process to get necessary arguments. When done, it cleans itself up and shuts down the server in the first (non-elevated) updater.
4. The first (non-elevated) updater restarts Firefox and shuts down.
5. The newly started Firefox instance checks success/failure of the update and responds to it in the same way as our existing (non-elevated) updates.

Comment 43

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Signing of firefox and org.mozilla.updater binaries (WIP)

In order for SMJobBless to accept our binaries and allow us to run an elevated update, we need to sign our firefox and updater binaries. This will need to be integrated into our build system. This patch demonstrates how to use a local certificate to sign the binaries.

Comment 44

[Stephen A Pohl [:spohl]]

(In reply to Lukas Blakk [:lsblakk] from comment #40)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #39)
> Having just experienced this and being concerned about user updates, I
> wonder if the solution for this would be able to reach people's user
> accounts that are already not able to update or if this will require an
> update first to be on track for future updates across multiple admin
> accounts.

Unfortunately, I could not find a way to fix this for existing installations. A manual update will be necessary for people experiencing this issue today.

Comment 45

[Stephen A Pohl [:spohl]]

Comment on attachment 8389942 [details] [diff] [review]
Part 2: Main logic that allows for elevated updates on OSX.

Review of attachment 8389942 [details] [diff] [review]:
-----------------------------------------------------------------

Since review might already have started I'm making a note of a few things that I missed before uploading this patch.

::: toolkit/mozapps/update/updater/macbuild/Contents/Launchd.plist
@@ +4,5 @@
> +<dict>
> +	<key>Label</key>
> +	<string>org.mozilla.updater</string>
> +  <key>RunAtLoad</key>
> +  <true/>

fix indent

::: toolkit/mozapps/update/updater/updater.cpp
@@ +73,2 @@
>  void LaunchMacPostProcess(const char* aAppExe);
> +bool ServeElevatedUpdate(int argc, const char** argv);

reorder these declarations to match the order in launchchild_osx.mm

@@ +3251,5 @@
>        LaunchCallbackApp(argv[4], 
>                          argc - callbackIndex, 
>                          argv + callbackIndex, 
> +                        sUsingService
> +                        );

revert this change

Comment 46

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

Comment on attachment 8389942 [details] [diff] [review]
Part 2: Main logic that allows for elevated updates on OSX.

The main issues that still need to be addressed are:
1. there needs to be a unique updates directory for each install location. I think using essentially the same logic as used for Windows should be sufficient though instead of using a hash of the install directory path you could just append the path to the install directory.
2. Always return the external directory from GetUpdateRootDir. The additional complexity is just not worth it.
Note: the above two items will simplify things greatly elsewhere in the patch.
3. head_update.js needs a mac implementation of getMockUpdRootD to return the same directory as returned by GetUpdateRootDir.

Comment 47

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

Comment on attachment 8389947 [details] [diff] [review]
Part 2: Signing of firefox and org.mozilla.updater binaries (WIP)

Ben, heads up that this will require changing how Mac builds get signed.

Comment 48

[[e-mail] (:bhearsum)]

Comment on attachment 8389947 [details] [diff] [review]
Part 2: Signing of firefox and org.mozilla.updater binaries (WIP)

Review of attachment 8389947 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/Makefile.in
@@ +167,5 @@
>  	$(MKDIR) -p $(dist_dest)/Contents/Library/LaunchServices
>  	mv -f $(dist_dest)/Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater $(dist_dest)/Contents/Library/LaunchServices
>  	ln -s ../../../../Library/LaunchServices/org.mozilla.updater $(dist_dest)/Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater
> +	codesign --deep --force --continue -s DBE11E4A23799101CD416D8504DEEB0D85084AFB $(dist_dest)/Contents/MacOS/firefox
> +	codesign --deep --force --continue -s DBE11E4A23799101CD416D8504DEEB0D85084AFB $(dist_dest)/Contents/Library/LaunchServices/org.mozilla.updater

I don't think we want this in the Makefile...but we will need to adjust our signing script to do this. Is there any downside to doing this everywhere? Is this bug going to be blocked on the signing server changes?

::: browser/app/macbuild/Contents/Info.plist.in
@@ +221,5 @@
> +	<key>SMPrivilegedExecutables</key>
> +	<dict>
> +		<key>org.mozilla.updater</key>
> +		<string>identifier "org.mozilla.updater" and anchor apple generic and certificate leaf[subject.CN] = "Mac Developer: Stephen Pohl (N7SW584TN4)" and certificate 1[field.1.2.840.113635.100.6.2.1] /* exists */</string>
> +	</dict>

Does this or the other Info.plist change require any signing changes?

Comment 49

[Stephen A Pohl [:spohl]]

(In reply to Ben Hearsum [:bhearsum] from comment #48)
> I don't think we want this in the Makefile...but we will need to adjust our
> signing script to do this.

Right, this particular patch doesn't make any claim about how this should be incorporated into our build system (as I have yet to learn enough about it). It's just what worked for me locally and allowed me to write patches part 1 and 2.

> Is there any downside to doing this everywhere?

I'm not sure. What does 'everywhere' mean here?

> Is this bug going to be blocked on the signing server changes?

It is, but this bug is also blocked by bug 978596. This will be my next focus.

> ::: browser/app/macbuild/Contents/Info.plist.in
> @@ +221,5 @@
> > +	<key>SMPrivilegedExecutables</key>
> > +	<dict>
> > +		<key>org.mozilla.updater</key>
> > +		<string>identifier "org.mozilla.updater" and anchor apple generic and certificate leaf[subject.CN] = "Mac Developer: Stephen Pohl (N7SW584TN4)" and certificate 1[field.1.2.840.113635.100.6.2.1] /* exists */</string>
> > +	</dict>
> 
> Does this or the other Info.plist change require any signing changes?

There is a little bit of complexity to this. The changes to the Info.plist files will need be done dynamically during build time. The workflow should look something like this:
1. Sign firefox binary.
2. Call 'codesign --display -r-'[1] on the signed firefox binary to extract its internal requirements.
3. Add the extracted internal requirements from the firefox binary to org.mozilla.updater's Info.plist.
4. Build org.mozilla.updater. With the changes to toolkit/mozapps/update/updater/Makefile.in from part 2 of the patches, this will compile the Info.plist into the actual org.mozilla.updater binary.
5. Sign org.mozilla.updater.
6. Call 'codesign --display -r-'[1] on the signed org.mozilla.updater binary to extract its internal requirements.
7. Add the extracted internal requirements from the org.mozilla.updater binary to firefox's Info.plist.


[1] https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/codesign.1.html

Comment 50

[[e-mail] (:bhearsum)]

(In reply to Stephen Pohl [:spohl] from comment #49)
> (In reply to Ben Hearsum [:bhearsum] from comment #48)
> > Is there any downside to doing this everywhere?
> 
> I'm not sure. What does 'everywhere' mean here?

All of our old branches, back to esr24. It's much easier for us if these new codesign commands can be safely run on branches that aren't going to have all of this new code.

> 
> > Is this bug going to be blocked on the signing server changes?
> 
> It is, but this bug is also blocked by bug 978596. This will be my next
> focus.

OK. Can you file a bug on RelEng: General Automation so we can track this? Any ETA on when you're hoping to land would be helpful too, so we can try to stay out of the critical path.

> > ::: browser/app/macbuild/Contents/Info.plist.in
> > @@ +221,5 @@
> > > +	<key>SMPrivilegedExecutables</key>
> > > +	<dict>
> > > +		<key>org.mozilla.updater</key>
> > > +		<string>identifier "org.mozilla.updater" and anchor apple generic and certificate leaf[subject.CN] = "Mac Developer: Stephen Pohl (N7SW584TN4)" and certificate 1[field.1.2.840.113635.100.6.2.1] /* exists */</string>
> > > +	</dict>
> > 
> > Does this or the other Info.plist change require any signing changes?
> 
> There is a little bit of complexity to this. The changes to the Info.plist
> files will need be done dynamically during build time. The workflow should
> look something like this:
> 1. Sign firefox binary.
> 2. Call 'codesign --display -r-'[1] on the signed firefox binary to extract
> its internal requirements.

Are you sure we need to sign the binary to get the requirements? We specify those as part of our codesign command: https://github.com/mozilla/build-tools/blob/master/lib/python/signing/utils.py#L17 and https://github.com/mozilla/build-tools/blob/master/lib/python/signing/utils.py#L232. We may need to consider moving that string or duplicating it in tree. This would be pretty similar to how we embed fingerprints of our Windows code signing certificates, I think.

> 3. Add the extracted internal requirements from the firefox binary to
> org.mozilla.updater's Info.plist.
> 4. Build org.mozilla.updater. With the changes to
> toolkit/mozapps/update/updater/Makefile.in from part 2 of the patches, this
> will compile the Info.plist into the actual org.mozilla.updater binary.
> 5. Sign org.mozilla.updater.

This will require a separate $(MOZ_SIGN_CMD) invocation, similar to https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#574 (note that MOZ_SIGN_CMD is part of MOZ_SIGN_PREPARED_PACKAGE_CMD). I imagine this will have to be done in https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/Makefile.in or somewhere similar.

> 6. Call 'codesign --display -r-'[1] on the signed org.mozilla.updater binary
> to extract its internal requirements.
> 7. Add the extracted internal requirements from the org.mozilla.updater
> binary to firefox's Info.plist.

We should be able to specify the updater requirements to avoid the need for this round trip, too.


To summarize: Since we can specify our requirements at signing time, I don't think this process can be simplified to:
* Add Firefox requirements to updater's Info.plist
* Add updater requirements to Firefox's Info.plist
* Sign updater
* Sign Firefox

(The order really doesn't matter though, as long as you don't sign before updating the relevant Info.plist.)

Can we give that a shot Stephen?

Comment 51

[Stephen A Pohl [:spohl]]

(In reply to Ben Hearsum [:bhearsum] from comment #50)
> (In reply to Stephen Pohl [:spohl] from comment #49)
> > (In reply to Ben Hearsum [:bhearsum] from comment #48)
> > > Is there any downside to doing this everywhere?
> > 
> > I'm not sure. What does 'everywhere' mean here?
> 
> All of our old branches, back to esr24. It's much easier for us if these new
> codesign commands can be safely run on branches that aren't going to have
> all of this new code.

I can't see any downsides to having signed binaries on our other branches. However, we may not be able to sign these binaries without making changes that could result in bugs. One issue is that the firefox binary seemingly cannot be signed without part 1 of the patches in this bug. Also, the org.mozilla.updater binary used to be called updater until part 2 of the patches here. Speaking with only limited knowledge of our build system, it may be easier and safer to add this additional signing for our current branch only, rather than backporting the patches here to our other branches.


> > > Is this bug going to be blocked on the signing server changes?
> > 
> > It is, but this bug is also blocked by bug 978596. This will be my next
> > focus.
> 
> OK. Can you file a bug on RelEng: General Automation so we can track this?

Done: bug 983728. I'm happy to take further discussion about our signing requirements over there if you'd like.


> Any ETA on when you're hoping to land would be helpful too, so we can try to
> stay out of the critical path.

I have yet to look at the details and native APIs for bug 978596, so I don't have a clear ETA yet. All I can say is that bug 978596 will be my main focus starting on Monday when I'm back from PTO.


> Are you sure we need to sign the binary to get the requirements? We specify
> those as part of our codesign command:
> https://github.com/mozilla/build-tools/blob/master/lib/python/signing/utils.
> py#L17 and
> https://github.com/mozilla/build-tools/blob/master/lib/python/signing/utils.
> py#L232. We may need to consider moving that string or duplicating it in
> tree. This would be pretty similar to how we embed fingerprints of our
> Windows code signing certificates, I think.

No, I'm not sure that we need to sign the binary before we can get the requirements. If we have a better way of doing this already, that's awesome and we should definitely use that! :-)

 
> We should be able to specify the updater requirements to avoid the need for
> this round trip, too.

Awesome! Anything we can do to reduce complexity.


> To summarize: Since we can specify our requirements at signing time, I don't
> think this process can be simplified to:
> * Add Firefox requirements to updater's Info.plist
> * Add updater requirements to Firefox's Info.plist
> * Sign updater
> * Sign Firefox

I've copied these steps into the description of bug 983728. I agree that if we have the signing requirements available, these steps should be all we need.


> (The order really doesn't matter though, as long as you don't sign before
> updating the relevant Info.plist.)

We could technically sign the firefox binary before adding the updater's requirements to firefox's Info.plist (since the Info.plist will not be compiled into the firefox binary), but we may as well just keep the order that you mentioned above.


> Can we give that a shot Stephen?

I like it! Is there anything you'd like me to try out locally?

Thanks for all your help, Ben!

Comment 52

[[e-mail] (:bhearsum)]

(In reply to Stephen Pohl [:spohl] (pto 3/14) from comment #51)
> (In reply to Ben Hearsum [:bhearsum] from comment #50)
> > (In reply to Stephen Pohl [:spohl] from comment #49)
> > > (In reply to Ben Hearsum [:bhearsum] from comment #48)
> > > > Is there any downside to doing this everywhere?
> > > 
> > > I'm not sure. What does 'everywhere' mean here?
> > 
> > All of our old branches, back to esr24. It's much easier for us if these new
> > codesign commands can be safely run on branches that aren't going to have
> > all of this new code.
> 
> I can't see any downsides to having signed binaries on our other branches.
> However, we may not be able to sign these binaries without making changes
> that could result in bugs. One issue is that the firefox binary seemingly
> cannot be signed without part 1 of the patches in this bug. Also, the
> org.mozilla.updater binary used to be called updater until part 2 of the
> patches here. Speaking with only limited knowledge of our build system, it
> may be easier and safer to add this additional signing for our current
> branch only, rather than backporting the patches here to our other branches.

It's sounding to me like this change will need to ride the trains. That's not the end of the world, but it will make bug 983728 a bit more complicated.

I'd also like to clarify what you mean by signing the firefox binary. I'm not sure what the difference is, and just signing the binary doesn't sound right to me - I suspect that CodeResources is getting ignored, which could lead to modified behaviour without breaking the signature. I _think_ you want to be running the same command on the app-bundle, but I'd also love to talk a look at one of your signed .app's to see what it looks like. We can chat on IRC or Vidyo about this if you want.

> > Any ETA on when you're hoping to land would be helpful too, so we can try to
> > stay out of the critical path.
> 
> I have yet to look at the details and native APIs for bug 978596, so I don't
> have a clear ETA yet. All I can say is that bug 978596 will be my main focus
> starting on Monday when I'm back from PTO.

OK. That bug probably requires a few days work including all the testing, but I'm not going to look at it just yet, since you're focused on something else.

> > Can we give that a shot Stephen?
> 
> I like it! Is there anything you'd like me to try out locally?

Related to the above, I'd really like to know what happens when you sign the .app instead of firefox. My suspicion is that signing the .app is a superset of signing just the firefox binary.

Comment 53

[Stephen A Pohl [:spohl]]

Ben and I discussed the signing requirements a bit on IRC. I'll be running a few tests locally to see how much we can simplify the requirements for the build system.

Comment 54

[[e-mail] (:bhearsum)]

Comment on attachment 8389947 [details] [diff] [review]
Part 2: Signing of firefox and org.mozilla.updater binaries (WIP)

Remove feedback request for now. Stephen, we should probably sync up again sometime this week to make sure we're both still on the same page.

I seem to recall that some of the testing required me to do some signing on our 10.6 signing servers too...

Comment 55

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Main logic that allows for elevated updates on OSX.

Addressed most feedback. The changes to head_update.js will be posted in a separate patch part 4 along with any other necessary changes to make our tests pass.

Comment 56

[Stephen A Pohl [:spohl]]

Attachment: Part 3: Test changes (WIP)

rstrong's changes to make our non-staging tests pass. We still need to look into our staging tests and execution paths before we can call this done.

Comment 57

[Stephen A Pohl [:spohl]]

Comment on attachment 8389934 [details] [diff] [review]
Part 1: Move MainMenu.nib to a location that allows for signing of the firefox binary

I'm working with Ben to figure out our specific signing requirements. This patch may not be needed for the final solution, so canceling review request for now.

Comment 58

[Stephen A Pohl [:spohl]]

Thanks to Ben's help, we were able to confirm that the signing requirements currently available on the build system satisfy SMJobBless' requirements. It is also sufficient to sign the Firefox .app bundle (rather than the firefox binary inside the bundle). Here is a summary of what we need the build system to do:
1. Add signing requirements, identity and OU to the two plists. One for the Firefox bundle, one for the org.mozilla.updater binary.
2. Ensure that org.mozilla.updater is built after the signing requirements were added to its plist (because the plist will get compiled directly into the binary).
3. Sign the org.mozilla.updater binary under Contents/Library/LaunchServices in the Firefox .app bundle with the certificate that matches the signing requirements from point 1 above.
4. Sign the Firefox .app bundle with the certificate that matches the signing requirements from point 1 above.

Comment 59

[Stephen A Pohl [:spohl]]

Comment on attachment 8389934 [details] [diff] [review]
Part 1: Move MainMenu.nib to a location that allows for signing of the firefox binary

Based on our current understanding of the signing requirements, this patch appears to be obsolete.

Comment 60

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Main logic that allows for elevated updates on OSX.

Updated for the fact that patch part 1 is now obsolete.

Comment 61

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

Comment on attachment 8394209 [details] [diff] [review]
Part 2: Main logic that allows for elevated updates on OSX.

Partial review. I'll try to get the rest done today.

Note: there will need to be a way to disable using SMJobBless similar to how we allow disabling the service.

>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>...
>@@ -990,17 +990,45 @@ nsXREDirProvider::GetUpdateRootDir(nsIFi
> 
> #else
>   nsCOMPtr<nsIFile> appFile;
>   bool per = false;
>   nsresult rv = GetFile(XRE_EXECUTABLE_FILE, &per, getter_AddRefs(appFile));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = appFile->GetParent(getter_AddRefs(updRoot));
>   NS_ENSURE_SUCCESS(rv, rv);
>+#ifdef XP_MACOSX
>+  bool hasVendor = gAppData->vendor && strlen(gAppData->vendor) != 0;
> 
>+  if (hasVendor || gAppData->name) {
>+    nsCOMPtr<nsIFile> appRootDirFile;
>+    nsCOMPtr<nsIFile> localDir;
>+    nsAutoString appDirPath;
>+
>+    if (NS_SUCCEEDED(appFile->GetParent(getter_AddRefs(appRootDirFile))) &&
>+        NS_SUCCEEDED(appRootDirFile->GetPath(appDirPath))) {
>+      int32_t dotIndex = appDirPath.RFind(".app");
>+      if (dotIndex == kNotFound) {
>+        dotIndex = appDirPath.Length();
I think this is the most straightforward way to deal with not having a .app but I'd like to know if you think it would be overly complex to just fallback to the directory below?

>+      }
>+      appDirPath = Substring(appDirPath, 1, dotIndex - 1);
>+
>+      if (NS_SUCCEEDED(GetUserDataDirectoryHome(getter_AddRefs(localDir),
>+                                                true)) &&
>+          NS_SUCCEEDED(localDir->AppendNative(nsDependentCString(hasVendor ?
>+                                                gAppData->vendor :
>+                                                gAppData->name))) &&
>+          NS_SUCCEEDED(localDir->Append(NS_LITERAL_STRING("updates"))) &&
>+          NS_SUCCEEDED(localDir->AppendRelativePath(appDirPath))) {
>+        NS_ADDREF(*aResult = localDir);
>+        return NS_OK;
>+      }
>+    }
>+  }
>+#endif // XP_MACOSX
> #ifdef XP_WIN
> 
>   nsAutoString pathHash;
>   bool pathHashResult = false;
>   bool hasVendor = gAppData->vendor && strlen(gAppData->vendor) != 0;
> 
>   nsAutoString appDirPath;
>   if (SUCCEEDED(updRoot->GetPath(appDirPath))) {
>@@ -1081,17 +1109,17 @@ nsXREDirProvider::GetUpdateRootDir(nsIFi
>   }
> 
>   rv = GetUserLocalDataDirectory(getter_AddRefs(updRoot));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = updRoot->AppendRelativePath(programName);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-#endif
>+#endif // XP_WIN
> #endif
>   NS_ADDREF(*aResult = updRoot);
>   return NS_OK;
> }
> 
> nsresult
> nsXREDirProvider::GetProfileStartupDir(nsIFile* *aResult)
> {
>diff --git a/xpcom/build/nsXULAppAPI.h b/xpcom/build/nsXULAppAPI.h
>--- a/xpcom/build/nsXULAppAPI.h
>+++ b/xpcom/build/nsXULAppAPI.h
>@@ -112,20 +112,16 @@
> /**
>  * A directory service key which specifies the distribution specific files for
>  * the application.
>  */
> #define XRE_APP_DISTRIBUTION_DIR "XREAppDist"
> 
> /**
>  * A directory service key which provides the update directory.
>- * At present this is supported only on Windows.
>- * Windows: Documents and Settings\<User>\Local Settings\Application Data\
>- *          <Vendor>\<Application>\<relative path to app dir from Program Files>
>- * If appDir is not under the Program Files, directory service will fail.
Get rid of this bit and file a followup bug to cleanup the comment.

Comment 62

[[e-mail] (:bhearsum)]

Sorry for the delay in response, I've been thinking about how best to integrate with our signing infrastructure. Lots below:

(In reply to Stephen Pohl [:spohl] from comment #58)
> Thanks to Ben's help, we were able to confirm that the signing requirements
> currently available on the build system satisfy SMJobBless' requirements. It
> is also sufficient to sign the Firefox .app bundle (rather than the firefox
> binary inside the bundle). Here is a summary of what we need the build
> system to do:
> 1. Add signing requirements, identity and OU to the two plists.

To break this down a bit more, here's an example (from the updater's Info.plist):
identifier "org.mozilla.nightly" and ( (anchor apple generic and certificate leaf[field.1.2.840.113635.100.6.1.9] ) or (anchor apple generic and    certificate 1[field.1.2.840.113635.100.6.2.6]  and    certificate leaf[field.1.2.840.113635.100.6.1.13] and    certificate leaf[subject.OU] = "43AQ936H96"))

The identifier part _should_ be easy to figure out. We reference it as "%MOZ_MACBUNDLE_ID%" in Firefox's Info.plist (https://mxr.mozilla.org/mozilla-central/source/browser/app/macbuild/Contents/Info.plist.in#152).

Everything before the subject.OU is the same across all build types, so I don't see any reason that can't be hardcoded into the tree.

The subject.OU value differs depending on which certificate the build is signed with. On-change and Try builds use one value, Nightly/Aurora nightlies use a different value, and Betas and Releases use another different value. It's probably O.K. to hardcode this into the tree, and choose the right value similar to how we choose MAR fingerprints: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.rc#48, https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/moz.build#77 -- perhaps time put those variables at a higher level and rename them, though.

> 2. Ensure that org.mozilla.updater is built after the signing requirements
> were added to its plist (because the plist will get compiled directly into
> the binary).

This should be fine if we're hardcoding like above.

> 3. Sign the org.mozilla.updater binary under Contents/Library/LaunchServices
> in the Firefox .app bundle with the certificate that matches the signing
> requirements from point 1 above.

I think an easier alternative is to sign Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater, and then copy it over to Contents/Library/LaunchServices afterwards. I'm actually really confused about how just signing the LaunchServices one works...I don't see how it could be taking the updater's Info.plist into account given that. In any case, I'm certain that we want the two updater binaries to bit for bit identical.

Mechanically, this should be pretty easy to do. You'll need to get $(MOZ_SIGN_CMD) called (when defined, and only on Mac) at some point during the updater's build process. It looks like https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/Makefile.in#53 may be a good place, but you'll probably know better than me. Calling it should end up being almost the same as https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/makensis.mk#49, but you'll want MOZ_INTERNAL_SIGNING_FORMAT instead of EXTERNAL.

This won't work quite yet though, because I need to adjust our builders to give the build machine a signing token before the build starts. I'll probably co-opt bug 983728 to do that, since that work is likely to be done here now.

> 4. Sign the Firefox .app bundle with the certificate that matches the
> signing requirements from point 1 above.

Like #2, this should be easy if we're hardcoding.


This is quite a braindump, please let me know if anything is unclear or I haven't addressed everything.

Comment 63

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

Comment on attachment 8394209 [details] [diff] [review]
Part 2: Main logic that allows for elevated updates on OSX.

Replacing review request with feedback request until after the mac mar signing is done.

Comment 64

[Stephen A Pohl [:spohl]]

Attachment: Flow Diagram

This may address two requirements for a security review[1]:
1. Architecture Diagram, since it lists all components of the proposed solution.
2. Detailed Application Diagram, since it illustrates each path that data can flow through.

https://wiki.mozilla.org/Security/ReviewProcess

Comment 65

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Main logic that allows for elevated updates on OSX.

Updated for current trunk.

Comment 66

[Stephen A Pohl [:spohl]]

Started a document for the security review here:
https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185

Comment 67

[Stephen A Pohl [:spohl]]

1. Who is/are the point of contact(s) for this review?
Stephen Pohl [:spohl]

2. Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
When one administrative user installed Firefox, but another administrative user tried to update Firefox on Mac OSX, the update used to fail. This was due to fact that the second user did not have write access to the application folder. By implementing elevated updates on OSX, the second user can now update Firefox without changing the permissions on the application folder. Elevated updates are only used when needed.

3. Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:
Architecture & detailed application diagram, data-flow enumeration and threat analysis can be found here: https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185

4. Does this request block another bug? If so, please indicate the bug number
This feature could be used to address bug 318855.

5. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?
It would be great to have this reviewed in time for landing in Q2.

6. To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?
Universal MAR verification and elevated updates on OSX (this bug) were candidates to be added to our Q2 goals. Not sure if they actually made it on the list.

Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)
8. Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?
This feature affects any product that uses our updater on OSX.

9. Are there any portions of the project that interact with 3rd party services?
No.

10. Will your application/service collect user data? If so, please describe 
No additional user data is collected as part of this feature.

11. If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):
n/a

12. Desired Date of review (if known from https://mail.mozilla.com/home/[e-mail]/Security%20Review.html) and whom to invite. 
Date: No specific date desired.
People to invite: rstrong, bbondy


(This was based on the template at https://wiki.mozilla.org/Security/Reviews/Review_Request_Form#Questions_to_Address_within_Request_Body)

Comment 68

[Stephen A Pohl [:spohl]]

This came as a bit of a surprise, but I will be out for two weeks starting this coming Monday and landing of these patches in Q2 is now unlikely. Please feel free to adjust the priority of the security review accordingly. I'll be checking email periodically and will respond to any questions/feedback as time permits.

Comment 70

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #61) 
> Note: there will need to be a way to disable using SMJobBless similar to how
> we allow disabling the service.

Could you explain why this is necessary, and how this is similar to the service? My understanding is that disabling the service allows us to fall back to a different form of installation, which is handy if the service doesn't work as expected. On Mac however, if we don't use SMJobBless, the installation will simply be broken again in the same way it was before. In other words, if SMJobBless doesn't work as expected and we have to turn it off, the installation will still be broken for the affected users.

Comment 71

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #61)
> >+    if (NS_SUCCEEDED(appFile->GetParent(getter_AddRefs(appRootDirFile))) &&
> >+        NS_SUCCEEDED(appRootDirFile->GetPath(appDirPath))) {
> >+      int32_t dotIndex = appDirPath.RFind(".app");
> >+      if (dotIndex == kNotFound) {
> >+        dotIndex = appDirPath.Length();
> I think this is the most straightforward way to deal with not having a .app
> but I'd like to know if you think it would be overly complex to just
> fallback to the directory below?

We should always find ".app" in this RFind invocation. Setting dotIndex to appDirPath.Length() is just to be safe in the very unlikely event that we didn't find ".app". We could search for the last '/' when we couldn't find ".app", but we would probably still want to follow this up with the same dotIndex = appDirPath.Length() when we couldn't find '/'. I would probably prefer leaving this the way it is, but please let me know if you disagree.

Comment 72

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

(In reply to Stephen Pohl [:spohl] from comment #70)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #61) 
> > Note: there will need to be a way to disable using SMJobBless similar to how
> > we allow disabling the service.
> 
> Could you explain why this is necessary, and how this is similar to the
> service? My understanding is that disabling the service allows us to fall
> back to a different form of installation, which is handy if the service
> doesn't work as expected. On Mac however, if we don't use SMJobBless, the
> installation will simply be broken again in the same way it was before. In
> other words, if SMJobBless doesn't work as expected and we have to turn it
> off, the installation will still be broken for the affected users.
Let's discuss this on Friday.

Comment 75

[Dave Miller [:justdave]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #72)
> Let's discuss this on Friday.

Did Friday ever happen? ;)  Just got bit by this trying to walk people through picking up the NSS fix.

Comment 76

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

Work has commenced but Stephen keeps getting pulled off to work on other things... currently it is Mac v2 signing which we need for Firefox 34.

Comment 77

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

Comment on attachment 8431040 [details] [diff] [review]
Part 1: Main logic that allows for elevated updates on OSX.

Brian, could you give feedback on this approach? I suspect there will need to be additional code and UI to handle the failure case where the user doesn't have the rights to do this.

Comment 78

[Brian R. Bondy [:bbondy]]

Comment on attachment 8431040 [details] [diff] [review]
Part 1: Main logic that allows for elevated updates on OSX.

Review of attachment 8431040 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/updater/updater.cpp
@@ +3209,5 @@
>  
>    LogFinish();
>  
> +#ifdef XP_MACOSX
> +  } else { // if (!requiresElevation)

nit: The comment after this else was a little confusing to me, I read it as this else condition is for the case of !requiresElevation. But it's for the opposite.

@@ +3210,5 @@
>    LogFinish();
>  
> +#ifdef XP_MACOSX
> +  } else { // if (!requiresElevation)
> +    gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);

When ServeElevatedUpdate is called is there any UI presented to the user?
If not I think we should do a different error code other than ELEVATION_CANCELED.

If you do end up adding a new status code, I think we may be at a maximum of error codes right now for the telemetry probe range.
If that's the case you can just grab the code for UPDATER_ALL_STATUS_CODES from the patch in: https://bugzilla.mozilla.org/show_bug.cgi?id=945192 and just copy it to your patch before adding the new one.

Depending on if ServeElevatedUpdate shows UI or not, please also verify that what you want is to just set the status back to pending w/o UI:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1490
If ServeElevatedUpdate does show UI asking the user to install some kind of privileged helper, then just setting it back to pending sounds good. If not we need to add handling for the new error code and showing an error when Firefox restarts in nsUpdateService.js (showUpdateError).

It may also be helpful to return multiple different errors for different failure points from ServeElevatedUpdate.

::: toolkit/xre/MacLaunchHelper.mm
@@ +168,5 @@
> +bool LaunchElevatedUpdate(int argc, char** argv, uint32_t aRestartType,
> +                          pid_t* pid)
> +{
> +  LaunchChildMac(argc, argv, aRestartType, pid);
> +  bool didSucceed = InstallPrivilegedHelper();

I'm confused why we LaunchChildMac before we call InstallPrivilegedHelper, should it be the other way around?

::: toolkit/xre/nsUpdateDriver.cpp
@@ +912,5 @@
> +  if (restart && access(appFilePath.get(), W_OK) != 0) {
> +    if (!LaunchElevatedUpdate(argc, argv, 0, outpid)) {
> +      LOG(("Failed to launch elevated update!"));
> +      exit(1);
> +    }

I think we should report this case to telemetry so we can see how often InstallPrivilegedHelper fails.  We may also want to show some kind of error to the user in this case so they know something went wrong.

Comment 80

[Stephen A Pohl [:spohl]]

It's unlikely that I will get to work on this in the next two weeks, so unassigning myself for now. I will respond to the feedback in comment 78 once I get back to this.

Comment 82

[Benjamin Kerensa [:bkerensa]]

Are you able to work on this? We really could use a fix for this soon.

Comment 83

[Stephen A Pohl [:spohl]]

Let me start by responding to the feedback I've gotten so far:

(In reply to Brian R. Bondy [:bbondy] from comment #78)
> Comment on attachment 8431040 [details] [diff] [review]
> Part 1: Main logic that allows for elevated updates on OSX.
> 
> Review of attachment 8431040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +3209,5 @@
> >  
> >    LogFinish();
> >  
> > +#ifdef XP_MACOSX
> > +  } else { // if (!requiresElevation)
> 
> nit: The comment after this else was a little confusing to me, I read it as
> this else condition is for the case of !requiresElevation. But it's for the
> opposite.

Agreed. This should be removed in the next iteration of the patch.

> 
> @@ +3210,5 @@
> >    LogFinish();
> >  
> > +#ifdef XP_MACOSX
> > +  } else { // if (!requiresElevation)
> > +    gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
> 
> When ServeElevatedUpdate is called is there any UI presented to the user?

Yes, the OS will display an elevation dialog and the user has to enter the username and password of an admin user on the system.

> Depending on if ServeElevatedUpdate shows UI or not, please also verify that
> what you want is to just set the status back to pending w/o UI:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> nsUpdateService.js#1490
> If ServeElevatedUpdate does show UI asking the user to install some kind of
> privileged helper, then just setting it back to pending sounds good.

Great!

> It may also be helpful to return multiple different errors for different
> failure points from ServeElevatedUpdate.

I'll look into this when I get back to this bug.

> ::: toolkit/xre/MacLaunchHelper.mm
> @@ +168,5 @@
> > +bool LaunchElevatedUpdate(int argc, char** argv, uint32_t aRestartType,
> > +                          pid_t* pid)
> > +{
> > +  LaunchChildMac(argc, argv, aRestartType, pid);
> > +  bool didSucceed = InstallPrivilegedHelper();
> 
> I'm confused why we LaunchChildMac before we call InstallPrivilegedHelper,
> should it be the other way around?

The child will wait for the privileged helper to connect to it. If that was unsuccessful, the elevated update will be aborted.

> ::: toolkit/xre/nsUpdateDriver.cpp
> @@ +912,5 @@
> > +  if (restart && access(appFilePath.get(), W_OK) != 0) {
> > +    if (!LaunchElevatedUpdate(argc, argv, 0, outpid)) {
> > +      LOG(("Failed to launch elevated update!"));
> > +      exit(1);
> > +    }
> 
> I think we should report this case to telemetry so we can see how often
> InstallPrivilegedHelper fails.  We may also want to show some kind of error
> to the user in this case so they know something went wrong.

The patch doesn't currently show any additional UI (aside from the native elevation dialog). If we want to display a custom error here (which may make sense), this will need some additional work.

Comment 85

[Stephen A Pohl [:spohl]]

Comment on attachment 8431040 [details] [diff] [review]
Part 1: Main logic that allows for elevated updates on OSX.

Review of attachment 8431040 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/updater/Makefile.in
@@ +29,5 @@
>  endif
>  
> +ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> +MOZ_GLUE_PROGRAM_LDFLAGS += -sectcreate __TEXT __info_plist $(srcdir)/macbuild/Contents/Info.plist
> +MOZ_GLUE_PROGRAM_LDFLAGS += -sectcreate __TEXT __launchd_plist $(srcdir)/macbuild/Contents/Launchd.plist

glandium, I see that MOZ_GLUE_PROGRAM_LDFLAGS was removed with bug 1077148 comment 4, but I'm not 100% sure what to replace it with here. I'm trying to update this patch for current trunk. Are you able to give a recommendation? Thanks!

Comment 86

[Mike Hommey [:glandium]]

Note that MOZ_GLUE_PROGRAM_LDFLAGS was not the right thing to use originally.

Use LDFLAGS += ['-sectcreate', '__TEXT', '__info_plist', SRCDIR + '/macbuild/Contents/Info.plist']
in moz.build.

Comment 87

[Stephen A Pohl [:spohl]]

Comment on attachment 8389947 [details] [diff] [review]
Part 2: Signing of firefox and org.mozilla.updater binaries (WIP)

This patch is no longer necessary since we're already signing all the necessary bits as part of Mac v2 signing.

Comment 88

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch

Updated for current trunk (thanks, glandium!).

Comment 89

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch

Removed an unused variable and changed the executable name to 'org.mozilla.updater' in the updater.app's Info.plist file.

Comment 90

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch

This patch no longer packages Launchd.plist in the updater.app. This was never necessary and actually broke v2 OSX signing.

This patch successfully builds on oak:
https://treeherder.mozilla.org/#/jobs?repo=oak&revision=b49d0883c824

Comment 91

[Stephen A Pohl [:spohl]]

Attachment: 2. Certificate info

This will add the signing certificate information to the Info.plist files.

Ben, could you list the three Subject.OUs that I should be adding to the two Makefiles? I'm assuming the filter for the update channel is already the right one, since we already use it to assign PRIMARY_CERT and SECONDARY_CERT in the updater's Makefile. If not, I'd appreciate it if you could tell me what branches each Subject.OU applies to. Thanks!

Comment 92

[[e-mail] (:bhearsum)]

(In reply to Stephen A Pohl [:spohl] from comment #91)
> Created attachment 8645936 [details] [diff] [review]
> 2. Certificate info
> 
> This will add the signing certificate information to the Info.plist files.
> 
> Ben, could you list the three Subject.OUs that I should be adding to the two
> Makefiles? I'm assuming the filter for the update channel is already the
> right one, since we already use it to assign PRIMARY_CERT and SECONDARY_CERT
> in the updater's Makefile. If not, I'd appreciate it if you could tell me
> what branches each Subject.OU applies to. Thanks!

I'm assuming you need the subject OU of our certs (as opposed to the root it was generated by). If so, release and nightly are both "43AQ936H96", and dep is "Release Engineering". Here's the fun part, though: it's not constant on a single branch:
* CI builds on all branches (_including_ beta and release) use the dep cert
* Nightly builds on certain branches use the "nightly" cert. mozilla-central and aurora always do, but other branches are configured in https://github.com/mozilla/build-buildbot-configs/blob/master/mozilla/project_branches.py (anything using "nightly-signing" will use the nightly cert, otherwise they'll use dep). Currently, the following branches are using the nightly cert: elm, gum, maple, oak.
* Release builds on beta, release, and ESR use the release cert.

Your current conditionals probably work fine for now if you add the other nightly channels that currently use the nightly cert, but this will break as soon as one of those branches gets repurposed. If this is code that's running after signing happens, I strongly encourage you to extract the subject OU from the .app instead. You should be able to run the following to get it:
codesign -d --extract-certificates Whatever.app
openssl x509 -in codesign0 -inform DER -noout -subject | cut -d/ -f4 | cut -d= -f2

Comment 93

[Stephen A Pohl [:spohl]]

(In reply to Ben Hearsum (:bhearsum) from comment #92)
> Your current conditionals probably work fine for now if you add the other
> nightly channels that currently use the nightly cert, but this will break as
> soon as one of those branches gets repurposed. If this is code that's
> running after signing happens, I strongly encourage you to extract the
> subject OU from the .app instead. You should be able to run the following to
> get it:
> codesign -d --extract-certificates Whatever.app
> openssl x509 -in codesign0 -inform DER -noout -subject | cut -d/ -f4 | cut
> -d= -f2

Unfortunately, this won't work. The subject OU and the rest of the certificate info gets embedded in both the firefox and the org.mozilla.updater binary before signing.

Comment 94

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch

This removes a write permission check on the app directory that was added during the v2 signing work (bug 1064523). We're now assuming that all OSX users can update Firefox if the user elevates to update.

Comment 95

[Stephen A Pohl [:spohl]]

Attachment: 2. Certificate info

Based on comment 92, this may be all we need for the signing certificate info to be embedded into the Info.plist files.

Comment 96

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

(In reply to Stephen A Pohl [:spohl] from comment #94)
> Created attachment 8647289 [details] [diff] [review]
> 1. Patch
> 
> This removes a write permission check on the app directory that was added
> during the v2 signing work (bug 1064523). We're now assuming that all OSX
> users can update Firefox if the user elevates to update.
I am pretty certain we'll need a pref for this... when the pref is true it will bypass the write check and it will be set to false if there are repeated failure.

Comment 97

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #96)
> (In reply to Stephen A Pohl [:spohl] from comment #94)
> > Created attachment 8647289 [details] [diff] [review]
> > 1. Patch
> > 
> > This removes a write permission check on the app directory that was added
> > during the v2 signing work (bug 1064523). We're now assuming that all OSX
> > users can update Firefox if the user elevates to update.
> I am pretty certain we'll need a pref for this... when the pref is true it
> will bypass the write check and it will be set to false if there are
> repeated failure.

Yes, right now I'm just trying to get the behavior back that we had over a year ago to confirm that things are still working like they did back then and that nothing else has cropped up (due to new versions of OSX or similar). Once we're back to where we were back then, I'm planning to write a separate patch in this bug that will deal with things like:
1. What kind of UI is necessary to explain that elevation is needed to update (currently, we're only presenting the native OSX UI for elevation, with no explanation why elevation is required).
2. Provide a fallback for users who cannot elevate (rather than showing the elevation prompt at every restart of Firefox).
3. Provide a fallback for repeated failures of elevated updates despite proper credentials.

Once I've been able to confirm that this patch (finally) works again the way it did back then I'll be sure to circle back with you to discuss everything that's still needed before we can land the patches in this bug.

Comment 98

[Stephen A Pohl [:spohl]]

Working through a few issues in the original design. Clearing the sec-review flag for now.

Comment 99

[Stephen A Pohl [:spohl]]

I've put together a wiki page for a UX review:
https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201

Philipp, could you let me know if this is sufficient and who could perform this review? Thank you!

Comment 100

[Stephen A Pohl [:spohl]]

I've updated the wiki page at https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185 with the new design that I discussed with dveditz over IRC. Requesting formal sec-review.

1. Who is/are the point of contact(s) for this review?
Stephen A Pohl [:spohl]

2. Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
When one user installed Firefox, but another user tried to update Firefox on Mac OSX, the update used to fail. This was due to fact that the second user did not have write access to the application folder. By implementing elevated updates on OSX, the second user can now update Firefox. Future updates for administrative users will not require elevation. Standard users will continue to be able to install updates by elevating.

3. Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:
Architecture & detailed application diagram, data-flow enumeration and threat analysis can be found here: https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185
UX flow can be found here:
https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201

4. Does this request block another bug? If so, please indicate the bug number
This feature could be used to address bug 318855.

5. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?
This is a longstanding bug and it would be great to get this fixed soon.

6. To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?
Not currently listed, but may get listed for next quarter.

Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)
8. Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?
This feature affects any product that uses our updater on OSX.

9. Are there any portions of the project that interact with 3rd party services?
No.

10. Will your application/service collect user data? If so, please describe 
No additional user data is collected as part of this feature.

11. If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):
n/a

12. Desired Date of review (if known from https://mail.mozilla.com/home/[e-mail]/Security%20Review.html) and whom to invite. 
Date: No specific date desired.
People to invite: rstrong, bsmedberg (as optional)


(This was based on the template at https://wiki.mozilla.org/Security/Reviews/Review_Request_Form#Questions_to_Address_within_Request_Body)

Comment 101

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Updated for current trunk.

Comment 102

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Stephen A Pohl [:spohl] from comment #99)
> I've put together a wiki page for a UX review:
> https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201
> 
> Philipp, could you let me know if this is sufficient and who could perform
> this review? Thank you!

Looks good in general!
One nit: could we avoid using the term »elevation« in the UI? (last screen)
It's a very technical term that very few people will understand.
How about replacing the string »The update could not be installed (elevation failed)« with »You don't have the permissions necessary to install this update. Please contact your system administrator«. That would at least catch the handful of non-wizard users that could end up in this situation.

Comment 103

[Stephen A Pohl [:spohl]]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #102)
> (In reply to Stephen A Pohl [:spohl] from comment #99)
> > I've put together a wiki page for a UX review:
> > https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201
> > 
> > Philipp, could you let me know if this is sufficient and who could perform
> > this review? Thank you!
> 
> Looks good in general!
> One nit: could we avoid using the term »elevation« in the UI? (last screen)
> It's a very technical term that very few people will understand.
> How about replacing the string »The update could not be installed (elevation
> failed)« with »You don't have the permissions necessary to install this
> update. Please contact your system administrator«. That would at least catch
> the handful of non-wizard users that could end up in this situation.

Thank you for your feedback! I've gone ahead and changed the string in my local patch and updated the wiki.

Comment 104

[Stephen A Pohl [:spohl]]

Philipp and Robert, I've gone ahead and updated the UX wiki page with the behavior that is currently implemented. Could you have another look at it and let me know if anything needs to change?
https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201

The main differences are:
1. Instead of using a checkbox to decline the update, we're reusing the existing "No Thanks" button.
2. The button labels changed to what we've already been using in our UI, i.e. "Restart Later", "No Thanks" and "Restart Now".
3. Added a screenshot (and described the behavior) of the About dialog in case a user clicked "No Thanks" for a particular update version.
4. Updated screenshots with actual screenshots of the currently implemented solution instead of mockups.

I've kept the previous proposal at the bottom of the wiki page to make it easier to compare the previous and current proposal.

Thank you!

Comment 105

[Stephen A Pohl [:spohl]]

Dan, do you know if I need to request sec-review from someone specific? I left the space blank assuming that this would get triaged and assigned by the security team. Is there anything else I should do to get the security review for this bug on our schedule (see comment 100)? Thanks!

Comment 106

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c94a4a03ab8

Comment 107

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

(In reply to Stephen A Pohl [:spohl] from comment #104)
> Philipp and Robert, I've gone ahead and updated the UX wiki page with the
> behavior that is currently implemented. Could you have another look at it
> and let me know if anything needs to change?
> https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201
> 
> The main differences are:
> 1. Instead of using a checkbox to decline the update, we're reusing the
> existing "No Thanks" button.
> 2. The button labels changed to what we've already been using in our UI,
> i.e. "Restart Later", "No Thanks" and "Restart Now".
> 3. Added a screenshot (and described the behavior) of the About dialog in
> case a user clicked "No Thanks" for a particular update version.
> 4. Updated screenshots with actual screenshots of the currently implemented
> solution instead of mockups.
> 
> I've kept the previous proposal at the bottom of the wiki page to make it
> easier to compare the previous and current proposal.
> 
> Thank you!
I like this overall but I will defer to UX. I am hoping that after the failure the user has the option to try again as well like we do on Windows when the choose to not elevate.

Comment 108

[Stephen A Pohl [:spohl]]

Comment on attachment 8424096 [details]
Flow Diagram

The flow diagram is kept up-to-date here:
https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185

Comment 109

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Comment 110

[Stephen A Pohl [:spohl]]

Attachment: 2. Set permissions on .app bundle

Comment 111

[Stephen A Pohl [:spohl]]

Attachment: 3. Add signing certificate info to Info.plist files for Firefox and updater

Chris, would you be able to forward this review to someone during bhearsum's absence? Thank you!

Comment 112

[Stephen A Pohl [:spohl]]

Attachment: 4. UX

Waiting to request review until Philipp was able to weigh in.

Comment 113

[Stephen A Pohl [:spohl]]

Attachment: 5. Telemetry

Comment 114

[Stephen A Pohl [:spohl]]

fyi: I'm working on a patch to add new tests and update two existing ones. I will then run everything through additional testing on oak.

Comment 115

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d79ae3162af9

Comment 116

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Fixed some incorrect whitespace.

Comment 117

[Stephen A Pohl [:spohl]]

Attachment: 6. Update existing tests

This makes all the existing tests pass locally. Sent to try for confirmation (see comment 115).

Comment 118

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Stephen A Pohl [:spohl] from comment #104)
> Philipp and Robert, I've gone ahead and updated the UX wiki page with the
> behavior that is currently implemented. Could you have another look at it
> and let me know if anything needs to change?
> https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201
> 
> The main differences are:
> 1. Instead of using a checkbox to decline the update, we're reusing the
> existing "No Thanks" button.
> 2. The button labels changed to what we've already been using in our UI,
> i.e. "Restart Later", "No Thanks" and "Restart Now".
> 3. Added a screenshot (and described the behavior) of the About dialog in
> case a user clicked "No Thanks" for a particular update version.
> 4. Updated screenshots with actual screenshots of the currently implemented
> solution instead of mockups.
> 
> I've kept the previous proposal at the bottom of the wiki page to make it
> easier to compare the previous and current proposal.
> 
> Thank you!

Looks good!

Comment 119

[Stephen A Pohl [:spohl]]

Comment on attachment 8683244 [details] [diff] [review]
6. Update existing tests

This needs more work.

Comment 120

[Stephen A Pohl [:spohl]]

Comment on attachment 8682661 [details] [diff] [review]
4. UX

Setting to r? based on comment 118.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #107)
> I like this overall but I will defer to UX. I am hoping that after the
> failure the user has the option to try again as well like we do on Windows
> when the choose to not elevate.

The user will be prompted about the update again the next time Firefox is restarted. Does Windows have a button to try again in the failure dialog?

Comment 121

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a17f3cc190be

Comment 122

[Stephen A Pohl [:spohl]]

Attachment: 6. Update existing tests

Sent to try (comment 121).

Comment 123

[Stephen A Pohl [:spohl]]

Comment on attachment 8683709 [details] [diff] [review]
6. Update existing tests

Try is green.

Comment 124

[Chris AtLee [:catlee]]

Comment on attachment 8682657 [details] [diff] [review]
3. Add signing certificate info to Info.plist files for Firefox and updater

sorry, I don't know

Comment 125

[Stephen A Pohl [:spohl]]

Attachment: 7. Tests (wip)

Robert, I was hoping to get your feedback on these tests in progress.

I was going to add a chrome test to verify that the proper dialog is displayed when elevation is required. This requires that we create a locked update.test file. I've added a |lock| and |unlock| method to the nsILocalFileMac interface and added stubs to nsLocalFileUnix.cpp. However, I wanted to get your feedback on whether or not this is the right approach before implementing this. I've confirmed that the test succeeds if I manually create the locked update.test file in the application bundle before starting the test, so we just need to a way to create this file programmatically. I'm wondering if adding this to the nsILocalFileMac interface is the right thing to do (since this is only used for tests), or if there's a different way to achieve the same.

I was also going to add a chrome test to verify that the proper error dialog is displayed when elevation failed. I was hoping to achieve this by writing the elevation failure code to the update.status file and simulate a restart of Firefox to trigger the error dialog. However, despite my best efforts I could not figure out how to simulate this restart. Is there a way to achieve this?

I haven't been able to come up with meaningful xpcshell tests. Would you like to see something tested by xpcshell tests, or are there any other chrome tests that I should add? Thanks!

Comment 127

[[e-mail] (:bhearsum)]

Comment on attachment 8682657 [details] [diff] [review]
3. Add signing certificate info to Info.plist files for Firefox and updater

The plist parks look fine to me, but I'll defer to mshal for the Makefile part.

Comment 128

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

Comment on attachment 8682653 [details] [diff] [review]
2. Set permissions on .app bundle

I'm going to ask mstange to review this since he is more familiar with Objective-C. I'd like to briefly discuss this with you next week just to verify a couple of things.

Comment 129

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

Comment on attachment 8682653 [details] [diff] [review]
2. Set permissions on .app bundle

spohl, can you refresh my memory as to what the end result permissions we decided upon here and why?

Comment 130

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #129)
> Comment on attachment 8682653 [details] [diff] [review]
> 2. Set permissions on .app bundle
> 
> spohl, can you refresh my memory as to what the end result permissions we
> decided upon here and why?

The group ownership should be set to "admin" (80) and we want to ensure write permission for the group. For Firefox.app, this means permissions of 775. For every file within the .app bundle we do a bitwise OR 0x10 on the existing permissions to ensure write permission for the group.

The only concern here is that new files installed during a future, un-elevated update would get ownership of user:staff instead of user:admin and wouldn't have write permission for the group. These files would cause future updates by a different user to fail again. One solution to this problem is what I suggested in bug 1226586 comment 8:

> We could (and probably should) improve our permissions check, as this would
> also come handy when we're trying to detect when an update requires
> elevation in bug 394984. Robert, what are your thoughts on improving the
> "canApply" check to recursively check the permissions on the files in the
> bundle instead of creating the update.test file in the root of the bundle?

I'm happy to file a new bug and write a patch to improve the "canApply" check, if that's what we want.

Comment 131

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

(In reply to Stephen A Pohl [:spohl] from comment #130)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #129)
> > Comment on attachment 8682653 [details] [diff] [review]
> > 2. Set permissions on .app bundle
> > 
> > spohl, can you refresh my memory as to what the end result permissions we
> > decided upon here and why?
> 
> The group ownership should be set to "admin" (80) and we want to ensure
> write permission for the group. For Firefox.app, this means permissions of
> 775. For every file within the .app bundle we do a bitwise OR 0x10 on the
> existing permissions to ensure write permission for the group.
> 
> The only concern here is that new files installed during a future,
> un-elevated update would get ownership of user:staff instead of user:admin
> and wouldn't have write permission for the group. These files would cause
> future updates by a different user to fail again. One solution to this
> problem is what I suggested in bug 1226586 comment 8:
I recall discussing with you whether or not the user that currently owns the files or the user currently performing the elevated update should get ownership in this case as well. This appears to do neither... is that the case?

> > We could (and probably should) improve our permissions check, as this would
> > also come handy when we're trying to detect when an update requires
> > elevation in bug 394984. Robert, what are your thoughts on improving the
> > "canApply" check to recursively check the permissions on the files in the
> > bundle instead of creating the update.test file in the root of the bundle?
> 
> I'm happy to file a new bug and write a patch to improve the "canApply"
> check, if that's what we want.
I already added a comment to this affect in my review notes and think it should be done as part of this bug.

Comment 132

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #131)
> (In reply to Stephen A Pohl [:spohl] from comment #130)
> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> > comment #129)
> > > Comment on attachment 8682653 [details] [diff] [review]
> > > 2. Set permissions on .app bundle
> > > 
> > > spohl, can you refresh my memory as to what the end result permissions we
> > > decided upon here and why?
> > 
> > The group ownership should be set to "admin" (80) and we want to ensure
> > write permission for the group. For Firefox.app, this means permissions of
> > 775. For every file within the .app bundle we do a bitwise OR 0x10 on the
> > existing permissions to ensure write permission for the group.
> > 
> > The only concern here is that new files installed during a future,
> > un-elevated update would get ownership of user:staff instead of user:admin
> > and wouldn't have write permission for the group. These files would cause
> > future updates by a different user to fail again. One solution to this
> > problem is what I suggested in bug 1226586 comment 8:
> I recall discussing with you whether or not the user that currently owns the
> files or the user currently performing the elevated update should get
> ownership in this case as well. This appears to do neither... is that the
> case?
For clarity, I vaguely recall us discussing this and I'm asking for the rationale that we came up with to go this direction or if the plan was to go a different direction.

Comment 133

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #132)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #131)
> > I recall discussing with you whether or not the user that currently owns the
> > files or the user currently performing the elevated update should get
> > ownership in this case as well. This appears to do neither... is that the
> > case?
> For clarity, I vaguely recall us discussing this and I'm asking for the
> rationale that we came up with to go this direction or if the plan was to go
> a different direction.

One proposal was to change ownership to the new user, but we agreed to not do this. If I recall correctly, you had concerns that user ownership might be set deliberately in some places. The original patches in this bug left user and group ownerships untouched. However, during an irc conversation with :dveditz I realized that we would gain a significant advantage by changing the group ownership as proposed here. The advantage is that any subsequent updates by any admin user would no longer require elevation.

FYI: Since the update is an elevated update, new files will actually get user ownership of root. Since we're setting group ownership to "admin", I couldn't think of a reason why this would be a problem. Existing files will keep the current user ownership. As mentioned previously, the only scenario that I could think of where user ownership could become a problem is when:
1. Admin-user-A installs Firefox
2. Admin-user-B performs an elevated update
3. Admin-user-B performs a subsequent un-elevated update that installs new files
4. Admin-user-A or Admin-user-C tries to perform an update

The new files installed in step 3 have ownership of Admin-user-B:staff, so step 4 could fail because we're currently unable to detect that we're missing write permission to these new files and will attempt an un-elevated update, which will fail. Based on your comment 131 it seems like we agree that we should improve the "canApply" check, which will take care of this. I'll post an additional patch that will address this.


(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #129)
> Comment on attachment 8682653 [details] [diff] [review]
> 2. Set permissions on .app bundle
> 
> spohl, can you refresh my memory as to what the end result permissions we
> decided upon here and why?

I actually forgot to answer the "why": Having group ownership of "admin" and write permission for the group allows admin users to update in the future without having to elevate. Non-admin users can update as well, but they will need to elevate every time since they don't have write permission. I've put together a mana page[1] with a focus on the security aspects.

[1] https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185

Comment 134

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

(In reply to Stephen A Pohl [:spohl] from comment #133)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #132)
> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> > comment #131)
> > > I recall discussing with you whether or not the user that currently owns the
> > > files or the user currently performing the elevated update should get
> > > ownership in this case as well. This appears to do neither... is that the
> > > case?
> > For clarity, I vaguely recall us discussing this and I'm asking for the
> > rationale that we came up with to go this direction or if the plan was to go
> > a different direction.
> 
> One proposal was to change ownership to the new user, but we agreed to not
> do this. If I recall correctly, you had concerns that user ownership might
> be set deliberately in some places. The original patches in this bug left
> user and group ownerships untouched. However, during an irc conversation
> with :dveditz I realized that we would gain a significant advantage by
> changing the group ownership as proposed here. The advantage is that any
> subsequent updates by any admin user would no longer require elevation.
> 
> FYI: Since the update is an elevated update, new files will actually get
> user ownership of root. Since we're setting group ownership to "admin", I
> couldn't think of a reason why this would be a problem. Existing files will
> keep the current user ownership. As mentioned previously, the only scenario
> that I could think of where user ownership could become a problem is when:
> 1. Admin-user-A installs Firefox
> 2. Admin-user-B performs an elevated update
> 3. Admin-user-B performs a subsequent un-elevated update that installs new
> files
> 4. Admin-user-A or Admin-user-C tries to perform an update
> 
> The new files installed in step 3 have ownership of Admin-user-B:staff, so
> step 4 could fail because we're currently unable to detect that we're
> missing write permission to these new files and will attempt an un-elevated
> update, which will fail.
It seems like we can end up with different permissions depending on the flow which I really don't like. If it is acceptable per the security team to change permissions in this manner so all Admin users can update is it possible to also set the permissions on new files when performing an unelevated update?

Comment 135

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #134)
> It seems like we can end up with different permissions depending on the flow
> which I really don't like. If it is acceptable per the security team to
> change permissions in this manner so all Admin users can update is it
> possible to also set the permissions on new files when performing an
> unelevated update?

The solution here might be to set the same group ownership and write permission on the new files as the enclosing folder. If a user never ran an elevated update this would be the same as today (user:staff). If an elevated update occurred, we'd set group ownership to "admin" and add write permission for the group. I would need to confirm that this is indeed possible during an unelevated update (I think it is).

Comment 136

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

It would be a "good thing" to re-verify with dveditz that it is ok to change the permissions so all admin users can write to this directory without being prompted especially since an admin user process would be able to change the files in the directory.

Comment 137

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

Comment on attachment 8683239 [details] [diff] [review]
1. Patch to elevate updater

In case you are updating the patch already I am submitting partial review comments now so you can update those as well.

>diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
>--- a/browser/app/Makefile.in
>+++ b/browser/app/Makefile.in
>@@ -98,10 +98,13 @@ tools repackage:: $(PROGRAM)
> 	sed -e 's/%APP_VERSION%/$(MOZ_APP_VERSION)/' -e 's/%MAC_APP_NAME%/$(MAC_APP_NAME)/' -e 's/%MOZ_MACBUNDLE_ID%/$(MOZ_MACBUNDLE_ID)/' -e 's/%MAC_BUNDLE_VERSION%/$(MAC_BUNDLE_VERSION)/' $(srcdir)/macbuild/Contents/Info.plist.in > $(dist_dest)/Contents/Info.plist
> 	sed -e 's/%MAC_APP_NAME%/$(MAC_APP_NAME)/' $(srcdir)/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | iconv -f UTF-8 -t UTF-16 > $(dist_dest)/$(LPROJ)/InfoPlist.strings
> 	rsync -a --exclude-from='$(srcdir)/macbuild/Contents/MacOS-files.in' $(DIST)/bin/ $(dist_dest)/Contents/Resources
> 	rsync -a --include-from='$(srcdir)/macbuild/Contents/MacOS-files.in' --exclude '*' $(DIST)/bin/ $(dist_dest)/Contents/MacOS
> 	$(RM) $(dist_dest)/Contents/MacOS/$(PROGRAM)
> 	rsync -aL $(PROGRAM) $(dist_dest)/Contents/MacOS
> 	cp -RL $(DIST)/branding/firefox.icns $(dist_dest)/Contents/Resources/firefox.icns
> 	cp -RL $(DIST)/branding/document.icns $(dist_dest)/Contents/Resources/document.icns
>+	$(MKDIR) -p $(dist_dest)/Contents/Library/LaunchServices
>+	mv -f $(dist_dest)/Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater $(dist_dest)/Contents/Library/LaunchServices
>+	ln -s ../../../../Library/LaunchServices/org.mozilla.updater $(dist_dest)/Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater
> 	printf APPLMOZB > $(dist_dest)/Contents/PkgInfo
> endif
Please move the Makefile.in changes from [patch] 6. Update existing tests to this patch or another patch with just build changes. Also, build changes will need to be reviewed by a build peer.


>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -4194,17 +4194,17 @@ cairo-qt)
>     ;;
> 
> cairo-cocoa)
>     MOZ_WIDGET_TOOLKIT=cocoa
>     AC_DEFINE(MOZ_WIDGET_COCOA)
>     LDFLAGS="$LDFLAGS -framework Cocoa -lobjc"
>     # Use -Wl as a trick to avoid -framework and framework names from
>     # being separated by AC_SUBST_LIST.
>-    TK_LIBS='-Wl,-framework,CoreLocation -Wl,-framework,QuartzCore -Wl,-framework,Carbon -Wl,-framework,CoreAudio -Wl,-framework,AudioToolbox -Wl,-framework,AudioUnit -Wl,-framework,AddressBook -Wl,-framework,OpenGL'
>+    TK_LIBS='-Wl,-framework,CoreLocation -Wl,-framework,QuartzCore -Wl,-framework,Carbon -Wl,-framework,CoreAudio -Wl,-framework,AudioToolbox -Wl,-framework,AudioUnit -Wl,-framework,AddressBook -Wl,-framework,OpenGL -Wl,-framework,Security -Wl,-framework,ServiceManagement'
Please get a build peer to review this.


>diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl
>--- a/toolkit/mozapps/update/nsIUpdateService.idl
>+++ b/toolkit/mozapps/update/nsIUpdateService.idl
>@@ -221,41 +221,48 @@ interface nsIUpdate : nsISupports
> 
>   /**
>    * The currently selected patch for this update.
>    */
>   readonly attribute nsIUpdatePatch selectedPatch;
> 
>   /**
>    * The state of the selected patch:
>-   *   "downloading"        The update is being downloaded.
>-   *   "pending"            The update is ready to be applied.
>-   *   "pending-service"    The update is ready to be applied with the service.
>-   *   "applying"           The update is being applied.
>-   *   "applied"            The update is ready to be switched to.
>-   *   "applied-os"         The update is OS update and to be installed.
>-   *   "applied-service"    The update is ready to be switched to with the service.
>-   *   "succeeded"          The update was successfully applied.
>-   *   "download-failed"    The update failed to be downloaded.
>-   *   "failed"             The update failed to be applied.
>+   *   "downloading"           The update is being downloaded.
>+   *   "pending"               The update is ready to be applied.
>+   *   "pending-service"       The update is ready to be applied with the service.
>+   *   "pending-elevation-req" The update is ready to be applied but requires elevation.
>+   *   "applying"              The update is being applied.
>+   *   "applied"               The update is ready to be switched to.
>+   *   "applied-os"            The update is OS update and to be installed.
>+   *   "applied-service"       The update is ready to be switched to with the service.
>+   *   "applied-elevation-req" The update is ready to be switched to but requires elevation.
>+   *   "succeeded"             The update was successfully applied.
>+   *   "download-failed"       The update failed to be downloaded.
>+   *   "failed"                The update failed to be applied.
>    */
How about just pending-elevate and applied-elevate? If you're ok with that, here and elsewhere.

>   attribute AString state;
> 
>   /**
>    * A numeric error code that conveys additional information about the state
>    * of a failed update or failed certificate attribute check during an update
>    * check. If the update is not in the "failed" state or the certificate
>    * attribute check has not failed the value is zero.
>    *
>    * TODO: Define typical error codes (for now, see updater/errors.h and the
>    *       CERT_ATTR_CHECK_FAILED_* values in nsUpdateService.js)
>    */
>   attribute long errorCode;
> 
>   /**
>+   * Whether an elevation failure has been encountered for this update.
>+   */
>+  attribute boolean elevationFailure;
Bump the uuid.

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -25,16 +25,18 @@ const PREF_APP_UPDATE_BACKGROUND_INTERVA
> const PREF_APP_UPDATE_BACKGROUNDERRORS    = "app.update.backgroundErrors";
> const PREF_APP_UPDATE_BACKGROUNDMAXERRORS = "app.update.backgroundMaxErrors";
> const PREF_APP_UPDATE_CANCELATIONS        = "app.update.cancelations";
> const PREF_APP_UPDATE_CERTS_BRANCH        = "app.update.certs.";
> const PREF_APP_UPDATE_CERT_CHECKATTRS     = "app.update.cert.checkAttributes";
> const PREF_APP_UPDATE_CERT_ERRORS         = "app.update.cert.errors";
> const PREF_APP_UPDATE_CERT_MAXERRORS      = "app.update.cert.maxErrors";
> const PREF_APP_UPDATE_CERT_REQUIREBUILTIN = "app.update.cert.requireBuiltIn";
>+const PREF_APP_UPDATE_ELEVATION_NEVER     = "app.update.elevation.never";
>+const PREF_APP_UPDATE_ELEVATION_VERSION   = "app.update.elevation.version";
s/elevation/elevate/

>@@ -76,27 +78,29 @@ const FILE_UPDATE_ARCHIVE = "update.mar"
> const FILE_UPDATE_LINK    = "update.link";
> const FILE_UPDATE_LOG     = "update.log";
> const FILE_UPDATES_DB     = "updates.xml";
> const FILE_UPDATE_ACTIVE  = "active-update.xml";
> const FILE_PERMS_TEST     = "update.test";
> const FILE_LAST_LOG       = "last-update.log";
> const FILE_BACKUP_LOG     = "backup-update.log";
> 
>-const STATE_NONE            = "null";
>-const STATE_DOWNLOADING     = "downloading";
>-const STATE_PENDING         = "pending";
>-const STATE_PENDING_SVC     = "pending-service";
>-const STATE_APPLYING        = "applying";
>-const STATE_APPLIED         = "applied";
>-const STATE_APPLIED_OS      = "applied-os";
>-const STATE_APPLIED_SVC     = "applied-service";
>-const STATE_SUCCEEDED       = "succeeded";
>-const STATE_DOWNLOAD_FAILED = "download-failed";
>-const STATE_FAILED          = "failed";
>+const STATE_NONE                  = "null";
>+const STATE_DOWNLOADING           = "downloading";
>+const STATE_PENDING               = "pending";
>+const STATE_PENDING_SVC           = "pending-service";
>+const STATE_PENDING_ELEVATION_REQ = "pending-elevation-req";
>+const STATE_APPLYING              = "applying";
>+const STATE_APPLIED               = "applied";
>+const STATE_APPLIED_OS            = "applied-os";
>+const STATE_APPLIED_SVC           = "applied-service";
>+const STATE_APPLIED_ELEVATION_REQ = "applied-elevation-req";
If you're ok with it, STATE_APPLIED_ELEVATE and STATE_PENDING_ELEVATE

Might as well change STATE_PENDING_SVC and STATE_APPLIED_SVC to STATE_PENDING_SERVICE and STATE_APPLIED_SERVICE

>@@ -183,16 +187,19 @@ const DEFAULT_SERVICE_MAX_ERRORS = 10;
> 
> // The number of consecutive socket errors to allow before falling back to
> // downloading a different MAR file or failing if already downloading the full.
> const DEFAULT_SOCKET_MAX_ERRORS = 10;
> 
> // The number of milliseconds to wait before retrying a connection error.
> const DEFAULT_UPDATE_RETRY_TIMEOUT = 2000;
> 
>+// Maximum number of elevation cancelations per update version before giving up.
>+const DEFAULT_MAX_ELEVATION_CANCELATIONS = 5;
Should applications and users be able to customize this with a pref as well?

>@@ -332,56 +339,78 @@ function hasUpdateMutex() {
>     return true;
>   }
>   if (!gUpdateMutexHandle) {
>     gUpdateMutexHandle = createMutex(getPerInstallationMutexName(true), false);
>   }
>   return !!gUpdateMutexHandle;
> }
> 
>+/**
>+ * OSX only function to determine if the user requires elevation to be able to
>+ * write to the application bundle.
>+ *
>+ * @return true if elevation is required, false otherwise
>+ */
>+function getElevationRequired() {
>+  if (AppConstants.platform != "macosx") {
>+    return false;
>+  }
>+
>+  try {
>+    // Check that the application bundle can be written to.
>+    let appDirTestFile = getAppBaseDir();
>+    appDirTestFile.append(FILE_PERMS_TEST);
>+    LOG("getElevationRequired - testing write access " + appDirTestFile.path);
>+    if (appDirTestFile.exists()) {
>+      appDirTestFile.remove(false);
>+    }
>+    appDirTestFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE,
>+                          FileUtils.PERMS_FILE);
>+    appDirTestFile.remove(false);
I'm concerned that there will be cases where the user can write to the directory but can't modify / delete the files in the directory. Can you come up with a a check for that as well?

>+  } catch (ex) {
>+    LOG("getElevationRequired - unable to write to application bundle, " +
>+        "elevation required");
>+    return true;
>+  }
>+  LOG("getElevationRequired - able to write to application bundle, elevation " +
>+      "not required");
>+  return false;
>+}
>+

>@@ -444,18 +473,23 @@ function getCanApplyUpdates() {
>  * Whether or not the application can stage an update for the current session.
>  * These checks are only performed once per session due to using a lazy getter.
>  *
>  * @return true if updates can be staged for this session.
>  */
> XPCOMUtils.defineLazyGetter(this, "gCanStageUpdatesSession",
>                             function aus_gCanStageUpdatesSession() {
>   try {
>-    let updateTestFile = getInstallDirRoot();
>-    updateTestFile.append(FILE_PERMS_TEST);
>+    let updateTestFile;
>+    if (AppConstants.platform == "macosx") {
>+      updateTestFile = getUpdateFile([FILE_PERMS_TEST]);
Note for myself to check that this is correct.

>+    } else {
>+      updateTestFile = getInstallDirRoot();
>+      updateTestFile.append(FILE_PERMS_TEST);
>+    }
>     LOG("gCanStageUpdatesSession - testing write access " +
>         updateTestFile.path);
>     testWriteAccess(updateTestFile, true);
>     if (AppConstants.platform != "macosx") {
>       // On all platforms except Mac, we need to test the parent directory as
>       // well, as we need to be able to move files in that directory during the
>       // replacing step.
>       updateTestFile = getInstallDirRoot().parent;

>diff --git a/toolkit/mozapps/update/updater/launchchild_osx.mm b/toolkit/mozapps/update/updater/launchchild_osx.mm
>--- a/toolkit/mozapps/update/updater/launchchild_osx.mm
>+++ b/toolkit/mozapps/update/updater/launchchild_osx.mm
Could you get mstange to review these bits?

>diff --git a/toolkit/xre/MacLaunchHelper.mm b/toolkit/xre/MacLaunchHelper.mm
>--- a/toolkit/xre/MacLaunchHelper.mm
>+++ b/toolkit/xre/MacLaunchHelper.mm
Could you get mstange to review these bits?

Comment 138

[Markus Stange [:mstange]]

Comment on attachment 8682653 [details] [diff] [review]
2. Set permissions on .app bundle

Review of attachment 8682653 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/updater/launchchild_osx.mm
@@ +269,5 @@
>  }
> +
> +void SetOwnershipAndPermissions(const char* aAppBundle)
> +{
> +  NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];

Is this file a place where you can include toolkit/xre/MacAutoreleasePool.h? That would make the early returns a little simpler.

@@ +304,5 @@
> +  // For all descendants of Firefox.app, set group ownership to 80 ("admin") and
> +  // add write permission for the group.
> +  NSEnumerator* pathEnumerator = [paths objectEnumerator];
> +  NSString* currPath;
> +  while((currPath = (NSString*)[pathEnumerator nextObject])) {

You should be able to use "fast enumeration" here (for (NSString* currPath in paths)).

@@ +331,5 @@
> +    }
> +  }
> +
> +  [pool drain];
> +  return;

This return is kinda useless.

Comment 139

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #136)
> It would be a "good thing" to re-verify with dveditz that it is ok to change
> the permissions so all admin users can write to this directory without being
> prompted especially since an admin user process would be able to change the
> files in the directory.

I just spoke with dveditz at Mozlando and this is still ok. With regards to new files being installed after an elevated update, the suggested solution is to check a path that's always present and set new files to group ownership "admin" with write permission if the path has the same. The top level .app directory seems like a good path to check. The next step is to verify that an unelevated update can indeed set group ownership of new files to "admin" with write permission. I'll look into that next.

dveditz also confirmed that once reviewed, we can go ahead with the landing of the patches, test them and iterate on them when necessary.

Comment 140

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #137)
> Comment on attachment 8683239 [details] [diff] [review]
> 1. Patch to elevate updater
> 
>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> >--- a/toolkit/mozapps/update/nsUpdateService.js
> >+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -2446,18 +2496,68 @@ UpdateService.prototype = {
>         default:
>           LOG("UpdateService:selectUpdate - skipping unknown update type: " +
>               aUpdate.type);
>           lastCheckCode = AUSTLMY.CHK_UPDATE_INVALID_TYPE;
>           break;
>       }
>     });
> 
>-    var update = minorUpdate || majorUpdate;
>-    if (!update) {
>+    let update = minorUpdate || majorUpdate;
>+    if (update) {
>+      if (getElevationRequired()) {
>+        let installAttemptVersion = getPref("getCharPref",
>+                                            PREF_APP_UPDATE_ELEVATION_VERSION,
>+                                            null);
>+        if (vc.compare(installAttemptVersion, update.appVersion) != 0) {
>+          Services.prefs.setCharPref(PREF_APP_UPDATE_ELEVATION_VERSION,
>+                                     update.appVersion);
>+          if (Services.prefs.prefHasUserValue(PREF_APP_UPDATE_CANCELATIONS)) {
>+            Services.prefs.clearUserPref(PREF_APP_UPDATE_CANCELATIONS);
>+          }
>+          if (Services.prefs.prefHasUserValue(
>+                PREF_APP_UPDATE_ELEVATION_NEVER)) {
>+            Services.prefs.clearUserPref(PREF_APP_UPDATE_ELEVATION_NEVER);
>+          }
>+        } else {
>+          let numCancels = getPref("getIntPref", PREF_APP_UPDATE_CANCELATIONS,
>+                                   0);
>+          let rejectedVersion = getPref("getCharPref",
>+                                        PREF_APP_UPDATE_ELEVATION_NEVER, "");
>+          if (numCancels >= DEFAULT_MAX_ELEVATION_CANCELATIONS) {
>+            LOG("UpdateService:selectUpdate - the user requires elevation to " +
>+                "install this update, but the user has exceeded the max number of " +
>+                "elevation attempts.");
>+            update.elevationFailure = true;
>+          } else if (vc.compare(rejectedVersion, update.appVersion) == 0) {
>+            LOG("UpdateService:selectUpdate - the user requires elevation to " +
>+                "install this update, but elevation is disabled for this " +
>+                "version.");
>+            update.elevationFailure = true;
>+          } else {
>+            LOG("UpdateService:selectUpdate - the user requires elevation to " +
>+                "install the update.");
>+          }
>+        }
>+      } else {
>+        // Clear elevation-related prefs since they no longer apply (the user
>+        // may have gained write access to the Firefox directory or an update
>+        // was executed with a different profile).
>+        if (Services.prefs.prefHasUserValue(
>+              PREF_APP_UPDATE_ELEVATION_VERSION)) {
nit: skip the wrapping

>+          Services.prefs.clearUserPref(PREF_APP_UPDATE_ELEVATION_VERSION);
>+        }
>+        if (Services.prefs.prefHasUserValue(PREF_APP_UPDATE_CANCELATIONS)) {
>+          Services.prefs.clearUserPref(PREF_APP_UPDATE_CANCELATIONS);
>+        }
The PREF_APP_UPDATE_CANCELATIONS is a count of consecutive update cancellations and unless I'm mistaken clearing it here above will clear it every time an update is found even when the previous update was cancelled.


(In reply to Stephen A Pohl [:spohl] from comment #139)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #136)
> > It would be a "good thing" to re-verify with dveditz that it is ok to change
> > the permissions so all admin users can write to this directory without being
> > prompted especially since an admin user process would be able to change the
> > files in the directory.
> 
> I just spoke with dveditz at Mozlando and this is still ok. With regards to
> new files being installed after an elevated update, the suggested solution
> is to check a path that's always present and set new files to group
> ownership "admin" with write permission if the path has the same. The top
> level .app directory seems like a good path to check. The next step is to
> verify that an unelevated update can indeed set group ownership of new files
> to "admin" with write permission. I'll look into that next.
> 
> dveditz also confirmed that once reviewed, we can go ahead with the landing
> of the patches, test them and iterate on them when necessary.
Good to hear. The only other comment I have is how this can change administratively set permissions. I don't think that is that big of a deal but it should be documented and the enterprise group should be notified of this new behavior.

Comment 141

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

Comment on attachment 8683239 [details] [diff] [review]
1. Patch to elevate updater

Is the update-canceled observer because you are requesting elevation while Firefox is running? Let's discuss this over vidyo next week... please ping me.

Comment 142

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #141)
> Comment on attachment 8683239 [details] [diff] [review]
> 1. Patch to elevate updater
> 
> Is the update-canceled observer because you are requesting elevation while
> Firefox is running? Let's discuss this over vidyo next week... please ping
> me.

The second part to this is in patch "4. UX". When a user chooses not to install an update that requires elevation (by clicking the "No Thanks" button in the update prompt), we want to clean up the update, hide the arrow "badge" on green background on the hamburger menu and we want to reset the About dialog to not display the "Restart to install" button. Patch 1 here observes "update-canceled" to clean up the staged or applied update asynchronously.

Unfortunately, I couldn't come up with a particularly good way to split this logic between the two patches.

Comment 143

[Michael Shal [:mshal]]

Comment on attachment 8682657 [details] [diff] [review]
3. Add signing certificate info to Info.plist files for Firefox and updater

>+export::
>+	sed -e 's/%MOZ_MACBUNDLE_ID%/$(MOZ_MACBUNDLE_ID)/' $(srcdir)/macbuild/Contents/Info.plist.in > $(srcdir)/macbuild/Contents/Info.plist

We shouldn't generate files in the srcdir. Is there a reason this needs to be in a separate step under export rather than in the libs commands below?

> libs::
> 	$(NSINSTALL) -D $(DIST)/bin/updater.app
> 	rsync -a -C --exclude '*.in' $(srcdir)/macbuild/Contents $(DIST)/bin/updater.app 
> 	sed -e 's/%APP_NAME%/$(MOZ_APP_DISPLAYNAME)/' $(srcdir)/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \
> 	  iconv -f UTF-8 -t UTF-16 > $(DIST)/bin/updater.app/Contents/Resources/English.lproj/InfoPlist.strings

I think you could add the sed command for Info.plist.in here (similar to the sed command above) and write Info.plist into $(DIST)/bin/updater.app/...

> 	$(NSINSTALL) -D $(DIST)/bin/updater.app/Contents/MacOS
> 	$(NSINSTALL) $(DIST)/bin/org.mozilla.updater $(DIST)/bin/updater.app/Contents/MacOS
> 	rm -f $(DIST)/bin/org.mozilla.updater
>+	rm -f $(srcdir)/macbuild/Contents/Info.plist

If Info.plist is in $(DIST)/bin, I don't think you'd need to explicitly remove it afterward.

Comment 144

[Stephen A Pohl [:spohl]]

(In reply to Michael Shal [:mshal] from comment #143)
> Comment on attachment 8682657 [details] [diff] [review]
> 3. Add signing certificate info to Info.plist files for Firefox and updater
> 
> >+export::
> >+	sed -e 's/%MOZ_MACBUNDLE_ID%/$(MOZ_MACBUNDLE_ID)/' $(srcdir)/macbuild/Contents/Info.plist.in > $(srcdir)/macbuild/Contents/Info.plist
> 
> We shouldn't generate files in the srcdir. Is there a reason this needs to
> be in a separate step under export rather than in the libs commands below?

Right, I was reluctant to generate this file in the srcdir as well. However, I couldn't come up with a better way. We need to generate this file with the proper MOZ_MACBUNDLE_ID prior to compilation because it gets compiled into the updater binary via the -sectcreate LDFLAG (see toolkit/mozapps/update/updater/moz.build in attachment 8683239 [details] [diff] [review]). export:: appears to execute prior to compilation while libs:: is executed after. I tried to generate it and place it into dstdir, but that does not seem to exist yet (or I couldn't find the proper way to reference it from toolkit/mozapps/update/updater/moz.build). I should have added you as reviewer of attachment 8683239 [details] [diff] [review] as well. Doing so now.

Comment 145

[Michael Shal [:mshal]]

Comment on attachment 8683239 [details] [diff] [review]
1. Patch to elevate updater

>diff --git a/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist b/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist
>--- a/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist
>+++ b/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist
>@@ -1,16 +1,16 @@
> <?xml version="1.0" encoding="UTF-8"?>
> <!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
> <plist version="1.0">
> <dict>
> 	<key>CFBundleDevelopmentRegion</key>
> 	<string>English</string>
> 	<key>CFBundleExecutable</key>
>-	<string>updater</string>
>+	<string>org.mozilla.updater</string>
> 	<key>CFBundleIconFile</key>
> 	<string>updater.icns</string>
> 	<key>CFBundleIdentifier</key>
> 	<string>org.mozilla.updater</string>
> 	<key>CFBundleInfoDictionaryVersion</key>
> 	<string>6.0</string>
> 	<key>CFBundlePackageType</key>
> 	<string>APPL</string>
>diff --git a/toolkit/mozapps/update/updater/moz.build b/toolkit/mozapps/update/updater/moz.build
>--- a/toolkit/mozapps/update/updater/moz.build
>+++ b/toolkit/mozapps/update/updater/moz.build
>@@ -1,14 +1,27 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
>-Program('updater')
>+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
>+    Program('org.mozilla.updater')
>+else:
>+    Program('updater')

Why do we need to change 'updater' to 'org.mozilla.updater' for this patch? Is this something we should be doing on all platforms?

The patch looks fine to me otherwise.

Comment 146

[Stephen A Pohl [:spohl]]

Thanks for your feedback!

(In reply to Michael Shal [:mshal] from comment #145)
> Comment on attachment 8683239 [details] [diff] [review]
> 1. Patch to elevate updater
> 
> >diff --git a/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist b/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist
> >--- a/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist
> >+++ b/toolkit/mozapps/update/updater/macbuild/Contents/Info.plist
> >@@ -1,16 +1,16 @@
> > <?xml version="1.0" encoding="UTF-8"?>
> > <!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
> > <plist version="1.0">
> > <dict>
> > 	<key>CFBundleDevelopmentRegion</key>
> > 	<string>English</string>
> > 	<key>CFBundleExecutable</key>
> >-	<string>updater</string>
> >+	<string>org.mozilla.updater</string>
> > 	<key>CFBundleIconFile</key>
> > 	<string>updater.icns</string>
> > 	<key>CFBundleIdentifier</key>
> > 	<string>org.mozilla.updater</string>
> > 	<key>CFBundleInfoDictionaryVersion</key>
> > 	<string>6.0</string>
> > 	<key>CFBundlePackageType</key>
> > 	<string>APPL</string>
> >diff --git a/toolkit/mozapps/update/updater/moz.build b/toolkit/mozapps/update/updater/moz.build
> >--- a/toolkit/mozapps/update/updater/moz.build
> >+++ b/toolkit/mozapps/update/updater/moz.build
> >@@ -1,14 +1,27 @@
> > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> > # vim: set filetype=python:
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> >-Program('updater')
> >+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
> >+    Program('org.mozilla.updater')
> >+else:
> >+    Program('updater')
> 
> Why do we need to change 'updater' to 'org.mozilla.updater' for this patch?
> Is this something we should be doing on all platforms?
> 
> The patch looks fine to me otherwise.

This is because the binary can now be installed as a privileged helper tool on OSX[1]. When this happens, the binary is extracted from the .app bundle and the name of the executable is then used to identify it among all the privileged helper tools. "updater" is too generic and privileged helper tools are typically named in the same way as bundle identifiers for .app bundles. The updater already has a bundle identifier of "org.mozilla.updater" (see toolkit/mozapps/update/updater/macbuild/Contents/Info.plist in the diff above).

Since this is very OSX specific, I don't think we need to apply this to other platforms. On some platforms like Windows it might even look weird or out-of-place to have a binary name of org.mozilla.updater.exe.

Michael, since you r+'d this patch can I interpret your f+ for patch 3 (comment 143, comment 144) as an r+ as well? If not, what changes would you like to see to patch 3 (which would most likely also require changes to patch 1)? Thanks in advance!

[1] https://developer.apple.com/library/mac/documentation/Security/Conceptual/SecureCodingGuide/Articles/AccessControl.html#//apple_ref/doc/uid/TP40002589-SW2

Comment 147

[Stephen A Pohl [:spohl]]

Comment on attachment 8683709 [details] [diff] [review]
6. Update existing tests

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #137)
> Comment on attachment 8683239 [details] [diff] [review]
> >diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
> >--- a/browser/app/Makefile.in
> >+++ b/browser/app/Makefile.in
> >@@ -98,10 +98,13 @@ tools repackage:: $(PROGRAM)
> > cp -RL $(DIST)/branding/document.icns $(dist_dest)/Contents/Resources/document.icns
> >+	$(MKDIR) -p $(dist_dest)/Contents/Library/LaunchServices
> >+	mv -f $(dist_dest)/Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater $(dist_dest)/Contents/Library/LaunchServices
> >+	ln -s ../../../../Library/LaunchServices/org.mozilla.updater $(dist_dest)/Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater
> > printf APPLMOZB > $(dist_dest)/Contents/PkgInfo
> > endif
> Please move the Makefile.in changes from [patch] 6. Update existing tests to
> this patch or another patch with just build changes. Also, build changes
> will need to be reviewed by a build peer.

Sorry, I forgot about this before asking :mshal to review patch 1. Michael, would you mind reviewing the additional build change in this patch here? Thanks.

Comment 148

[Michael Shal [:mshal]]

(In reply to Stephen A Pohl [:spohl] from comment #146)
> Michael, since you r+'d this patch can I interpret your f+ for patch 3
> (comment 143, comment 144) as an r+ as well? If not, what changes would you
> like to see to patch 3 (which would most likely also require changes to
> patch 1)? Thanks in advance!

I'd still like to see if we can get Info.plist generated in the objdir before r+. I didn't realize it needs to be in export - that's fine, so ignore my suggestion to put it in libs :). But I think we need to generate it in the objdir, reference the objdir in the LDFLAGS, and then skip the removal in the libs rule. Can you show what you tried and what issue you ran into?

Comment 149

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

(In reply to Stephen A Pohl [:spohl] from comment #142)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #141)
> > Comment on attachment 8683239 [details] [diff] [review]
> > 1. Patch to elevate updater
> > 
> > Is the update-canceled observer because you are requesting elevation while
> > Firefox is running? Let's discuss this over vidyo next week... please ping
> > me.
> 
> The second part to this is in patch "4. UX". When a user chooses not to
> install an update that requires elevation (by clicking the "No Thanks"
> button in the update prompt), we want to clean up the update, hide the arrow
> "badge" on green background on the hamburger menu and we want to reset the
> About dialog to not display the "Restart to install" button. Patch 1 here
> observes "update-canceled" to clean up the staged or applied update
> asynchronously.
> 
> Unfortunately, I couldn't come up with a particularly good way to split this
> logic between the two patches.
Let's go with a new method instead of using an observer.

Comment 150

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

Comment on attachment 8683239 [details] [diff] [review]
1. Patch to elevate updater

>@@ -2609,16 +2563,109 @@ int NS_main(int argc, NS_tchar **argv)
>       sStagedUpdate = true;
>     } else if (NS_tstrstr(argv[4], NS_T("/replace"))) {
>       // We're processing a request to replace the application with a staged
>       // update.
>       sReplaceRequest = true;
>     }
>   }
> 
>+#if defined(XP_MACOSX)
>+  bool isElevated =
>+    strstr(argv[0], "/Library/PrivilegedHelperTools/org.mozilla.updater") != 0;
>+  if (isElevated) {
>+    if (!ObtainUpdaterArguments(&argc, &argv)) {
>+      // Won't actually get here because ObtainUpdaterArguments will terminate
>+      // the current process on failure.
>+      return 1;
>+    }
>+  } else {
>+    sRequiresElevation = access(argv[2], W_OK) != 0;
>+  }
>+#endif
>+
>+  // The directory containing the update information.
>+  gPatchDirPath = argv[1];
>+
>+#ifdef XP_MACOSX
>+  if (!sRequiresElevation || sStagedUpdate) {
Why the sStagedUpdate check?

>+#endif
>+  InitProgressUI(&argc, &argv);
>+
>+  // To process an update the updater command line must at a minimum have the
>+  // directory path containing the updater.mar file to process as the first
>+  // argument, the install directory as the second argument, and the directory
>+  // to apply the update to as the third argument. When the updater is launched
>+  // by another process the PID of the parent process should be provided in the
>+  // optional fourth argument and the updater will wait on the parent process to
>+  // exit if the value is non-zero and the process is present. This is necessary
>+  // due to not being able to update files that are in use on Windows. The
>+  // optional fifth argument is the callback's working directory and the
>+  // optional sixth argument is the callback path. The callback is the
>+  // application to launch after updating and it will be launched when these
>+  // arguments are provided whether the update was successful or not. All
>+  // remaining arguments are optional and are passed to the callback when it is
>+  // launched.
>+  if (argc < 4) {
>+    fprintf(stderr, "Usage: updater patch-dir install-dir apply-to-dir [wait-pid [callback-working-dir callback-path args...]]\n");
>+#ifdef XP_MACOSX
>+    if (isElevated) {
>+      freeArguments(argc, argv);
>+      CleanupElevatedMacUpdate(true);
>+    }
>+#endif
>+    return 1;
>+  }

It seems like this could be cleaned up a bit so it is easier to read. I'll take a look at what can be done in the next review.

Comment 151

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

Comment on attachment 8683239 [details] [diff] [review]
1. Patch to elevate updater

If you could please split out the build config parts of the patch especially since they have been reviewed. Would be helpful to do the same for the Objective-C parts as well.

Comment 152

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

Comment on attachment 8682661 [details] [diff] [review]
4. UX

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js
>--- a/browser/base/content/aboutDialog.js
>+++ b/browser/base/content/aboutDialog.js
>...
>@@ -263,16 +270,23 @@ appUpdater.prototype =
>       this.updateDeck.selectedPanel = panel;
>     }
>   },
> 
>   /**
>    * Check for updates
>    */
>   checkForUpdates: function() {
>+    // Clear prefs that could prevent a user from discovering available updates.
>+    if (Services.prefs.prefHasUserValue(PREF_APP_UPDATE_CANCELATIONS)) {
>+      Services.prefs.clearUserPref(PREF_APP_UPDATE_CANCELATIONS);
This is count is used by telemetry and shouldn't be cleared here. I think this can be reworked to clear on successful check if it isn't already cleared on successful check.

>@@ -356,17 +372,18 @@ appUpdater.prototype =
>         if (gAppUpdater.update.detailsURL) {
>           let unsupportedLink = document.getElementById("unsupportedLink");
>           unsupportedLink.href = gAppUpdater.update.detailsURL;
>         }
>         gAppUpdater.selectPanel("unsupportedSystem");
>         return;
>       }
> 
>-      if (!gAppUpdater.aus.canApplyUpdates) {
>+      if (!gAppUpdater.aus.canApplyUpdates ||
>+          gAppUpdater.update.elevationFailure) {
Let's talk about how the flow ends up here when checking for an update.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -2687,16 +2687,22 @@ var gMenuButtonUpdateBadge = {
>     } else {
>       // open the page for manual update
>       let url = Services.urlFormatter.formatURLPref("app.update.url.manual");
>       openUILinkIn(url, "tab");
>     }
>   },
> 
>   observe: function (subject, topic, status) {
>+    if (topic == "update-canceled") {
>+      Services.obs.removeObserver(this, "update-canceled");
>+      this.hideBadge();
>+      this.init();
>+      return;
>+    }
>     if (status == "failed") {
>       // Background update has failed, let's show the UI responsible for
>       // prompting the user to update manually.
>       this.displayBadge(false);
>       this.uninit();
>       return;
>     }
> 
>@@ -2723,25 +2729,33 @@ var gMenuButtonUpdateBadge = {
>     let stringId;
>     let updateButtonText;
>     if (succeeded) {
>       let brandBundle = document.getElementById("bundle_brand");
>       let brandShortName = brandBundle.getString("brandShortName");
>       stringId = "appmenu.restartNeeded.description";
>       updateButtonText = gNavigatorBundle.getFormattedString(stringId,
>                                                              [brandShortName]);
>+      Services.obs.addObserver(this, "update-canceled", false);
Not sure of a better way to do this but even if this stays I'd like to get rid of the observers in nsUpdateService.js if at all possible per the previous review comment.

>diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js
>--- a/toolkit/mozapps/update/content/updates.js
>+++ b/toolkit/mozapps/update/content/updates.js
>@@ -14,40 +14,43 @@ CoU.import("resource://gre/modules/Addon
> CoU.import("resource://gre/modules/Services.jsm", this);
> CoU.import("resource://gre/modules/UpdateTelemetry.jsm", this);
> 
> const XMLNS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> 
> const PREF_APP_UPDATE_BACKGROUNDERRORS    = "app.update.backgroundErrors";
> const PREF_APP_UPDATE_BILLBOARD_TEST_URL  = "app.update.billboard.test_url";
> const PREF_APP_UPDATE_CERT_ERRORS         = "app.update.cert.errors";
>+const PREF_APP_UPDATE_ELEVATION_NEVER     = "app.update.elevation.never";
> const PREF_APP_UPDATE_ENABLED             = "app.update.enabled";
> const PREF_APP_UPDATE_LOG                 = "app.update.log";
> const PREF_APP_UPDATE_NOTIFIEDUNSUPPORTED = "app.update.notifiedUnsupported";
> const PREF_APP_UPDATE_TEST_LOOP           = "app.update.test.loop";
> const PREF_APP_UPDATE_URL_MANUAL          = "app.update.url.manual";
> 
>-const PREFBRANCH_APP_UPDATE_NEVER         = "app.update.never.";
>+const PREF_APP_UPDATE_NEVER_BRANCH        = "app.update.never.";
Why this change? It is clearer that it isn't a single pref when it starts with PREFBRANCH.

>@@ -238,18 +241,24 @@ var gUpdates = {
>       return this.strings.getFormattedString(key, strings);
>     return this.strings.getString(key);
>   },
> 
>   never: function () {
>     // If the user clicks "No Thanks", we should not prompt them to update to
>     // this version again unless they manually select "Check for Updates..."
>     // which will clear all of the "never" prefs.
>-    var neverPrefName = PREFBRANCH_APP_UPDATE_NEVER + this.update.appVersion;
>+    let neverPrefName = PREF_APP_UPDATE_NEVER_BRANCH + this.update.appVersion;
>     Services.prefs.setBoolPref(neverPrefName, true);
>+    let aus = CoC["@mozilla.org/updates/update-service;1"].
>+              getService(CoI.nsIApplicationUpdateService);
>+    if (aus.elevationRequired) {
>+      Services.prefs.setCharPref(PREF_APP_UPDATE_ELEVATION_NEVER,
>+                                 this.update.appVersion);
>+    }
I'm sure you asked if they can be combined but do you think these can be combined? Let's go over this when we talk.

>@@ -648,17 +664,17 @@ var gCheckingPage = {
>       gUpdates.setUpdate(aus.selectUpdate(updates, updates.length));
>       if (gUpdates.update) {
>         LOG("gCheckingPage", "onCheckComplete - update found");
>         if (gUpdates.update.unsupported) {
>           gUpdates.wiz.goTo("unsupported");
>           return;
>         }
> 
>-        if (!aus.canApplyUpdates) {
>+        if (!aus.canApplyUpdates || gUpdates.update.elevationFailure) {
Will a user get more than one notification for elevation failures?

>           // Prevent multiple notifications for the same update when the user is
>           // unable to apply updates.
>           gUpdates.never();
>           gUpdates.wiz.goTo("manualUpdate");
>           return;
>         }
> 
>         if (gUpdates.update.licenseURL) {
>...
>@@ -1850,16 +1896,27 @@ var gFinishedPage = {
>   },
> 
>   /**
>    * When the user clicks the "Restart Later" instead of the Restart Now" button
>    * in the wizard after an update has been downloaded.
>    */
>   onExtra1: function() {
>     gUpdates.wiz.cancel();
>+  },
>+
>+  /**
>+   * When elevation is required and the user clicks "No Thanks" in the wizard.
>+   */
>+  onExtra2: function() {
>+    let aus = CoC["@mozilla.org/updates/update-service;1"].
>+              getService(CoI.nsIApplicationUpdateService);
>+    Services.obs.notifyObservers(null, "update-canceled", null);
>+    gUpdates.never();
Will this stop future background checks for this version?

>+    gUpdates.wiz.cancel();
>   }
> };
> 
> /**
>  * The "Update was Installed Successfully" page.
>  */
> var gInstalledPage = {
>   /**
>diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl
>--- a/toolkit/mozapps/update/nsIUpdateService.idl
>+++ b/toolkit/mozapps/update/nsIUpdateService.idl
>@@ -514,23 +514,29 @@ interface nsIUpdateManager : nsISupports
>    * Saves all updates to disk.
>    */
>   void saveUpdates();
> 
>   /**
>    * Refresh the update status based on the information in update.status.
>    */
>   void refreshUpdateStatus();
>+
>+  /**
>+   * The user was made aware that an update requires elevation and we can
>+   * now prompt for elevation.
>+   */
>+  void promptingForElevationPermitted();
bump uuid unless it is bumped elsewhere.

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>...
>@@ -3338,16 +3349,37 @@ UpdateManager.prototype = {
>       // Notify the user that an update has been staged and is ready for
>       // installation (i.e. that they should restart the application).
>       let prompter = Cc["@mozilla.org/updates/update-prompt;1"].
>                      createInstance(Ci.nsIUpdatePrompt);
>       prompter.showUpdateDownloaded(update, true);
>     }
>   },
> 
>+  /**
>+   * See nsIUpdateService.idl
>+   */
>+  promptingForElevationPermitted: function UM_promptingForElevationPermitted() {
This doesn't prompt. Let's discuss why it is needed to change the status file.

>+    // The user has been been made aware that the update requires elevation.
>+    let update = this._activeUpdate;
>+    if (!update) {
>+      return;
>+    }
>+    let status = readStatusFile(getUpdatesDir());
>+    let parts = status.split(":");
>+    update.state = parts[0];
>+    if (update.state == STATE_PENDING_ELEVATION_REQ) {
>+      // Proceed with the pending update.
>+      writeStatusFile(getUpdatesDir(), STATE_PENDING);
>+    } else if (update.state == STATE_APPLIED_ELEVATION_REQ) {
>+      // Proceed with the applied update.
>+      writeStatusFile(getUpdatesDir(), STATE_APPLIED);
>+    }
>+  },
>+
>   classID: Components.ID("{093C2356-4843-4C65-8709-D7DBCBBE7DFB}"),
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIUpdateManager, Ci.nsIObserver])
> };
> 
> /**
>  * Checker
>  * Checks for new Updates
>  * @constructor
>@@ -4570,16 +4604,27 @@ UpdatePrompt.prototype = {
>    * See nsIUpdateService.idl
>    */
>   showUpdateHistory: function UP_showUpdateHistory(parent) {
>     this._showUI(parent, URI_UPDATE_HISTORY_DIALOG, "modal,dialog=yes",
>                  "Update:History", null, null);
>   },
> 
>   /**
>+   * See nsIUpdateService.idl
>+   */
>+  showUpdateElevationRequired:
>+  function UP_showUpdateElevationRequired() {
nit: no need to wrap

>+    let um = Cc["@mozilla.org/updates/update-manager;1"].
>+             getService(Ci.nsIUpdateManager);
>+    this._showUI(null, URI_UPDATE_PROMPT_DIALOG, null,
>+                 UPDATE_WINDOW_NAME, "finishedBackground", um.activeUpdate);
>+  },
This doesn't have the typical checks for UI. Aren't they needed here too?

Comment 153

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

Comment on attachment 8682661 [details] [diff] [review]
4. UX

Could you combine patch 1 and 4... having to refer between them is making reviewing difficult.

Comment 154

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

Comment on attachment 8682663 [details] [diff] [review]
5. Telemetry

>diff --git a/toolkit/mozapps/update/UpdateTelemetry.jsm b/toolkit/mozapps/update/UpdateTelemetry.jsm
>--- a/toolkit/mozapps/update/UpdateTelemetry.jsm
>+++ b/toolkit/mozapps/update/UpdateTelemetry.jsm
>@@ -107,16 +107,22 @@ this.AUSTLMY = {
>   // Unable to check for updates due to no OS ABI (no notification)
>   CHK_NO_OS_ABI: 31,
>   // Invalid url for app.update.url default preference (no notification)
>   CHK_INVALID_DEFAULT_URL: 32,
>   // Invalid url for app.update.url user preference (no notification)
>   CHK_INVALID_USER_OVERRIDE_URL: 33,
>   // Invalid url for app.update.url.override user preference (no notification)
>   CHK_INVALID_DEFAULT_OVERRIDE_URL: 34,
>+  // Elevation failed or has been canceled too many times for this version of
>+  // the update to proceed (OSX only).
How about
>+  // Update elevation failures or cancellations threshold reached for this
>+  // version (OSX only)
Is there a notification (see other telemetry comments)?

>+  CHK_ELEVATION_TRIES_EXCEEDED_FOR_VERSION: 35,
How about something like
CHK_ELEVATION_DISABLED_FOR_VERSION: 35,

>+  // User opted out of update elevation for a particular version of an update
>+  // (OSX only).
How about
User opted out of an elevated update for the available update version

Is there a notification (see other telemetry comments)?

>+  CHK_ELEVATION_DISABLED_FOR_VERSION: 36,
How about
CHK_ELEVATION_OPTOUT_FOR_VERSION: 36,

> 
>   /**
>    * Submit a telemetry ping for the update check result code or a telemetry
>    * ping for a count type histogram count when no update was found. The no
>    * update found ping is separate since it is the typical result, is less
>    * interesting than the other result codes, and it is easier to analyze the
>    * other codes without including it.
>    *
>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>...
>@@ -2457,21 +2465,26 @@ UpdateService.prototype = {
>                                    0);
>           let rejectedVersion = getPref("getCharPref",
>                                         PREF_APP_UPDATE_ELEVATION_NEVER, "");
>           if (numCancels >= DEFAULT_MAX_ELEVATION_CANCELATIONS) {
>             LOG("UpdateService:selectUpdate - the user requires elevation to " +
>                 "install this update, but the user has exceeded the max number of " +
>                 "elevation attempts.");
>             update.elevationFailure = true;
>+            AUSTLMY.pingCheckCode(
>+              this._pingSuffix,
>+              AUSTLMY.CHK_ELEVATION_TRIES_EXCEEDED_FOR_VERSION);
Why not set the PREF_APP_UPDATE_ELEVATION_NEVER pref here?

Please combine this with the other two main patches. Minus since I'll go through this again when I see the patches combined.

Comment 155

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

Comment on attachment 8685536 [details] [diff] [review]
7. Tests (wip)

I dislike adding test code especially like this to the application itself. Not sure what else can be done and we can discuss this tomorrow.

Comment 156

[Stephen A Pohl [:spohl]]

Thanks for all the feedback! I'll start by addressing the observer question:

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #149)
> (In reply to Stephen A Pohl [:spohl] from comment #142)
> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> > comment #141)
> > > Comment on attachment 8683239 [details] [diff] [review]
> > > 1. Patch to elevate updater
> > > 
> > > Is the update-canceled observer because you are requesting elevation while
> > > Firefox is running? Let's discuss this over vidyo next week... please ping
> > > me.
> > 
> > The second part to this is in patch "4. UX". When a user chooses not to
> > install an update that requires elevation (by clicking the "No Thanks"
> > button in the update prompt), we want to clean up the update, hide the arrow
> > "badge" on green background on the hamburger menu and we want to reset the
> > About dialog to not display the "Restart to install" button. Patch 1 here
> > observes "update-canceled" to clean up the staged or applied update
> > asynchronously.
> > 
> > Unfortunately, I couldn't come up with a particularly good way to split this
> > logic between the two patches.
> Let's go with a new method instead of using an observer.

One benefit of an observer is that we're not locking the main thread. My first implementation used a new method, but it caused significant jank when the user canceled an update. Another benefit is that we're able to observe this event in two different places, i.e. the front-end code (to reset the UI) and the lower-level code (to clean up files on the file system etc). I thought using an observer was a good way to avoid jank and to avoid grouping logically separate things (UI- vs. file-system interactions) into one method.

Comment 157

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

I think this is still main thread and the jank you are seeing is the UI waiting on the method returning. There are other ways to accomplish that and it looked to me that there are cases where you don't remove the observers before shutdown.

Comment 158

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Lots of comments to address, but not a ton of code changes. I'll give my feedback to all the comments below to make sure I didn't leave anything out.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #140)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #137)
> > Comment on attachment 8683239 [details] [diff] [review]
> > 1. Patch to elevate updater
> > 
> >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> > >--- a/toolkit/mozapps/update/nsUpdateService.js
> > >+++ b/toolkit/mozapps/update/nsUpdateService.js
> >@@ -2446,18 +2496,68 @@ UpdateService.prototype = {
> >+        // Clear elevation-related prefs since they no longer apply (the user
> >+        // may have gained write access to the Firefox directory or an update
> >+        // was executed with a different profile).
> >+        if (Services.prefs.prefHasUserValue(
> >+              PREF_APP_UPDATE_ELEVATION_VERSION)) {
> nit: skip the wrapping

Done.

> >+          Services.prefs.clearUserPref(PREF_APP_UPDATE_ELEVATION_VERSION);
> >+        }
> >+        if (Services.prefs.prefHasUserValue(PREF_APP_UPDATE_CANCELATIONS)) {
> >+          Services.prefs.clearUserPref(PREF_APP_UPDATE_CANCELATIONS);
> >+        }
> The PREF_APP_UPDATE_CANCELATIONS is a count of consecutive update
> cancellations and unless I'm mistaken clearing it here above will clear it
> every time an update is found even when the previous update was cancelled.

From the comment a few lines above:
 // Clear elevation-related prefs since they no longer apply (the user
 // may have gained write access to the Firefox directory or an update
 // was executed with a different profile).

We don't get here if elevation is required.


> (In reply to Stephen A Pohl [:spohl] from comment #139)
> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> > comment #136)
> > > It would be a "good thing" to re-verify with dveditz that it is ok to change
> > > the permissions so all admin users can write to this directory without being
> > > prompted especially since an admin user process would be able to change the
> > > files in the directory.
> > 
> > I just spoke with dveditz at Mozlando and this is still ok. With regards to
> > new files being installed after an elevated update, the suggested solution
> > is to check a path that's always present and set new files to group
> > ownership "admin" with write permission if the path has the same. The top
> > level .app directory seems like a good path to check. The next step is to
> > verify that an unelevated update can indeed set group ownership of new files
> > to "admin" with write permission. I'll look into that next.
> > 
> > dveditz also confirmed that once reviewed, we can go ahead with the landing
> > of the patches, test them and iterate on them when necessary.
> Good to hear. The only other comment I have is how this can change
> administratively set permissions. I don't think that is that big of a deal
> but it should be documented and the enterprise group should be notified of
> this new behavior.

Could you let me know where these things are typically documented? Also, is the enterprise group a mailing list of some sort that you could point me to? Thanks!

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #150)
> Comment on attachment 8683239 [details] [diff] [review]
> 1. Patch to elevate updater
> 
> >@@ -2609,16 +2563,109 @@ int NS_main(int argc, NS_tchar **argv)
> >+
> >+  // The directory containing the update information.
> >+  gPatchDirPath = argv[1];
> >+
> >+#ifdef XP_MACOSX
> >+  if (!sRequiresElevation || sStagedUpdate) {
> Why the sStagedUpdate check?

If this is a staged update, only the "apply" phase requires elevation. Staging occurs in the ~/Library directory, which the user should always have write access to. We will notify the user that the update requires elevation after staging, but before trying to apply the update.

> >+  if (argc < 4) {
> >+    fprintf(stderr, "Usage: updater patch-dir install-dir apply-to-dir [wait-pid [callback-working-dir callback-path args...]]\n");
> >+#ifdef XP_MACOSX
> >+    if (isElevated) {
> >+      freeArguments(argc, argv);
> >+      CleanupElevatedMacUpdate(true);
> >+    }
> >+#endif
> >+    return 1;
> >+  }
> 
> It seems like this could be cleaned up a bit so it is easier to read. I'll
> take a look at what can be done in the next review.

Ok, just let me know.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #151)
> Comment on attachment 8683239 [details] [diff] [review]
> 1. Patch to elevate updater
> 
> If you could please split out the build config parts of the patch especially
> since they have been reviewed. Would be helpful to do the same for the
> Objective-C parts as well.

Done.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #152)
> Comment on attachment 8682661 [details] [diff] [review]
> 4. UX
> 
> >diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js
> >--- a/browser/base/content/aboutDialog.js
> >+++ b/browser/base/content/aboutDialog.js
> >...
> >@@ -263,16 +270,23 @@ appUpdater.prototype =
> >       this.updateDeck.selectedPanel = panel;
> >     }
> >   },
> > 
> >   /**
> >    * Check for updates
> >    */
> >   checkForUpdates: function() {
> >+    // Clear prefs that could prevent a user from discovering available updates.
> >+    if (Services.prefs.prefHasUserValue(PREF_APP_UPDATE_CANCELATIONS)) {
> >+      Services.prefs.clearUserPref(PREF_APP_UPDATE_CANCELATIONS);
> This is count is used by telemetry and shouldn't be cleared here. I think
> this can be reworked to clear on successful check if it isn't already
> cleared on successful check.

The idea here was that even though a user may have canceled elevated updates a few times, or even rejected a specific version, he may later come back and manually check for updates via the about dialog. I was thinking of a manual check as a reset for these prefs. Currently, if these prefs are set, we may not display an available update because the user may have exceeded the number of elevation attempts or rejected the update. You mention that we could clear the prefs after a successful check, but isn't the *attempt* to check for updates enough to show intent by the user to discover updates?

> >@@ -356,17 +372,18 @@ appUpdater.prototype =
> >         if (gAppUpdater.update.detailsURL) {
> >           let unsupportedLink = document.getElementById("unsupportedLink");
> >           unsupportedLink.href = gAppUpdater.update.detailsURL;
> >         }
> >         gAppUpdater.selectPanel("unsupportedSystem");
> >         return;
> >       }
> > 
> >-      if (!gAppUpdater.aus.canApplyUpdates) {
> >+      if (!gAppUpdater.aus.canApplyUpdates ||
> >+          gAppUpdater.update.elevationFailure) {
> Let's talk about how the flow ends up here when checking for an update.

Originally, I had this here to switch to the manual update screen if a user exceeded the allowed number of elevation attempts or refused a particular update version. Later, I thought that a better solution was to reset the two prefs in |checkForUpdates| (see above), but forgot to remove this. I still think resetting the prefs leads to a better user experience, so I've reverted this change here.

> >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js
> >--- a/toolkit/mozapps/update/content/updates.js
> >+++ b/toolkit/mozapps/update/content/updates.js
> >@@ -14,40 +14,43 @@ CoU.import("resource://gre/modules/Addon
> > CoU.import("resource://gre/modules/Services.jsm", this);
> > CoU.import("resource://gre/modules/UpdateTelemetry.jsm", this);
> > 
> > const XMLNS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> > 
> > const PREF_APP_UPDATE_BACKGROUNDERRORS    = "app.update.backgroundErrors";
> > const PREF_APP_UPDATE_BILLBOARD_TEST_URL  = "app.update.billboard.test_url";
> > const PREF_APP_UPDATE_CERT_ERRORS         = "app.update.cert.errors";
> >+const PREF_APP_UPDATE_ELEVATION_NEVER     = "app.update.elevation.never";
> > const PREF_APP_UPDATE_ENABLED             = "app.update.enabled";
> > const PREF_APP_UPDATE_LOG                 = "app.update.log";
> > const PREF_APP_UPDATE_NOTIFIEDUNSUPPORTED = "app.update.notifiedUnsupported";
> > const PREF_APP_UPDATE_TEST_LOOP           = "app.update.test.loop";
> > const PREF_APP_UPDATE_URL_MANUAL          = "app.update.url.manual";
> > 
> >-const PREFBRANCH_APP_UPDATE_NEVER         = "app.update.never.";
> >+const PREF_APP_UPDATE_NEVER_BRANCH        = "app.update.never.";
> Why this change? It is clearer that it isn't a single pref when it starts
> with PREFBRANCH.

We're calling the same pref PREF_APP_UPDATE_NEVER_BRANCH in toolkit/mozapps/update/tests/data/shared.js and toolkit/mozapps/update/nsUpdateService.js, but PREFBRANCH_APP_UPDATE_NEVER in toolkit/mozapps/update/content/updates.js. This change is so we only use one name for the same pref across the various files. If you'd prefer PREFBRANCH_APP_UPDATE_NEVER as the name, I'm happy to change it in the other two files.

> >@@ -238,18 +241,24 @@ var gUpdates = {
> >       return this.strings.getFormattedString(key, strings);
> >     return this.strings.getString(key);
> >   },
> > 
> >   never: function () {
> >     // If the user clicks "No Thanks", we should not prompt them to update to
> >     // this version again unless they manually select "Check for Updates..."
> >     // which will clear all of the "never" prefs.
> >-    var neverPrefName = PREFBRANCH_APP_UPDATE_NEVER + this.update.appVersion;
> >+    let neverPrefName = PREF_APP_UPDATE_NEVER_BRANCH + this.update.appVersion;
> >     Services.prefs.setBoolPref(neverPrefName, true);
> >+    let aus = CoC["@mozilla.org/updates/update-service;1"].
> >+              getService(CoI.nsIApplicationUpdateService);
> >+    if (aus.elevationRequired) {
> >+      Services.prefs.setCharPref(PREF_APP_UPDATE_ELEVATION_NEVER,
> >+                                 this.update.appVersion);
> >+    }
> I'm sure you asked if they can be combined but do you think these can be
> combined? Let's go over this when we talk.

Yes, we spoke about this on IRC and the conclusion was that I should use a new pref. Here's the transcript:
[...]
2:39:26 PM rstrong: If you need a pref just use a new one. If the day comes when we have new ui I'd like to get rid of the option
2:41:19 PM spohl: sounds good, thanks
2:41:43 PM rstrong: Just looked... that was added for backwards compatibility with the old update xml format and is not available with the new format. It is not set by releng in the update xml itself.
2:43:01 PM spohl: so I should just use a different pref? or do you think I can just repurpose this one?
2:43:15 PM rstrong: We used to show that UI when displaying the major update billboard UI way back when and it would still be shown if we served updates with a billboard which we haven't in years
2:43:23 PM rstrong: Use a new pref
2:43:28 PM spohl: ok, cool!

> >@@ -648,17 +664,17 @@ var gCheckingPage = {
> >       gUpdates.setUpdate(aus.selectUpdate(updates, updates.length));
> >       if (gUpdates.update) {
> >         LOG("gCheckingPage", "onCheckComplete - update found");
> >         if (gUpdates.update.unsupported) {
> >           gUpdates.wiz.goTo("unsupported");
> >           return;
> >         }
> > 
> >-        if (!aus.canApplyUpdates) {
> >+        if (!aus.canApplyUpdates || gUpdates.update.elevationFailure) {
> Will a user get more than one notification for elevation failures?

If the user has failed to elevate 5 times, or rejected this update, we use the same mechanism as if the user was unable to apply updates, which is to prevent multiple notifications for the same update (see comment on the next two lines).

> >@@ -1850,16 +1896,27 @@ var gFinishedPage = {
> >+  /**
> >+   * When elevation is required and the user clicks "No Thanks" in the wizard.
> >+   */
> >+  onExtra2: function() {
> >+    let aus = CoC["@mozilla.org/updates/update-service;1"].
> >+              getService(CoI.nsIApplicationUpdateService);
> >+    Services.obs.notifyObservers(null, "update-canceled", null);
> >+    gUpdates.never();
> Will this stop future background checks for this version?

Yes

> >diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl
> >--- a/toolkit/mozapps/update/nsIUpdateService.idl
> >+++ b/toolkit/mozapps/update/nsIUpdateService.idl
> >@@ -514,23 +514,29 @@ interface nsIUpdateManager : nsISupports
> >+
> >+  /**
> >+   * The user was made aware that an update requires elevation and we can
> >+   * now prompt for elevation.
> >+   */
> >+  void promptingForElevationPermitted();
> bump uuid unless it is bumped elsewhere.

Done.
 
> >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> >--- a/toolkit/mozapps/update/nsUpdateService.js
> >+++ b/toolkit/mozapps/update/nsUpdateService.js
> >+  /**
> >+   * See nsIUpdateService.idl
> >+   */
> >+  promptingForElevationPermitted: function UM_promptingForElevationPermitted() {
> This doesn't prompt. Let's discuss why it is needed to change the status
> file.

This function is used to tell us when we gained permission to prompt the user for elevation during an update (I couldn't find a better function name, but I'm happy to change it if there's a better suggestion). Based on the UX flow that we agreed on, we are required to let the user know that an update will require admin privileges before displaying the native elevation prompt (otherwise, the user may be surprised and not trust that the elevation prompt is a legitimate part of our update and cancel out). If the user was notified, we switch the status, which will cause us to show the elevation prompt upon the next restart of Firefox.

> >@@ -4570,16 +4604,27 @@ UpdatePrompt.prototype = {
> >   /**
> >+   * See nsIUpdateService.idl
> >+   */
> >+  showUpdateElevationRequired:
> >+  function UP_showUpdateElevationRequired() {
> nit: no need to wrap

Fixed.
 
> >+    let um = Cc["@mozilla.org/updates/update-manager;1"].
> >+             getService(Ci.nsIUpdateManager);
> >+    this._showUI(null, URI_UPDATE_PROMPT_DIALOG, null,
> >+                 UPDATE_WINDOW_NAME, "finishedBackground", um.activeUpdate);
> >+  },
> This doesn't have the typical checks for UI. Aren't they needed here too?

Added checks and removed unnecessary code in |showUpdateInstalled| while I was there (we return early if |this._getUpdateWindow()| is non-null).

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #153)
> Comment on attachment 8682661 [details] [diff] [review]
> 4. UX
> 
> Could you combine patch 1 and 4... having to refer between them is making
> reviewing difficult.

Done.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #154)
> Comment on attachment 8682663 [details] [diff] [review]
> 5. Telemetry
> 
> >diff --git a/toolkit/mozapps/update/UpdateTelemetry.jsm b/toolkit/mozapps/update/UpdateTelemetry.jsm
> >--- a/toolkit/mozapps/update/UpdateTelemetry.jsm
> >+++ b/toolkit/mozapps/update/UpdateTelemetry.jsm
> >@@ -107,16 +107,22 @@ this.AUSTLMY = {
> >+  // Elevation failed or has been canceled too many times for this version of
> >+  // the update to proceed (OSX only).
> How about
> Update elevation failures or cancellations threshold reached for this
> version (OSX only)

Done.

> Is there a notification (see other telemetry comments)?

After every elevation failure or cancelation, the user is notified that the update has failed for that reason. There is no additional notification once the user has hit the threshold.

> >+  CHK_ELEVATION_TRIES_EXCEEDED_FOR_VERSION: 35,
> How about something like
> CHK_ELEVATION_DISABLED_FOR_VERSION: 35,

Done.

> >+  // User opted out of update elevation for a particular version of an update
> >+  // (OSX only).
> How about
> User opted out of an elevated update for the available update version

Changed to: User opted out of elevated updates for the available update version (OSX only).

> Is there a notification (see other telemetry comments)?

After clicking "No Thanks" in the UI, there is no additional notification for an available update version.

> >+  CHK_ELEVATION_DISABLED_FOR_VERSION: 36,
> How about
> CHK_ELEVATION_OPTOUT_FOR_VERSION: 36,

Done.

> >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> >--- a/toolkit/mozapps/update/nsUpdateService.js
> >+++ b/toolkit/mozapps/update/nsUpdateService.js
> >...
> >@@ -2457,21 +2465,26 @@ UpdateService.prototype = {
> >                                    0);
> >           let rejectedVersion = getPref("getCharPref",
> >                                         PREF_APP_UPDATE_ELEVATION_NEVER, "");
> >           if (numCancels >= DEFAULT_MAX_ELEVATION_CANCELATIONS) {
> >             LOG("UpdateService:selectUpdate - the user requires elevation to " +
> >                 "install this update, but the user has exceeded the max number of " +
> >                 "elevation attempts.");
> >             update.elevationFailure = true;
> >+            AUSTLMY.pingCheckCode(
> >+              this._pingSuffix,
> >+              AUSTLMY.CHK_ELEVATION_TRIES_EXCEEDED_FOR_VERSION);
> Why not set the PREF_APP_UPDATE_ELEVATION_NEVER pref here?

Not sure. What would be the advantage?

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #157)
> I think this is still main thread and the jank you are seeing is the UI
> waiting on the method returning. There are other ways to accomplish that and
> it looked to me that there are cases where you don't remove the observers
> before shutdown.

Now that the patches have been combined, do the observers make more sense? If you're still in favor of a separate method, could you clarify how I can avoid the jank in the UI while files get deleted on disk etc?

Comment 159

[Stephen A Pohl [:spohl]]

Comment on attachment 8701096 [details] [diff] [review]
1. Patch to elevate updater

There was one more comment:

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #137)
> Comment on attachment 8683239 [details] [diff] [review]
> 1. Patch to elevate updater
>
> Please move the Makefile.in changes from [patch] 6. Update existing tests to
> this patch or another patch with just build changes. Also, build changes
> will need to be reviewed by a build peer.

Split out the build config changes. Will upload a separate patch once I've addressed :mshal's feedback.

> >diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl
> >--- a/toolkit/mozapps/update/nsIUpdateService.idl
> >+++ b/toolkit/mozapps/update/nsIUpdateService.idl
> >@@ -221,41 +221,48 @@ interface nsIUpdate : nsISupports
> > 
> >   /**
> >    * The currently selected patch for this update.
> >    */
> >   readonly attribute nsIUpdatePatch selectedPatch;
> > 
> >   /**
> >    * The state of the selected patch:
> >-   *   "downloading"        The update is being downloaded.
> >-   *   "pending"            The update is ready to be applied.
> >-   *   "pending-service"    The update is ready to be applied with the service.
> >-   *   "applying"           The update is being applied.
> >-   *   "applied"            The update is ready to be switched to.
> >-   *   "applied-os"         The update is OS update and to be installed.
> >-   *   "applied-service"    The update is ready to be switched to with the service.
> >-   *   "succeeded"          The update was successfully applied.
> >-   *   "download-failed"    The update failed to be downloaded.
> >-   *   "failed"             The update failed to be applied.
> >+   *   "downloading"           The update is being downloaded.
> >+   *   "pending"               The update is ready to be applied.
> >+   *   "pending-service"       The update is ready to be applied with the service.
> >+   *   "pending-elevation-req" The update is ready to be applied but requires elevation.
> >+   *   "applying"              The update is being applied.
> >+   *   "applied"               The update is ready to be switched to.
> >+   *   "applied-os"            The update is OS update and to be installed.
> >+   *   "applied-service"       The update is ready to be switched to with the service.
> >+   *   "applied-elevation-req" The update is ready to be switched to but requires elevation.
> >+   *   "succeeded"             The update was successfully applied.
> >+   *   "download-failed"       The update failed to be downloaded.
> >+   *   "failed"                The update failed to be applied.
> >    */
> How about just pending-elevate and applied-elevate? If you're ok with that,
> here and elsewhere.

Done.

> > 
> >   /**
> >+   * Whether an elevation failure has been encountered for this update.
> >+   */
> >+  attribute boolean elevationFailure;
> Bump the uuid.

Done.

> >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> >--- a/toolkit/mozapps/update/nsUpdateService.js
> >+++ b/toolkit/mozapps/update/nsUpdateService.js
> >@@ -25,16 +25,18 @@ const PREF_APP_UPDATE_BACKGROUND_INTERVA
> > const PREF_APP_UPDATE_BACKGROUNDERRORS    = "app.update.backgroundErrors";
> > const PREF_APP_UPDATE_BACKGROUNDMAXERRORS = "app.update.backgroundMaxErrors";
> > const PREF_APP_UPDATE_CANCELATIONS        = "app.update.cancelations";
> > const PREF_APP_UPDATE_CERTS_BRANCH        = "app.update.certs.";
> > const PREF_APP_UPDATE_CERT_CHECKATTRS     = "app.update.cert.checkAttributes";
> > const PREF_APP_UPDATE_CERT_ERRORS         = "app.update.cert.errors";
> > const PREF_APP_UPDATE_CERT_MAXERRORS      = "app.update.cert.maxErrors";
> > const PREF_APP_UPDATE_CERT_REQUIREBUILTIN = "app.update.cert.requireBuiltIn";
> >+const PREF_APP_UPDATE_ELEVATION_NEVER     = "app.update.elevation.never";
> >+const PREF_APP_UPDATE_ELEVATION_VERSION   = "app.update.elevation.version";
> s/elevation/elevate/

Done.

> >@@ -76,27 +78,29 @@ const FILE_UPDATE_ARCHIVE = "update.mar"
> > const FILE_UPDATE_LINK    = "update.link";
> > const FILE_UPDATE_LOG     = "update.log";
> > const FILE_UPDATES_DB     = "updates.xml";
> > const FILE_UPDATE_ACTIVE  = "active-update.xml";
> > const FILE_PERMS_TEST     = "update.test";
> > const FILE_LAST_LOG       = "last-update.log";
> > const FILE_BACKUP_LOG     = "backup-update.log";
> > 
> >-const STATE_NONE            = "null";
> >-const STATE_DOWNLOADING     = "downloading";
> >-const STATE_PENDING         = "pending";
> >-const STATE_PENDING_SVC     = "pending-service";
> >-const STATE_APPLYING        = "applying";
> >-const STATE_APPLIED         = "applied";
> >-const STATE_APPLIED_OS      = "applied-os";
> >-const STATE_APPLIED_SVC     = "applied-service";
> >-const STATE_SUCCEEDED       = "succeeded";
> >-const STATE_DOWNLOAD_FAILED = "download-failed";
> >-const STATE_FAILED          = "failed";
> >+const STATE_NONE                  = "null";
> >+const STATE_DOWNLOADING           = "downloading";
> >+const STATE_PENDING               = "pending";
> >+const STATE_PENDING_SVC           = "pending-service";
> >+const STATE_PENDING_ELEVATION_REQ = "pending-elevation-req";
> >+const STATE_APPLYING              = "applying";
> >+const STATE_APPLIED               = "applied";
> >+const STATE_APPLIED_OS            = "applied-os";
> >+const STATE_APPLIED_SVC           = "applied-service";
> >+const STATE_APPLIED_ELEVATION_REQ = "applied-elevation-req";
> If you're ok with it, STATE_APPLIED_ELEVATE and STATE_PENDING_ELEVATE
> 
> Might as well change STATE_PENDING_SVC and STATE_APPLIED_SVC to
> STATE_PENDING_SERVICE and STATE_APPLIED_SERVICE

Done.

> >@@ -183,16 +187,19 @@ const DEFAULT_SERVICE_MAX_ERRORS = 10;
> > 
> > // The number of consecutive socket errors to allow before falling back to
> > // downloading a different MAR file or failing if already downloading the full.
> > const DEFAULT_SOCKET_MAX_ERRORS = 10;
> > 
> > // The number of milliseconds to wait before retrying a connection error.
> > const DEFAULT_UPDATE_RETRY_TIMEOUT = 2000;
> > 
> >+// Maximum number of elevation cancelations per update version before giving up.
> >+const DEFAULT_MAX_ELEVATION_CANCELATIONS = 5;
> Should applications and users be able to customize this with a pref as well?

I didn't think there was enough value in this to justify the maintenance burden of a pref, even if minor.

> >@@ -332,56 +339,78 @@ function hasUpdateMutex() {
> >     return true;
> >   }
> >   if (!gUpdateMutexHandle) {
> >     gUpdateMutexHandle = createMutex(getPerInstallationMutexName(true), false);
> >   }
> >   return !!gUpdateMutexHandle;
> > }
> > 
> >+/**
> >+ * OSX only function to determine if the user requires elevation to be able to
> >+ * write to the application bundle.
> >+ *
> >+ * @return true if elevation is required, false otherwise
> >+ */
> >+function getElevationRequired() {
> >+  if (AppConstants.platform != "macosx") {
> >+    return false;
> >+  }
> >+
> >+  try {
> >+    // Check that the application bundle can be written to.
> >+    let appDirTestFile = getAppBaseDir();
> >+    appDirTestFile.append(FILE_PERMS_TEST);
> >+    LOG("getElevationRequired - testing write access " + appDirTestFile.path);
> >+    if (appDirTestFile.exists()) {
> >+      appDirTestFile.remove(false);
> >+    }
> >+    appDirTestFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE,
> >+                          FileUtils.PERMS_FILE);
> >+    appDirTestFile.remove(false);
> I'm concerned that there will be cases where the user can write to the
> directory but can't modify / delete the files in the directory. Can you come
> up with a a check for that as well?

Added a recursive check to ensure that all files/directories are writable.

> >@@ -444,18 +473,23 @@ function getCanApplyUpdates() {
> >  * Whether or not the application can stage an update for the current session.
> >  * These checks are only performed once per session due to using a lazy getter.
> >  *
> >  * @return true if updates can be staged for this session.
> >  */
> > XPCOMUtils.defineLazyGetter(this, "gCanStageUpdatesSession",
> >                             function aus_gCanStageUpdatesSession() {
> >   try {
> >-    let updateTestFile = getInstallDirRoot();
> >-    updateTestFile.append(FILE_PERMS_TEST);
> >+    let updateTestFile;
> >+    if (AppConstants.platform == "macosx") {
> >+      updateTestFile = getUpdateFile([FILE_PERMS_TEST]);
> Note for myself to check that this is correct.

Fyi: this change is because we need to check that the subdirectory in ~/Library can be written to, not the root of the .app bundle.

> >diff --git a/toolkit/mozapps/update/updater/launchchild_osx.mm b/toolkit/mozapps/update/updater/launchchild_osx.mm
> >--- a/toolkit/mozapps/update/updater/launchchild_osx.mm
> >+++ b/toolkit/mozapps/update/updater/launchchild_osx.mm
> Could you get mstange to review these bits?
> 
> >diff --git a/toolkit/xre/MacLaunchHelper.mm b/toolkit/xre/MacLaunchHelper.mm
> >--- a/toolkit/xre/MacLaunchHelper.mm
> >+++ b/toolkit/xre/MacLaunchHelper.mm
> Could you get mstange to review these bits?

Split out the Objective-C changes. Will upload a separate patch once I've addressed :mstange's feedback.

Comment 160

[Stephen A Pohl [:spohl]]

Comment on attachment 8701096 [details] [diff] [review]
1. Patch to elevate updater

Discussed this with Robert via vidyo and will rework it based on our conversation. Will summarize the changes when I upload the next patch.

Comment 161

[Stephen A Pohl [:spohl]]

Attachment: elevation prompt diff

Philipp, I was hoping to get your feedback one more time. Two questions came up during code review:
1. Would it seem reasonable to reduce the number of elevation attempts from 5 to 3? This means that if a user has failed (or canceled) elevation 3 times, we would proceed by displaying manual update options instead.
2. The suggestion came up that we might also want to present a manual update link in the "elevation is required" prompt. This attachment shows the difference between what we've previously discussed[1] and this new suggestion. This is the exact string and URL that we already use if a user encounters an error during an update. Could you let me know what you think? Thanks!

[1] https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201

Comment 162

[Stephen A Pohl [:spohl]]

Attachment: 3. Build config changes

(In reply to Michael Shal [:mshal] from comment #148)
> (In reply to Stephen A Pohl [:spohl] from comment #146)
> > Michael, since you r+'d this patch can I interpret your f+ for patch 3
> > (comment 143, comment 144) as an r+ as well? If not, what changes would you
> > like to see to patch 3 (which would most likely also require changes to
> > patch 1)? Thanks in advance!
> 
> I'd still like to see if we can get Info.plist generated in the objdir
> before r+. I didn't realize it needs to be in export - that's fine, so
> ignore my suggestion to put it in libs :). But I think we need to generate
> it in the objdir, reference the objdir in the LDFLAGS, and then skip the
> removal in the libs rule. Can you show what you tried and what issue you ran
> into?

I was able to get this working. I also folded the build config changes from attachment 8683709 [details] [diff] [review] into this patch, which have been r+'d previously (after comment 147).

Comment 163

[Stephen A Pohl [:spohl]]

Attachment: 5. Update existing tests

Split out build config changes into patch 3. Carrying over r+.

Comment 164

[Stephen A Pohl [:spohl]]

Attachment: 3. Build config changes

Forgot to update for current trunk.

Comment 165

[Michael Shal [:mshal]]

Comment on attachment 8707113 [details] [diff] [review]
3. Build config changes

Looks good to me now. Thanks for fixing the srcdir issue!

Comment 166

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

This patch addresses the following:
1. In UpdateTelemetry.jsm, added “(no notification)” to the descriptions of CHK_ELEVATION_DISABLED_FOR_VERSION and CHK_ELEVATION_OPTOUT_FOR_VERSION.
2. In nsUpdateService.js:
2.1 Removed the cancelation of "never" prefs.
2.2 Dropped the name change to PREFBRANCH_APP_UPDATE_NEVER
3. In toolkit/mozapps/update/content/updates.js:
3.1 Added a comment why we also set the old “NEVER” pref in the UI code
3.2 Added a new, OSX-specific cancelation pref.
3.3 Restored the existing cancelation pref to be used for telemetry purposes only and avoided resetting it. 
4. In toolkit/mozapps/update/nsUpdateService.js:
4.1 Changed the function name of promptingForElevationPermitted to userElevationOptedIn.
4.2 Added a pref for max_cancelations for users to control the number of cancelations, or turn off the feature completely by setting it to 0.
5. If a previous update occurred as elevated user, ensured that additional updates maintain group ownership of admin and write permission on all the files in the Firefox.app bundle (by calling SetGroupOwnershipAndPermissions in updater.cpp).
6. Replaced most uses of the "update-canceled" observer notification and ensured that the remaining uses in browser.js and updates.js don't leak observers.
7. Improved the getElevationRequired check by recursively checking all files in the Firefox.app bundle for write access. This patch holds the js implementation of this functionality in nsUpdateService.js. There is a native implementation in patch 2 that I will upload shortly to support updater.cpp and toolkit/xre/nsUpdateDriver.cpp.

Comment 167

[Stephen A Pohl [:spohl]]

Attachment: 2. Obj-C changes

I split out all the Objective-C changes into this patch for easier review.

Comment 168

[Stephen A Pohl [:spohl]]

Attachment: 3.b Buildconfig changes to support patch 2

Markus, does it seem reasonable to create updaterfileutils_osx.h|.mm under /toolkit/mozapps/update/updater to be shared between /toolkit/mozapps/update/updater/ and /toolkit/xre/ in patch 2? This would require the buildconfig changes here. If so, I'll ask :mshal to review the changes here. If not, please let me know if you have a better suggestion. Thanks!

Comment 169

[Stephen A Pohl [:spohl]]

Attachment: 4. Signing

Split out the .plist changes that were r+'d by bhearsum in comment 127 from the buildconfig changes in patch 3. Carrying over r+.

Comment 170

[Stephen A Pohl [:spohl]]

Attachment: 1.b additional UX changes

This implements the additional UX changes mentioned in comment 161. Philipp, have you had a chance to review the questions there yet? Thanks!

Comment 171

[Markus Stange [:mstange]]

Comment on attachment 8707892 [details] [diff] [review]
3.b Buildconfig changes to support patch 2

I don't know whether this is a good way to do this or whether there are better ways. Please ask a build peer.
What's the code that you want to share?

Comment 172

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #171)
> What's the code that you want to share?

I'd like to have a shared |IsRecursivelyWritable| function (see updaterfileutils_osx.mm in patch 2). This function is responsible for detecting the cases when we require elevation to apply an update. The logic needs to be the same for the browser and the updater. Sharing the code would avoid the risk of future changes only being applied to the browser's or the updater's copy of the function.

Comment 173

[Stephen A Pohl [:spohl]]

Comment on attachment 8707892 [details] [diff] [review]
3.b Buildconfig changes to support patch 2

Please see comment 168, comment 171 and comment 172.

Comment 174

[Markus Stange [:mstange]]

Comment on attachment 8707891 [details] [diff] [review]
2. Obj-C changes

Review of attachment 8707891 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok, as far as I can tell. I don't have any experience with the APIs you're using here, though.

::: toolkit/mozapps/update/updater/launchchild_osx.mm
@@ +24,5 @@
> +    }
> +    NSTask* task = [[NSTask alloc] init];
> +    [task setLaunchPath:launchPath];
> +    [task setArguments:arguments];
> +    [task launch];

Do you know if this way of launching the task preserves the old behavior of choosing the same architecture as the running architecture? Or does that not matter?

@@ +127,5 @@
> +  const char* launchctlArgs[] = {"/bin/launchctl",
> +                                 "remove",
> +                                 "org.mozilla.updater"};
> +  // The following will terminate the current process.
> +  LaunchChild(3, launchctlArgs);

How does it terminate the current process?

@@ +241,5 @@
> +}
> +
> +- (void)shutdown
> +{
> +  mShouldKeepRunning = NO;

Do you need to poke the NSRunLoop here? Or will there be enough native events so that runMode:beforeDate: will return in the not too distant future?

@@ +261,5 @@
> +
> +  ElevatedUpdateServer* updater =
> +    [[ElevatedUpdateServer alloc] initWithArgs:[updaterArguments copy]];
> +  bool didSucceed =
> +    [updater runServer] == YES ? true : false;

BOOL converts to bool implicitly, the way you want it to.

@@ +311,5 @@
> +    [NSArray arrayWithObjects:[NSNumber numberWithUnsignedLong:80],
> +                              [NSNumber numberWithUnsignedLong:0775],
> +                              nil];
> +  NSDictionary* attributes = [NSDictionary dictionaryWithObjects:permObjects
> +                                                         forKeys:permKeys];

We should figure out what we need to do so that we can write this as
@{ NSFileGroupOwnerAccountID: @(80), NSFilePosixPermissions: @(0775) }.

::: toolkit/xre/MacLaunchHelper.mm
@@ +110,5 @@
> +
> +  BOOL result = NO;
> +  AuthorizationItem authItem    = { kSMRightBlessPrivilegedHelper, 0, NULL, 0 };
> +  AuthorizationRights authRights  = { 1, &authItem };
> +  AuthorizationFlags flags    = kAuthorizationFlagDefaults |

some funky indentation here - I'd just use one space in front of the equals signs, they don't line up anyway

@@ +143,5 @@
> +
> +  return result;
> +}
> +
> +void AbortElevatedUpdate()

Oh, is this where you'd like to call CleanupElevatedMacUpdate instead? I wouldn't worry about sharing that code if it's really just this function.

Comment 175

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Stephen A Pohl [:spohl] from comment #161)
> Created attachment 8703797 [details]
> elevation prompt diff
> 
> Philipp, I was hoping to get your feedback one more time. Two questions came
> up during code review:
> 1. Would it seem reasonable to reduce the number of elevation attempts from
> 5 to 3? This means that if a user has failed (or canceled) elevation 3
> times, we would proceed by displaying manual update options instead.
> 2. The suggestion came up that we might also want to present a manual update
> link in the "elevation is required" prompt. This attachment shows the
> difference between what we've previously discussed[1] and this new
> suggestion. This is the exact string and URL that we already use if a user
> encounters an error during an update. Could you let me know what you think?
> Thanks!
> 
> [1] https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201

Hey! Sorry this took a while.
Looks good to me!

Comment 176

[Michael Shal [:mshal]]

Comment on attachment 8707892 [details] [diff] [review]
3.b Buildconfig changes to support patch 2

I don't see any problems with this.

Comment 177

[Stephen A Pohl [:spohl]]

Comment on attachment 8707896 [details] [diff] [review]
1.b additional UX changes

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #175)
> (In reply to Stephen A Pohl [:spohl] from comment #161)
> > Created attachment 8703797 [details]
> > elevation prompt diff
> > 
> > Philipp, I was hoping to get your feedback one more time. Two questions came
> > up during code review:
> > 1. Would it seem reasonable to reduce the number of elevation attempts from
> > 5 to 3? This means that if a user has failed (or canceled) elevation 3
> > times, we would proceed by displaying manual update options instead.
> > 2. The suggestion came up that we might also want to present a manual update
> > link in the "elevation is required" prompt. This attachment shows the
> > difference between what we've previously discussed[1] and this new
> > suggestion. This is the exact string and URL that we already use if a user
> > encounters an error during an update. Could you let me know what you think?
> > Thanks!
> > 
> > [1] https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201
> 
> Hey! Sorry this took a while.
> Looks good to me!

Sweet, thanks!

Comment 178

[Stephen A Pohl [:spohl]]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #175)
> (In reply to Stephen A Pohl [:spohl] from comment #161)
> > Created attachment 8703797 [details]
> > elevation prompt diff
> > 
> > Philipp, I was hoping to get your feedback one more time. Two questions came
> > up during code review:
> > 1. Would it seem reasonable to reduce the number of elevation attempts from
> > 5 to 3? This means that if a user has failed (or canceled) elevation 3
> > times, we would proceed by displaying manual update options instead.
> > 2. The suggestion came up that we might also want to present a manual update
> > link in the "elevation is required" prompt. This attachment shows the
> > difference between what we've previously discussed[1] and this new
> > suggestion. This is the exact string and URL that we already use if a user
> > encounters an error during an update. Could you let me know what you think?
> > Thanks!
> > 
> > [1] https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201
> 
> Hey! Sorry this took a while.
> Looks good to me!

I've updated the wiki page with the final UX changes:
https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201

Comment 179

[Stephen A Pohl [:spohl]]

Attachment: 3. Build config changes

Folded patch 3 and patch 3.b, which were reviewed at separate times. Carrying over r+.

Comment 180

[Stephen A Pohl [:spohl]]

Attachment: 2. Obj-C changes

Thanks, Markus! When I went through your feedback and retested this patch, I noticed that the recursive write check (|IsRecursivelyWritable| in updaterfileutils_osx.mm) failed to check the root directory for write permission. I'm sending this back for your review to get your eyes on this change and to make sure I've addressed your feedback appropriately (see comments inline below).

(In reply to Markus Stange [:mstange] from comment #174)
> ::: toolkit/mozapps/update/updater/launchchild_osx.mm
> @@ +24,5 @@
> > +    }
> > +    NSTask* task = [[NSTask alloc] init];
> > +    [task setLaunchPath:launchPath];
> > +    [task setArguments:arguments];
> > +    [task launch];
> 
> Do you know if this way of launching the task preserves the old behavior of
> choosing the same architecture as the running architecture? Or does that not
> matter?

My understanding is that the previous code was introduced in bug 600362 to make sure that updates using the i386 architecture don't try to launch our x86_64 binaries on OSX 10.5 because they weren't compatible with 10.5 at the time. We've dropped support for 10.5 and our x86_64 binaries appear to be compatible with OSX 10.6 and above now. I felt comfortable replacing this with the NSTask approach at this point. It also appears that even though NSTask was introduced in OSX 10.0[1], it didn't appear to be discussed as an option in bug 600362.

However, after having another look at this I've changed the implementation slightly to call NSTask's |+ launchedTaskWithLaunchPath:arguments:| directly. This avoids the alloc/init and release.

> @@ +127,5 @@
> > +  const char* launchctlArgs[] = {"/bin/launchctl",
> > +                                 "remove",
> > +                                 "org.mozilla.updater"};
> > +  // The following will terminate the current process.
> > +  LaunchChild(3, launchctlArgs);
> 
> How does it terminate the current process?

The current process here is the org.mozilla.updater daemon (i.e. the elevated updater). It calls launchctl with the "remove" argument to remove itself from the list of daemons and stop the process. I couldn't find documentation that guarantees that the "remove" argument will stop the process, but I verified this behavior experimentally back when I first implemented it and again just now. Since this is the last thing to be called before the elevated updater exits, there wouldn't be any negative consequences if removing the daemon didn't stop it too.

> @@ +241,5 @@
> > +}
> > +
> > +- (void)shutdown
> > +{
> > +  mShouldKeepRunning = NO;
> 
> Do you need to poke the NSRunLoop here? Or will there be enough native
> events so that runMode:beforeDate: will return in the not too distant future?

I've followed the example in [2] almost to a t, which makes me believe that we don't need to poke the NSRunLoop here. I haven't encountered any issues in my testing.

> 
> @@ +261,5 @@
> > +
> > +  ElevatedUpdateServer* updater =
> > +    [[ElevatedUpdateServer alloc] initWithArgs:[updaterArguments copy]];
> > +  bool didSucceed =
> > +    [updater runServer] == YES ? true : false;
> 
> BOOL converts to bool implicitly, the way you want it to.

Fixed.

> @@ +311,5 @@
> > +    [NSArray arrayWithObjects:[NSNumber numberWithUnsignedLong:80],
> > +                              [NSNumber numberWithUnsignedLong:0775],
> > +                              nil];
> > +  NSDictionary* attributes = [NSDictionary dictionaryWithObjects:permObjects
> > +                                                         forKeys:permKeys];
> 
> We should figure out what we need to do so that we can write this as
> @{ NSFileGroupOwnerAccountID: @(80), NSFilePosixPermissions: @(0775) }.

Fixed.

> ::: toolkit/xre/MacLaunchHelper.mm
> @@ +110,5 @@
> > +
> > +  BOOL result = NO;
> > +  AuthorizationItem authItem    = { kSMRightBlessPrivilegedHelper, 0, NULL, 0 };
> > +  AuthorizationRights authRights  = { 1, &authItem };
> > +  AuthorizationFlags flags    = kAuthorizationFlagDefaults |
> 
> some funky indentation here - I'd just use one space in front of the equals
> signs, they don't line up anyway

Fixed.

> @@ +143,5 @@
> > +
> > +  return result;
> > +}
> > +
> > +void AbortElevatedUpdate()
> 
> Oh, is this where you'd like to call CleanupElevatedMacUpdate instead? I
> wouldn't worry about sharing that code if it's really just this function.

I'm not sure I understand the reference to "sharing code" here. |AbortElevatedUpdate| is called if we were unable to install the privileged helper tool. If we were unable to install it, there shouldn't be an elevated updater to clean up and thus we don't call |CleanupElevatedMacUpdate|.


[1] https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSTask_Class/index.html
[2] https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSRunLoop_Class/index.html#//apple_ref/occ/instm/NSRunLoop/run

Comment 181

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Changed |getElevationRequired| in nsUpdateService.js to also check for write permission on the root of the install directory, not just its descendants.

Comment 182

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

So far I believe we've been able to keep all dependencies that are shared between xre and updater live under xre. With this in mind, I "think" that updaterfileutils_osx.h should either live under xre or be incorporated into MacLaunchHelper.

Comment 183

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

Comment on attachment 8712792 [details] [diff] [review]
1. Patch to elevate updater

The changes I am suggesting for the updater.cpp code is significant so I am submitting this partial review since if they make sense it will change the code significantly.

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>...
>@@ -68,18 +69,24 @@
> // Use a large value for debug builds since the xpcshell tests take a long time.
> #define PARENT_WAIT 30000
> #else
> #define PARENT_WAIT 10000
> #endif
> 
> #if defined(XP_MACOSX)
> // These functions are defined in launchchild_osx.mm
>-void LaunchChild(int argc, char **argv);
>+void LaunchChild(int argc, const char** argv);
> void LaunchMacPostProcess(const char* aAppBundle);
>+void CleanupElevatedMacUpdate(bool aFailureOccurred);
>+bool IsRecursivelyWritable(const char* aPath);
>+bool ObtainUpdaterArguments(int* argc, char*** argv);
>+bool ServeElevatedUpdate(int argc, const char** argv);
>+bool IsOwnedByGroupAdmin(const char* aAppBundle);
>+void SetGroupOwnershipAndPermissions(const char* aAppBundle);
nit: would be nice to declare these in alphabetical order

>@@ -295,16 +302,17 @@ static NS_tchar* gPatchDirPath;
> static NS_tchar gInstallDirPath[MAXPATHLEN];
> static NS_tchar gWorkingDirPath[MAXPATHLEN];
> static ArchiveReader gArchiveReader;
> static bool gSucceeded = false;
> static bool sStagedUpdate = false;
> static bool sReplaceRequest = false;
> static bool sUsingService = false;
> static bool sIsOSUpdate = false;
>+static bool sRequiresElevation = false;
> 
> #ifdef XP_WIN
> // The current working directory specified in the command line.
> static NS_tchar* gDestPath;
> static NS_tchar gCallbackRelPath[MAXPATHLEN];
> static NS_tchar gCallbackBackupPath[MAXPATHLEN];
> #endif
> 
>@@ -2023,17 +2031,21 @@ WriteStatusFile(const char* aStatus)
> static void
> WriteStatusFile(int status)
> {
>   const char *text;
> 
>   char buf[32];
>   if (status == OK) {
>     if (sStagedUpdate) {
>-      text = "applied\n";
>+      if (sRequiresElevation) {
>+        text = "applied-elevate\n";
>+      } else {
>+        text = "applied\n";
>+      }
We currently handle this on Windows in refreshUpdateStatus in nsUpdateService.js

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3244

Any reason not to do the same on Mac so the complexity is kept in the JavaScript code? I "think" you might be able to get rid of sRequiresElevation by doing this (see suggested change below) since it is only checked in one other place.

>     } else {
>       text = "succeeded\n";
>     }
>   } else {
>     snprintf(buf, sizeof(buf)/sizeof(buf[0]), "failed: %d\n", status);
>     text = buf;
>   }
> 
>@@ -2517,85 +2544,16 @@ int NS_main(int argc, NS_tchar **argv)
>   if (NSS_NoDB_Init(NULL) != SECSuccess) {
>    PRErrorCode error = PR_GetError();
>    fprintf(stderr, "Could not initialize NSS: %s (%d)",
>            PR_ErrorToName(error), (int) error);
>     _exit(1);
>   }
> #endif
> 
>-  InitProgressUI(&argc, &argv);
>-
>-  // To process an update the updater command line must at a minimum have the
>-  // directory path containing the updater.mar file to process as the first
>-  // argument, the install directory as the second argument, and the directory
>-  // to apply the update to as the third argument. When the updater is launched
>-  // by another process the PID of the parent process should be provided in the
>-  // optional fourth argument and the updater will wait on the parent process to
>-  // exit if the value is non-zero and the process is present. This is necessary
>-  // due to not being able to update files that are in use on Windows. The
>-  // optional fifth argument is the callback's working directory and the
>-  // optional sixth argument is the callback path. The callback is the
>-  // application to launch after updating and it will be launched when these
>-  // arguments are provided whether the update was successful or not. All
>-  // remaining arguments are optional and are passed to the callback when it is
>-  // launched.
>-  if (argc < 4) {
>-    fprintf(stderr, "Usage: updater patch-dir install-dir apply-to-dir [wait-pid [callback-working-dir callback-path args...]]\n");
>-    return 1;
>-  }
Is it really necessary to move the "if (argc < 4) {" block of code? It seems like you could just keep it where it was and not add the isElevated check etc where you added it further below.

>-
>-  // The directory containing the update information.
>-  gPatchDirPath = argv[1];
>-  // The directory we're going to update to.
>-  // We copy this string because we need to remove trailing slashes.  The C++
>-  // standard says that it's always safe to write to strings pointed to by argv
>-  // elements, but I don't necessarily believe it.
>-  NS_tstrncpy(gInstallDirPath, argv[2], MAXPATHLEN);
>-  gInstallDirPath[MAXPATHLEN - 1] = NS_T('\0');
>-  NS_tchar *slash = NS_tstrrchr(gInstallDirPath, NS_SLASH);
>-  if (slash && !slash[1]) {
>-    *slash = NS_T('\0');
>-  }
>-
>-#ifdef XP_WIN
>-  bool useService = false;
>-  bool testOnlyFallbackKeyExists = false;
>-  bool noServiceFallback = EnvHasValue("MOZ_NO_SERVICE_FALLBACK");
>-  putenv(const_cast<char*>("MOZ_NO_SERVICE_FALLBACK="));
>-
>-  // We never want the service to be used unless we build with
>-  // the maintenance service.
>-#ifdef MOZ_MAINTENANCE_SERVICE
>-  useService = IsUpdateStatusPendingService();
>-  // Our tests run with a different apply directory for each test.
>-  // We use this registry key on our test slaves to store the
>-  // allowed name/issuers.
>-  testOnlyFallbackKeyExists = DoesFallbackKeyExist();
>-#endif
>-
>-  // Remove everything except close window from the context menu
>-  {
>-    HKEY hkApp = nullptr;
>-    RegCreateKeyExW(HKEY_CURRENT_USER, L"Software\\Classes\\Applications",
>-                    0, nullptr, REG_OPTION_NON_VOLATILE, KEY_SET_VALUE, nullptr,
>-                    &hkApp, nullptr);
>-    RegCloseKey(hkApp);
>-    if (RegCreateKeyExW(HKEY_CURRENT_USER,
>-                        L"Software\\Classes\\Applications\\updater.exe",
>-                        0, nullptr, REG_OPTION_VOLATILE, KEY_SET_VALUE, nullptr,
>-                        &hkApp, nullptr) == ERROR_SUCCESS) {
>-      RegSetValueExW(hkApp, L"IsHostApp", 0, REG_NONE, 0, 0);
>-      RegSetValueExW(hkApp, L"NoOpenWith", 0, REG_NONE, 0, 0);
>-      RegSetValueExW(hkApp, L"NoStartPage", 0, REG_NONE, 0, 0);
>-      RegCloseKey(hkApp);
>-    }
>-  }
>-#endif
Is it really necessary to move the "#ifdef XP_WIN" block of code?


>@@ -2609,16 +2567,109 @@ int NS_main(int argc, NS_tchar **argv)
>       sStagedUpdate = true;
>     } else if (NS_tstrstr(argv[4], NS_T("/replace"))) {
>       // We're processing a request to replace the application with a staged
>       // update.
>       sReplaceRequest = true;
>     }
>   }
> 
>+#if defined(XP_MACOSX)
>+  bool isElevated =
>+    strstr(argv[0], "/Library/PrivilegedHelperTools/org.mozilla.updater") != 0;
>+  if (isElevated) {
>+    if (!ObtainUpdaterArguments(&argc, &argv)) {
>+      // Won't actually get here because ObtainUpdaterArguments will terminate
>+      // the current process on failure.
>+      return 1;
>+    }
>+  } else {
>+    sRequiresElevation = !IsRecursivelyWritable(argv[2]);
>+  }
>+#endif
>+
>+  // The directory containing the update information.
>+  gPatchDirPath = argv[1];
>+
>+#ifdef XP_MACOSX
>+  if (!sRequiresElevation || sStagedUpdate) {
>+#endif
I'd really like to get rid of the ifdef above and the corresponding one way down below. I'm fine with the 

How about something along the lines of this?

  bool runNormalUpdate = true;
#ifdef XP_MACOSX
  if (!sStagedUpdate && !IsRecursivelyWritable(argv[2])) {
    gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
    if (!gSucceeded) {
      WriteStatusFile(ELEVATION_CANCELED);
    }
    runNormalUpdate = false;
  }
#endif

  if (runNormalUpdate) {
    etc.

>+  InitProgressUI(&argc, &argv);
>+


>diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp
>--- a/toolkit/xre/nsUpdateDriver.cpp
>+++ b/toolkit/xre/nsUpdateDriver.cpp
>...
>@@ -645,21 +658,32 @@ SwitchToUpdatedApp(nsIFile *greDir, nsIF
> #elif defined(XP_WIN)
>   // Switch the application using updater.exe
>   if (!WinLaunchChild(updaterPathW.get(), argc, argv)) {
>     return;
>   }
>   _exit(0);
> #elif defined(XP_MACOSX)
>   CommandLineServiceMac::SetupMacCommandLine(argc, argv, true);
>-  // LaunchChildMac uses posix_spawnp and prefers the current
>-  // architecture when launching. It doesn't require a
>-  // null-terminated string but it doesn't matter if we pass one.
>-  LaunchChildMac(argc, argv);
>-  exit(0);
>+  // We need to detect whether elevation is required for this update. This can
>+  // occur when a first admin user installs the application, but another admin
>+  // user attempts to update (see bug 394984).
>+  if (!IsRecursivelyWritable(installDirPath.get())) {
>+    if (!LaunchElevatedUpdate(argc, argv, 0, 0)) {
>+      LOG(("Failed to launch elevated update!"));
>+      exit(1);
It seems as if you wouldn't want to exit when the updater isn't successfully launched. What is the user experience after the LaunchElevatedUpdate failure?

>+    }
>+    exit(0);
>+  } else {
>+    // LaunchChildMac uses posix_spawnp and prefers the current
>+    // architecture when launching. It doesn't require a
>+    // null-terminated string but it doesn't matter if we pass one.
>+    LaunchChildMac(argc, argv);
>+    exit(0);
>+  }
> #else
>   PR_CreateProcessDetached(updaterPath.get(), argv, nullptr, nullptr);
>   exit(0);
> #endif
> }
> 
> #if defined(MOZ_WIDGET_GONK)
> static nsresult
>@@ -947,23 +971,31 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
>     return;
>   }
> 
>   if (restart) {
>     // We are going to process an update so we should exit now
>     _exit(0);
>   }
> #elif defined(XP_MACOSX)
>-  CommandLineServiceMac::SetupMacCommandLine(argc, argv, true);
>-  // LaunchChildMac uses posix_spawnp and prefers the current
>-  // architecture when launching. It doesn't require a
>-  // null-terminated string but it doesn't matter if we pass one.
>-  LaunchChildMac(argc, argv, 0, outpid);
>-  if (restart) {
>+  CommandLineServiceMac::SetupMacCommandLine(argc, argv, restart);
>+  // We need to detect whether elevation is required for this update. This can
>+  // occur when a first admin user installs the application, but another admin
nit: s/occur when a first admin/occur when a admin/

Comment 184

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #183)
> I'd really like to get rid of the ifdef above and the corresponding one way
> down below. I'm fine with the code block being below
the normal update path if it can be done in a clean way similar to this.

Comment 185

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #183)
> How about something along the lines of this?
> 
>   bool runNormalUpdate = true;
> #ifdef XP_MACOSX
>   if (!sStagedUpdate && !IsRecursivelyWritable(argv[2])) {
>     gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
>     if (!gSucceeded) {
>       WriteStatusFile(ELEVATION_CANCELED);
>     }
>     runNormalUpdate = false;
>   }
> #endif
I wouldn't be against (actually, I think I'm for it) putting the code required to finish the update for the unelevated updater that requires elevation in this block if it is simple enough instead of having the
"if (runNormalUpdate) {" block.

Comment 186

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

I'm somewhat concerned about the staged update case where files can be replaced after staging is complete. I think the security team needs to understand the ramifications of this though with making it so an admin user can just replace files in the app dir will also end up with the same end result.

Comment 187

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #182)
> So far I believe we've been able to keep all dependencies that are shared
> between xre and updater live under xre. With this in mind, I "think" that
> updaterfileutils_osx.h should either live under xre or be incorporated into
> MacLaunchHelper.

There are currently no shared dependencies between xre and the updater as far as I can see. If there are, could you help me figure out which ones those are? Incorporating this into MacLaunchHelper would require that we include a bunch of additional (and unnecessary) code for the updater build, which I was trying to avoid. If, despite the aforementioned, you'd like me to move updaterfileutils_osx.h to xre, just let me know and I'll be happy to do that.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #183)
>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
> >--- a/toolkit/mozapps/update/updater/updater.cpp
> >+++ b/toolkit/mozapps/update/updater/updater.cpp
> >...
> > #if defined(XP_MACOSX)
> > // These functions are defined in launchchild_osx.mm
> >-void LaunchChild(int argc, char **argv);
> >+void LaunchChild(int argc, const char** argv);
> > void LaunchMacPostProcess(const char* aAppBundle);
> >+void CleanupElevatedMacUpdate(bool aFailureOccurred);
> >+bool IsRecursivelyWritable(const char* aPath);
> >+bool ObtainUpdaterArguments(int* argc, char*** argv);
> >+bool ServeElevatedUpdate(int argc, const char** argv);
> >+bool IsOwnedByGroupAdmin(const char* aAppBundle);
> >+void SetGroupOwnershipAndPermissions(const char* aAppBundle);
> nit: would be nice to declare these in alphabetical order

Done.

> >@@ -2023,17 +2031,21 @@ WriteStatusFile(const char* aStatus)
> > static void
> > WriteStatusFile(int status)
> > {
> >   const char *text;
> > 
> >   char buf[32];
> >   if (status == OK) {
> >     if (sStagedUpdate) {
> >-      text = "applied\n";
> >+      if (sRequiresElevation) {
> >+        text = "applied-elevate\n";
> >+      } else {
> >+        text = "applied\n";
> >+      }
> We currently handle this on Windows in refreshUpdateStatus in
> nsUpdateService.js
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> nsUpdateService.js#3244
> 
> Any reason not to do the same on Mac so the complexity is kept in the
> JavaScript code? I "think" you might be able to get rid of
> sRequiresElevation by doing this (see suggested change below) since it is
> only checked in one other place.

The UX workflow requires that we display a notification if elevation is necessary prior to displaying the elevation dialog. Since it is the updater that is applying the update in the background, the updater has to write a status that indicates to Firefox that an elevation notification is necessary. When the status is "applied-elevate", Firefox will display the update notification. If the user accepts to proceed with an elevated update, Firefox changes the status to "applied" and the staged update can proceed. If the updater wasn't writing the status of "applied-elevate", Firefox would be unable to differentiate between the two states.

I am, however, considering dropping support for elevated, staged updates for security reasons, which would make this moot. See discussion below.

> >-  InitProgressUI(&argc, &argv);
> >-
> >-  // To process an update the updater command line must at a minimum have the
> >-  // directory path containing the updater.mar file to process as the first
> >-  // argument, the install directory as the second argument, and the directory
> >-  // to apply the update to as the third argument. When the updater is launched
> >-  // by another process the PID of the parent process should be provided in the
> >-  // optional fourth argument and the updater will wait on the parent process to
> >-  // exit if the value is non-zero and the process is present. This is necessary
> >-  // due to not being able to update files that are in use on Windows. The
> >-  // optional fifth argument is the callback's working directory and the
> >-  // optional sixth argument is the callback path. The callback is the
> >-  // application to launch after updating and it will be launched when these
> >-  // arguments are provided whether the update was successful or not. All
> >-  // remaining arguments are optional and are passed to the callback when it is
> >-  // launched.
> >-  if (argc < 4) {
> >-    fprintf(stderr, "Usage: updater patch-dir install-dir apply-to-dir [wait-pid [callback-working-dir callback-path args...]]\n");
> >-    return 1;
> >-  }
> Is it really necessary to move the "if (argc < 4) {" block of code? It seems
> like you could just keep it where it was and not add the isElevated check
> etc where you added it further below.

Yes, this block of code needs to move. There are three states that need to be handled by the updater:
1. The update can be performed in unelevated fashion.
2. The updater is run in unelevated fashion, but elevation is required and the unelevated updater will have to communicate with the elevated updater via IPC.
3. A second updater is launched as an elevated process.

Depending on the state, the updater has to execute different code paths here. This block of code had to move because:
A. This code needs to run in both the elevated (3) and unelevated case where elevation is not required (1), but this block of code should not run when elevation is required but the updater is unelevated (2). Also, additional code needs to run to clean up when the updater is run as elevated process (3).
B. Before moving this block, two separate blocks had to satisfy the conditions in A. Moving this block made it so that we only have to check for these conditions once.
C. |gPatchDirPath| always needs to be defined, regardless whether the update is elevated (3), unelevated (1) or unelevated with elevation required (2).

> >-#ifdef XP_WIN
> >-  bool useService = false;
> >-  bool testOnlyFallbackKeyExists = false;
> >-  bool noServiceFallback = EnvHasValue("MOZ_NO_SERVICE_FALLBACK");
> >-  putenv(const_cast<char*>("MOZ_NO_SERVICE_FALLBACK="));
> >-
> >-  // We never want the service to be used unless we build with
> >-  // the maintenance service.
> >-#ifdef MOZ_MAINTENANCE_SERVICE
> >-  useService = IsUpdateStatusPendingService();
> >-  // Our tests run with a different apply directory for each test.
> >-  // We use this registry key on our test slaves to store the
> >-  // allowed name/issuers.
> >-  testOnlyFallbackKeyExists = DoesFallbackKeyExist();
> >-#endif
> >-
> >-  // Remove everything except close window from the context menu
> >-  {
> >-    HKEY hkApp = nullptr;
> >-    RegCreateKeyExW(HKEY_CURRENT_USER, L"Software\\Classes\\Applications",
> >-                    0, nullptr, REG_OPTION_NON_VOLATILE, KEY_SET_VALUE, nullptr,
> >-                    &hkApp, nullptr);
> >-    RegCloseKey(hkApp);
> >-    if (RegCreateKeyExW(HKEY_CURRENT_USER,
> >-                        L"Software\\Classes\\Applications\\updater.exe",
> >-                        0, nullptr, REG_OPTION_VOLATILE, KEY_SET_VALUE, nullptr,
> >-                        &hkApp, nullptr) == ERROR_SUCCESS) {
> >-      RegSetValueExW(hkApp, L"IsHostApp", 0, REG_NONE, 0, 0);
> >-      RegSetValueExW(hkApp, L"NoOpenWith", 0, REG_NONE, 0, 0);
> >-      RegSetValueExW(hkApp, L"NoStartPage", 0, REG_NONE, 0, 0);
> >-      RegCloseKey(hkApp);
> >-    }
> >-  }
> >-#endif
> Is it really necessary to move the "#ifdef XP_WIN" block of code?

No, this code didn't have to move. Reverted.

> >diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp
> >--- a/toolkit/xre/nsUpdateDriver.cpp
> >+++ b/toolkit/xre/nsUpdateDriver.cpp
> >...
> >@@ -645,21 +658,32 @@ SwitchToUpdatedApp(nsIFile *greDir, nsIF
> > #elif defined(XP_WIN)
> >   // Switch the application using updater.exe
> >   if (!WinLaunchChild(updaterPathW.get(), argc, argv)) {
> >     return;
> >   }
> >   _exit(0);
> > #elif defined(XP_MACOSX)
> >   CommandLineServiceMac::SetupMacCommandLine(argc, argv, true);
> >-  // LaunchChildMac uses posix_spawnp and prefers the current
> >-  // architecture when launching. It doesn't require a
> >-  // null-terminated string but it doesn't matter if we pass one.
> >-  LaunchChildMac(argc, argv);
> >-  exit(0);
> >+  // We need to detect whether elevation is required for this update. This can
> >+  // occur when a first admin user installs the application, but another admin
> >+  // user attempts to update (see bug 394984).
> >+  if (!IsRecursivelyWritable(installDirPath.get())) {
> >+    if (!LaunchElevatedUpdate(argc, argv, 0, 0)) {
> >+      LOG(("Failed to launch elevated update!"));
> >+      exit(1);
> It seems as if you wouldn't want to exit when the updater isn't successfully
> launched. What is the user experience after the LaunchElevatedUpdate failure?

Firefox restarts and displays an "elevation failed" message (see https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201).

> >@@ -947,23 +971,31 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
> > [...]
> >+  CommandLineServiceMac::SetupMacCommandLine(argc, argv, restart);
> >+  // We need to detect whether elevation is required for this update. This can
> >+  // occur when a first admin user installs the application, but another admin
> nit: s/occur when a first admin/occur when a admin/

Fixed.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #184)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #183)
> > I'd really like to get rid of the ifdef above and the corresponding one way
> > down below. I'm fine with the code block being below
> the normal update path if it can be done in a clean way similar to this.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #185)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #183)
> > How about something along the lines of this?
> > 
> >   bool runNormalUpdate = true;
> > #ifdef XP_MACOSX
> >   if (!sStagedUpdate && !IsRecursivelyWritable(argv[2])) {
> >     gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
> >     if (!gSucceeded) {
> >       WriteStatusFile(ELEVATION_CANCELED);
> >     }
> >     runNormalUpdate = false;
> >   }
> > #endif
> I wouldn't be against (actually, I think I'm for it) putting the code
> required to finish the update for the unelevated updater that requires
> elevation in this block if it is simple enough instead of having the
> "if (runNormalUpdate) {" block.

I moved the block to serve the elevated update up. Let me know if this makes it more readable.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #186)
> I'm somewhat concerned about the staged update case where files can be
> replaced after staging is complete. I think the security team needs to
> understand the ramifications of this though with making it so an admin user
> can just replace files in the app dir will also end up with the same end
> result.

You made me think about this one more time and I'm not sure anymore that we should support staged updates at this point. After MAR verification, there is no additional check to ensure that a staged update actually contains the proper files from the MAR rather than potentially harmful files. If you're okay with it, I'll make it so that we fall back to non-staged updates when elevation is required.

Comment 188

[Markus Stange [:mstange]]

Comment on attachment 8712784 [details] [diff] [review]
2. Obj-C changes

Review of attachment 8712784 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, somehow this fell through the cracks.

(In reply to Stephen A Pohl [:spohl] from comment #180)
> Thanks, Markus! When I went through your feedback and retested this patch, I
> noticed that the recursive write check (|IsRecursivelyWritable| in
> updaterfileutils_osx.mm) failed to check the root directory for write
> permission. I'm sending this back for your review to get your eyes on this
> change

Looks good.

> and to make sure I've addressed your feedback appropriately (see
> comments inline below).
> 
> (In reply to Markus Stange [:mstange] from comment #174)
> > ::: toolkit/mozapps/update/updater/launchchild_osx.mm
> > @@ +24,5 @@
> > > +    }
> > > +    NSTask* task = [[NSTask alloc] init];
> > > +    [task setLaunchPath:launchPath];
> > > +    [task setArguments:arguments];
> > > +    [task launch];
> > 
> > Do you know if this way of launching the task preserves the old behavior of
> > choosing the same architecture as the running architecture? Or does that not
> > matter?
> 
> My understanding is that the previous code was introduced in bug 600362 to
> make sure that updates using the i386 architecture don't try to launch our
> x86_64 binaries on OSX 10.5 because they weren't compatible with 10.5 at the
> time. We've dropped support for 10.5 and our x86_64 binaries appear to be
> compatible with OSX 10.6 and above now. I felt comfortable replacing this
> with the NSTask approach at this point. It also appears that even though
> NSTask was introduced in OSX 10.0[1], it didn't appear to be discussed as an
> option in bug 600362.
> 
> However, after having another look at this I've changed the implementation
> slightly to call NSTask's |+ launchedTaskWithLaunchPath:arguments:|
> directly. This avoids the alloc/init and release.

OK, sure.

> > @@ +127,5 @@
> > > +  const char* launchctlArgs[] = {"/bin/launchctl",
> > > +                                 "remove",
> > > +                                 "org.mozilla.updater"};
> > > +  // The following will terminate the current process.
> > > +  LaunchChild(3, launchctlArgs);
> > 
> > How does it terminate the current process?
> 
> The current process here is the org.mozilla.updater daemon (i.e. the
> elevated updater). It calls launchctl with the "remove" argument to remove
> itself from the list of daemons and stop the process. I couldn't find
> documentation that guarantees that the "remove" argument will stop the
> process, but I verified this behavior experimentally back when I first
> implemented it and again just now. Since this is the last thing to be called
> before the elevated updater exits, there wouldn't be any negative
> consequences if removing the daemon didn't stop it too.

OK. Mind adding a comment that the "remove" argument is what causes the termination?

> > @@ +241,5 @@
> > > +}
> > > +
> > > +- (void)shutdown
> > > +{
> > > +  mShouldKeepRunning = NO;
> > 
> > Do you need to poke the NSRunLoop here? Or will there be enough native
> > events so that runMode:beforeDate: will return in the not too distant future?
> 
> I've followed the example in [2] almost to a t, which makes me believe that
> we don't need to poke the NSRunLoop here. I haven't encountered any issues
> in my testing.

It's not clear whether this example guarantees that setting shouldKeepRunning to NO will exit the loop soon. But I think the answer is: If there are input sources attached to the runloop, then one of these sources will probably generate an event soon that will cause runMode:beforeDate: to return. And if no source is attached, runMode:beforeDate: will exit right away and return NO, which also stops the loop. So this should be fine.

> > @@ +311,5 @@
> > > +    [NSArray arrayWithObjects:[NSNumber numberWithUnsignedLong:80],
> > > +                              [NSNumber numberWithUnsignedLong:0775],
> > > +                              nil];
> > > +  NSDictionary* attributes = [NSDictionary dictionaryWithObjects:permObjects
> > > +                                                         forKeys:permKeys];
> > 
> > We should figure out what we need to do so that we can write this as
> > @{ NSFileGroupOwnerAccountID: @(80), NSFilePosixPermissions: @(0775) }.
> 
> Fixed.

Oh, when I wrote that I wasn't actually sure whether we can write it like that, but I guess we'll find out if we can't (due to having to support old compilers, for example).

> > @@ +143,5 @@
> > > +
> > > +  return result;
> > > +}
> > > +
> > > +void AbortElevatedUpdate()
> > 
> > Oh, is this where you'd like to call CleanupElevatedMacUpdate instead? I
> > wouldn't worry about sharing that code if it's really just this function.
> 
> I'm not sure I understand the reference to "sharing code" here.
> |AbortElevatedUpdate| is called if we were unable to install the privileged
> helper tool. If we were unable to install it, there shouldn't be an elevated
> updater to clean up and thus we don't call |CleanupElevatedMacUpdate|.

Never mind. The function you're sharing is IsRecursivelyWritable, not CleanupElevatedMacUpdate. You can ignore this comment.

Comment 189

[Stephen A Pohl [:spohl]]

Attachment: 2. Obj-C changes

Thanks, Markus! This patch clarifies the comment that the "remove" argument will terminate the current process when passed to /bin/launchctl.

Comment 190

[Stephen A Pohl [:spohl]]

Comment on attachment 8719881 [details] [diff] [review]
1. Patch to elevate updater

I discussed this patch with Robert via email. We're going to turn off staging when elevation is required. I'm running a new patch through its paces before uploading.

Comment 191

[Stephen A Pohl [:spohl]]

Comment on attachment 8707896 [details] [diff] [review]
1.b additional UX changes

I'll fold this patch into the new patch "1".

Comment 192

[Stephen A Pohl [:spohl]]

Attachment: 2. Obj-C changes

Robert pointed out that the best place for updaterfileutils_osx.h|.mm is under toolkit/xre instead of toolkit/mozapps/update/updater. This patch moves these two files to the new location. Carrying over r+.

Comment 193

[Stephen A Pohl [:spohl]]

Attachment: 3. Build config changes

Michael, the configure.in file moved to old-configure.in and a comment was added that changes should get input from a build peer. Is it still okay to make these suggested changes here? Or do I need to figure out how to add this to the new configure.in?

Comment 194

[Michael Shal [:mshal]]

Comment on attachment 8727570 [details] [diff] [review]
3. Build config changes

>diff --git a/old-configure.in b/old-configure.in
>-    TK_LIBS='-Wl,-framework,CoreLocation -Wl,-framework,QuartzCore -Wl,-framework,Carbon -Wl,-framework,CoreAudio -Wl,-framework,AudioToolbox -Wl,-framework,AudioUnit -Wl,-framework,AddressBook -Wl,-framework,OpenGL'
>+    TK_LIBS='-Wl,-framework,CoreLocation -Wl,-framework,QuartzCore -Wl,-framework,Carbon -Wl,-framework,CoreAudio -Wl,-framework,AudioToolbox -Wl,-framework,AudioUnit -Wl,-framework,AddressBook -Wl,-framework,OpenGL -Wl,-framework,Security -Wl,-framework,ServiceManagement'

Adding these flags to old-configure.in is fine for now. We're just starting to move things out of old-configure.in, and when that happens new configure checks will need to go into the new system. But this is just changing an existing set of flags, so it doesn't make sense to convert everything just to add the new ones.

Comment 195

[Stephen A Pohl [:spohl]]

Attachment: 3. Build config changes

Thanks, Michael!

As mentioned in comment 192, we decided to move updaterfileutils_osx.h|mm to toolkit/xre. There was a mistake in the previous update to this patch, which resulted in build failures. This patch fixes this. Carrying over r+.

Comment 196

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Removed the ability to stage an update when elevation is required. We now fall back to non-staged updates whenever elevation is required. Also fixed an issue where the update.status file would be incorrectly set to "pending" when it should have been "pending-elevate" if a user checked for an update via the about dialog. This ensures that a dialog is shown to the user that elevation will be required before prompting for elevation.

Comment 197

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

FYI: hoping to have this done today.

Comment 198

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

Just about done but I'm going to spend a little more time on the updater.cpp changes before finishing the review. So far everything is either a nit or can be done in a followup bug.

Comment 199

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

From irc convo today (includes correction
>@@ -2609,16 +2598,81 @@ int NS_main(int argc, NS_tchar **argv)
>       sStagedUpdate = true;
>     } else if (NS_tstrstr(argv[4], NS_T("/replace"))) {
>       // We're processing a request to replace the application with a staged
>       // update.
>       sReplaceRequest = true;
>     }
>   }
> 
>+#if defined(XP_MACOSX)
>+  bool isElevated =
>+    strstr(argv[0], "/Library/PrivilegedHelperTools/org.mozilla.updater") != 0;
>+  if (isElevated) {
>+    if (!ObtainUpdaterArguments(&argc, &argv)) {
>+      // Won't actually get here because ObtainUpdaterArguments will terminate
>+      // the current process on failure.
>+      return 1;
>+    }
>+  } else {
>+    sRequiresElevation = !IsRecursivelyWritable(argv[2]);
>+  }
>+#endif
>+
>+  // The directory containing the update information.
>+  gPatchDirPath = argv[1];
>+
>+#ifdef XP_MACOSX
>+  if (sRequiresElevation) {
>+    gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
>+    if (!gSucceeded) {
>+      WriteStatusFile(ELEVATION_CANCELED);
>+    }
Add   return gSucceeded ? 0 : 1;
Add }

Remove >+  } else {
Remove >+    // start of "elevation-not-required" update block
>+#endif
>+
>+  InitProgressUI(&argc, &argv);


>@@ -3370,16 +3437,21 @@ int NS_main(int argc, NS_tchar **argv)
>         }
>       }
>     }
>   }
> #endif /* XP_MACOSX */
> 
>   LogFinish();
> 

Remove the next 5 lines
>+#ifdef XP_MACOSX
>+    // end of elevation-not-required update block
>+  }
>+#endif
>+
>   if (argc > callbackIndex) {

Comment 200

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #199)
> From irc convo today (includes correction
> >@@ -2609,16 +2598,81 @@ int NS_main(int argc, NS_tchar **argv)
> >       sStagedUpdate = true;
> >     } else if (NS_tstrstr(argv[4], NS_T("/replace"))) {
> >       // We're processing a request to replace the application with a staged
> >       // update.
> >       sReplaceRequest = true;
> >     }
> >   }
> > 
> >+#if defined(XP_MACOSX)
> >+  bool isElevated =
> >+    strstr(argv[0], "/Library/PrivilegedHelperTools/org.mozilla.updater") != 0;
> >+  if (isElevated) {
> >+    if (!ObtainUpdaterArguments(&argc, &argv)) {
> >+      // Won't actually get here because ObtainUpdaterArguments will terminate
> >+      // the current process on failure.
> >+      return 1;
> >+    }
> >+  } else {
> >+    sRequiresElevation = !IsRecursivelyWritable(argv[2]);
> >+  }
> >+#endif
> >+
> >+  // The directory containing the update information.
> >+  gPatchDirPath = argv[1];
> >+
> >+#ifdef XP_MACOSX
> >+  if (sRequiresElevation) {
> >+    gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
> >+    if (!gSucceeded) {
> >+      WriteStatusFile(ELEVATION_CANCELED);
> >+    }
Not certain but I think you will also need to launch the callback and post update app here.

> Add   return gSucceeded ? 0 : 1;
> Add }

Comment 201

[Stephen A Pohl [:spohl]]

Attachment: 3. Build config changes

Updated for current trunk, carrying over r+.

Comment 202

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

Comment on attachment 8728586 [details] [diff] [review]
1. Patch to elevate updater

>diff --git a/browser/base/content/aboutDialog-appUpdater.js b/browser/base/content/aboutDialog-appUpdater.js
>--- a/browser/base/content/aboutDialog-appUpdater.js
>+++ b/browser/base/content/aboutDialog-appUpdater.js
>...
>@@ -72,17 +75,18 @@ function appUpdater()
>     // selectPanel("downloading") is called from setupDownloadingUI().
>     return;
>   }
> 
>   // Honor the "Never check for updates" option by not only disabling background
>   // update checks, but also in the About dialog, by presenting a
>   // "Check for updates" button.
>   // If updates are found, the user is then asked if he wants to "Update to <version>".
>-  if (!this.updateEnabled) {
>+  if (!this.updateEnabled ||
>+      Services.prefs.prefHasUserValue(PREF_APP_UPDATE_ELEVATE_NEVER)) {
Note: current behavior is to show a download link to the latest when an update is available and the user can't update. With this change we now show the check for updates button. Showing both for this case would be best but I'm not going to let perfect be the enemy of good. file a followup?

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -183,16 +187,21 @@ const DEFAULT_SERVICE_MAX_ERRORS = 10;
> 
> // The number of consecutive socket errors to allow before falling back to
> // downloading a different MAR file or failing if already downloading the full.
> const DEFAULT_SOCKET_MAX_ERRORS = 10;
> 
> // The number of milliseconds to wait before retrying a connection error.
> const DEFAULT_UPDATE_RETRY_TIMEOUT = 2000;
> 
>+// Default maximum number of elevation cancelations per update version before
>+// giving up.
>+const DEFAULT_MAX_OSX_CANCELATIONS = 3;
>+const PREF_APP_UPDATE_MAX_OSX_CANCELATIONS = "app.update.cancelations.osx.max";
Put the preference with the rest of the preferences

>@@ -332,56 +341,93 @@ function hasUpdateMutex() {
>     return true;
>   }
>   if (!gUpdateMutexHandle) {
>     gUpdateMutexHandle = createMutex(getPerInstallationMutexName(true), false);
>   }
>   return !!gUpdateMutexHandle;
> }
> 

Please add function info as you have done elsewhere
>+function ensureWritableDescendants(aDir) {
>+  let items = aDir.directoryEntries;
>+  while (items.hasMoreElements()) {
>+    let item = items.getNext().QueryInterface(Ci.nsIFile);
>+    if (!item.isWritable()) {
>+      LOG("ensureWritableDescendants - unable to write to " + item.path);
>+      return false;
>+    }
>+    if (item.isDirectory() && !ensureWritableDescendants(item)) {
>+      return false;
>+    }
>+  }
>+  return true;
>+}
ensure* names are for ensuring the condition is met. The name should be along the lines of areDirectoryEntriesWriteable since this just checks.

>+
>+/**
>+ * OSX only function to determine if the user requires elevation to be able to
>+ * write to the application bundle.
>+ *
>+ * @return true if elevation is required, false otherwise
>+ */
>+function getElevationRequired() {
>+  if (AppConstants.platform != "macosx") {
>+    return false;
>+  }
>+
>+  try {
>+    // Recursively check that the application bundle (and its descendants) can
>+    // be written to.
>+    LOG("getElevationRequired - recursively testing write access on " +
>+        getInstallDirRoot().path);
>+    if (!getInstallDirRoot().isWritable() ||
>+        !ensureWritableDescendants(getInstallDirRoot())) {
>+      LOG("getElevationRequired - unable to write to application bundle, " +
>+          "elevation required");
>+      return true;
>+    }
>+  } catch (ex) {
>+    LOG("getElevationRequired - unable to write to application bundle, " +
>+        "elevation required. Exception: " + ex);
>+    return true;
>+  }
>+  LOG("getElevationRequired - able to write to application bundle, elevation " +
>+      "not required");
>+  return false;
>+}
>+

Though you didn't add this could you please add function info?
> function getCanApplyUpdates() {
>   let useService = false;
>   if (shouldUseService()) {
>     // No need to perform directory write checks, the maintenance service will
>     // be able to write to all directories.
>     LOG("getCanApplyUpdates - bypass the write checks because we'll use the service");
>     useService = true;
>   }
> 

>@@ -2284,23 +2375,32 @@ UpdateService.prototype = {
>     AUSTLMY.pingBoolPref("UPDATE_NOT_PREF_UPDATE_AUTO_" + this._pingSuffix,
>                          PREF_APP_UPDATE_AUTO, true, true);
>     // Histogram IDs:
>     // UPDATE_NOT_PREF_UPDATE_STAGING_ENABLED_EXTERNAL
>     // UPDATE_NOT_PREF_UPDATE_STAGING_ENABLED_NOTIFY
>     AUSTLMY.pingBoolPref("UPDATE_NOT_PREF_UPDATE_STAGING_ENABLED_" +
>                          this._pingSuffix,
>                          PREF_APP_UPDATE_STAGING_ENABLED, true, true);
>-    if (AppConstants.platform == "win") {
>+    if (AppConstants.platform == "win" ||
>+        AppConstants.platform == "macosx") {
nit: no need to put this on two lines

>       // Histogram IDs:
>       // UPDATE_PREF_UPDATE_CANCELATIONS_EXTERNAL
>       // UPDATE_PREF_UPDATE_CANCELATIONS_NOTIFY
>       AUSTLMY.pingIntPref("UPDATE_PREF_UPDATE_CANCELATIONS_" + this._pingSuffix,
>                           PREF_APP_UPDATE_CANCELATIONS, 0, 0);
>     }
>+    if (AppConstants.platform == "macosx") {
>+      // Histogram IDs:
>+      // UPDATE_PREF_UPDATE_CANCELATIONS_EXTERNAL
>+      // UPDATE_PREF_UPDATE_CANCELATIONS_NOTIFY
// UPDATE_PREF_UPDATE_CANCELATIONS_OSX_EXTERNAL
// UPDATE_PREF_UPDATE_CANCELATIONS_OSX_NOTIFY

>+      AUSTLMY.pingIntPref("UPDATE_PREF_UPDATE_CANCELATIONS_OSX" +
>+                          this._pingSuffix,
>+                          PREF_APP_UPDATE_CANCELATIONS_OSX, 0, 0);
>+    }
>     if (AppConstants.MOZ_MAINTENANCE_SERVICE) {
>       // Histogram IDs:
>       // UPDATE_NOT_PREF_UPDATE_SERVICE_ENABLED_EXTERNAL
>       // UPDATE_NOT_PREF_UPDATE_SERVICE_ENABLED_NOTIFY
>       AUSTLMY.pingBoolPref("UPDATE_NOT_PREF_UPDATE_SERVICE_ENABLED_" +
>                            this._pingSuffix,
>                            PREF_APP_UPDATE_SERVICE_ENABLED, true);
>       // Histogram IDs:
>@@ -2446,18 +2547,76 @@ UpdateService.prototype = {
>         default:
>           LOG("UpdateService:selectUpdate - skipping unknown update type: " +
>               aUpdate.type);
>           lastCheckCode = AUSTLMY.CHK_UPDATE_INVALID_TYPE;
>           break;
>       }
>     });
> 
>-    var update = minorUpdate || majorUpdate;
>-    if (!update) {
>+    let update = minorUpdate || majorUpdate;
>+    if (update && AppConstants.platform == "macosx") {
nit: I would prefer if the check were reversed so other platforms don't check update as follows:
if (AppConstants.platform == "macosx" && update) {

>@@ -3278,26 +3447,54 @@ UpdateManager.prototype = {
>     }
>     // Only prompt when the UI isn't already open.
>     let windowType = getPref("getCharPref", PREF_APP_UPDATE_ALTWINDOWTYPE, null);
>     if (Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME) ||
>         windowType && Services.wm.getMostRecentWindow(windowType)) {
>       return;
>     }
> 
>-    if (update.state == STATE_APPLIED || update.state == STATE_APPLIED_SVC ||
>-        update.state == STATE_PENDING || update.state == STATE_PENDING_SVC) {
>-        // Notify the user that an update has been staged and is ready for
>-        // installation (i.e. that they should restart the application).
>+    if (update.state == STATE_APPLIED ||
>+        update.state == STATE_APPLIED_SERVICE ||
>+        update.state == STATE_PENDING ||
>+        update.state == STATE_PENDING_SERVICE ||
>+        update.state == STATE_PENDING_ELEVATE) {
>+      // Notify the user that an update has been staged and is ready for
>+      // installation (i.e. that they should restart the application).
>       let prompter = Cc["@mozilla.org/updates/update-prompt;1"].
>                      createInstance(Ci.nsIUpdatePrompt);
>       prompter.showUpdateDownloaded(update, true);
>     }
>   },
> 
>+  /**
>+   * See nsIUpdateService.idl
>+   */
>+  userElevationOptedIn: function UM_userElevationOptedIn() {
I think this should just be elevationOptedIn since user is a given.

>+    // The user has been been made aware that the update requires elevation.
>+    let update = this._activeUpdate;
>+    if (!update) {
>+      return;
>+    }
>+    let status = readStatusFile(getUpdatesDir());
>+    let parts = status.split(":");
>+    update.state = parts[0];
>+    if (update.state == STATE_PENDING_ELEVATE) {
>+      // Proceed with the pending update.
>+      writeStatusFile(getUpdatesDir(), STATE_PENDING);
Why STATE_PENDING instead of STATE_PENDING_ELEVATE?

>+    }
>+  },

>@@ -4434,33 +4650,25 @@ UpdatePrompt.prototype = {
>   },
> 
>   /**
>    * See nsIUpdateService.idl
>    */
>   showUpdateInstalled: function UP_showUpdateInstalled() {
>     if (getPref("getBoolPref", PREF_APP_UPDATE_SILENT, false) ||
>         !getPref("getBoolPref", PREF_APP_UPDATE_SHOW_INSTALLED_UI, false) ||
>-        this._getUpdateWindow())
>+        this._getUpdateWindow()) {
>       return;
>-
>-    var page = "installed";
>-    var win = this._getUpdateWindow();
>-    if (win) {
>-      if (page && "setCurrentPage" in win)
>-        win.setCurrentPage(page);
>-      win.focus();
>     }
>-    else {
>-      var openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
>-      var arg = Cc["@mozilla.org/supports-string;1"].
>-                createInstance(Ci.nsISupportsString);
>-      arg.data = page;
>-      Services.ww.openWindow(null, URI_UPDATE_PROMPT_DIALOG, null, openFeatures, arg);
>-    }
>+
>+    let openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
>+    let arg = Cc["@mozilla.org/supports-string;1"].
>+              createInstance(Ci.nsISupportsString);
>+    arg.data = "installed";
>+    Services.ww.openWindow(null, URI_UPDATE_PROMPT_DIALOG, null, openFeatures, arg);
I suspect it is but are you certain this removal is ok?

along with the updater.cpp changes in comment #199

r=me with those changes.

Comment 203

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

Would be good to give a heads up to Thunderbird and SeaMonkey.

Comment 204

[John Hesling [:John99] (NeedInfo me)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #203)
> Would be good to give a heads up to Thunderbird and SeaMonkey.

I wonder if it may also be worth adding to this bug, a see also for bug 1258515

Comment 205

[Javi Rueda :javirid]

Tested on TB 31.8. Admin which installed it is able to update the usual way -it downloads the update and then a button appears to restart Thunderbird-. However, another admin user gets a link where he/she could download the dmg.

Comment 206

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Addressed feedback. Carrying over r+, but setting to f? to get your eyes on the refactor I made to address comment 199 and comment 200, as well as my answers to your questions in comment 202 below:

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #202)
> Comment on attachment 8728586 [details] [diff] [review]
> 1. Patch to elevate updater
> 
> >diff --git a/browser/base/content/aboutDialog-appUpdater.js b/browser/base/content/aboutDialog-appUpdater.js
> >--- a/browser/base/content/aboutDialog-appUpdater.js
> >+++ b/browser/base/content/aboutDialog-appUpdater.js
> >...
> >@@ -72,17 +75,18 @@ function appUpdater()
> >     // selectPanel("downloading") is called from setupDownloadingUI().
> >     return;
> >   }
> > 
> >   // Honor the "Never check for updates" option by not only disabling background
> >   // update checks, but also in the About dialog, by presenting a
> >   // "Check for updates" button.
> >   // If updates are found, the user is then asked if he wants to "Update to <version>".
> >-  if (!this.updateEnabled) {
> >+  if (!this.updateEnabled ||
> >+      Services.prefs.prefHasUserValue(PREF_APP_UPDATE_ELEVATE_NEVER)) {
> Note: current behavior is to show a download link to the latest when an
> update is available and the user can't update. With this change we now show
> the check for updates button. Showing both for this case would be best but
> I'm not going to let perfect be the enemy of good. file a followup?

This change is for the case when a user selected "No Thanks" in the dialog that notifies the user of an update that requires elevation. By clicking the button, he can manually trigger an update check (which ignores the fact that he clicked "No Thanks" previously). If the currently available update requires elevation again, the link to the latest update will be presented in the dialog that notifies the user of the elevation requirement. Also, displaying both the button and the link in the about dialog would require more space since they are both placed in the same place at the moment. In light of this, should I still go ahead and file a followup bug?

> >+    // The user has been been made aware that the update requires elevation.
> >+    let update = this._activeUpdate;
> >+    if (!update) {
> >+      return;
> >+    }
> >+    let status = readStatusFile(getUpdatesDir());
> >+    let parts = status.split(":");
> >+    update.state = parts[0];
> >+    if (update.state == STATE_PENDING_ELEVATE) {
> >+      // Proceed with the pending update.
> >+      writeStatusFile(getUpdatesDir(), STATE_PENDING);
> Why STATE_PENDING instead of STATE_PENDING_ELEVATE?

STATE_PENDING_ELEVATE stands for "pending user's approval to proceed with an elevated update". As long as we see this state, we will notify the user of the availability of an update that requires elevation. |elevationOptedIn| is called when the user gives us approval to proceed, so we want to switch to STATE_PENDING. The updater then detects whether or not elevation is required and displays the elevation prompt if necessary. This last step does not depend on the state in the status file.

> >+    }
> >+  },
> 
> >@@ -4434,33 +4650,25 @@ UpdatePrompt.prototype = {
> >   },
> > 
> >   /**
> >    * See nsIUpdateService.idl
> >    */
> >   showUpdateInstalled: function UP_showUpdateInstalled() {
> >     if (getPref("getBoolPref", PREF_APP_UPDATE_SILENT, false) ||
> >         !getPref("getBoolPref", PREF_APP_UPDATE_SHOW_INSTALLED_UI, false) ||
> >-        this._getUpdateWindow())
> >+        this._getUpdateWindow()) {
> >       return;
> >-
> >-    var page = "installed";
> >-    var win = this._getUpdateWindow();
> >-    if (win) {
> >-      if (page && "setCurrentPage" in win)
> >-        win.setCurrentPage(page);
> >-      win.focus();
> >     }
> >-    else {
> >-      var openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
> >-      var arg = Cc["@mozilla.org/supports-string;1"].
> >-                createInstance(Ci.nsISupportsString);
> >-      arg.data = page;
> >-      Services.ww.openWindow(null, URI_UPDATE_PROMPT_DIALOG, null, openFeatures, arg);
> >-    }
> >+
> >+    let openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
> >+    let arg = Cc["@mozilla.org/supports-string;1"].
> >+              createInstance(Ci.nsISupportsString);
> >+    arg.data = "installed";
> >+    Services.ww.openWindow(null, URI_UPDATE_PROMPT_DIALOG, null, openFeatures, arg);
> I suspect it is but are you certain this removal is ok?

This has the same behavior. I haven't checked whether or not the current behavior is correct though. The |if (win)| condition is never entered, because if |win| after |var win = this._getUpdateWindow()| is non-null, then we would have returned early already due to the guard at the beginning of the function (i.e. if |this._getUpdateWindow()| is non-null, we return early). If this is an existing mistake, I'm happy to file a followup.

Comment 207

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb9fcacd1ce2

Comment 208

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

(In reply to Stephen A Pohl [:spohl] from comment #206)
> Created attachment 8737469 [details] [diff] [review]
> 1. Patch to elevate updater
> 
> Addressed feedback. Carrying over r+, but setting to f? to get your eyes on
> the refactor I made to address comment 199 and comment 200, as well as my
> answers to your questions in comment 202 below:
> 
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #202)
> > Comment on attachment 8728586 [details] [diff] [review]
> > 1. Patch to elevate updater
> > 
> > >diff --git a/browser/base/content/aboutDialog-appUpdater.js b/browser/base/content/aboutDialog-appUpdater.js
> > >--- a/browser/base/content/aboutDialog-appUpdater.js
> > >+++ b/browser/base/content/aboutDialog-appUpdater.js
> > >...
> > >@@ -72,17 +75,18 @@ function appUpdater()
> > >     // selectPanel("downloading") is called from setupDownloadingUI().
> > >     return;
> > >   }
> > > 
> > >   // Honor the "Never check for updates" option by not only disabling background
> > >   // update checks, but also in the About dialog, by presenting a
> > >   // "Check for updates" button.
> > >   // If updates are found, the user is then asked if he wants to "Update to <version>".
> > >-  if (!this.updateEnabled) {
> > >+  if (!this.updateEnabled ||
> > >+      Services.prefs.prefHasUserValue(PREF_APP_UPDATE_ELEVATE_NEVER)) {
> > Note: current behavior is to show a download link to the latest when an
> > update is available and the user can't update. With this change we now show
> > the check for updates button. Showing both for this case would be best but
> > I'm not going to let perfect be the enemy of good. file a followup?
> 
> This change is for the case when a user selected "No Thanks" in the dialog
> that notifies the user of an update that requires elevation. By clicking the
> button, he can manually trigger an update check (which ignores the fact that
> he clicked "No Thanks" previously). If the currently available update
> requires elevation again, the link to the latest update will be presented in
> the dialog that notifies the user of the elevation requirement. Also,
> displaying both the button and the link in the about dialog would require
> more space since they are both placed in the same place at the moment. In
> light of this, should I still go ahead and file a followup bug?
My concern is that the user will only see the download link in the update dialog until they go through the steps again whereas currently if they don't have permissions the default setting is to auto check for updates when opening the about dialog and showing the download link. I agree with everything you wrote and I'm not sure what a good solution would be but I think it might be possible to improve the experience.

> 
> > >+    // The user has been been made aware that the update requires elevation.
> > >+    let update = this._activeUpdate;
> > >+    if (!update) {
> > >+      return;
> > >+    }
> > >+    let status = readStatusFile(getUpdatesDir());
> > >+    let parts = status.split(":");
> > >+    update.state = parts[0];
> > >+    if (update.state == STATE_PENDING_ELEVATE) {
> > >+      // Proceed with the pending update.
> > >+      writeStatusFile(getUpdatesDir(), STATE_PENDING);
> > Why STATE_PENDING instead of STATE_PENDING_ELEVATE?
> 
> STATE_PENDING_ELEVATE stands for "pending user's approval to proceed with an
> elevated update". As long as we see this state, we will notify the user of
> the availability of an update that requires elevation. |elevationOptedIn| is
> called when the user gives us approval to proceed, so we want to switch to
> STATE_PENDING. The updater then detects whether or not elevation is required
> and displays the elevation prompt if necessary. This last step does not
> depend on the state in the status file.
Thanks. Could you add a comment to this affect either in this bug or a followup?

> 
> > >+    }
> > >+  },
> > 
> > >@@ -4434,33 +4650,25 @@ UpdatePrompt.prototype = {
> > >   },
> > > 
> > >   /**
> > >    * See nsIUpdateService.idl
> > >    */
> > >   showUpdateInstalled: function UP_showUpdateInstalled() {
> > >     if (getPref("getBoolPref", PREF_APP_UPDATE_SILENT, false) ||
> > >         !getPref("getBoolPref", PREF_APP_UPDATE_SHOW_INSTALLED_UI, false) ||
> > >-        this._getUpdateWindow())
> > >+        this._getUpdateWindow()) {
> > >       return;
> > >-
> > >-    var page = "installed";
> > >-    var win = this._getUpdateWindow();
> > >-    if (win) {
> > >-      if (page && "setCurrentPage" in win)
> > >-        win.setCurrentPage(page);
> > >-      win.focus();
> > >     }
> > >-    else {
> > >-      var openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
> > >-      var arg = Cc["@mozilla.org/supports-string;1"].
> > >-                createInstance(Ci.nsISupportsString);
> > >-      arg.data = page;
> > >-      Services.ww.openWindow(null, URI_UPDATE_PROMPT_DIALOG, null, openFeatures, arg);
> > >-    }
> > >+
> > >+    let openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
> > >+    let arg = Cc["@mozilla.org/supports-string;1"].
> > >+              createInstance(Ci.nsISupportsString);
> > >+    arg.data = "installed";
> > >+    Services.ww.openWindow(null, URI_UPDATE_PROMPT_DIALOG, null, openFeatures, arg);
> > I suspect it is but are you certain this removal is ok?
> 
> This has the same behavior. I haven't checked whether or not the current
> behavior is correct though. The |if (win)| condition is never entered,
> because if |win| after |var win = this._getUpdateWindow()| is non-null, then
> we would have returned early already due to the guard at the beginning of
> the function (i.e. if |this._getUpdateWindow()| is non-null, we return
> early). If this is an existing mistake, I'm happy to file a followup.
I'm good with it. This is called on startup and should only happen when there is no update dialog anyways.

Comment 209

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Added comment to |elevationOptedIn| about the meaning of STATE_PENDING_ELEVATE. Carrying over r+, leaving f? set for the refactor stemming from comment 199 and comment 200 in updater.cpp.

Comment 210

[Stephen A Pohl [:spohl]]

Comment on attachment 8737474 [details] [diff] [review]
1. Patch to elevate updater

Clearing f? until build bustage on Windows and Linux is resolved.

Comment 211

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2f5f5ab6784

Comment 212

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

BTW: I cleaned up oak and merged m-c over so it should be all set when you are ready to land it there

Comment 213

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f036eb1b3ecf

Comment 214

[Stephen A Pohl [:spohl]]

Comment on attachment 8737474 [details] [diff] [review]
1. Patch to elevate updater

Try is finally green. Setting to f? to get your eyes on the refactor to address comment 199 and comment 200 (see |LaunchCallbackAndPostProcessApps|).

Comment 215

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #212)
> BTW: I cleaned up oak and merged m-c over so it should be all set when you
> are ready to land it there

Thank you! Will land as soon as I get your final ok on patch 1.

Comment 216

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

Comment on attachment 8737474 [details] [diff] [review]
1. Patch to elevate updater

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -295,16 +302,17 @@ static NS_tchar* gPatchDirPath;
> static NS_tchar gInstallDirPath[MAXPATHLEN];
> static NS_tchar gWorkingDirPath[MAXPATHLEN];
> static ArchiveReader gArchiveReader;
> static bool gSucceeded = false;
> static bool sStagedUpdate = false;
> static bool sReplaceRequest = false;
> static bool sUsingService = false;
> static bool sIsOSUpdate = false;
>+static bool sRequiresElevation = false;
Remove along with the changes below

>@@ -2483,18 +2491,80 @@ UpdateThreadFunc(void *param)
>     }
>     WriteStatusFile(rv);
>   }
> 
>   LOG(("calling QuitProgressUI"));
>   QuitProgressUI();
> }
> 
>+#ifdef XP_MACOSX
>+void freeArguments(int argc, char** argv)
>+{
>+  for (int i = 0; i < argc; i++) {
>+    free(argv[i]);
>+  }
>+  free(argv);
>+}
>+#endif
>+
>+void LaunchCallbackAndPostProcessApps(int argc, char** argv, int callbackIndex
>+#if defined(XP_WIN)
>+                                      , NS_tchar elevatedLockFilePath
>+                                      , HANDLE updateLockFileHandle
>+#endif /* XP_WIN */
>+#ifdef XP_MACOSX
Just use
#elif XP_MACOSX

>+                                      , bool isElevated
>+#endif /* XP_MACOSX */
>+                                      )
>+{
>+  if (argc > callbackIndex) {
>+#if defined(XP_WIN)
>+    if (gSucceeded) {
>+      if (!LaunchWinPostProcess(gInstallDirPath, gPatchDirPath)) {
>+        fprintf(stderr, "The post update process was not launched");
>+      }
>+
>+      // The service update will only be executed if it is already installed.
>+      // For first time installs of the service, the install will happen from
>+      // the PostUpdate process. We do the service update process here
>+      // because it's possible we are updating with updater.exe without the
>+      // service if the service failed to apply the update. We want to update
>+      // the service to a newer version in that case. If we are not running
>+      // through the service, then MOZ_USING_SERVICE will not exist.
>+      if (!sUsingService) {
>+        StartServiceUpdate(gInstallDirPath);
>+      }
>+    }
>+    EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 0);
>+#endif /* XP_WIN */
>+#ifdef XP_MACOSX
Just use
#elif XP_MACOSX

>+    if (!isElevated) {
>+      if (gSucceeded) {
>+        LaunchMacPostProcess(gInstallDirPath);
This will be able to modify the install directory because of the permission changes?

Note: I think this is ok but we may want the ability to perform modifications elevated as we are able to on Windows.

>+      }
>+#endif /* XP_MACOSX */
>+
>+    LaunchCallbackApp(argv[5],
>+                      argc - callbackIndex,
>+                      argv + callbackIndex,
>+                      sUsingService);
>+#ifdef XP_MACOSX
>+    } // if (!isElevated)
>+#endif /* XP_MACOSX */
>+  }
>+}
>@@ -2609,16 +2645,82 @@ int NS_main(int argc, NS_tchar **argv)
>       sStagedUpdate = true;
>     } else if (NS_tstrstr(argv[4], NS_T("/replace"))) {
>       // We're processing a request to replace the application with a staged
>       // update.
>       sReplaceRequest = true;
>     }
>   }
> 
>+#if defined(XP_MACOSX)
>+  bool isElevated =
>+    strstr(argv[0], "/Library/PrivilegedHelperTools/org.mozilla.updater") != 0;
>+  if (isElevated) {
>+    if (!ObtainUpdaterArguments(&argc, &argv)) {
>+      // Won't actually get here because ObtainUpdaterArguments will terminate
>+      // the current process on failure.
>+      return 1;
>+    }
>+  } else {
>+    sRequiresElevation = !IsRecursivelyWritable(argv[2]);
>+  }
Get rid of the else block

>+#endif
>+
>+  // The directory containing the update information.
>+  gPatchDirPath = argv[1];
>+
>+#ifdef XP_MACOSX
>+  if (sRequiresElevation) {
Change this to
if (!isElevated && !IsRecursivelyWritable(argv[2])) {

>+    gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
>+    if (!gSucceeded) {
>+      WriteStatusFile(ELEVATION_CANCELED);
>+    }
>+    LaunchCallbackAndPostProcessApps(argc, argv, callbackIndex, false);
>+    return gSucceeded ? 0 : 1;
>+  }
>+#endif

>@@ -3383,51 +3498,41 @@ int NS_main(int argc, NS_tchar **argv)
>         rv = rename_file(oldDistDir, newDistDir, true);
>         if (rv) {
>           LOG(("Moving old distribution directory to new location failed - " \
>                "err: %d", rv));
>         }
>       }
>     }
>   }
>+
>+  if (isElevated) {
>+    SetGroupOwnershipAndPermissions(gInstallDirPath);
>+    freeArguments(argc, argv);
>+    CleanupElevatedMacUpdate(false);
>+  } else if (IsOwnedByGroupAdmin(gInstallDirPath)) {
>+    // If the group ownership of the Firefox .app bundle was set to the "admin"
>+    // group during a previous elevated update, we need to ensure that all files
>+    // in the bundle have group ownership of "admin" as well as write permission
>+    // for the group to not break updates in the future.
>+    SetGroupOwnershipAndPermissions(gInstallDirPath);
>+  }
> #endif /* XP_MACOSX */
> 
>   LogFinish();
> 
>-  if (argc > callbackIndex) {
>-#if defined(XP_WIN)
>-    if (gSucceeded) {
>-      if (!LaunchWinPostProcess(gInstallDirPath, gPatchDirPath)) {
>-        fprintf(stderr, "The post update process was not launched");
>-      }
>-
>-      // The service update will only be executed if it is already installed.
>-      // For first time installs of the service, the install will happen from
>-      // the PostUpdate process. We do the service update process here
>-      // because it's possible we are updating with updater.exe without the
>-      // service if the service failed to apply the update. We want to update
>-      // the service to a newer version in that case. If we are not running
>-      // through the service, then MOZ_USING_SERVICE will not exist.
>-      if (!sUsingService) {
>-        StartServiceUpdate(gInstallDirPath);
>-      }
>-    }
>-    EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 0);
>+  LaunchCallbackAndPostProcessApps(argc, argv, callbackIndex
>+#ifdef XP_WIN
>+                                   , elevatedLockFilePath
>+                                   , updateLockFileHandle
> #endif /* XP_WIN */
> #ifdef XP_MACOSX
Just use
#elif XP_MACOSX

>-    if (gSucceeded) {
>-      LaunchMacPostProcess(gInstallDirPath);
>-    }
>+                                   , isElevated
> #endif /* XP_MACOSX */
>-
>-    LaunchCallbackApp(argv[5],
>-                      argc - callbackIndex,
>-                      argv + callbackIndex,
>-                      sUsingService);
>-  }
>+                                  );

r=me with that

Comment 217

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Thanks, Robert! Addressed feedback, carrying over r+. Will land on Oak shortly.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #216)
> Comment on attachment 8737474 [details] [diff] [review]
> 1. Patch to elevate updater
> 
> >diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
> >+    if (!isElevated) {
> >+      if (gSucceeded) {
> >+        LaunchMacPostProcess(gInstallDirPath);
> This will be able to modify the install directory because of the permission
> changes?

Correct.

> Note: I think this is ok but we may want the ability to perform
> modifications elevated as we are able to on Windows.

I've tried to add as little functionality to the elevated process as possible. If we need this in the future, we can certainly add it and review it from a security perspective at that point.

Comment 218

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/acbd16915de8df6db1ce2c3a9e3bcb735c001560
Bug 394984: Enable any admin user on OSX to update Firefox, front-end and updater changes. r=rstrong

https://hg.mozilla.org/projects/oak/rev/bdc7720fcf3c233a174ed37e7a1da7d3e84ed7d2
Bug 394984: Enable any admin user on OSX to update Firefox, native OSX changes. r=mstange

https://hg.mozilla.org/projects/oak/rev/94cc6d062da29b6e93255bd8c39477bec31992d5
Bug 394984: Enable any admin user on OSX to update Firefox, build config changes. r=mshal

https://hg.mozilla.org/projects/oak/rev/d80412c56b8a3b65172d96846800eaa7b2af95ce
Bug 394984: Add signing certificate info to Info.plist files for Firefox and updater. r=bhearsum

https://hg.mozilla.org/projects/oak/rev/e5b2aee32e97e78c1ffe46b7c45a32f65528ee77
Bug 394984: Update xpcshell tests to use the new updater binary name. r=rstrong

Comment 219

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/06bcb6569cbbd0c62574a144345c67c0e16a3ea1
Bug 394984: Ensure that the elevated updater on OSX finds the proper patch directory. r=me

Comment 220

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

The refactor in the previous iteration of this patch accidentally made it impossible for the elevated updater to find the patch directory. Carrying over r+ since this reverts back to the previous behavior.

Comment 221

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/3eae248283a05f7338454ddad7b019ed3ba3426c
Bug 394984: Fix write permission check in OSX updater. r=me

Comment 222

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/aa569c4b3c0cf7a6d959a622f8b350d2bc881887
Bug 394984: Never try to show progress UI by the elevated process on OSX. r=me

Comment 223

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 1

Testing by SoftVision seems to indicate that the elevated updater can get hung when trying to show the progress UI. I've been unable to confirm this myself, but it's a good idea to avoid this situation in any case.

This patch has landed on Oak and no reoccurrence of this issue has been reported since.

Comment 224

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

Comment on attachment 8745506 [details] [diff] [review]
Addition to patch 1

The time it takes to apply a complete update can be substantial and this would make it so those users won't see any indication that an update is being applied. It would be a good thing to find out why this is happening and if it can't be avoided or if it would be better to just move forward with this without any UI then please get UX to sign off on this before requesting review.

Comment 225

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

BTW: I might have reversed complete with partial but I know one of them takes a substantial amount of time on Mac.

Comment 226

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

We discussed this over irc and Stephen is going to try out the same method we use on Windows where we show an indeterminate progress bar from the unelevated process when using the service.

Comment 227

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 2

The write permission check for the elevated updater was flawed. The check as previously implemented checked whether the group ownership of a file matched the primary gid of the current user. However, a user can (and usually does) belong to several groups. An admin user can have a primary gid of 20(staff), but also belong to 80(admin). The check would see 20 != 80 and tell us to run an elevated update, when in reality the user might have had write permission all along.

Unbelievably, I completely missed NSFileManager's |isWritableFileAtPath|, so this is a pretty straightforward fix.

This has landed on Oak and testing so far indicates that this works as expected.

Comment 228

[Markus Stange [:mstange]]

Comment on attachment 8745517 [details] [diff] [review]
Addition to patch 2

Review of attachment 8745517 [details] [diff] [review]:
-----------------------------------------------------------------

That is indeed a very useful API.

Comment 229

[Bruce Wolfe]

Yes, this is very useful but because this issue has been plaguing Macintosh users since 2007 it is really time to get this show on the road. Why? Because your fix will help drive tardy Macintosh developers to fix this systemwide. Please make the fix today for the very next update.

Comment 230

[Stephen A Pohl [:spohl]]

Comment on attachment 8745517 [details] [diff] [review]
Addition to patch 2

Hi Robert, now that the file permission check is finally working with this patch, we run into failures in marAppInUseStageSuccessComplete_unix.js. In this test, we create two symlinks to non-existing files. Since the files don't exist, the write check in this patch is failing and we're trying to launch an elevated update on our test slaves.

Could you clarify what the behavior should be here? Do we actually expect to see links to non-existing files inside the Firefox.app bundle in the real world? Do we ever expect to see links to files that are outside the .app bundle? This test does both.

Here are the symlinks I'm referring to inside dir.app/Contents/Resources:
lrwxr-xr-x   1 Stephen  staff     27 Apr 27 23:00 link -> /tmp/moz-foo/moz-bar/target
lrwxr-xr-x   1 Stephen  staff     30 Apr 27 23:00 link2 -> /tmp/moz-foo2/moz-bar2/target2

Comment 231

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

Everything is possible in the "real world" and yes, we have seen links to files and dirs outside of the app dir and I would expect we would also see them to non-existent files and dirs as well. :)

Would the elevated update succeed with these links?

Comment 232

[Stephen A Pohl [:spohl]]

Okay, it turns out that the reason why I thought those files don't exist was because the test is changing the permissions on a parent directory in such a way that the user loses execute permission to the parent directory, so can't list the contents. More specifically:

1. The test creates a symlink dir.app/Contents/Resources/link2 that points to /tmp/moz-foo2/moz-bar2/target2.
2. We then call the test helper process with the "change-perm"[1] argument.
3. The test helper changes the permission of the /tmp/moz-foo2 directory to 0644, which removes execute permission and the user can no longer list the contents of the directory.
4. Our write check correctly fails, because /tmp/moz-foo2/moz-bar2/target2 is no longer accessible or writable.

I've confirmed that the elevated update correctly prompts for an elevated update in this situation, and the update succeeds. However, as written, the test probably only used to succeed because the MAR that we're trying to apply doesn't change the two symlinks. It would otherwise try to write to inaccessible files and fail.

It seems like this test should be updated. Robert, what is the link2 symlink currently trying to test?


[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_base_updater/marAppInUseStageSuccessComplete_unix.js#118
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/TestAUSHelper.cpp#254

Comment 233

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

(In reply to Stephen A Pohl [:spohl] from comment #232)
>...
> I've confirmed that the elevated update correctly prompts for an elevated
> update in this situation, and the update succeeds. However, as written, the
> test probably only used to succeed because the MAR that we're trying to
> apply doesn't change the two symlinks. It would otherwise try to write to
> inaccessible files and fail.
> 
> It seems like this test should be updated. Robert, what is the link2 symlink
> currently trying to test?
That is intentional since the only cases where we've had reports of symlinks were with non app managed files causing problems with staging.

Comment 234

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

s/reports/bugs/

Comment 235

[Stephen A Pohl [:spohl]]

So, what's the path forward here?

Comment 236

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

Does the elevation only occur because of the file's permissions or does it happen for all symlinks?

Comment 237

[Stephen A Pohl [:spohl]]

Only because of the target2's parent directory's permissions, which is 0644 for /tmp/moz-foo2

Comment 238

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

It isn't clear to me whether the permissions check should be checking symlinks. Can you give me examples of why it should?

Comment 239

[Stephen A Pohl [:spohl]]

The concept of symlinks in a .app bundle that point to files outside of the bundle is new to me. I don't know when this would be used, and whether we should or shouldn't check for writability. It might also break the new v2 signatures on our .app bundle, since we're changing the contents of the .app bundle after it was signed.

I'm happy to skip the write check on symlinks.

Comment 240

[Stephen A Pohl [:spohl]]

While we're at it, we could also change the way we set new permissions on files after an elevated update. By skipping over symlinks we would constrain the permission changes to files inside of the .app bundle, which is probably a good idea.

Comment 241

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

(In reply to Stephen A Pohl [:spohl] from comment #239)
> The concept of symlinks in a .app bundle that point to files outside of the
> bundle is new to me. I don't know when this would be used, and whether we
> should or shouldn't check for writability. It might also break the new v2
> signatures on our .app bundle, since we're changing the contents of the .app
> bundle after it was signed.
You mean the admin or user would be breaking the signing and not us... right?

> I'm happy to skip the write check on symlinks.

(In reply to Stephen A Pohl [:spohl] from comment #240)
> While we're at it, we could also change the way we set new permissions on
> files after an elevated update. By skipping over symlinks we would constrain
> the permission changes to files inside of the .app bundle, which is probably
> a good idea.
I am fairly certain that skipping symlinks for both would be a good thing.

Comment 242

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #241)
> (In reply to Stephen A Pohl [:spohl] from comment #239)
> > The concept of symlinks in a .app bundle that point to files outside of the
> > bundle is new to me. I don't know when this would be used, and whether we
> > should or shouldn't check for writability. It might also break the new v2
> > signatures on our .app bundle, since we're changing the contents of the .app
> > bundle after it was signed.
> You mean the admin or user would be breaking the signing and not us... right?

Yes. I worded this poorly.

> > I'm happy to skip the write check on symlinks.
> 
> (In reply to Stephen A Pohl [:spohl] from comment #240)
> > While we're at it, we could also change the way we set new permissions on
> > files after an elevated update. By skipping over symlinks we would constrain
> > the permission changes to files inside of the .app bundle, which is probably
> > a good idea.
> I am fairly certain that skipping symlinks for both would be a good thing.

Ok, I'll go with this. Thanks!

Comment 243

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 2

Hi Markus, here is another revision of the write check that skips over symlinks, since they may be pointing to files that are outside of the .app bundle. For the same reason, we'll want to skip symlinks when we set the new group ownership and permissions.

See comment 230 to comment 242 for discussion.

Thank you!

Comment 244

[Stephen A Pohl [:spohl]]

Thanks, Markus!

For future reference: symlinks that are added to the .app bundle after the bundle was signed do indeed break the signature on the bundle:

$ ls -al /Applications/NightlyDebug.app/Contents/Resources
[...]
lrwxr-xr-x   1 testadmin  staff        27 Apr 28 20:51 link -> /tmp/moz-foo/moz-bar/target
lrwxr-xr-x   1 testadmin  staff        30 Apr 28 20:51 link2 -> /tmp/moz-foo2/moz-bar2/target2
lrwxr-xr-x   1 testadmin  admin        12 Apr 28 22:05 link3 -> platform.ini
[...]

$ codesign -vvvv /Applications/NightlyDebug.app
[...]
/Applications/NightlyDebug.app: a sealed resource is missing or invalid
file added: /Applications/NightlyDebug.app/Contents/Resources/link
file added: /Applications/NightlyDebug.app/Contents/Resources/link2
file added: /Applications/NightlyDebug.app/Contents/Resources/link3

Comment 245

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/4734d09a05a0c431fba09fb0c0d6e420c4d3fb54
Bug 394984: Fix write permission check in OSX updater. r=mstange

Comment 246

[Stephen A Pohl [:spohl]]

Attachment: 2nd addition to patch 2, add NSAutoreleasePools where missing

The tests on Oak[1] revealed that |ServeElevatedUpdate| was missing an NSAutoreleasePool and I noticed that |ObtainUpdaterArguments| was suffering the same issue. I tried using MacAutoreleasePool.h/mm in launchchild_osx.mm, but this would have required the inclusion of a multitude of additional files in UNIFIED_SOURCES in toolkit/mozapps/update/updater/moz.build, so I chose to just add a new |MacAutoreleasePool| class.

Note that |IsRecursivelyWritable| in updaterfileutils_osx.mm (see patch 2) still does the manual NSAutoreleasePool alloc/init. This is because this file is shared among Firefox, the updater as well as the xpcshell executable. Adding a new |MacAutoreleasePool| class in this file would cause a name conflict with |mozilla::MacAutoreleasePool| from MacAutoreleasePool.h/mm and we can't use MacAutoreleasePool.h/mm without including a multitude of additional files in UNIFIED_SOURCES in toolkit/mozapps/update/updater/moz.build.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=150219&repo=oak

Comment 247

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/d154bc80a284d6cbb34a359b2b56079f7f04271f
Bug 394984: Improve initializations of NSAutoReleasePools. r=mstange

Comment 248

[Stephen A Pohl [:spohl]]

Attachment: 2. Obj-C changes

Thanks, Markus! Folded all the patches with native changes into patch 2. Carrying over r+.

Comment 249

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 1

The unelevated update process now shows an indeterminate progress UI while waiting for the user to elevate and until the elevated update completes. We also prevent attempts to show a progress UI from the elevated update process.

This patch also includes the apostrophe fix, as discussed.

Comment 250

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 1

Comment 251

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8749432 [details] [diff] [review]
Addition to patch 1

Review of attachment 8749432 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2690,5 @@
>      // If the app directory isn't recursively writeable, an elevated update is
>      // required.
> +    UpdateServerThreadArgs threadArgs;
> +    threadArgs.argc = argc;
> +    threadArgs.argv = (const NS_tchar**)argv;

Use const_cast, so it's obvious this cast is only to add const-ness.

Comment 252

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 1

Comment 253

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 1

Now with |hg qrefresh|

Comment 254

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/aad19b0e37a40fccb0614bc52d31259f99249181
Bug 394984: Show indeterminate progress UI while waiting for elevated update to complete on OSX. r=rstrong,mhowell

Comment 255

[Stephen A Pohl [:spohl]]

review ping

Comment 256

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

Comment on attachment 8749446 [details] [diff] [review]
Addition to patch 1

>diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>--- a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>+++ b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>@@ -76,17 +76,17 @@ hideButton.accesskey=H
> applyingUpdate=Applying updateâ¦
> 
> updatesfound_minor.title=Update Available
> updatesfound_major.title=New Version Available
> 
> installSuccess=The Update was successfully installed
> installPending=Install Pending
> patchApplyFailure=The Update could not be installed (patch apply failed)
>-elevationFailure=You don't have the permissions necessary to install this update. Please contact your system administrator.
>+elevationFailure=You donât have the permissions necessary to install this update. Please contact your system administrator.
Weird, this didn't come through right. Please verify (over irc is fine) the character that should be used for the apostrophe with someone that does a lot of front end work.

>diff --git a/toolkit/mozapps/update/updater/progressui.h b/toolkit/mozapps/update/updater/progressui.h
>--- a/toolkit/mozapps/update/updater/progressui.h
>+++ b/toolkit/mozapps/update/updater/progressui.h
>@@ -4,31 +4,34 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #ifndef PROGRESSUI_H__
> #define PROGRESSUI_H__
> 
> #include "updatedefines.h"
> 
>-#if defined(XP_WIN)
>+#ifdef XP_WIN
>   typedef WCHAR NS_tchar;
>   #define NS_main wmain
> #else
>   typedef char NS_tchar;
>   #define NS_main main
> #endif
> 
> // Called to perform any initialization of the widget toolkit
> int InitProgressUI(int *argc, NS_tchar ***argv);
> 
>-#if defined(XP_WIN)
>+#ifdef XP_WIN
>   // Called on the main thread at startup
>   int ShowProgressUI(bool indeterminate = false, bool initUIStrings = true);
>   int InitProgressUIStrings();
>+#elif XP_MACOSX
BTW: the main thing I am looking for is consistency in the file so it doesn't make someone scratch their head and ask why is this different than the other cases. With that in mind you could have just used #elif defined(XP_MACOSX)

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>...
>@@ -2662,31 +2678,39 @@ int NS_main(int argc, NS_tchar **argv)
>       // update.
>       sReplaceRequest = true;
>     }
>   }
> 
>   // The directory containing the update information.
>   gPatchDirPath = argv[1];
> 
>+  InitProgressUI(&argc, &argv);
I don't think you want to call this when isElevated is true.

>+
> #ifdef XP_MACOSX
>   if (!isElevated && !IsRecursivelyWritable(argv[2])) {
>     // If the app directory isn't recursively writeable, an elevated update is
>     // required.
>-    gSucceeded = ServeElevatedUpdate(argc, (const char**)argv);
>-    if (!gSucceeded) {
>-      WriteStatusFile(ELEVATION_CANCELED);
>+    UpdateServerThreadArgs threadArgs;
>+    threadArgs.argc = argc;
>+    threadArgs.argv = const_cast<const NS_tchar**>(argv);
>+
>+    Thread t1;
>+    if (t1.Run(ServeElevatedUpdateThreadFunc, &threadArgs) == 0) {
>+      // Show an indeterminate progress bar while an elevated update is in
>+      // progress.
>+      ShowProgressUI(true);
>     }
>+    t1.Join();
>+
>     LaunchCallbackAndPostProcessApps(argc, argv, callbackIndex, false);
>     return gSucceeded ? 0 : 1;
>   }
> #endif
> 
>-  InitProgressUI(&argc, &argv);
>-

r=me with that

Comment 257

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #256)
> >diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
> >--- a/toolkit/mozapps/update/updater/updater.cpp
> >+++ b/toolkit/mozapps/update/updater/updater.cpp
> >...
> >@@ -2662,31 +2678,39 @@ int NS_main(int argc, NS_tchar **argv)
> >       // update.
> >       sReplaceRequest = true;
> >     }
> >   }
> > 
> >   // The directory containing the update information.
> >   gPatchDirPath = argv[1];
> > 
> >+  InitProgressUI(&argc, &argv);
> I don't think you want to call this when isElevated is true.
Actually it *might* be needed when elevated as well. Please check and if it isn't please don't call it.

Comment 258

[Stephen A Pohl [:spohl]]

Attachment: Addition to patch 1

Thanks, Robert. Sending this back for one last look.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #256)
> >diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
> >--- a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
> >+++ b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
[...]
> >-elevationFailure=You don't have the permissions necessary to install this update. Please contact your system administrator.
> >+elevationFailure=You donât have the permissions necessary to install this update. Please contact your system administrator.
> Weird, this didn't come through right. Please verify (over irc is fine) the
> character that should be used for the apostrophe with someone that does a
> lot of front end work.

I've grabbed your patch from Oak this time and folded it into this patch. Your patch passes the tests on Oak.

> >diff --git a/toolkit/mozapps/update/updater/progressui.h b/toolkit/mozapps/update/updater/progressui.h
> >--- a/toolkit/mozapps/update/updater/progressui.h
> >+++ b/toolkit/mozapps/update/updater/progressui.h
[...]
> >-#if defined(XP_WIN)
> >+#ifdef XP_WIN
> >   // Called on the main thread at startup
> >   int ShowProgressUI(bool indeterminate = false, bool initUIStrings = true);
> >   int InitProgressUIStrings();
> >+#elif XP_MACOSX
> BTW: the main thing I am looking for is consistency in the file so it
> doesn't make someone scratch their head and ask why is this different than
> the other cases. With that in mind you could have just used #elif
> defined(XP_MACOSX)

Changed to |#elif defined(XP_MACOSX)|.

> >diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
> >--- a/toolkit/mozapps/update/updater/updater.cpp
> >+++ b/toolkit/mozapps/update/updater/updater.cpp
> >...
> >+  InitProgressUI(&argc, &argv);
> I don't think you want to call this when isElevated is true.

I don't think it hurts, but it was unnecessary. I changed this to only be called when we're not elevated (or not on OSX).

Comment 259

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

Comment on attachment 8751927 [details] [diff] [review]
Addition to patch 1

>From: Stephen A Pohl <[e-mail]>
>
>Bug 394984: Show indeterminate progress UI while waiting for elevated update to complete on OSX. r=rstrong
>
>diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>--- a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>+++ b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>@@ -76,17 +76,17 @@ hideButton.accesskey=H
> applyingUpdate=Applying updateâ¦
> 
> updatesfound_minor.title=Update Available
> updatesfound_major.title=New Version Available
> 
> installSuccess=The Update was successfully installed
> installPending=Install Pending
> patchApplyFailure=The Update could not be installed (patch apply failed)
>-elevationFailure=You don't have the permissions necessary to install this update. Please contact your system administrator.
>+elevationFailure=You donât have the permissions necessary to install this update. Please contact your system administrator.
This is still not coming through. Please verify (over irc is fine) the character that should be used for the apostrophe with someone that does a lot of front end work.

No need for another review and r=me with that

Comment 260

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #259)
> Comment on attachment 8751927 [details] [diff] [review]
> Addition to patch 1
> 
> >From: Stephen A Pohl <[e-mail]>
> >
> >Bug 394984: Show indeterminate progress UI while waiting for elevated update to complete on OSX. r=rstrong
> >
> >diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
> >--- a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
> >+++ b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
> >@@ -76,17 +76,17 @@ hideButton.accesskey=H
> > applyingUpdate=Applying updateâ¦
> > 
> > updatesfound_minor.title=Update Available
> > updatesfound_major.title=New Version Available
> > 
> > installSuccess=The Update was successfully installed
> > installPending=Install Pending
> > patchApplyFailure=The Update could not be installed (patch apply failed)
> >-elevationFailure=You don't have the permissions necessary to install this update. Please contact your system administrator.
> >+elevationFailure=You donât have the permissions necessary to install this update. Please contact your system administrator.
> This is still not coming through. Please verify (over irc is fine) the
> character that should be used for the apostrophe with someone that does a
> lot of front end work.
> 
> No need for another review and r=me with that

FYI: we've verified that this is the proper character using a hex editor. Bugzilla seems to struggle with this character when clicking on "Edit as comment" on the patch, but the patch itself is correct.

Comment 261

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Updated for current trunk and folded attachment 8751927 [details] [diff] [review] into this patch. Carrying over r+.

Comment 262

[Stephen A Pohl [:spohl]]

Attachment: 3. Build config changes

Updated for current trunk. Carrying over r+.

Comment 263

[Stephen A Pohl [:spohl]]

Attachment: 4. Signing

Updated for current trunk. Carrying over r+.

Comment 264

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/856668f46beb39ef6b866ef9b59d033fbe03caad
Bug 394984: Enable any admin user on OSX to update Firefox, front-end and updater changes. r=rstrong

https://hg.mozilla.org/projects/oak/rev/4067bd399f603254c080c1078bb3a38b74e2ffdf
Bug 394984: Enable any admin user on OSX to update Firefox, native OSX changes. r=mstange

https://hg.mozilla.org/projects/oak/rev/5ceafec1ec1f59d94673b3b22f9dfad21340a89b
Bug 394984: Enable any admin user on OSX to update Firefox, build config changes. r=mshal

https://hg.mozilla.org/projects/oak/rev/c67766e70df912ce15baee26c953b0121803b40c
Bug 394984: Add signing certificate info to Info.plist files for Firefox and updater. r=bhearsum

Comment 265

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/e3799b6017b9d8bae07d4893f5fd0eee2a5779a7
Bug 394984: Update xpcshell tests to use the new updater binary name. r=rstrong

Comment 266

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/0cee74c3de68994ee53c4640c5bebc0644c121d1
Bug 394984: Enable any admin user on OSX to update Firefox, front-end and updater changes. r=rstrong

https://hg.mozilla.org/projects/oak/rev/7a3e03cd333da58cad74254b4dfb0bc1db3a8e0c
Bug 394984: Enable any admin user on OSX to update Firefox, native OSX changes. r=mstange

https://hg.mozilla.org/projects/oak/rev/9448761166f47ab9557f674909f90318a86e3d72
Bug 394984: Enable any admin user on OSX to update Firefox, build config changes. r=mshal

https://hg.mozilla.org/projects/oak/rev/00831cadc1a6cc0f93b67788e89b94db4dd7e9dd
Bug 394984: Add signing certificate info to Info.plist files for Firefox and updater. r=bhearsum

Comment 267

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/ccc38fd400731910be189e6aab353ec25b9936c0
Bug 394984: Update xpcshell tests to use the new updater binary name. r=rstrong

Comment 268

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Patch accidentally used an #elif where #else was required. Carrying over r+.

Comment 269

[zschuetz]

Hi folks, 

Very glad to see that work is being done on this. Do we have an estimate on when it will reach production ESR?

Comment 270

[Stephen A Pohl [:spohl]]

Attachment: 4. Signing

Updated for current trunk, carrying over r+.

Comment 271

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

This addresses bug 1273890.

Comment 272

[Stephen A Pohl [:spohl]]

Attachment: Diff for patch 1

Diff between attachment #8752431 [details] [diff] [review] and attachment #8754144 [details] [diff] [review].

Comment 273

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

As discussed.

Comment 274

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/991e0d46b42787eed4af24e354aba55d46c56565
Bug 394984: Fix Windows maintenance service updates, busted by elevated OSX updates. r=rstrong

Comment 275

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/e4b9309b562cbd4394754a9334e28df1b926640b
Bug 394984: Minor code cleanup. r=rstrong

Comment 276

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch to elevate updater

Updated for current trunk, carrying over r+.

Comment 277

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e33952e456ea3a7371c66b1a702ef8a0e17efb8b
Bug 394984: Enable any admin user on OSX to update Firefox, front-end and updater changes. r=rstrong

https://hg.mozilla.org/integration/mozilla-inbound/rev/a468fe7af93708141cbbc201890909adf4acad83
Bug 394984: Enable any admin user on OSX to update Firefox, native OSX changes. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd11745dc4526123de7a67d80c0084e5e089e85
Bug 394984: Enable any admin user on OSX to update Firefox, build config changes. r=mshal

https://hg.mozilla.org/integration/mozilla-inbound/rev/f12cdffc7f1ed51a790f8a57a8f57768d648691a
Bug 394984: Add signing certificate info to Info.plist files for Firefox and updater. r=bhearsum

https://hg.mozilla.org/integration/mozilla-inbound/rev/281d032de5ae9c4262903311e2cb3079acd2b9b3
Bug 394984: Update xpcshell tests to use the new updater binary name. r=rstrong

Comment 278

[Dave Miller [:justdave]]

It makes me so very happy to see this finally get fixed after 9 years.  Thank you very much to everyone who got involved to help pull this off and finally get it landed!

Comment 279

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e33952e456ea
https://hg.mozilla.org/mozilla-central/rev/a468fe7af937
https://hg.mozilla.org/mozilla-central/rev/0cd11745dc45
https://hg.mozilla.org/mozilla-central/rev/f12cdffc7f1e
https://hg.mozilla.org/mozilla-central/rev/281d032de5ae

Comment 282

[Stephen A Pohl [:spohl]]

(In reply to zschuetz from comment #269)
> Hi folks, 
> 
> Very glad to see that work is being done on this. Do we have an estimate on
> when it will reach production ESR?

This landed in Nightly version 49, so should make it into ESR 52.

Comment 283

[zschuetz]

(In reply to Stephen A Pohl [:spohl] from comment #282)
> This landed in Nightly version 49, so should make it into ESR 52.

Hmm, any chance of an uplift to the 45 series? The sooner the better on this one.

Comment 284

[Stephen A Pohl [:spohl]]

(In reply to zschuetz from comment #283)
> (In reply to Stephen A Pohl [:spohl] from comment #282)
> > This landed in Nightly version 49, so should make it into ESR 52.
> 
> Hmm, any chance of an uplift to the 45 series? The sooner the better on this
> one.

Highly unlikely. This is a large change/feature that should ride the trains. developer.mozilla.org[1] has more info if you're looking to manage Firefox deployments.

[1] https://developer.mozilla.org/en-US/Firefox/Enterprise_deployment

Comment 285

[Marcia Knous [:marcia]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Long standing bug that made it difficult for non admins to update Firefox
[Suggested wording]: 
Users who did not originally install Firefox can now update it. 
[Links (documentation, blog post, etc)]:

ni on Stephen to see if he agrees with the suggested wording.

Comment 286

[Stephen A Pohl [:spohl]]

(In reply to Marcia Knous [:marcia - use ni] from comment #285)
Thanks, Marcia! I would change the release notes slightly to the following:

Release Note Request (optional, but appreciated)
[Why is this notable]: Long standing bug on OSX that made it difficult for users on multi-user systems to update Firefox.
[Suggested wording]: 
Users who did not originally install Firefox can now update it if they can provide administrator credentials.
[Links (documentation, blog post, etc)]:
SecReview document: https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185
UX review document: https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=52265201
These documents aren't public, but could be made public. The main reason these documents live on mana is because it provides integrated support for Gliphy to display diagrams etc.

Comment 287

[Andrei Vaida [:avaida]]

[Test Plan]:
https://wiki.mozilla.org/QA/Elevated_update

Comment 289

[Marco Brandizi]

Didn't work for me with 71->72.0.1 update. I tried it from 'About', clicked on new version link, clicked on 'Restart Firefox' as requested, when Firefox restarts, it asks again to restart and hasn't written anything. I don't keep Firefox.app on /Applications, cause I'm more comfortable with having the Apps that I added to the macOS defaults somewhere like /Applications/Local. The latter is writeable by admin users only and my usual user hasn't admin powers, because, in case someone needs it to be reminded, it's stupid to work as admin on a regular basis.

Thunderbird has had a similar problem for years, now it seems to work right and it's appalling that Mozilla can't factorise code for this simple functionality. It's even more incredible that this problem comes up over and over again, not just for FF, I don't get why it's so difficult to understand that you have to ask to switch user when the current one hasn't enough rights, and you can't assume that all users in the world are so computer illiterate that they regularly work with admin powers.

I'm quite resigned to do it the usual clumsy way: download the update and put the .app in place, as if it was a new installation.

Comment 290

[Yannis Juglaret [:yannis]]

Hi! Following recent discussions, the phrasing of the Install Helper dialog still seems to scare some users when it appears today. And the UX review here mentions that we do not have control over the text of that dialog. So I would like to leave a note here that although it cannot be completely replaced, I believe it should be possible to add an extra sentence that we control before the default one "__APPNAME__ is trying to install a new helper tool.". Here is an example of what this can look like for another application.

The documentation for AuthorizationCopyRights mentions that In macOS 10.3 and later, you can pass icon or prompt data to be used in the authentication dialog box, and in our case that prompt data should become a prefix to the standard OS string. I believe that would require providing a custom environment to AuthorizationCopyRights in InstallPrivilegedHelper instead of kAuthorizationEmptyEnvironment. The custom environment should contain a kAuthorizationEnvironmentPrompt item. Here is an example of how such an environment can be created.

I hope this information can be useful!

Comment 291

[Dave Miller [:justdave]]

(In reply to Yannis Juglaret from comment #290)

Because this bug is marked FIXED, I believe this needs to be filed as a new bug, which can be added as dependent on this one.

Comment 292

[Yannis Juglaret [:yannis]]

(In reply to Dave Miller [:justdave] from comment #291)

(In reply to Yannis Juglaret from comment #290)

Because this bug is marked FIXED, I believe this needs to be filed as a new bug, which can be added as dependent on this one.

Here is the new one: bug 1789676. Thanks!