MAGETWO-37981: Incorrect action name checks leads to customer info leak

Dale Sikkema · Dale Sikkema · commit da2f15e4c3bc · 2015-06-15T23:41:25.000-05:00
diff --git a/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification.php b/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification.php
@@ -14,26 +14,6 @@ class Notification extends \Magento\Backend\App\AbstractAction
      */
     protected function _isAllowed()
     {
-        switch ($this->getRequest()->getActionName()) {
-            case 'markAsRead':
-                $acl = 'Magento_AdminNotification::mark_as_read';
-                break;
-
-            case 'massMarkAsRead':
-                $acl = 'Magento_AdminNotification::mark_as_read';
-                break;
-
-            case 'remove':
-                $acl = 'Magento_AdminNotification::adminnotification_remove';
-                break;
-
-            case 'massRemove':
-                $acl = 'Magento_AdminNotification::adminnotification_remove';
-                break;
-
-            default:
-                $acl = 'Magento_AdminNotification::show_list';
-        }
-        return $this->_authorization->isAllowed($acl);
+        return $this->_authorization->isAllowed('Magento_AdminNotification::show_list');
     }
 }
diff --git a/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MarkAsRead.php b/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MarkAsRead.php
@@ -36,4 +36,12 @@ public function execute()
         }
         $this->_redirect('adminhtml/*/');
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_AdminNotification::mark_as_read');
+    }
 }
diff --git a/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassMarkAsRead.php b/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassMarkAsRead.php
@@ -38,4 +38,12 @@ public function execute()
         }
         $this->_redirect('adminhtml/*/');
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_AdminNotification::mark_as_read');
+    }
 }
diff --git a/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassRemove.php b/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/MassRemove.php
@@ -33,4 +33,12 @@ public function execute()
         }
         $this->getResponse()->setRedirect($this->_redirect->getRedirectUrl($this->getUrl('*')));
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_AdminNotification::adminnotification_remove');
+    }
 }
diff --git a/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/Remove.php b/app/code/Magento/AdminNotification/Controller/Adminhtml/Notification/Remove.php
@@ -35,4 +35,12 @@ public function execute()
         }
         $this->_redirect('adminhtml/*/');
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_AdminNotification::adminnotification_remove');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order.php b/app/code/Magento/Sales/Controller/Adminhtml/Order.php
@@ -124,47 +124,10 @@ protected function _initOrder()
     }
 
     /**
-     * Acl check for admin
-     *
      * @return bool
-     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
      */
     protected function _isAllowed()
     {
-        $action = strtolower($this->getRequest()->getActionName());
-        switch ($action) {
-            case 'hold':
-                $aclResource = 'Magento_Sales::hold';
-                break;
-            case 'unhold':
-                $aclResource = 'Magento_Sales::unhold';
-                break;
-            case 'email':
-                $aclResource = 'Magento_Sales::email';
-                break;
-            case 'cancel':
-                $aclResource = 'Magento_Sales::cancel';
-                break;
-            case 'view':
-                $aclResource = 'Magento_Sales::actions_view';
-                break;
-            case 'addcomment':
-                $aclResource = 'Magento_Sales::comment';
-                break;
-            case 'creditmemos':
-                $aclResource = 'Magento_Sales::creditmemo';
-                break;
-            case 'reviewpayment':
-                $aclResource = 'Magento_Sales::review_payment';
-                break;
-            case 'address':
-            case 'addresssave':
-                $aclResource = 'Magento_Sales::actions_edit';
-                break;
-            default:
-                $aclResource = 'Magento_Sales::sales_order';
-                break;
-        }
-        return $this->_authorization->isAllowed($aclResource);
+        return $this->_authorization->isAllowed('Magento_Sales::sales_order');
     }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/AddComment.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/AddComment.php
@@ -57,4 +57,12 @@ public function execute()
         }
         return $this->resultRedirectFactory->create()->setPath('sales/*/');
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::comment');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/Address.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/Address.php
@@ -31,4 +31,12 @@ public function execute()
             return $this->resultRedirectFactory->create()->setPath('sales/*/');
         }
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::actions_edit');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/AddressSave.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/AddressSave.php
@@ -35,4 +35,12 @@ public function execute()
             return $resultRedirect->setPath('sales/*/');
         }
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::actions_edit');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/Cancel.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/Cancel.php
@@ -31,4 +31,12 @@ public function execute()
         }
         return $resultRedirect->setPath('sales/*/');
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::cancel');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/Create.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/Create.php
@@ -373,7 +373,7 @@ protected function _reloadQuote()
      */
     protected function _isAllowed()
     {
-        return $this->_authorization->isAllowed($this->_getAclResource());
+        return $this->_authorization->isAllowed('Magento_Sales::actions');
     }
 
     /**
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/Creditmemos.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/Creditmemos.php
@@ -18,4 +18,12 @@ public function execute()
         $resultLayout = $this->resultLayoutFactory->create();
         return $resultLayout;
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::creditmemo');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/Email.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/Email.php
@@ -29,4 +29,12 @@ public function execute()
         }
         return $this->resultRedirectFactory->create()->setPath('sales/*/');
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::email');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/Hold.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/Hold.php
@@ -31,4 +31,12 @@ public function execute()
         $resultRedirect->setPath('sales/*/');
         return $resultRedirect;
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::hold');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/ReviewPayment.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/ReviewPayment.php
@@ -53,4 +53,12 @@ public function execute()
         $resultRedirect->setPath('sales/order/view', ['order_id' => $order->getId()]);
         return $resultRedirect;
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::review_payment');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/Unhold.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/Unhold.php
@@ -31,4 +31,12 @@ public function execute()
         $resultRedirect->setPath('sales/*/');
         return $resultRedirect;
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::unhold');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Order/View.php b/app/code/Magento/Sales/Controller/Adminhtml/Order/View.php
@@ -34,4 +34,12 @@ public function execute()
         $resultRedirect->setPath('sales/*/');
         return $resultRedirect;
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::actions_view');
+    }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Transactions.php b/app/code/Magento/Sales/Controller/Adminhtml/Transactions.php
@@ -86,13 +86,6 @@ protected function _initTransaction()
      */
     protected function _isAllowed()
     {
-        switch ($this->getRequest()->getActionName()) {
-            case 'fetch':
-                return $this->_authorization->isAllowed('Magento_Sales::transactions_fetch');
-                break;
-            default:
-                return $this->_authorization->isAllowed('Magento_Sales::transactions');
-                break;
-        }
+        return $this->_authorization->isAllowed('Magento_Sales::transactions');
     }
 }
diff --git a/app/code/Magento/Sales/Controller/Adminhtml/Transactions/Fetch.php b/app/code/Magento/Sales/Controller/Adminhtml/Transactions/Fetch.php
@@ -37,4 +37,12 @@ public function getDefaultResult()
         $resultRedirect = $this->resultRedirectFactory->create();
         return $resultRedirect->setPath('sales/transactions/view', ['_current' => true]);
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_Sales::transactions_fetch');
+    }
 }
diff --git a/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate.php b/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate.php
@@ -32,18 +32,6 @@ public function __construct(
      */
     protected function _isAllowed()
     {
-        switch ($this->getRequest()->getActionName()) {
-            case 'importExport':
-                return $this->_authorization->isAllowed('Magento_TaxImportExport::import_export');
-            case 'importPost':
-            case 'exportPost':
-                return $this->_authorization->isAllowed(
-                    'Magento_Tax::manage_tax'
-                ) || $this->_authorization->isAllowed(
-                    'Magento_TaxImportExport::import_export'
-                );
-            default:
-                return $this->_authorization->isAllowed('Magento_Tax::manage_tax');
-        }
+        return $this->_authorization->isAllowed('Magento_Tax::manage_tax');
     }
 }
diff --git a/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate/ExportPost.php b/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate/ExportPost.php
@@ -83,4 +83,17 @@ public function execute()
         }
         return $this->fileFactory->create('tax_rates.csv', $content, DirectoryList::VAR_DIR);
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed(
+            'Magento_Tax::manage_tax'
+        ) || $this->_authorization->isAllowed(
+            'Magento_TaxImportExport::import_export'
+        );
+
+    }
 }
diff --git a/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate/ImportExport.php b/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate/ImportExport.php
@@ -30,4 +30,12 @@ public function execute()
         $resultPage->getConfig()->getTitle()->prepend(__('Import and Export Tax Rates'));
         return $resultPage;
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed('Magento_TaxImportExport::import_export');
+    }
 }
diff --git a/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate/ImportPost.php b/app/code/Magento/TaxImportExport/Controller/Adminhtml/Rate/ImportPost.php
@@ -36,4 +36,17 @@ public function execute()
         $resultRedirect->setUrl($this->_redirect->getRedirectUrl());
         return $resultRedirect;
     }
+
+    /**
+     * @return bool
+     */
+    protected function _isAllowed()
+    {
+        return $this->_authorization->isAllowed(
+            'Magento_Tax::manage_tax'
+        ) || $this->_authorization->isAllowed(
+            'Magento_TaxImportExport::import_export'
+        );
+
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -36,4 +36,12 @@ public function execute()`
`36`	`36`	`}`
`37`	`37`	`$this->_redirect('adminhtml/*/');`
`38`	`38`	`}`
	`39`	`+`
	`40`	`+ /**`
	`41`	`+ * @return bool`
	`42`	`+ */`
	`43`	`+ protected function _isAllowed()`
	`44`	`+ {`
	`45`	`+ return $this->_authorization->isAllowed('Magento_AdminNotification::mark_as_read');`
	`46`	`+ }`
`39`	`47`	`}`
Original file line number	Diff line number	Diff line change
`@@ -38,4 +38,12 @@ public function execute()`
`38`	`38`	`}`
`39`	`39`	`$this->_redirect('adminhtml/*/');`
`40`	`40`	`}`
	`41`	`+`
	`42`	`+ /**`
	`43`	`+ * @return bool`
	`44`	`+ */`
	`45`	`+ protected function _isAllowed()`
	`46`	`+ {`
	`47`	`+ return $this->_authorization->isAllowed('Magento_AdminNotification::mark_as_read');`
	`48`	`+ }`
`41`	`49`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,4 +33,12 @@ public function execute()`
`33`	`33`	`}`
`34`	`34`	`$this->getResponse()->setRedirect($this->_redirect->getRedirectUrl($this->getUrl('*')));`
`35`	`35`	`}`
	`36`	`+`
	`37`	`+ /**`
	`38`	`+ * @return bool`
	`39`	`+ */`
	`40`	`+ protected function _isAllowed()`
	`41`	`+ {`
	`42`	`+ return $this->_authorization->isAllowed('Magento_AdminNotification::adminnotification_remove');`
	`43`	`+ }`
`36`	`44`	`}`
Original file line number	Diff line number	Diff line change
`@@ -35,4 +35,12 @@ public function execute()`
`35`	`35`	`}`
`36`	`36`	`$this->_redirect('adminhtml/*/');`
`37`	`37`	`}`
	`38`	`+`
	`39`	`+ /**`
	`40`	`+ * @return bool`
	`41`	`+ */`
	`42`	`+ protected function _isAllowed()`
	`43`	`+ {`
	`44`	`+ return $this->_authorization->isAllowed('Magento_AdminNotification::adminnotification_remove');`
	`45`	`+ }`
`38`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -57,4 +57,12 @@ public function execute()`
`57`	`57`	`}`
`58`	`58`	`return $this->resultRedirectFactory->create()->setPath('sales/*/');`
`59`	`59`	`}`
	`60`	`+`
	`61`	`+ /**`
	`62`	`+ * @return bool`
	`63`	`+ */`
	`64`	`+ protected function _isAllowed()`
	`65`	`+ {`
	`66`	`+ return $this->_authorization->isAllowed('Magento_Sales::comment');`
	`67`	`+ }`
`60`	`68`	`}`
Original file line number	Diff line number	Diff line change
`@@ -31,4 +31,12 @@ public function execute()`
`31`	`31`	`return $this->resultRedirectFactory->create()->setPath('sales/*/');`
`32`	`32`	`}`
`33`	`33`	`}`
	`34`	`+`
	`35`	`+ /**`
	`36`	`+ * @return bool`
	`37`	`+ */`
	`38`	`+ protected function _isAllowed()`
	`39`	`+ {`
	`40`	`+ return $this->_authorization->isAllowed('Magento_Sales::actions_edit');`
	`41`	`+ }`
`34`	`42`	`}`