Change personal data export path stored in request meta to relative paths

[allendav]

Props @sergey for the idea.

I'd like to change it so we store file names relative to the exports directory rather than full path names (the latter would break when moving to another server, and I don't think we store full paths anywhere else in core, except for the list of recent files in plugin/theme editor), but I guess that can wait for 4.9.7, and since the files expire in a few days, doesn't seem like a big issue.

[desrosj]

Moving to the new Privacy component.

[desrosj]

Marking privacy bugs as introduced in 4.9.6.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[mrTall]

On this diff it store only filename, cuz we have full path through $exports_dir = wp_privacy_exports_dir();

[desrosj]

4.9.8 has entered release candidate stages and this still needs testing and unit tests.

[desrosj]

Thanks for the patch, @mrTall! 44038.patch​ stores the relative patch for new exports, but it has one issue, and there are some additional scenarios that should be considered for backwards compatibility.

44038.patch​ assumes the value stored in meta is always relative and adds the $exports_dir to the beginning of the value without examining it. Say a site has an old export (created before this change) that includes the full path in the meta value (/srv/www/wordpress/public_html/wp-content/exports/exportfile.zip, for example). The patch will always add the path to the beginning, even if it is the same, becoming /srv/www/wordpress/public_html/wp-content/exports/srv/www/wordpress/public_html/wp-content/exports/exportfile.zip.

These are the scenarios that should also be accounted for:

1. A relative path stored in meta (accounted for in 44038.patch​).
2. A relative path stored in meta that becomes invalid (maybe a new plugin changes the data export directory via a filter, for example)
3. A full path stored in meta that remains valid after this change.
4. A full path stored in meta that becomes invalid when a site is migrated (either to a new server, or moving WordPress to a new directory on the same server).
5. A full path stored in meta that remains valid, but the export directory has changed (plugin changes the data export directory).

All these things considered, I think this is the best plan forward:

• If a relative path is stored, assume it is in the current export directory. Unfortunately, old links will become dead, but this is how attachment files are currently handled (see _wp_attached_file meta key).
• If a full path is stored, check if it is still valid and usable. If it is and it can be protected from being browsable, leave the full path in meta and continue to use that path to preserve export links.
• If a full path is stored and it is not valid, usable, or cannot be protected from browsing, use the same filename but the current export directory and save the relative path to meta. Old links will also become dead here, though.

@SergeyBiryukov does this make sense? Did I miss anything?

The unit tests for this also depend on #44233. They should reside within the test class being created there.

[garrett-eclipse]

Thanks for the clear direction @desrosj as there's a straight-forward plan here going to milestone for 5.3 and label good-first-bug in preparation for wceu contributor day.

[cameronamcintyre]

@desrosj I think I was able to cover everything this way?

[nmenescardi]

I’ve been reviewing this ticket. I applied last patch (44038.2) and ran existing tests in privacy group, they all passed.

Also, I created two more tests related with this ticket using absolute paths. I tried to follow some of the points highlighted on @desrosj comment: https://core.trac.wordpress.org/ticket/44038#comment:12

I found two possible issues with this patch:
1) if a not valid path is saved (eg: change hosting) I believe it doesn’t resolve a new valid directory and file name.

2) If an old valid absolute path was saved, it doesn’t keep that value. Instead, it generates and saves a relative path as new value. It works with new links but probably it doesn’t preserve old ones.

Tests are added in a new patch: 44038.3 along with previous changes.

[nmenescardi]

It contains some tests an changes in 44038.2 patch

[garrett-eclipse]

Refresh to have indentation conform to CS

[garrett-eclipse]

Thanks for the patches @nmenescardi / @cameronamcintyre / @mrTall

I've refreshed the patch in 44038.4.diff for minor CS items. It's currently passing unit tests as well.

As there are some concerns with the current implementation (thanks @nmenescardi) I'm flagging with dev-feedback and moving to 5.4 since we're already in 5.3-beta2.

@SergeyBiryukov or @desrosj would you mind reviewing the implementation and providing further direction and feedback.

Thank you

[xkon]

I've tested the 44038.4.diff​ but it was generating extra .zip files for me in some instances depending on the if statements that are already present.

44038.5.diff​ takes a different approach and removes all previous postmeta used for the files _export_file_url and _export_file_path while keeping/creating the new _export_file_name that .4 patch introduced.

This way the database only keeps a filename and no records of hard paths/links at all so everything has to pass via the wp_privacy_exports_dir() & wp_privacy_exports_url() to build a full path and url which are following the upload_dir() so they should be fully filterable and updatable from installation to installation.

We still need to get the _export_file_url and _export_file_path via get_post_meta() for the sanity checks and to gradually move away from them.

To test this:

1] Add some random export requests ( no need to have actual data as the zips are getting generated either way ). On some, proceed to the download so the "original" postmeta can be created and on some others leave them as pending.

2] Apply the patch and click on a Download Personal Data or send an e-mail etc normally.

The database should be fully updated to use only _export_file_name and the links, as well as .zips, should be unique and generated properly without leaving any extra files or database trails to not-used .zips.

I think that this gets everything on what @SergeyBiryukov proposed and it should be fully backward compatible due to the addition/removal of the postmeta on each case if I didn't miss anything :D.

[xkon]

Marking this for 5.5 as 5.4 beta is close, the patch works but I'd like some 2nd eyes first and more testing for edge cases before this goes for a commit :-).

[whyisjake]

In 48087:

Privacy: Use relative paths for exported personal data.

Ensures back-compat while moving to paths off of the /exports directory.

Fixes: #44038.

Props: allendav, mrTall, desrosj, garrett-eclipse, cameronamcintyre, nmenescardi, xkon, whyisjake.

[whyisjake]

In 48088:

Privacy: Revert use relative paths for exported personal data.

Tests need to be updated to pass.

See: #44038.

Unprops: whyisjake.

[whyisjake]

Tests need to be updated, @xkon can you take a look?

[davidbaumwald]

Patch with unit tests updates

[whyisjake]

In 48127:

Privacy: Use relative paths for exported personal data.

Ensures back-compat while moving to paths off of the /exports directory.

Fixes: #44038.

Props: allendav, mrTall, desrosj, garrett-eclipse, cameronamcintyre, nmenescardi, xkon, whyisjake, davidbaumwald.

[SergeyBiryukov]

In 48330:

Privacy: Simplify the logic for updating the meta values for personal data export requests from absolute to relative paths.

Follow-up to [48127].

See #44038.