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

Introduces improved AdminSecurity and DemoRestricted annotations usage with ability to pass query parameters to keep #11075

Merged
merged 7 commits into from Oct 23, 2018

Conversation

Projects
None yet
4 participants
@tomas862
Contributor

tomas862 commented Oct 19, 2018

Questions Answers
Branch? develop
Description? I have a situation where I have mandatory route parameter called cmsPageCategoryId on my controller. I started adding AdminSecurity and DemoRestricted annotations and faced with some limitations: I was unable to pass my mandatory route parameter by only using the annotation. So I decided I can do that by just adding some syntax to the annotation. See below for sample of usage 👇
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? no
How to test? No such scenario exists currently - planning to introduce it in #10877 . If all tests passes then it's good to go
/**
     * @AdminSecurity(
     *     "is_granted(['update', 'create','delete'], request.get('_legacy_controller'))",
     *      redirectRoute="admin_localization_show_settings",
     *      redirectQueryParamsToKeep={"myMandatoryId", "anotherId"},
     *      message="You do not have permission to edit this."
     * )
     * @DemoRestricted(
     *    redirectRoute="admin_localization_show_settings",
     *    redirectQueryParamsToKeep={"myMandatoryId"}
     *   )
     */

This change is Reviewable

@tomas862 tomas862 changed the title from [WIP] Introduces improved AdminSecurity and DemoRestricted annotations usage with ability to pass query parameters to keep to Introduces improved AdminSecurity and DemoRestricted annotations usage with ability to pass query parameters to keep Oct 19, 2018

@tomas862 tomas862 changed the title from Introduces improved AdminSecurity and DemoRestricted annotations usage with ability to pass query parameters to keep to [WIP] Introduces improved AdminSecurity and DemoRestricted annotations usage with ability to pass query parameters to keep Oct 19, 2018

@tomas862 tomas862 changed the title from [WIP] Introduces improved AdminSecurity and DemoRestricted annotations usage with ability to pass query parameters to keep to Introduces improved AdminSecurity and DemoRestricted annotations usage with ability to pass query parameters to keep Oct 19, 2018

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Oct 19, 2018

Hello @tomas862 , apply PHP CS Fixer on your code, this should make Travis happy 👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Oct 19, 2018

This is a good idea, I let @matks review and decide 👍

@matks matks added the migration label Oct 19, 2018

@matks

One small thing to improve, else the idea is very good 👍 and useful

$result = [];
foreach ($queryParametersToKeep as $queryParameterName) {
if ($value = $request->get($queryParameterName)) {

This comment has been minimized.

@matks

matks Oct 19, 2018

Contributor

Please do not assign variables inside if statement. It works, but it is not easy to read, and it might have unwanted consequences. For example what if $request->get($queryParameterName) returns 0 ? Then it is going to be skipped.

This comment has been minimized.

@tomas862

tomas862 Oct 20, 2018

Contributor

done - thank you! 👍

removes variable assign operation from if statement, check value of n…
…ull to allow more valid route parameters to be passed
@matks

matks approved these changes Oct 23, 2018

@matks matks added this to the 1.7.6.0 milestone Oct 23, 2018

@matks

This comment has been minimized.

Contributor

matks commented Oct 23, 2018

Thanks @tomas862 👍
No QA validation here as this is a "hidden" technical feature. However it will be validated through #10877

@matks matks merged commit 76be877 into PrestaShop:develop Oct 23, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment