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

Harden access to validation data #6760

Merged
merged 14 commits into from
Dec 6, 2021
Merged

Harden access to validation data #6760

merged 14 commits into from
Dec 6, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Dec 3, 2021

Summary

By default the amp_validate capability is mapped to manage_options. However, it doesn't have to be. A site could disable access to validation data entirely—even to administrators—using a plugin like:

add_filter(
	'map_meta_cap',
	function ( $caps, $cap ) {
		if ( AMP_Validation_Manager::VALIDATE_CAPABILITY === $cap ) {
			$caps[] = 'do_not_allow';
		}
		return $caps;
	},
	10,
	3
);

If I try doing that, some unexpected things happen for new screens in 2.2:

  1. The settings screen throws an error. Site Scan should be omitted entirely.
  2. Site Scanning is able to proceed in the Onboarding Wizard. Site Scan step should be skipped.
  3. Support screen is accessible. It should not be available.

Screen Shot 2021-12-03 at 14 24 05

We need to make sure that if a site is restricting access to any validation data by checking first if the user AMP_Validation_Manager::has_cap().

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added the P0 High priority label Dec 3, 2021
@westonruter westonruter added this to the v2.2 milestone Dec 3, 2021
@delawski delawski marked this pull request as ready for review December 6, 2021 19:14
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2021

Plugin builds for 6746ec2 are ready 🛎️!

@delawski
Copy link
Collaborator

delawski commented Dec 6, 2021

@westonruter In ae702a3 you'll find the most important change – a new scanner status is introduced there, STATUS_SKIPPED. It is set if there's no VALIDATE_NONCE passed from the backend meaning that the current user has no capabilities to do AMP validation.

In e656fbf a minor refactor has been done so that the complexity of the siteScanReducer function is within the predefined limit.

Lastly, in e483fbe new E2E tests have been introduced. Thanks to them we're sure that if a user has no validation capability, Site Scan won't be available on the AMP Settings screen, in the Onboarding Wizard and after plugin activation.

I think it's now ready for your review.


As a side note, I think that our context provider trees are becoming pretty big and complex, with various dependencies between providers. I'm thinking of reevaluating this approach and maybe looking into a more systematic solution like React Query that would abstract away data fetching, caching and updating. Since data is stored in a global variable (internally), it can be exposed everywhere in a React app using a set of hooks. This might simplify and decouple the client-side data layer.

…ate-access

* 'develop' of github.com:ampproject/amp-wp:
  Run ergebnis/composer-normalize
  Supply aspect-ratio to videos in Video Blocks
  Add height:auto to videos in Video blocks
  Use intrinsic layout for video instead of responsive
  Add missing import for createInterpolateElement
  Improve translation strings
  Move Sandboxing level settings drawer after Analytics
  Eliminate sandboxing experiment filter in favor of option
Copy link
Member Author

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

@delawski Thank you for the additional changes! Looks great and now behaves as expected.

Comment on lines 238 to +243
await installLocalPlugin( 'e2e-tests-demo-plugin' );
await installLocalPlugin( 'do-not-allow-amp-validate-capability' );

// If the demo plugin has been already installed it might be activated, too.
// If the plugins have been already installed, they may be activated, too. Try deactivating them, just in case.
await deactivatePlugin( 'e2e-tests-demo-plugin' );
await deactivatePlugin( 'do-not-allow-amp-validate-capability' );
Copy link
Member Author

Choose a reason for hiding this comment

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

Nit: the list of plugins could be in an array, and then they could be looped over.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I was thinking of the same, though there might be some issues when using await inside a loop. Let's take care of that as soon as we need to add another plugin.

@westonruter
Copy link
Member Author

Merging without approval since I'm the one who started this PR. @delawski I'm assuming you approve 😄 If you can sanity check my additional changes that would be great.

@westonruter westonruter merged commit 237dc6e into develop Dec 6, 2021
@westonruter westonruter deleted the fix/validate-access branch December 6, 2021 21:15
@delawski
Copy link
Collaborator

delawski commented Dec 7, 2021

The changes (and the PR overall) look good to me 👍

@dhaval-parekh
Copy link
Collaborator

QA Passed

After using the plugin with the code given above (in the description).

✅ The "AMP Setting" page is functioning correctly and site scanning is omitted on the page.

✅ Support page, Validated URL and Error Index page is disabled for the user.

image

✅ On the "Onboarding Wizard" step for site scan is removed.

image

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. P0 High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants