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

Global Styles: Don't remove Custom CSS for users with the correct caps #47062

Merged
merged 7 commits into from
Jan 16, 2023

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Jan 11, 2023

What?

Related #46815.

PR updates WP_Theme_JSON::remove_insecure_properties to skip custom CSS sanitization when a user has edit_css capabilities.

Why?

When KSES filters are registered, the remove_insecure_properties remove the custom CSS values without checking the right capabilities.

Testing Instructions

  1. Enable the KSES filters with add_action( 'init', 'kses_init_filters' );
  2. Enable Custom CSS via Gutenberg > Experiments
  3. Go to Appearance > Editor
  4. Open the Styles sidebar
  5. Select "Custom CSS"
  6. Add some CSS code
  7. Confirm CSS code is saved

@Mamaduka Mamaduka marked this pull request as ready for review January 11, 2023 12:56
@Mamaduka Mamaduka self-assigned this Jan 11, 2023
@Mamaduka Mamaduka removed the request for review from spacedmonkey January 11, 2023 12:58
@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Jan 11, 2023
@github-actions
Copy link

github-actions bot commented Jan 11, 2023

Flaky tests detected in 11b9a75.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3900135614
📝 Reported issues:

@Mamaduka
Copy link
Member Author

Mamaduka commented Jan 11, 2023

Can anyone confirm if the PHPUnit tests are failing locally as well?

They are passing without an issue for me but failing on CI.

npm run test:unit:php

@ironprogrammer
Copy link
Contributor

@Mamaduka, the singled-out test from CI succeeds locally in my environment as well. The following was performed with a fresh set of Docker images:

$ npm run test:unit:php -- --testdox --filter test_allows_custom_css_for_users_with_caps
...
Runtime:       PHP 8.0.27
Configuration: /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist

WP_Theme_JSON_Gutenberg_
 ✔ Allows custom css for users with caps  30 ms

Time: 00:00.064, Memory: 46.50 MB

OK (1 test, 3 assertions)
...

@Mamaduka
Copy link
Member Author

Thanks for testing, @ironprogrammer!

Do you get a failure when running the entire test suite?

P.S. I've destroyed and rebuilt my local wp-env a few times but couldn't reproduce the CI failure.

@ironprogrammer
Copy link
Contributor

Reply to @Mamaduka:

Do you get a failure when running the entire test suite?

No, the entire suite succeeds as well.

@Mamaduka Mamaduka force-pushed the try/custom-css-secure-property branch from cd7fadd to 7801eca Compare January 11, 2023 19:45
@Mamaduka
Copy link
Member Author

Found the source of the failure. The multisite unit tests fail - npm run test:unit:php:multisite. This is because the administrator roles on MS don't have unfiltered_html or edit_css roles.

I will try to get this resolved tomorrow. Meanwhile, PR is ready for manual testing.

@Mamaduka
Copy link
Member Author

Resolved the failing tests by explicitly granting capabilities to the administrator users. PR is ready for review.

Comment on lines 1622 to 1629
// Explicitly grant 'edit_css' capabilities.
$grant_edit_css_cap = function( $caps, $cap ) {
if ( 'edit_css' === $cap ) {
$caps = array( 'edit_theme_options' );
}
return $caps;
};
add_filter( 'map_meta_cap', $grant_edit_css_cap, 10, 2 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, in the set_up_before_class() fixture, you can add this code which grants this specific admin Super Admin access.

if ( is_multisite() ) {
	grant_super_admin( self::$administrator_id );
}

Let me push an update to see if it resolves the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 35eb041 and f05482f. PHPUnit tests are passing ✅

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, @hellofromtonya 🙇

@hellofromtonya
Copy link
Contributor

Hey @Mamaduka, I'm sorry for my delay in helping with the failing PHPUnit tests. Pushed commits that removed your changes in favor of granting the defined admin Super Admin access. This approach is consistent with other Core tests.

@hellofromtonya hellofromtonya force-pushed the try/custom-css-secure-property branch from 35eb041 to a2a9aca Compare January 12, 2023 20:13
Copy link
Contributor

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

Thanks! This tests well for me, and it matches the behavior provided by the Additional CSS feature in the classic Customizer (where the custom CSS is always saved even if it's invalid, as long as the user has the edit_css capability).

@glendaviesnz
Copy link
Contributor

@Mamaduka do you want me to port these changes as part of the main custom-css port, or do you want to handle it separately?

@Mamaduka
Copy link
Member Author

Thank you, @glendaviesnz. I think migrating these two together makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants