Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font Library: font family and font uploads capability checks #59294

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,14 @@ public function create_item( $request ) {
$settings = $request->get_param( 'font_face_settings' );
$file_params = $request->get_file_params();

if ( ! empty( $file_params ) && ! current_user_can( 'upload_fonts' ) ) {
return new WP_Error(
'rest_cannot_upload_fonts',
__( 'You are not allowed to upload font files.', 'gutenberg' ),
array( 'status' => 403 )
);
}

// Check that the necessary font face properties are unique.
$query = new WP_Query(
array(
Expand Down
147 changes: 139 additions & 8 deletions lib/compat/wordpress-6.5/fonts/fonts.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,31 +85,147 @@ function gutenberg_create_initial_post_types() {
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/**
/**
* 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;
}
/**

* Initializes REST routes.
* 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.
*
* @since 6.5
* @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 ) {
// Do not allow deleting posts that do not exist.
$caps[] = 'do_not_allow';
return $caps;
}

// Check for font post types.
$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;
}
add_filter( 'map_meta_cap', 'gutenberg_delete_font_post_meta_caps', 10, 4 );

/**
* Filters the user capabilities to grant the 'upload_fonts' capability as necessary.
*
* To grant the 'upload_fonts' capability, file modifications must be allowed, the fonts directory must be
* writable, and the user must have the 'edit_theme_options' capability.
*
* @since 6.5.0
*
* @param bool[] $allcaps An array of all the user's capabilities.
* @return bool[] Filtered array of the user's capabilities.
*/
function gutenberg_maybe_grant_upload_font_cap( $allcaps, $caps ) {
peterwilsoncc marked this conversation as resolved.
Show resolved Hide resolved
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 );
Comment on lines +182 to +199
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_fonts to any user that can $post_type->cap->create_posts.
  • In a separate map_meta_cap filter callback, include do_not_allow as 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.

Copy link
Contributor

@azaozz azaozz Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/**
* Initializes REST routes.
*/
function gutenberg_create_initial_rest_routes() {
$font_collections_controller = new WP_REST_Font_Collections_Controller();
$font_collections_controller->register_routes();
global $wp_version;

// Runs only if the Font Library is not available in core ( i.e. in core < 6.5-alpha ).
if ( version_compare( $wp_version, '6.5-alpha', '<' ) ) {
$font_collections_controller = new WP_REST_Font_Collections_Controller();
$font_collections_controller->register_routes();
}
}

add_action( 'rest_api_init', 'gutenberg_create_initial_rest_routes' );

/**
* Initializes REST routes and post types.
*
* @since 6.5
*/
function gutenberg_init_font_library() {
global $wp_version;

// Runs only if the Font Library is not available in core ( i.e. in core < 6.5-alpha ).
if ( version_compare( $wp_version, '6.5-alpha', '<' ) ) {
gutenberg_create_initial_post_types();
gutenberg_create_initial_rest_routes();
}
}

add_action( 'rest_api_init', 'gutenberg_init_font_library' );
add_action( 'init', 'gutenberg_init_font_library' );


if ( ! function_exists( 'wp_register_font_collection' ) ) {
Expand Down Expand Up @@ -303,6 +419,21 @@ function _wp_before_delete_font_face( $post_id, $post ) {
add_action( 'before_delete_post', '_wp_before_delete_font_face', 10, 2 );
}

/**
* Filters the block editor settings to enable or disable font uploads according to user capability.
*
* @since 6.5.0
*
* @param array $settings Block editor settings.
* @return array Block editor settings.
*/
function gutenberg_font_uploads_settings( $settings ) {
$settings['fontUploadsEnabled'] = current_user_can( 'upload_fonts' );

return $settings;
}
add_filter( 'block_editor_settings_all', 'gutenberg_font_uploads_settings' );

// @core-merge: Do not merge this back compat function, it is for supporting a legacy font family format only in Gutenberg.
/**
* Convert legacy font family posts to the new format.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import {
DropdownMenu,
} from '@wordpress/components';
import { debounce } from '@wordpress/compose';
import { useSelect } from '@wordpress/data';
import { store as editorStore } from '@wordpress/editor';
import { sprintf, __, _x } from '@wordpress/i18n';
import {
search,
Expand Down Expand Up @@ -86,6 +88,11 @@ function FontCollection( { slug } ) {
( collection ) => collection.slug === slug
);

const fontUploadsEnabled = useSelect( ( select ) => {
const { getEditorSettings } = select( editorStore );
return getEditorSettings().fontUploadsEnabled;
}, [] );

useEffect( () => {
const handleStorage = () => {
setRenderConfirmDialog(
Expand Down Expand Up @@ -416,7 +423,12 @@ function FontCollection( { slug } ) {
variant="primary"
onClick={ handleInstall }
isBusy={ isInstalling }
disabled={ fontsToInstall.length === 0 || isInstalling }
disabled={
fontsToInstall.length === 0 ||
isInstalling ||
( ! fontUploadsEnabled &&
selectedFont?.fontFace?.length )
}
__experimentalIsFocusable
>
{ __( 'Install' ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import {
Modal,
privateApis as componentsPrivateApis,
} from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { useSelect } from '@wordpress/data';
import { store as editorStore } from '@wordpress/editor';
import { useContext } from '@wordpress/element';

/**
Expand All @@ -19,16 +22,15 @@ import { unlock } from '../../../lock-unlock';

const { Tabs } = unlock( componentsPrivateApis );

const DEFAULT_TABS = [
{
id: 'installed-fonts',
title: __( 'Library' ),
},
{
id: 'upload-fonts',
title: __( 'Upload' ),
},
];
const DEFAULT_TAB = {
id: 'installed-fonts',
title: __( 'Library' ),
};

const UPLOAD_TAB = {
id: 'upload-fonts',
title: __( 'Upload' ),
};

const tabsFromCollections = ( collections ) =>
collections.map( ( { slug, name } ) => ( {
Expand All @@ -44,11 +46,24 @@ function FontLibraryModal( {
defaultTabId = 'installed-fonts',
} ) {
const { collections, setNotice } = useContext( FontLibraryContext );
const canUserCreate = useSelect( ( select ) => {
const { canUser } = select( coreStore );
return canUser( 'create', 'font-families' );
}, [] );
const fontUploadsEnabled = useSelect( ( select ) => {
const { getEditorSettings } = select( editorStore );
return getEditorSettings().fontUploadsEnabled;
}, [] );

const tabs = [ DEFAULT_TAB ];

if ( canUserCreate ) {
if ( fontUploadsEnabled ) {
tabs.push( UPLOAD_TAB );
}

const tabs = [
...DEFAULT_TABS,
...tabsFromCollections( collections || [] ),
];
tabs.push( ...tabsFromCollections( collections || [] ) );
}

// Reset notice when new tab is selected.
const onSelect = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import {
Spinner,
privateApis as componentsPrivateApis,
} from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { useSelect } from '@wordpress/data';
import { useContext, useEffect, useState } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';
import { chevronLeft } from '@wordpress/icons';
Expand Down Expand Up @@ -50,8 +52,24 @@ function InstalledFonts() {
} = useContext( FontLibraryContext );
const [ isConfirmDeleteOpen, setIsConfirmDeleteOpen ] = useState( false );

const customFontFamilyId =
peterwilsoncc marked this conversation as resolved.
Show resolved Hide resolved
libraryFontSelected?.source === 'custom' && libraryFontSelected?.id;

const canUserDelete = useSelect(
( select ) => {
const { canUser } = select( coreStore );
return (
customFontFamilyId &&
canUser( 'delete', 'font-families', customFontFamilyId )
);
},
[ customFontFamilyId ]
);

const shouldDisplayDeleteButton =
!! libraryFontSelected && libraryFontSelected?.source !== 'theme';
!! libraryFontSelected &&
libraryFontSelected?.source !== 'theme' &&
canUserDelete;

const handleUninstallClick = () => {
setIsConfirmDeleteOpen( true );
Expand Down
2 changes: 2 additions & 0 deletions packages/editor/src/store/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { SETTINGS_DEFAULTS } from '@wordpress/block-editor';
* @property {boolean} richEditingEnabled Whether rich editing is enabled or not
* @property {boolean} codeEditingEnabled Whether code editing is enabled or not
* @property {boolean} fontLibraryEnabled Whether the font library is enabled or not.
* @property {boolean} fontUploadsEnabled Whether uploading fonts in the font library is enabled or not.
* @property {boolean} enableCustomFields Whether the WordPress custom fields are enabled or not.
* true = the user has opted to show the Custom Fields panel at the bottom of the editor.
* false = the user has opted to hide the Custom Fields panel at the bottom of the editor.
Expand All @@ -28,6 +29,7 @@ export const EDITOR_SETTINGS_DEFAULTS = {
richEditingEnabled: true,
codeEditingEnabled: true,
fontLibraryEnabled: true,
fontUploadsEnabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

enableCustomFields: undefined,
defaultRenderingMode: 'post-only',
};
Loading
Loading