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

Fixed performance rights on Performance Clear Cache action #10898

Merged
merged 1 commit into from Oct 30, 2018

Conversation

Projects
None yet
7 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Oct 8, 2018

Questions Answers
Branch? 1.7.5.x
Description? Introduced a missing authorization check in Performance page
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? Create a user who has access to Performance Page but not the rights of DELETE and try to clear the cache. Before, it was possible and now it's now: which is the intended behavior.

This change is Reviewable

PageVoter::LEVEL_READ,
PageVoter::LEVEL_UPDATE,
PageVoter::LEVEL_CREATE,
PageVoter::LEVEL_DELETE,

This comment has been minimized.

@sarjon

sarjon Oct 8, 2018

Member

we've been doing this kind of checks, but is it correct? in BO you can assign user to have only create permissions, does it mean user can read as well? 🤔

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Oct 10, 2018

Contributor

thinking about it, we should not at this check in the core at all, only the one about clear cache action.

@@ -64,6 +65,20 @@ public function indexAction(FormInterface $form = null)
$form = is_null($form) ? $this->get('prestashop.adapter.performance.form_handler')->getForm() : $form;
if (!in_array(

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 8, 2018

Contributor

This change need to be done in line 50.

      * @AdminSecurity("is_granted('read', request.get('_legacy_controller'))", message="Access denied.")

to

      * @AdminSecurity("is_granted(['create', 'read', 'update', 'delete'], request.get('_legacy_controller'))", message="Access denied.")
@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Oct 11, 2018

Comments addressed, it's better now 👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Oct 11, 2018

You forget to change the line with@AdminSecurity notation ;)

Because we shouldn't do that in the core: the current rights are corrects :)

@mickaelandrieu mickaelandrieu force-pushed the mickaelandrieu:fix-performance-page-rights branch from 8053c71 to 6c543a9 Oct 29, 2018

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Oct 29, 2018

Pull request rebased => Waiting for QA :)

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Oct 29, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 30, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit c4b9d3b into PrestaShop:1.7.5.x Oct 30, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Oct 30, 2018

Thank you @mickaelandrieu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment