Skip to content

Commit ec25977

Browse files
dvoskoboinikovdvoskoboinikov
authored
Merge pull request #3593 from magento-qwerty/2.3-bugfixes-020119
Fixed issues: - MAGETWO-95945: Add a code mess rule for improper session and cookies usages - MAGETWO-95928: Remove RequestAwareBlockMethod and update Technical Vision
2 parents 45e3ddb + 6dae815 commit ec25977

File tree

5 files changed

+198
-71
lines changed

5 files changed

+198
-71
lines changed

‎app/code/Magento/Widget/Block/Adminhtml/Widget.php

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@ class Widget extends \Magento\Backend\Block\Widget\Form\Container
1616
{
1717
/**
1818
* @inheritdoc
19-
*
20-
* @SuppressWarnings(PHPMD.RequestAwareBlockMethod)
2119
*/
2220
protected function _construct()
2321
{

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

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
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+
foreach ($class->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) {
78+
if (preg_match('/^(after|around|before).+/i', $method->getName())) {
79+
try {
80+
$argument = $method->getParameters()[0]->getClass();
81+
} catch (\Throwable $exception) {
82+
//Non-existing class (autogenerated perhaps) or doesn't have an argument.
83+
continue;
84+
}
85+
if ($argument) {
86+
$isAction = $argument->isSubclassOf(\Magento\Framework\App\ActionInterface::class)
87+
|| $argument->getName() === \Magento\Framework\App\ActionInterface::class;
88+
if ($isAction) {
89+
return true;
90+
}
91+
}
92+
}
93+
}
94+
95+
return false;
96+
}
97+
98+
/**
99+
* Is given class a plugin for blocks?
100+
*
101+
* @param \ReflectionClass $class
102+
* @return bool
103+
*/
104+
private function isBlockPlugin(\ReflectionClass $class): bool
105+
{
106+
foreach ($class->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) {
107+
if (preg_match('/^(after|around|before).+/i', $method->getName())) {
108+
try {
109+
$argument = $method->getParameters()[0]->getClass();
110+
} catch (\Throwable $exception) {
111+
//Non-existing class (autogenerated perhaps) or doesn't have an argument.
112+
continue;
113+
}
114+
if ($argument) {
115+
$isBlock = $argument->isSubclassOf(\Magento\Framework\View\Element\BlockInterface::class)
116+
|| $argument->getName() === \Magento\Framework\View\Element\BlockInterface::class;
117+
if ($isBlock) {
118+
return true;
119+
}
120+
}
121+
}
122+
}
123+
124+
return false;
125+
}
126+
127+
/**
128+
* Whether given class depends on classes to pay attention to.
129+
*
130+
* @param \ReflectionClass $class
131+
* @return bool
132+
*/
133+
private function doesUseRestrictedClasses(\ReflectionClass $class): bool
134+
{
135+
$constructor = $class->getConstructor();
136+
if ($constructor) {
137+
foreach ($constructor->getParameters() as $argument) {
138+
try {
139+
if ($class = $argument->getClass()) {
140+
if ($class->isSubclassOf(\Magento\Framework\Session\SessionManagerInterface::class)
141+
|| $class->getName() === \Magento\Framework\Session\SessionManagerInterface::class
142+
|| $class->isSubclassOf(\Magento\Framework\Stdlib\Cookie\CookieReaderInterface::class)
143+
|| $class->getName() === \Magento\Framework\Stdlib\Cookie\CookieReaderInterface::class
144+
) {
145+
return true;
146+
}
147+
}
148+
} catch (\ReflectionException $exception) {
149+
//Failed to load the argument's class information
150+
continue;
151+
}
152+
}
153+
}
154+
155+
return false;
156+
}
157+
158+
/**
159+
* @inheritdoc
160+
*
161+
* @param ClassNode|ASTClass $node
162+
*/
163+
public function apply(AbstractNode $node)
164+
{
165+
try {
166+
$class = new \ReflectionClass($node->getFullQualifiedName());
167+
} catch (\Throwable $exception) {
168+
//Failed to load class, nothing we can do
169+
return;
170+
}
171+
172+
if ($this->doesUseRestrictedClasses($class)) {
173+
if (!$this->isController($class)
174+
&& !$this->isBlock($class)
175+
&& !$this->isUiDataProvider($class)
176+
&& !$this->isUiDocument($class)
177+
&& !$this->isControllerPlugin($class)
178+
&& !$this->isBlockPlugin($class)
179+
) {
180+
$this->addViolation($node, [$node->getFullQualifiedName()]);
181+
}
182+
}
183+
}
184+
}

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

Lines changed: 0 additions & 53 deletions
This file was deleted.

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

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,32 +58,30 @@ class PostOrder implements ActionInterface
5858
]]>
5959
</example>
6060
</rule>
61-
<rule name="RequestAwareBlockMethod"
62-
class="Magento\CodeMessDetector\Rule\Design\RequestAwareBlockMethod"
63-
message="{0} uses request object directly. Add user input validation and suppress this warning.">
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">
6464
<description>
6565
<![CDATA[
66-
Blocks must not depend on being used with certain controllers.
67-
If you use request object in a block directly you must validate all user input inside the block.
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. If you need to get current user use Magento\Authorization\Model\UserContextInterface
6868
]]>
6969
</description>
7070
<priority>2</priority>
7171
<properties />
7272
<example>
7373
<![CDATA[
74-
class MyOrder extends AbstractBlock
74+
class OrderProcessor
7575
{
76+
public function __construct(SessionManagerInterface $session) {
77+
$this->session = $session;
78+
}
7679
77-
.......
78-
79-
public function getOrder()
80+
public function place(OrderInterface $order)
8081
{
81-
$orderId = $this->getRequest()->getParam('order_id');
82-
//Validate customer having such order.
83-
if (!$this->hasOrder($this->getCustomerId(), $orderId)) {
84-
...deny access...
85-
}
86-
.....
82+
//Will not be present if processing a WebAPI request
83+
$currentOrder = $this->session->get('current_order');
84+
...
8785
}
8886
}
8987
]]>

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +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/RequestAwareBlockMethod" />
51+
<rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/CookieAndSessionMisuse" />
5252

5353
</ruleset>

0 commit comments

Comments
 (0)