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

RESOLVED FIXED in Firefox 49

Status

()

P1
normal
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: moco, Assigned: spohl)

Tracking

(Depends on: 5 bugs)

Trunk
mozilla49
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
sec-review +

Firefox Tracking Flags

(firefox49 fixed, relnote-firefox 49+)

Details

(URL)

Attachments

(6 attachments, 61 obsolete attachments)

149.59 KB, image/jpeg
phlsa
: feedback+
Details
1.01 KB, patch
spohl
: review+
Details | Diff | Splinter Review
19.89 KB, patch
spohl
: review+
Details | Diff | Splinter Review
9.88 KB, patch
spohl
: review+
Details | Diff | Splinter Review
2.09 KB, patch
spohl
: review+
Details | Diff | Splinter Review
101.09 KB, patch
spohl
: review+
Details | Diff | Splinter Review
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.
this was on Mac OS X 10.4.x
OS: Windows Vista → Mac OS X
Product: Firefox → Toolkit
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.
Duplicate of this bug: 518513
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.
Severity: normal → critical
Flags: blocking1.9.2?
Flags: blocking-thunderbird3?
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?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
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.
Severity: critical → normal
Flags: blocking1.9.2+
Flags: blocking-thunderbird3?

Updated

10 years ago
Duplicate of this bug: 425863

Comment 8

9 years ago
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/ ?
(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.
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

9 years ago
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
Summary: software update disabled on the mac if admin user is not the same admin user as the person who installed firefox → Unable to update on mac if admin user is not the same admin user as the person who installed firefox

Comment 16

9 years ago
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

9 years ago
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 #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

9 years ago
I think that hecate@gis.net 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

8 years ago
Does Firefox 4.0 resolve this issue?

Thanks

Comment 22

8 years ago
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

8 years ago
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
(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

8 years ago
awesome, much thanks.

Updated

8 years ago
Duplicate of this bug: 682049

Comment 28

8 years ago
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

8 years ago
See also this relating Camino bug:

Bug 445629 - Land Sparkle 1.5 once it's done
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

8 years ago
(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... :-(
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

8 years ago
(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.
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

8 years ago
(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.
Duplicate of this bug: 786719

Comment 38

6 years ago
Any progress on this bug, especially taking to account comment #10, comment #24 and comment #25 (as direct reply to the latter)?
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
(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.
(Assignee)

Comment 41

5 years ago
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.
Assignee: nobody → spohl.mozilla.bugs
Attachment #8389934 - Flags: review?(smichaud)
(Assignee)

Comment 42

5 years ago
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.
Attachment #8389942 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 43

5 years ago
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.
(Assignee)

Comment 44

5 years ago
(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.
(Assignee)

Comment 45

5 years ago
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 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.
Attachment #8389942 - Flags: review?(robert.strong.bugs) → review-
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.
Attachment #8389947 - Flags: feedback?(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?
(Assignee)

Comment 49

5 years ago
(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
Depends on: 978596
(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?
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Updated

5 years ago
Depends on: 983728
(Assignee)

Comment 51

5 years ago
(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!
Flags: needinfo?(spohl.mozilla.bugs)
(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.
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 53

5 years ago
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.
Flags: needinfo?(spohl.mozilla.bugs)
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...
Attachment #8389947 - Flags: feedback?(bhearsum)
(Assignee)

Comment 55

5 years ago
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.
Attachment #8389942 - Attachment is obsolete: true
Attachment #8393027 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 56

5 years ago
Posted patch Part 3: Test changes (WIP) (obsolete) — Splinter Review
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.
(Assignee)

Comment 57

5 years ago
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.
Attachment #8389934 - Flags: review?(smichaud)
(Assignee)

Comment 58

5 years ago
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.
(Assignee)

Comment 59

5 years ago
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.
Attachment #8389934 - Attachment is obsolete: true
(Assignee)

Comment 60

5 years ago
Updated for the fact that patch part 1 is now obsolete.
Attachment #8393027 - Attachment is obsolete: true
Attachment #8393027 - Flags: review?(robert.strong.bugs)
Attachment #8394209 - Flags: review?(robert.strong.bugs)
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.
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 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.
Attachment #8394209 - Flags: review?(robert.strong.bugs) → feedback?(robert.strong.bugs)
(Assignee)

Updated

5 years ago
Depends on: 978597
(Assignee)

Comment 64

5 years ago
Posted image Flow Diagram (obsolete) —
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
(Assignee)

Comment 65

5 years ago
Updated for current trunk.
Attachment #8431040 - Flags: feedback?(robert.strong.bugs)
(Assignee)

Updated

5 years ago
Attachment #8394209 - Attachment is obsolete: true
Attachment #8394209 - Flags: feedback?(robert.strong.bugs)
(Assignee)

Updated

5 years ago
Attachment #8393106 - Attachment description: Part 4: Test changes (WIP) → Part 3: Test changes (WIP)
(Assignee)

Updated

5 years ago
Attachment #8389947 - Attachment description: Part 3: Signing of firefox and org.mozilla.updater binaries (WIP) → Part 2: Signing of firefox and org.mozilla.updater binaries (WIP)
(Assignee)

Comment 66

5 years ago
Started a document for the security review here:
https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=40044185
(Assignee)

Comment 67

5 years ago
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/ckoenig@mozilla.com/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)
Flags: sec-review?
(Assignee)

Comment 68

5 years ago
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.
Flags: sec-review? → sec-review?(dveditz)
(Assignee)

Updated

5 years ago
Blocks: 1027651
(Assignee)

Comment 70

5 years ago
(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.
Flags: needinfo?(robert.strong.bugs)
(Assignee)

Comment 71

5 years ago
(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.
(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.
Flags: needinfo?(robert.strong.bugs)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 1026952
(Assignee)

Updated

5 years ago
Depends on: 1036424
(Assignee)

Updated

5 years ago
Duplicate of this bug: 1063472
(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.
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 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.
Attachment #8431040 - Flags: feedback?(robert.strong.bugs) → feedback?(netzen)
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.
Attachment #8431040 - Flags: feedback?(netzen)
(Assignee)

Comment 80

4 years ago
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.
Assignee: spohl.mozilla.bugs → nobody

Updated

4 years ago
Duplicate of this bug: 1115049
Are you able to work on this? We really could use a fix for this soon.
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 83

4 years ago
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.
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 85

4 years ago
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!
Attachment #8431040 - Flags: feedback?(mh+mozilla)
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.
Attachment #8431040 - Flags: feedback?(mh+mozilla)
(Assignee)

Updated

4 years ago
Assignee: nobody → spohl.mozilla.bugs
(Assignee)

Comment 87

4 years ago
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.
Attachment #8389947 - Attachment is obsolete: true
(Assignee)

Comment 88

4 years ago
Posted patch 1. Patch (obsolete) — Splinter Review
Updated for current trunk (thanks, glandium!).
Attachment #8431040 - Attachment is obsolete: true
(Assignee)

Comment 89

4 years ago
Posted patch 1. Patch (obsolete) — Splinter Review
Removed an unused variable and changed the executable name to 'org.mozilla.updater' in the updater.app's Info.plist file.
Attachment #8641077 - Attachment is obsolete: true
(Assignee)

Comment 90

4 years ago
Posted patch 1. Patch (obsolete) — Splinter Review
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
Attachment #8641853 - Attachment is obsolete: true
(Assignee)

Comment 91

4 years ago
Posted patch 2. Certificate info (obsolete) — Splinter Review
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!
Flags: needinfo?(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
Flags: needinfo?(bhearsum)
(Assignee)

Comment 93

4 years ago
(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.
(Assignee)

Comment 94

4 years ago
Posted patch 1. Patch (obsolete) — Splinter Review
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.
Attachment #8645905 - Attachment is obsolete: true
(Assignee)

Comment 95

4 years ago
Posted patch 2. Certificate info (obsolete) — Splinter Review
Based on comment 92, this may be all we need for the signing certificate info to be embedded into the Info.plist files.
Attachment #8645936 - Attachment is obsolete: true
(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.
(Assignee)

Comment 97

4 years ago
(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.
(Assignee)

Comment 98

4 years ago
Working through a few issues in the original design. Clearing the sec-review flag for now.
Flags: sec-review?(dveditz)
(Assignee)

Comment 99

4 years ago
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!
Flags: needinfo?(philipp)
(Assignee)

Comment 100

4 years ago
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/ckoenig@mozilla.com/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)
Flags: sec-review?
(Assignee)

Comment 101

4 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
Updated for current trunk.
Attachment #8647289 - Attachment is obsolete: true
(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.
Flags: needinfo?(philipp)
(Assignee)

Comment 103

4 years ago
(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.
(Assignee)

Comment 104

3 years ago
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!
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(philipp)
(Assignee)

Comment 105

3 years ago
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!
Flags: needinfo?(dveditz)
(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.
Flags: needinfo?(robert.strong.bugs)
(Assignee)

Comment 108

3 years ago
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
Attachment #8424096 - Attachment is obsolete: true
(Assignee)

Comment 109

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
Attachment #8667262 - Attachment is obsolete: true
Attachment #8682651 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 110

3 years ago
Attachment #8393106 - Attachment is obsolete: true
Attachment #8647290 - Attachment is obsolete: true
Attachment #8682653 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 111

3 years ago
Chris, would you be able to forward this review to someone during bhearsum's absence? Thank you!
Attachment #8682657 - Flags: review?(catlee)
(Assignee)

Comment 112

3 years ago
Posted patch 4. UX (obsolete) — Splinter Review
Waiting to request review until Philipp was able to weigh in.
(Assignee)

Comment 113

3 years ago
Posted patch 5. Telemetry (obsolete) — Splinter Review
Attachment #8682663 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 114

3 years ago
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.
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 116

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
Fixed some incorrect whitespace.
Attachment #8682651 - Attachment is obsolete: true
Attachment #8682651 - Flags: review?(robert.strong.bugs)
Attachment #8683239 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 117

3 years ago
Posted patch 6. Update existing tests (obsolete) — Splinter Review
This makes all the existing tests pass locally. Sent to try for confirmation (see comment 115).
Attachment #8683244 - Flags: review?(robert.strong.bugs)
(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!
Flags: needinfo?(philipp)
(Assignee)

Comment 119

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

This needs more work.
Attachment #8683244 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 120

3 years ago
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?
Attachment #8682661 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 122

3 years ago
Posted patch 6. Update existing tests (obsolete) — Splinter Review
Sent to try (comment 121).
Attachment #8683244 - Attachment is obsolete: true
(Assignee)

Comment 123

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

Try is green.
Attachment #8683709 - Flags: review?(robert.strong.bugs)
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
Attachment #8682657 - Flags: review?(catlee)
(Assignee)

Updated

3 years ago
Depends on: 1222555
(Assignee)

Comment 125

3 years ago
Posted patch 7. Tests (wip) (obsolete) — Splinter Review
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!
Attachment #8685536 - Flags: feedback?(robert.strong.bugs)

Updated

3 years ago
Duplicate of this bug: 1215561

Updated

3 years ago
See Also: → bug 1171792
(Assignee)

Updated

3 years ago
Attachment #8682657 - Flags: review?(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.
Attachment #8682657 - Flags: review?(mshal)
Attachment #8682657 - Flags: review?(bhearsum)
Attachment #8682657 - Flags: review+
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.
Attachment #8682653 - Flags: review?(robert.strong.bugs) → review?(mstange)
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?
(Assignee)

Comment 130

3 years ago
(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.
(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.
(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.
(Assignee)

Comment 133

3 years ago
(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
(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?
(Assignee)

Comment 135

3 years ago
(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).
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 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 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.
Attachment #8682653 - Flags: review?(mstange) → review+
(Assignee)

Comment 139

3 years ago
(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.
(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 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.
Attachment #8683709 - Flags: review?(robert.strong.bugs) → review+
(Assignee)

Comment 142

3 years ago
(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 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.
Attachment #8682657 - Flags: review?(mshal) → feedback+
(Assignee)

Comment 144

3 years ago
(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.
(Assignee)

Updated

3 years ago
Attachment #8683239 - Flags: review?(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.
Attachment #8683239 - Flags: review?(mshal) → review+
(Assignee)

Comment 146

3 years ago
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
Flags: needinfo?(mshal)
(Assignee)

Comment 147

3 years ago
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.
Attachment #8683709 - Flags: review?(mshal)
Attachment #8683709 - Flags: review?(mshal) → review+
(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?
Flags: needinfo?(mshal)
(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 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.
Attachment #8683239 - Flags: review?(robert.strong.bugs) → review-
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 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?
Attachment #8682661 - Flags: review?(robert.strong.bugs) → review-
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 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.
Attachment #8682663 - Flags: review?(robert.strong.bugs) → review-
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.
Attachment #8685536 - Flags: feedback?(robert.strong.bugs) → feedback-
(Assignee)

Comment 156

3 years ago
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.
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.
(Assignee)

Comment 158

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
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?
Attachment #8682661 - Attachment is obsolete: true
Attachment #8682663 - Attachment is obsolete: true
Attachment #8683239 - Attachment is obsolete: true
Attachment #8701096 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 159

3 years ago
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.
(Assignee)

Comment 160

3 years ago
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.
Attachment #8701096 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 161

3 years ago
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
Attachment #8703797 - Flags: feedback?(philipp)
(Assignee)

Comment 162

3 years ago
Posted patch 3. Build config changes (obsolete) — Splinter Review
(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).
Attachment #8682657 - Attachment is obsolete: true
Attachment #8707107 - Flags: review?(mshal)
(Assignee)

Comment 163

3 years ago
Split out build config changes into patch 3. Carrying over r+.
Attachment #8683709 - Attachment is obsolete: true
Attachment #8707108 - Flags: review+
(Assignee)

Comment 164

3 years ago
Posted patch 3. Build config changes (obsolete) — Splinter Review
Forgot to update for current trunk.
Attachment #8707107 - Attachment is obsolete: true
Attachment #8707107 - Flags: review?(mshal)
Attachment #8707113 - Flags: review?(mshal)
Comment on attachment 8707113 [details] [diff] [review]
3. Build config changes

Looks good to me now. Thanks for fixing the srcdir issue!
Attachment #8707113 - Flags: review?(mshal) → review+
(Assignee)

Comment 166

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
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.
Attachment #8701096 - Attachment is obsolete: true
Attachment #8707890 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 167

3 years ago
Posted patch 2. Obj-C changes (obsolete) — Splinter Review
I split out all the Objective-C changes into this patch for easier review.
Attachment #8682653 - Attachment is obsolete: true
Attachment #8707891 - Flags: review?(mstange)
(Assignee)

Comment 168

3 years ago
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!
Attachment #8707892 - Flags: feedback?(mstange)
(Assignee)

Comment 169

3 years ago
Posted patch 4. Signing (obsolete) — Splinter Review
Split out the .plist changes that were r+'d by bhearsum in comment 127 from the buildconfig changes in patch 3. Carrying over r+.
Attachment #8707894 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8685536 - Attachment is obsolete: true
(Assignee)

Comment 170

3 years ago
Posted patch 1.b additional UX changes (obsolete) — Splinter Review
This implements the additional UX changes mentioned in comment 161. Philipp, have you had a chance to review the questions there yet? Thanks!
Attachment #8707896 - Flags: feedback?(philipp)
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?
Attachment #8707892 - Flags: feedback?(mstange)
(Assignee)

Comment 172

3 years ago
(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.
(Assignee)

Comment 173

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

Please see comment 168, comment 171 and comment 172.
Attachment #8707892 - Flags: review?(mshal)
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.
Attachment #8707891 - Flags: review?(mstange) → review+
(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 on attachment 8707892 [details] [diff] [review]
3.b Buildconfig changes to support patch 2

I don't see any problems with this.
Attachment #8707892 - Flags: review?(mshal) → review+
(Assignee)

Comment 177

3 years ago
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!
Attachment #8707896 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 178

3 years ago
(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
(Assignee)

Comment 179

3 years ago
Posted patch 3. Build config changes (obsolete) — Splinter Review
Folded patch 3 and patch 3.b, which were reviewed at separate times. Carrying over r+.
Attachment #8707113 - Attachment is obsolete: true
Attachment #8707892 - Attachment is obsolete: true
Attachment #8712215 - Flags: review+
(Assignee)

Comment 180

3 years ago
Posted patch 2. Obj-C changes (obsolete) — Splinter Review
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
Attachment #8707891 - Attachment is obsolete: true
Attachment #8712784 - Flags: review?(mstange)
(Assignee)

Comment 181

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
Changed |getElevationRequired| in nsUpdateService.js to also check for write permission on the root of the install directory, not just its descendants.
Attachment #8707890 - Attachment is obsolete: true
Attachment #8707890 - Flags: review?(robert.strong.bugs)
Attachment #8712792 - Flags: review?(robert.strong.bugs)
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 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/
(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 #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'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.
(Assignee)

Comment 187

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
(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.
Attachment #8712792 - Attachment is obsolete: true
Attachment #8712792 - Flags: review?(robert.strong.bugs)
Attachment #8719881 - Flags: feedback?(robert.strong.bugs)
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.
Attachment #8712784 - Flags: review?(mstange) → review+
(Assignee)

Comment 189

3 years ago
Posted patch 2. Obj-C changes (obsolete) — Splinter Review
Thanks, Markus! This patch clarifies the comment that the "remove" argument will terminate the current process when passed to /bin/launchctl.
Attachment #8712784 - Attachment is obsolete: true
Attachment #8725715 - Flags: review+
(Assignee)

Comment 190

3 years ago
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.
Attachment #8719881 - Attachment is obsolete: true
Attachment #8719881 - Flags: feedback?(robert.strong.bugs)
(Assignee)

Comment 191

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

I'll fold this patch into the new patch "1".
Attachment #8707896 - Attachment is obsolete: true
Attachment #8707896 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 192

3 years ago
Posted patch 2. Obj-C changes (obsolete) — Splinter Review
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+.
Attachment #8725715 - Attachment is obsolete: true
Attachment #8727566 - Flags: review+
(Assignee)

Comment 193

3 years ago
Posted patch 3. Build config changes (obsolete) — Splinter Review
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?
Attachment #8712215 - Attachment is obsolete: true
Attachment #8727570 - Flags: review?(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.
Attachment #8727570 - Flags: review?(mshal) → review+
(Assignee)

Comment 195

3 years ago
Posted patch 3. Build config changes (obsolete) — Splinter Review
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+.
Attachment #8727570 - Attachment is obsolete: true
Attachment #8727931 - Flags: review+
(Assignee)

Comment 196

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
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.
Attachment #8728586 - Flags: review?(robert.strong.bugs)
FYI: hoping to have this done today.
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.
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) {
(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 }
(Assignee)

Comment 201

3 years ago
Posted patch 3. Build config changes (obsolete) — Splinter Review
Updated for current trunk, carrying over r+.
Attachment #8727931 - Attachment is obsolete: true
Attachment #8736111 - Flags: review+
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.
Attachment #8728586 - Flags: review?(robert.strong.bugs) → review+
Would be good to give a heads up to Thunderbird and SeaMonkey.
(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
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.
(Assignee)

Comment 206

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
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.
Attachment #8728586 - Attachment is obsolete: true
Attachment #8737469 - Flags: review+
Attachment #8737469 - Flags: feedback?(robert.strong.bugs)
(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.
(Assignee)

Comment 209

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
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.
Attachment #8737469 - Attachment is obsolete: true
Attachment #8737469 - Flags: feedback?(robert.strong.bugs)
Attachment #8737474 - Flags: review+
Attachment #8737474 - Flags: feedback?(robert.strong.bugs)
(Assignee)

Comment 210

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

Clearing f? until build bustage on Windows and Linux is resolved.
Attachment #8737474 - Flags: feedback?(robert.strong.bugs)
BTW: I cleaned up oak and merged m-c over so it should be all set when you are ready to land it there
(Assignee)

Comment 214

3 years ago
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|).
Attachment #8737474 - Flags: feedback?(robert.strong.bugs)
(Assignee)

Comment 215

3 years ago
(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 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
Attachment #8737474 - Flags: review+
Attachment #8737474 - Flags: feedback?(robert.strong.bugs)
(Assignee)

Comment 217

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
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.
Attachment #8737474 - Attachment is obsolete: true
Attachment #8738709 - Flags: review+
(Assignee)

Comment 218

3 years ago
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
(Assignee)

Comment 219

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

Comment 220

3 years ago
Posted patch 1. Patch to elevate updater (obsolete) — Splinter Review
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.
Attachment #8738709 - Attachment is obsolete: true
Attachment #8741437 - Flags: review+
(Assignee)

Comment 222

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

Comment 223

3 years ago
Posted patch Addition to patch 1 (obsolete) — Splinter Review
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.
Attachment #8745506 - Flags: review?(robert.strong.bugs)
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.
Attachment #8745506 - Flags: review?(robert.strong.bugs)
BTW: I might have reversed complete with partial but I know one of them takes a substantial amount of time on Mac.
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.
(Assignee)

Comment 227

3 years ago
Posted patch Addition to patch 2 (obsolete) — Splinter Review
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.
Attachment #8745517 - Flags: review?(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.
Attachment #8745517 - Flags: review?(mstange) → review+
Comment hidden (advocacy)
(Assignee)

Comment 230

3 years ago
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
Flags: needinfo?(robert.strong.bugs)
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?
Flags: needinfo?(robert.strong.bugs)