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

Make it unnecessary to append a '_' to the controller name when using the AdminSecurity annotation #9432

Merged
merged 2 commits into from Aug 13, 2018

Conversation

Projects
None yet
7 participants
@eternoendless
Member

eternoendless commented Aug 10, 2018

Questions Answers
Branch? develop
Description? Read below
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Access any of the migrated pages. If you can, it means the change works.

Currently, if you use the @AdminSecurity annotation in "modern" controllers, you need to append an underscore to the legacy controller name, like this:

request.get('_legacy_controller')~'_'

Let's take the following example from AdministrationController.php:

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

Roles (permissions) for controllers are stored in the database as ROLE_MOD_TAB_<CONTROLLERNAME>_<CREATE|READ|UPDATE|DELETE>.

In this example, the corresponding role is ROLE_MOD_TAB_ADMINADMINPREFERENCES_READ.

The PageVoter that is invoked when calling is_granted() receives "read" as attribute, and "AdminAdminPreferences" as subject (specified by the _legacy_controller property in the route yaml).

Using those elements, it builds the role ID and then hands it over to the legacy access control which performs the check.

Until now, the role was created by joining the subject and the attribute together, then converting them to upper case. This PR adds an underscore between them if there isn't one there already (to ensure backwards compatibility).

That's all! Now we can have cleaner calls to AdminSecurity. Check out said change in this commit: 95ce71c

The second commit is where I changed all the current calls so that they conform to the new spec.


This change is Reviewable

eternoendless added some commits Aug 10, 2018

Make it unnecessary to append a '_' to the controller name when using…
… the AdminSecurity annotation

The voter now adds the underscore automatically if necessary.
Remove unnecessary trailing underscore in controller names.
This is now handled automatically by PageVoter.

@eternoendless eternoendless added this to the 1.7.5.0 milestone Aug 10, 2018

@eternoendless eternoendless requested a review from mickaelandrieu Aug 10, 2018

}
return true;
return in_array($attribute, array(self::CREATE, self::UPDATE, self::DELETE, self::READ));

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 10, 2018

Contributor

👍

@PierreRambaud

PierreRambaud Aug 10, 2018

Contributor

👍

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 10, 2018

Contributor

would you mind to update the Symfony migration guide docs too? :)

Contributor

mickaelandrieu commented Aug 10, 2018

would you mind to update the Symfony migration guide docs too? :)

@ntiepresta ntiepresta self-assigned this Aug 13, 2018

@ntiepresta ntiepresta added QA ✔️ and removed waiting for QA labels Aug 13, 2018

@ntiepresta ntiepresta removed their assignment Aug 13, 2018

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Aug 13, 2018

Member

@mickaelandrieu Yeah I'm on it :)

Member

eternoendless commented Aug 13, 2018

@mickaelandrieu Yeah I'm on it :)

@mickaelandrieu mickaelandrieu merged commit 2394175 into PrestaShop:develop Aug 13, 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
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 13, 2018

Contributor

Thanks everyone !

Contributor

mickaelandrieu commented Aug 13, 2018

Thanks everyone !

@matks matks added the migration label Sep 18, 2018

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