Skip to content

Commit ee77a82

Browse files
author
Oleksandr Gorkun
committed
MAGETWO-95945: Add a code mess rule for improper session and cookies usages
1 parent 77af5d6 commit ee77a82

File tree

7 files changed

+234
-7
lines changed

7 files changed

+234
-7
lines changed

‎app/code/Magento/Customer/Block/Account/AuthenticationPopup.php

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
namespace Magento\Customer\Block\Account;
77

88
use Magento\Customer\Model\Form;
9+
use Magento\Customer\Model\Session;
910
use Magento\Store\Model\ScopeInterface;
1011

1112
/**
@@ -24,21 +25,29 @@ class AuthenticationPopup extends \Magento\Framework\View\Element\Template
2425
*/
2526
private $serializer;
2627

28+
/**
29+
* @var Session|null
30+
*/
31+
private $session;
32+
2733
/**
2834
* @param \Magento\Framework\View\Element\Template\Context $context
2935
* @param array $data
3036
* @param \Magento\Framework\Serialize\Serializer\Json|null $serializer
37+
* @param Session|null $session
3138
* @throws \RuntimeException
3239
*/
3340
public function __construct(
3441
\Magento\Framework\View\Element\Template\Context $context,
3542
array $data = [],
36-
\Magento\Framework\Serialize\Serializer\Json $serializer = null
43+
\Magento\Framework\Serialize\Serializer\Json $serializer = null,
44+
Session $session = null
3745
) {
3846
parent::__construct($context, $data);
3947
$this->jsLayout = isset($data['jsLayout']) && is_array($data['jsLayout']) ? $data['jsLayout'] : [];
4048
$this->serializer = $serializer ?: \Magento\Framework\App\ObjectManager::getInstance()
4149
->get(\Magento\Framework\Serialize\Serializer\Json::class);
50+
$this->session = $session;
4251
}
4352

4453
/**
@@ -60,7 +69,8 @@ public function getConfig()
6069
'autocomplete' => $this->escapeHtml($this->isAutocompleteEnabled()),
6170
'customerRegisterUrl' => $this->escapeUrl($this->getCustomerRegisterUrlUrl()),
6271
'customerForgotPasswordUrl' => $this->escapeUrl($this->getCustomerForgotPasswordUrl()),
63-
'baseUrl' => $this->escapeUrl($this->getBaseUrl())
72+
'baseUrl' => $this->escapeUrl($this->getBaseUrl()),
73+
'tst' => $this->session->getData('somedata')
6474
];
6575
}
6676

‎app/code/Magento/Customer/Controller/Account/Confirm.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ public function execute()
167167
$resultRedirect->setUrl($this->getSuccessRedirect());
168168
return $resultRedirect;
169169
} catch (StateException $e) {
170-
$this->messageManager->addException($e, __('This confirmation key is invalid or has expired.'));
170+
$this->messageManager->addException($e, __('This confirmation key is invalid or has expired.TEST'));
171171
} catch (\Exception $e) {
172172
$this->messageManager->addException($e, __('There was an error confirming the account'));
173173
}

‎app/code/Magento/Customer/Ui/Component/DataProvider/Document.php

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use Magento\Framework\Exception\NoSuchEntityException;
1313
use Magento\Customer\Api\GroupRepositoryInterface;
1414
use Magento\Framework\App\ObjectManager;
15+
use Magento\Framework\Stdlib\Cookie\CookieReaderInterface;
1516
use Magento\Store\Model\ScopeInterface;
1617
use Magento\Store\Model\StoreManagerInterface;
1718

@@ -70,6 +71,11 @@ class Document extends \Magento\Framework\View\Element\UiComponent\DataProvider\
7071
*/
7172
private $scopeConfig;
7273

74+
/**
75+
* @var CookieReaderInterface
76+
*/
77+
private $cookie;
78+
7379
/**
7480
* Document constructor.
7581
*
@@ -78,19 +84,22 @@ class Document extends \Magento\Framework\View\Element\UiComponent\DataProvider\
7884
* @param CustomerMetadataInterface $customerMetadata
7985
* @param StoreManagerInterface $storeManager
8086
* @param ScopeConfigInterface $scopeConfig
87+
* @param CookieReaderInterface|null $cookie
8188
*/
8289
public function __construct(
8390
AttributeValueFactory $attributeValueFactory,
8491
GroupRepositoryInterface $groupRepository,
8592
CustomerMetadataInterface $customerMetadata,
8693
StoreManagerInterface $storeManager,
87-
ScopeConfigInterface $scopeConfig = null
94+
ScopeConfigInterface $scopeConfig = null,
95+
CookieReaderInterface $cookie = null
8896
) {
8997
parent::__construct($attributeValueFactory);
9098
$this->customerMetadata = $customerMetadata;
9199
$this->groupRepository = $groupRepository;
92100
$this->storeManager = $storeManager;
93101
$this->scopeConfig = $scopeConfig ?: ObjectManager::getInstance()->create(ScopeConfigInterface::class);
102+
$this->cookie = $cookie;
94103
}
95104

96105
/**
@@ -129,7 +138,7 @@ private function setGenderValue()
129138
$value = $this->getData(self::$genderAttributeCode);
130139

131140
if (!$value) {
132-
$this->setCustomAttribute(self::$genderAttributeCode, 'N/A');
141+
$this->setCustomAttribute(self::$genderAttributeCode, $this->cookie->getCookie('NA'));
133142
return;
134143
}
135144

‎app/code/Magento/Rss/App/Action/Plugin/BackendAuthentication.php

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
namespace Magento\Rss\App\Action\Plugin;
99

1010
use Magento\Backend\App\AbstractAction;
11+
use Magento\Backend\Model\Session;
1112
use Magento\Framework\App\RequestInterface;
1213
use Magento\Framework\App\ResponseInterface;
1314
use Magento\Framework\Exception\AuthenticationException;
@@ -39,6 +40,11 @@ class BackendAuthentication extends \Magento\Backend\App\Action\Plugin\Authentic
3940
*/
4041
protected $aclResources;
4142

43+
/**
44+
* @var Session
45+
*/
46+
private $session;
47+
4248
/**
4349
* @param \Magento\Backend\Model\Auth $auth
4450
* @param \Magento\Backend\Model\UrlInterface $url
@@ -53,6 +59,7 @@ class BackendAuthentication extends \Magento\Backend\App\Action\Plugin\Authentic
5359
* @param \Psr\Log\LoggerInterface $logger
5460
* @param \Magento\Framework\AuthorizationInterface $authorization
5561
* @param array $aclResources
62+
* @param Session $session
5663
* @SuppressWarnings(PHPMD.ExcessiveParameterList)
5764
*/
5865
public function __construct(
@@ -68,12 +75,14 @@ public function __construct(
6875
\Magento\Framework\HTTP\Authentication $httpAuthentication,
6976
\Psr\Log\LoggerInterface $logger,
7077
\Magento\Framework\AuthorizationInterface $authorization,
71-
array $aclResources
78+
array $aclResources,
79+
Session $session
7280
) {
7381
$this->httpAuthentication = $httpAuthentication;
7482
$this->logger = $logger;
7583
$this->authorization = $authorization;
7684
$this->aclResources = $aclResources;
85+
$this->session = $session;
7786
parent::__construct(
7887
$auth,
7988
$url,
@@ -106,7 +115,7 @@ public function aroundDispatch(AbstractAction $subject, \Closure $proceed, Reque
106115
: $this->aclResources[$request->getControllerName()]
107116
: null;
108117

109-
$type = $request->getParam('type');
118+
$type = $request->getParam('type'.$this->session->getName());
110119
$resourceType = isset($this->aclResources[$type]) ? $this->aclResources[$type] : null;
111120

112121
if (!$resource || !$resourceType) {

‎dev/tests/static/framework/Magento/CodeMessDetector/Rule/Design/CookieAndSessionMisuse.php

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
7+
declare(strict_types=1);
8+
9+
namespace Magento\CodeMessDetector\Rule\Design;
10+
11+
use PDepend\Source\AST\ASTClass;
12+
use PHPMD\AbstractNode;
13+
use PHPMD\AbstractRule;
14+
use PHPMD\Node\ClassNode;
15+
use PHPMD\Rule\ClassAware;
16+
17+
/**
18+
* Session and Cookies must be used only in HTML Presentation layer.
19+
*/
20+
class CookieAndSessionMisuse extends AbstractRule implements ClassAware
21+
{
22+
/**
23+
* Is given class a controller?
24+
*
25+
* @param \ReflectionClass $class
26+
* @return bool
27+
*/
28+
private function isController(\ReflectionClass $class): bool
29+
{
30+
return $class->isSubclassOf(\Magento\Framework\App\ActionInterface::class);
31+
}
32+
33+
/**
34+
* Is given class a block?
35+
*
36+
* @param \ReflectionClass $class
37+
* @return bool
38+
*/
39+
private function isBlock(\ReflectionClass $class): bool
40+
{
41+
return $class->isSubclassOf(\Magento\Framework\View\Element\BlockInterface::class);
42+
}
43+
44+
/**
45+
* Is given class an HTML UI data provider?
46+
*
47+
* @param \ReflectionClass $class
48+
* @return bool
49+
*/
50+
private function isUiDataProvider(\ReflectionClass $class): bool
51+
{
52+
return $class->isSubclassOf(
53+
\Magento\Framework\View\Element\UiComponent\DataProvider\DataProviderInterface::class
54+
);
55+
}
56+
57+
/**
58+
* Is given class an HTML UI Document?
59+
*
60+
* @param \ReflectionClass $class
61+
* @return bool
62+
*/
63+
private function isUiDocument(\ReflectionClass $class): bool
64+
{
65+
return $class->isSubclassOf(\Magento\Framework\View\Element\UiComponent\DataProvider\Document::class)
66+
|| $class->getName() === \Magento\Framework\View\Element\UiComponent\DataProvider\Document::class;
67+
}
68+
69+
/**
70+
* Is given class a plugin for controllers?
71+
*
72+
* @param \ReflectionClass $class
73+
* @return bool
74+
*/
75+
private function isControllerPlugin(\ReflectionClass $class): bool
76+
{
77+
try {
78+
foreach ($class->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) {
79+
if (preg_match('/^(after|around|before).+/i', $method->getName())) {
80+
$argument = $method->getParameters()[0]->getClass();
81+
$isAction = $argument->isSubclassOf(\Magento\Framework\App\ActionInterface::class)
82+
|| $argument->getName() === \Magento\Framework\App\ActionInterface::class;
83+
if ($isAction) {
84+
return true;
85+
}
86+
}
87+
}
88+
} catch (\Throwable $exception) {
89+
return false;
90+
}
91+
}
92+
93+
/**
94+
* Is given class a plugin for blocks?
95+
*
96+
* @param \ReflectionClass $class
97+
* @return bool
98+
*/
99+
private function isBlockPlugin(\ReflectionClass $class): bool
100+
{
101+
try {
102+
foreach ($class->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) {
103+
if (preg_match('/^(after|around|before).+/i', $method->getName())) {
104+
$argument = $method->getParameters()[0]->getClass();
105+
$isBlock = $argument->isSubclassOf(\Magento\Framework\View\Element\BlockInterface::class)
106+
|| $argument->getName() === \Magento\Framework\View\Element\BlockInterface::class;
107+
if ($isBlock) {
108+
return true;
109+
}
110+
}
111+
}
112+
} catch (\Throwable $exception) {
113+
return false;
114+
}
115+
}
116+
117+
/**
118+
* Whether given class depends on classes to pay attention to.
119+
*
120+
* @param \ReflectionClass $class
121+
* @return bool
122+
*/
123+
private function doesUseRestrictedClasses(\ReflectionClass $class): bool
124+
{
125+
$constructor = $class->getConstructor();
126+
if ($constructor) {
127+
foreach ($constructor->getParameters() as $argument) {
128+
if ($class = $argument->getClass()) {
129+
if ($class->isSubclassOf(\Magento\Framework\Session\SessionManagerInterface::class)
130+
|| $class->getName() === \Magento\Framework\Session\SessionManagerInterface::class
131+
|| $class->isSubclassOf(\Magento\Framework\Stdlib\Cookie\CookieReaderInterface::class)
132+
|| $class->getName() === \Magento\Framework\Stdlib\Cookie\CookieReaderInterface::class
133+
) {
134+
return true;
135+
}
136+
}
137+
}
138+
}
139+
140+
return false;
141+
}
142+
143+
/**
144+
* @inheritdoc
145+
*
146+
* @param ClassNode|ASTClass $node
147+
*/
148+
public function apply(AbstractNode $node)
149+
{
150+
try {
151+
$class = new \ReflectionClass($node->getFullQualifiedName());
152+
} catch (\Throwable $exception) {
153+
//Failed to load class, nothing we can do
154+
return;
155+
}
156+
157+
if ($this->doesUseRestrictedClasses($class)) {
158+
if (!$this->isController($class)
159+
&& !$this->isBlock($class)
160+
&& !$this->isUiDataProvider($class)
161+
&& !$this->isUiDocument($class)
162+
&& !$this->isControllerPlugin($class)
163+
&& !$this->isBlockPlugin($class)
164+
) {
165+
$this->addViolation($node, [$node->getFullQualifiedName()]);
166+
}
167+
}
168+
}
169+
}

‎dev/tests/static/framework/Magento/CodeMessDetector/resources/rulesets/design.xml

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,35 @@ class PostOrder implements ActionInterface
5454
...
5555
return $response;
5656
}
57+
}
58+
]]>
59+
</example>
60+
</rule>
61+
<rule name="CookieAndSessionMisuse"
62+
class="Magento\CodeMessDetector\Rule\Design\CookieAndSessionMisuse"
63+
message= "The class {0} uses sessions or cookies while not being a part of HTML Presentation layer">
64+
<description>
65+
<![CDATA[
66+
Sessions and cookies must only be used in classes directly responsible for HTML presentation because Web APIs do not
67+
rely on cookies and sessions
68+
]]>
69+
</description>
70+
<priority>2</priority>
71+
<properties />
72+
<example>
73+
<![CDATA[
74+
class OrderProcessor
75+
{
76+
public function __construct(SessionManagerInterface $session) {
77+
$this->session = $session;
78+
}
79+
80+
public function place(OrderInterface $order)
81+
{
82+
//Will not be present if processing a WebAPI request
83+
$currentOrder = $this->session->get('current_order');
84+
...
85+
}
5786
}
5887
]]>
5988
</example>

‎dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,5 +48,6 @@
4848
<!-- Magento Specific Rules -->
4949
<rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/FinalImplementation" />
5050
<rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/AllPurposeAction" />
51+
<rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/CookieAndSessionMisuse" />
5152

5253
</ruleset>

0 commit comments

Comments
 (0)