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

Introduced re-usable authorization system #9028

Merged
merged 11 commits into from May 24, 2018

Conversation

@mickaelandrieu
Contributor

mickaelandrieu commented May 4, 2018

Questions Answers
Branch? 1.7.4.x
Description? Since 1.7.3 an employee with no rights can access to some migrated pages
Type? bug fix
Category? BO
BC breaks? no
Deprecations? yes, the functions  $authorizationLevel and isDemoEnabled and getDemoMessage in mid term
Fixed ticket? n/a
How to test? Create a user with no rights and access to every migrated page: you should always be redirected to OnBoarding page. In demo mode, some accesses are granted but some actions (create/update/delete something) should also redirect to some URLs.

Related PR: #9025

Note this is a very important security issue, and this patch should be ported to 1.7.3.x branch.

TLDR;

To implement authorizations into Controller actions, rely on AdminSecurity and DemoRestricted annotations:

    /**
     * Do stuff
     * @AdminSecurity("is_granted(['read','update', 'create','delete'], request.get('_legacy_controller')~'_')", message="You do not have permission to update this.")
     * @DemoRestricted("route_to_be_redirected", message="You can't do this when demo mode is enabled.")
     */
    public function fooAction(Request $request)

Important guidelines


This change is Reviewable

@mickaelandrieu mickaelandrieu changed the title from Introduced re-usable authorization system to [WIP] Introduced re-usable authorization system May 4, 2018

@@ -61,7 +61,7 @@ public function loadUserByUsername($username)
if (isset($this->legacyContext->employee) && $this->legacyContext->employee->email == $username) {
$employee = new Employee($this->legacyContext->employee);
$employee->setRoles(
Access::getRoles($this->legacyContext->employee->id_profile)
array_merge(['ROLE_EMPLOYEE'], Access::getRoles($this->legacyContext->employee->id_profile))

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

we need to add - at least - a minimal role or the User Provider will throw an exception!

This is not the case if you create a new profile and set no authorization at all: really uncommon, but still, this shouldn't throw a PHP exception 👍

This comment has been minimized.

@PierreRambaud

PierreRambaud May 4, 2018

Contributor

Maybe use const for roles? (Don't know it is for others)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

others are values in bdd, I can add a constant in EmployeeProvider.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

fixed 👍

$security = $event->getRequest()->get('_security');
if (empty($security) || !$security[0] instanceof BetterSecurity) {

This comment has been minimized.

@PierreRambaud

PierreRambaud May 4, 2018

Contributor

Hum, what happens if there is more than one element in security array?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

I don't know, and looking at security, I don't even know why I retrieve an array here :/

*/
private function throwNotificationMessage(BetterSecurity $betterSecurity)
{
$this->session->getFlashBag()->add('error', $this->translator->trans($betterSecurity->getMessage(), [], $betterSecurity->getDomain()));

This comment has been minimized.

@PierreRambaud

PierreRambaud May 4, 2018

Contributor

Line too long:

$this->session->getFlashBag()->add(
    'error',
    $this->translator->trans(
        $betterSecurity->getMessage(),
        [],
        $betterSecurity->getDomain()
    )
);

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

coding style etc... I'll launch php-cs-fixer on this file.

*/
public function onKernelException(GetResponseForExceptionEvent $event)
{
if (!$event->isMasterRequest()) {

This comment has been minimized.

@PierreRambaud

PierreRambaud May 4, 2018

Contributor

Maybe combine both conditions?

$security = $event->getRequest()->get('_security');
if (empty($security) || !$security[0] instanceof AdminSecurity) {

This comment has been minimized.

@PierreRambaud

PierreRambaud May 4, 2018

Contributor

Hum, what happens if there is more than one element in security array?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

anyway, fixed 👍

*/
private function throwNotificationMessage( AdminSecurity $adminSecurity)
{
$this->session->getFlashBag()->add('error', $this->translator->trans($adminSecurity->getMessage(), [], $adminSecurity->getDomain()));

This comment has been minimized.

@PierreRambaud

PierreRambaud May 4, 2018

Contributor

Line too long:

$this->session->getFlashBag()->add(
    'error',
    $this->translator->trans(
        $betterSecurity->getMessage(),
        [],
        $betterSecurity->getDomain()
    )
);
$this->throwNotificationMessage($securityConfiguration);
$url = $this->computeUrl($securityConfiguration);
$event->setResponse(new RedirectResponse($url));

This comment has been minimized.

@PierreRambaud

PierreRambaud May 4, 2018

Contributor

Break if response is set, no need to continue the foreach.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

return is even better, thanks :)

@mickaelandrieu mickaelandrieu changed the title from [WIP] Introduced re-usable authorization system to Introduced re-usable authorization system May 4, 2018

@@ -41,13 +42,12 @@ class AdministrationController extends FrameworkBundleAdminController
const CONTROLLER_NAME = 'AdminAdminPreferences';
/**
* Show administration page
*
* Show administration page*

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

remove the extra "*"

@@ -0,0 +1,36 @@
<?php

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 4, 2018

Contributor

useless, you can remove it

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 7, 2018

ping @eternoendless don't miss this one before 1.7.4 this is a security breach fix

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.4.0 milestone May 9, 2018

@eternoendless

This comment has been minimized.

Member

eternoendless commented May 14, 2018

Reviewed 1 of 2 files at r4, 13 of 14 files at r6.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/AdministrationController.php, line 45 at r6 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

remove the extra "*"

Don't forget to remove the extra *


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/AdministrationController.php, line 47 at r6 (raw file):

     * Show administration page*
     * @Template("@PrestaShop/Admin/Configure/AdvancedParameters/administration.html.twig")
     * @AdminSecurity("is_granted('read', request.get('_legacy_controller')~'_')", message="Access denied.")

Why is there an _ after _legacy_controller?


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/AdministrationController.php, line 70 at r6 (raw file):


    /**
     * Process the Performance configuration form.

Wrong page?


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/AdministrationController.php, line 71 at r6 (raw file):

    /**
     * Process the Performance configuration form.
     * @AdminSecurity("is_granted(['read','update', 'create','delete'], request.get('_legacy_controller')~'_')", message="You do not have permission to update this.", route="admin_administration")

Missing spaces after ,


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/LogsController.php, line 94 at r6 (raw file):

    /**
     * @AdminSecurity("is_granted(['read','update', 'create','delete'], request.get('_legacy_controller')~'_')", message="You do not have permission to update this.", route="admin_logs")
     * @DemoRestricted(route="admin_logs")

Why isn't there a message?


src/PrestaShopBundle/EventListener/AccessDeniedListener.php, line 75 at r6 (raw file):

        if (!$securityConfigurations = $event->getRequest()->attributes->get('_security')) {
            return;
        }

I think you can merge this if with the previous one


src/PrestaShopBundle/EventListener/AccessDeniedListener.php, line 79 at r6 (raw file):

        foreach ($securityConfigurations as $securityConfiguration) {
            if (!$securityConfiguration instanceof AdminSecurity) {
                continue;

You can inverse the if and get rid of the continue by moving all the other code inside the block.


src/PrestaShopBundle/EventListener/AccessDeniedListener.php, line 100 at r6 (raw file):

     * @return string
     */
    private function computeUrl(AdminSecurity $adminSecurity)

I think computeRedirectionUrl would be a more telling name.


src/PrestaShopBundle/EventListener/AccessDeniedListener.php, line 115 at r6 (raw file):

     * @param AdminSecurity $adminSecurity
     */
    private function throwNotificationMessage(AdminSecurity $adminSecurity)

I think it would be better not to use the word throw unless there's an exception involved. addNotificationMessage or showNotificationMessage would be a better fit in my opinion.


src/PrestaShopBundle/EventListener/DemoModeEnabledListener.php, line 87 at r6 (raw file):

        if (!$demoConfigurations = $event->getRequest()->attributes->get('_demo_restricted')) {
            return;
        }

You can merge the two if statements


src/PrestaShopBundle/EventListener/DemoModeEnabledListener.php, line 91 at r6 (raw file):

        foreach ($demoConfigurations as $demoConfiguration) {
            if (!$demoConfiguration instanceof DemoRestricted) {
                continue;

Here too, you can invert the if, move the rest of the code here and get rid of the continue.


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 64 at r1 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

fixed 👍

How about DEFAULT_ROLE instead?


src/PrestaShopBundle/Security/Annotation/DemoRestricted.php, line 49 at r6 (raw file):

     * @var string
     */
    protected $route;

How about redirectRoute?


src/PrestaShopBundle/Security/Annotation/DemoRestricted.php, line 102 at r6 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

miss end of line

Don't forget about it


src/PrestaShopBundle/Security/Exception/AccessOnDemoDeniedException.php, line 1 at r6 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

useless, you can remove it

To delete if unused


src/PrestaShopBundle/Security/Annotation/AdminSecurity.php, line 53 at r6 (raw file):

     * @var string
     */
    protected $route;

How about redirectRoute?


src/PrestaShopBundle/Security/Annotation/AdminSecurity.php, line 56 at r6 (raw file):


    /**
     * @deprecated 1.8.x once the back office is migrated, rely only on route.

You need to put the version where the deprecation starts instead (1.7.4.0)


src/PrestaShopBundle/Security/Annotation/AdminSecurity.php, line 61 at r6 (raw file):

     * @return string
     */
    protected $url = 'admin_domain';

How about redirectUrl? Either way, this does not look like a URL


Comments from Reviewable

@eternoendless

This comment has been minimized.

Member

eternoendless commented May 14, 2018

@mickaelandrieu needs rebase on 1.7.4.x, too

@eternoendless

This comment has been minimized.

Member

eternoendless commented May 23, 2018

Missing type

it's not me, it's PHPStorm: time to use PHP 7.1 so our PHPDoc will be useless once and for all :troll:

We'll get there in time! But only the type hints will be useless then 😄

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 23, 2018

The question is, why do you concatenate an underscore at the end of the string?

This is how works the current authorization system :/

See https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Controller/Admin/FrameworkBundleAdminController.php#L218 and https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Security/Voter/PageVoter.php#L87

We could improve that, but not here, in a future PR: what I want here is rely on the current system because it works and not introduce regression.

@eternoendless

This comment has been minimized.

Member

eternoendless commented May 23, 2018

We could improve that, but not here, in a future PR.

I agree. I didn't know about the trailing underscore thing, that's why I asked... it's really weird.

@eternoendless

This comment has been minimized.

Member

eternoendless commented May 23, 2018

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Member

eternoendless commented May 23, 2018

Don't forget to write the docs!

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 23, 2018

Don't forget to write the docs!

First in my todo list once it's merged 👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 24, 2018

=> Docs <=

$tokenAnnotation = DemoRestricted::class;
$classAnnotation = $this->annotationReader->getClassAnnotation(
new \ReflectionClass(ClassUtils::getClass($controllerObject)), $tokenAnnotation

This comment has been minimized.

@PierreRambaud

PierreRambaud May 24, 2018

Contributor

Can you use namespace please

*/
private function getAnnotation($controllerObject, $methodName)
{
$tokenAnnotation = DemoRestricted::class;

This comment has been minimized.

@PierreRambaud

PierreRambaud May 24, 2018

Contributor

No need to use this variable DemoRestricted::class is a const.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 24, 2018

Contributor

It's not the reason I'm using a variable, it's to explain what is expected here :)

}

This comment has been minimized.

@PierreRambaud

PierreRambaud May 24, 2018

Contributor

Double line break.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 24, 2018

Contributor

fixed

$event->setController(function () use ($url){
return new RedirectResponse($url);
});

This comment has been minimized.

@PierreRambaud

PierreRambaud May 24, 2018

Contributor

Useless line break.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 24, 2018

please @PierreRambaud, put the approval again 👍

cs
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented May 24, 2018

Sir YES Sir 👍

@mickaelandrieu mickaelandrieu merged commit fdc7350 into PrestaShop:1.7.4.x May 24, 2018

2 of 3 checks passed

code-review/reviewable 3 files, 4 discussions left (eternoendless, PierreRambaud, Quetzacoalt91)
Details
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.

Contributor

mickaelandrieu commented May 24, 2018

and ... shipped in 1.7.4.0 :)

Don't forget this pull request @eternoendless, very very very important one ^^

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:bogoss/reusable-authorization-system branch May 24, 2018

@mickaelandrieu mickaelandrieu referenced this pull request Jun 26, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Closed

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment