Font Library: font family and font uploads capability checks #59294
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
creativecoder
added
[Type] Bug
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ phpunit/tests/fonts/font-library/deleteFontPostMetaCaps.php ❔ phpunit/tests/fonts/font-library/maybeGrantUploadFontCap.php ❔ lib/compat/wordpress-6.5/fonts/class-wp-rest-font-faces-controller.php ❔ lib/compat/wordpress-6.5/fonts/fonts.php |
creativecoder
requested review from
pbking,
youknowriad,
getdave,
peterwilsoncc,
swissspidy,
matiasbenedetto and
mikachan
|
Size Change: +383 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in efa01ec. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8012036634
|
creativecoder
added
the
Backport to WP 6.7 Beta/RC
There was a problem hiding this comment.
I've added a few notes inline.
Some form of snowflake code for the post types meta cap will be needed but it would be good to avoid the recursive checking on the font-family post.
It might be possible to use pre_delete_post for font-families and if the font-faces can't be deleted then handle the use case with an early return.
Feel free to push back on anything, I'm playing catch up and you're much more familiar with the code than I am.
I appreciate your help working on these roles and caps issues. They're certainly a Thing.
| @@ -84,6 +84,35 @@ function gutenberg_create_initial_post_types() { | |||
| ); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
The delete_post meta capability will need to account for deleting fonts with files attached. This is to account for the file deletion hooks in wp_delete_post().
I'm not in love with the performance aspect of this code when checking the font family post type but can't think of more performant approach.
| /** | |
| /** | |
| * Modify the `delete_post` meta capability for font post types. | |
| * | |
| * For font families and font faces containing attached font files, file | |
| * system access is required by the user in order to delete posts. | |
| * | |
| * @param string[] $caps The primitive capabilities required for the given capability. | |
| * @param string $cap The capability being checked. | |
| * @param int $user_id The user ID. | |
| * @param array $args Context for the capability check. | |
| * @return string[] The modified primitive capabilities required for the given capability. | |
| */ | |
| function gutenberg_delete_font_post_meta_caps( $caps, $cap, $user_id, $args ) { | |
| if ( in_array( 'do_not_allow', $caps, true ) ) { | |
| // It's already known that the user is not allowed to perform the requested capability. | |
| return $caps; | |
| } | |
| if ( 'delete_post' !== $cap ) { | |
| // This filter is only concerned with the 'delete_post' meta capability. | |
| return $caps; | |
| } | |
| $post = get_post( $args[0] ); | |
| if ( ! $post ) { | |
| $caps[] = 'do_not_allow'; | |
| return $caps; | |
| } | |
| $post_type = get_post_type( $post ); | |
| if ( 'wp_font_face' === $post_type ) { | |
| // Are there any font files associated with this font face? | |
| $font_files = get_post_meta( $post->ID, '_wp_font_face_file', false ); | |
| if ( empty( $font_files ) ) { | |
| /* | |
| * No font files. | |
| * | |
| * The user can delete the post based on the 'delete_post' meta capability. | |
| */ | |
| return $caps; | |
| } | |
| // The user can only delete the post if they can modify the file system. | |
| $caps[] = 'upload_fonts'; | |
| return $caps; | |
| } | |
| if ( 'wp_font_family' === $post_type ) { | |
| // Are there any font faces associated with this font family? | |
| $font_faces = get_children( | |
| array( | |
| 'post_parent' => $post->ID, | |
| 'post_type' => 'wp_font_face', | |
| ) | |
| ); | |
| if ( empty( $font_faces ) ) { | |
| /* | |
| * No font faces. | |
| * | |
| * The user can delete the post based on the 'delete_post' meta capability. | |
| */ | |
| return $caps; | |
| } | |
| // If any of the font faces contain files, the user needs to be able to modify the file system. | |
| foreach ( $font_faces as $font_face ) { | |
| $font_files = get_post_meta( $font_face->ID, '_wp_font_face_file', false ); | |
| if ( ! empty( $font_files ) ) { | |
| $caps[] = 'upload_fonts'; | |
| // File system caps are required, so no need to check further. | |
| break; | |
| } | |
| } | |
| return $caps; | |
| } | |
| // Return existing caps if the post type is not a font family or font face. | |
| return $caps; | |
| } | |
| /** |
| if ( | ||
| wp_is_file_mod_allowed( 'can_upload_fonts' ) && | ||
| wp_is_writable( $fonts_dir ) && | ||
| ! empty( $allcaps['edit_theme_options'] ) |
There was a problem hiding this comment.
Developers can modify the cap required for editing the font face/families (similar to the example in #59104 (comment) ) so I am not sure this is required.
canUser( 'create' ... ) should catch this.
However, I'm not entirely sure it is not required, either, so will require a logic check on this.
packages/edit-site/src/components/global-styles/font-library-modal/index.js
Outdated
Show resolved
Hide resolved
| @@ -28,6 +29,7 @@ export const EDITOR_SETTINGS_DEFAULTS = { | |||
| richEditingEnabled: true, | |||
| codeEditingEnabled: true, | |||
| fontLibraryEnabled: true, | |||
| fontUploadsEnabled: true, | |||
There was a problem hiding this comment.
It's probably safer to assume false as the default if the filter is removed. That will avoid the UI showing only to display an error if the caps check fails.
packages/edit-site/src/components/global-styles/font-library-modal/installed-fonts.js
Show resolved
Hide resolved
|
For props, the following list will need to be combined with the bot's list as they contributed to earlier iterations of this PR. |
peterwilsoncc
added
the
props-bot
github-actions
bot
removed
the
props-bot
|
Note that I've copied over the UI permissions checks into a separate PR, as they are useful whether we add a new capability or not, so I want to consider them separately. |
@azaozz There are filters in place to allow the font path to be modified to use the uploads folder and the |
There was a problem hiding this comment.
This PR looks on the right track to me if we think the 2nd option in #55280 (comment) is the way to go.
It seems this is still being discussed on the issue though, so I think we need to wait for a decision before finalizing and merging this.
| function gutenberg_maybe_grant_upload_font_cap( $allcaps, $caps ) { | ||
| if ( ! in_array( 'upload_fonts', $caps, true ) ) { | ||
| return $allcaps; | ||
| } | ||
|
|
||
| $fonts_dir = wp_get_font_dir()['path']; | ||
| $post_type = get_post_type_object( 'wp_font_face' ); | ||
| if ( | ||
| wp_is_file_mod_allowed( 'can_upload_fonts' ) && | ||
| wp_is_writable( $fonts_dir ) && | ||
| current_user_can( $post_type->cap->create_posts ) | ||
| ) { | ||
| $allcaps['upload_fonts'] = true; | ||
| } | ||
|
|
||
| return $allcaps; | ||
| } | ||
| add_filter( 'user_has_cap', 'gutenberg_maybe_grant_upload_font_cap', 10, 2 ); |
There was a problem hiding this comment.
Technically speaking, the user_has_cap filter's recommended use is to grant capabilities generally, based on other capabilities.
For specific checks like wp_is_file_mod_allowed() and wp_is_writable(), it is recommended to use the map_meta_cap filter instead.
In other words, I'd suggest:
- Simplify this function to only focus on granting
upload_fontsto any user that can$post_type->cap->create_posts. - In a separate
map_meta_capfilter callback, includedo_not_allowas a required capability if the relevant file mods aren't allowed or if the directory is not writable. This way it will effectively not be granted to anyone, but the actual capability grant logic is still tied to purely the other capabilities on the relevant user.
There was a problem hiding this comment.
focus on granting upload_fonts to any user that can $post_type->cap->create_posts
Hmm, afaik "contributors" can create posts (that they cannot publish), but cannot upload files. Not sure if they should be able to upload (and install) fonts, seems inconsistent?
Also don't think "authors" should be able to upload and install fonts. The reason is: how are they going to be able to use a font after it has been uploaded? May be missing something but afaik "authors" cannot make CSS changes, neither "global" nor "local". So what's the point in uploading a file you cannot use?
(Also there seems to be some more considerations about who should be able to "install" fonts. Afaik adding another font to a site is more of a "theme option" rather than "post author choice". Using a lot of fonts in a post would make the site slower, especially if these fonts are "one-use-only", i.e. uploaded and installed by a post author. Seems that if post authors would like to use a font they should ask an editor (or admin) to provide/install it. This is also important for the decision whether the font should be local (uploaded) vs. used from a CDN, etc.)
There are the reasons why I'm thinking a new upload_fonts capability looks mostly redundant. It should match exactly edit_theme_options as far as I understand it, and should not be used separately. I can see a need for it to simplify capability checks if DISALLOW_FILE_MODS would affect font files, but this use would be somewhat inconsistent. I mean, users that have upload_fonts will also have be able to use these newly uploaded fonts or the UI and UX will be a total mess :)
There was a problem hiding this comment.
I think your response fits much better on the discussion in the issue rather than my feedback here. My comment here was specifically about the code and is not related to the larger conversation we're having.
Could you put this on the issue as well please so we can discuss in a single place?
Right. I agree that from a "software developer" point of view respecting that constant is the right choice. My main concerns are that this results in a somewhat diminished/subpar user experience as having As far as I see this boils down to: have a better UX and "inconvenience" some hosting companies as they will have to make a minor change to their servers settings and treat |
This is what I am trying to convey, it's not a choice that can be made now. It was decided when the decision was made to store fonts outside of the uploads folder. For systems in which the file system is immutable, the files simply won't upload so we need to hide the UI to avoid making false promises. On systems such as WP VIP it's not simply a case of Depending on how these servers are deployed, even if the fonts folder is writable, it may be destroyed between deploys as we've put it in an area in which the we've promised the constant applies. This could be as simple as a Hosts will need to update their file systems, end users will need to update S3 and similar plugins. Meta data/URLs will need to be filtered, security headers modified. |
|
FWIW even if we decide to go for the disruptive change of requiring hosts to change their setups and users to reconfigure their CDNs etc., this would be a major undertaking that would take time. As such, some sites would not be configured correctly when this feature launches. In other words: We would need a graceful fallback no matter what, for the scenario that the folders aren't writable. If it's technically impossible to upload fonts, it's a better UX to not show the UI to upload fonts, rather than showing it but then it fails. |
|
I agree with @peterwilsoncc and @felixarntz assessments. They've shared the impact and disruption to hosts, extenders, and users including:
As @felixarntz noted, it will take time for these changes to happen and will go beyond 6.5 shipping. So what will happen if they are required to make these changes? If the changes are not made for those impacted, then either:
I think @felixarntz summed it well:
Given the major hosts @peterwilsoncc listed, IMO |
Yea it would have been ideal if this discussion has happened when the location of the fonts dir was being decided...
Right. Actually seems the code here should probably ignore |
So generally "Hosts win, users loose"? :) |
|
@azaozz The fonts folder is in a disposable part of the file system. Even basic VPS config providers include Atomic deployments that swap the file system out between deploys (eg Runcloud). Users lose if the files they have uploaded get wiped out by their host. |
Yep, you're right of course. There are several steps to ensure the new I'm starting to thing perhaps we are overthinking this a bit? Likely there will always be some edge cases where the
Seems the above problem (2) is pretty easy to solve, and thinking this is pretty much ready to be committed? |
|
Part of the work originally added in this PR, the post-creation capabilities checks, were added in #59332. The only thing that would remain to implement from this PR is the
I'm closing this PR. Feel free to re-open if I missed something. |
getdave
removed
the
Backport to WP 6.7 Beta/RC
What?
Fixes #55280
Combines approaches from #58957 and #59104
Note that in the future, it should be possible to create/install fonts from a collection without uploading a font file to the site, as font-faces support using an existing url as the
src. The font could be loaded from a CDN provided by the collection. Or a plugin could provide bundled fonts that don't need the font files to be uploaded to the site because they're included with the plugin. See #58229.For this reason, font upload and font creation capabilities are separate, so that it's possible to install a font without the ability to upload font files.
Why?
DISALLOW_FILE_MODSconstant to prevent font uploads for sites that don't allow file modificationHow?
upload_fontsmeta capability that's tied toDISALLOW_FILE_MODS/wp_is_file_mod_allowedupload_fontscapability before allowing font file uploads on the font-faces endpointfontUploadsEnabledsetting the site editor that checks theupload_fontscapabilityTesting Instructions
IMPORTANT: Test with WP 6.4 to ensure you are running the Gutenberg code from this PR
Test with
DISALLOW_FILE_MODSdefine( 'DISALLOW_FILE_MODS', true );in wp-config.phpPOST wp/v2/font-families/<id>/font-faces)Test with custom permissions
Add the following to your site