Background updates should work with Group/World Writable

[dd32]

At present we apply a strict owner check to created files before using the 'direct' file operations (ie. not FTP) for updates, this is due to many hosts FTP access not allowing access to non-owned files.

Background Updates, by design, do not create new files (at least in-branch core updates & translations), and only alter existing WordPress files. As a result of this, we can relax the strict checks for these updates, allowing security releases to be installed automatically in more cases.

The 1.7 version-check API returns a flag on autoupdate responses new_files which specifies if the update from the current version to that package will mean new files (ie. 3.9 -> 4.0.1 is true, yet 4.0 -> 4.0.1 is false), we can leverage that flag to enable group-writable.

Please keep this ticket on track and lets not debate Group/World Writable for plugins/themes/others here, that's been done over in #10205 & #8794

[dd32]

Attached are working attempts at this for Background Core updates, translations not implemented.

Enabling this conditionally requires significant parameter passing, as the filesystem checks are called in many different locations.

30245.diff​ initially went with subclassing WP_Filesystem_Direct to WP_Filesystem_Direct_GroupWritable as a way of flagging that FS_CHMOD_DIR / FS_CHMOD_FILE should be initialized with a different default.
However, the problem here, is that we might fall through to the WorldWritable case and set world-writable, when in actual fact, it should only be owner/group writable.
Consider a scenario where the file owner check functions are unavailable or broken, but we were still able to write the files, this would result in it thinking it was world-writable.. and chmoding things to 0666 when it should only be 0644.

30245.2.diff​ works around this by not setting a higher default value for FS_CHMOD_FILE/FS_CHMOD_DIR and relying on fileperms() returning 0664 or 0666 as appropriate for the configuration.
The worst case scenario here, is that WordPress might do a background update with files owned as 0666 and alter the permissions of some files to 0644, I think this is a far better scenario than the alternative of accidentally making the files world writable, and the patch is far smaller in addition.

One other potential problem we've got is that we use PHP's copy() method to copy a file (from the extracted zip) owned by the PHP user over the top of a file owned by the user. PHP achieves this by simply opening both files, ​and then performing a stream copy which means file ownership is kept if the file exists, if the destination file doesn't exist, it's created as being owned by PHP instead of being owned by the user.

30245.3.diff​ attempts to work around this by setting a flag on WP_Upgrader when relaxed file ownership is in use, and triggers pre-flight file_exists() checks in Core_Upgrader, this appears to work as expected in the limited testing I've done.

[ocean90]

Can we use this for nightly builds after a beta is released too? If so, it should be in beta 2 IMO.

[dd32]

In 30384:

Background Updates: Introduce support to take advantage of Group Writable (or World Writable) to Core Background updates.
This is only enabled when new files will not be installed during the update (as indicated by the WordPress.org API), and does not apply to Plugin/Theme/Translation Background Updates.

Additionally, the code to determine if the 'direct' filesystem transport should be used has been tweaked for wider support (where getmyuid() was unavailalbe) which fixes #10424

See #10205, #30245

[dd32]

Can we use this for nightly builds after a beta is released too?

Yes, we'll tweak the WordPress.org API to understand that 4.1-beta builds don't have any new files (when updating from another install >= 4.1-beta1)

In 30384

The changes in 30245.3.diff​ to pre-check that the files exist has been dropped in this commit, after discussions we decided that having a incorrectly owned file was a far better user experience than having a file missing altogether. This scenario is highly unlikely to be hit however, as only in-branch releases will trigger this check, and usually with a partial build (And partial builds usually only modify core files, which if one is missing there'll generally be fatal errors anyway, unlike say a missing readme.html or tinymce image).

[johnbillion]

What's left here?

[dd32]

Please open new tickets for any issues regarding this. Closing as fixed for 4.1

[dd32]

In 30860:

Background Updates: Pass back whether Group Writable support is being leveraged for an update to the WordPress.org API.
See #30245

[dd32]

Re-opening for stats. In order to track success and error rates we need to pass the status of what method WordPress is using, 30245.stats.diff​ simply uses a global, it's pretty hacky, but it does what's needed and is actually pretty clean solution compared to the alternatives.

[30860] adds this to trunk, needs merging to branch.

Can we use this for nightly builds after a beta is released too?

Yes, we'll tweak the WordPress.org API to understand that 4.1-beta builds don't have any new files (when updating from another install >= 4.1-beta1)

It looks like this never happened unfortunately, as I've only just realised, so noting it here.

[johnbillion]

In 30920:

Background Updates: Pass back whether Group Writable support is being leveraged for an update to the WordPress.org API.

Merges [30860] to the 4.1 branch.

See #30245

[johnbillion]

[30860] is a yucky bit of code, but we don't have time to fix it.

The remaining tweaks are up to the WordPress.org API folks.