Skip to content

Commit f09d326

Browse files
author
Dale Sikkema
committed
Merge branch 'MAGETWO-36837-authentication-bypass' into develop
2 parents 0216eb7 + 1cd5e3d commit f09d326

File tree

5 files changed

+145
-47
lines changed

5 files changed

+145
-47
lines changed

‎app/code/Magento/Backend/App/AbstractAction.php

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@ abstract class AbstractAction extends \Magento\Framework\App\Action\Action
2222
*/
2323
const SESSION_NAMESPACE = 'adminhtml';
2424

25+
/**
26+
* Authorization level of a basic admin session
27+
*/
28+
const ADMIN_RESOURCE = 'Magento_Backend::admin';
29+
2530
/**
2631
* Array of actions which can be processed without secret key validation
2732
*
@@ -76,10 +81,17 @@ abstract class AbstractAction extends \Magento\Framework\App\Action\Action
7681
*/
7782
protected $_formKeyValidator;
7883

84+
/**
85+
* Resource used to authorize access to the controller
86+
*
87+
* @var string
88+
*/
89+
protected $resource;
90+
7991
/**
8092
* @param \Magento\Backend\App\Action\Context $context
8193
*/
82-
public function __construct(Action\Context $context)
94+
public function __construct(Action\Context $context, $resource = '')
8395
{
8496
parent::__construct($context);
8597
$this->_authorization = $context->getAuthorization();
@@ -97,7 +109,7 @@ public function __construct(Action\Context $context)
97109
*/
98110
protected function _isAllowed()
99111
{
100-
return true;
112+
return $this->_authorization->isAllowed($this->resource ?: self::ADMIN_RESOURCE);
101113
}
102114

103115
/**
@@ -228,14 +240,10 @@ public function dispatch(\Magento\Framework\App\RequestInterface $request)
228240
*/
229241
protected function _isUrlChecked()
230242
{
231-
return !$this->_actionFlag->get(
232-
'',
233-
self::FLAG_IS_URLS_CHECKED
234-
) && !$this->getRequest()->getParam(
235-
'forwarded'
236-
) && !$this->_getSession()->getIsUrlNotice(
237-
true
238-
) && !$this->_canUseBaseUrl;
243+
return !$this->_actionFlag->get('', self::FLAG_IS_URLS_CHECKED)
244+
&& !$this->getRequest()->isForwarded()
245+
&& !$this->_getSession()->getIsUrlNotice(true)
246+
&& !$this->_canUseBaseUrl;
239247
}
240248

241249
/**

‎app/code/Magento/Backend/App/Action/Plugin/Authentication.php

Lines changed: 16 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -147,46 +147,25 @@ protected function _processNotLoggedInUser(\Magento\Framework\App\RequestInterfa
147147
if ($request->getPost('login') && $this->_performLogin($request)) {
148148
$isRedirectNeeded = $this->_redirectIfNeededAfterLogin($request);
149149
}
150-
if (!$isRedirectNeeded && !$request->getParam('forwarded')) {
150+
if (!$isRedirectNeeded && !$request->isForwarded()) {
151151
if ($request->getParam('isIframe')) {
152-
$request->setParam(
153-
'forwarded',
154-
true
155-
)->setRouteName(
156-
'adminhtml'
157-
)->setControllerName(
158-
'auth'
159-
)->setActionName(
160-
'deniedIframe'
161-
)->setDispatched(
162-
false
163-
);
152+
$request->setForwarded(true)
153+
->setRouteName('adminhtml')
154+
->setControllerName('auth')
155+
->setActionName('deniedIframe')
156+
->setDispatched(false);
164157
} elseif ($request->getParam('isAjax')) {
165-
$request->setParam(
166-
'forwarded',
167-
true
168-
)->setRouteName(
169-
'adminhtml'
170-
)->setControllerName(
171-
'auth'
172-
)->setActionName(
173-
'deniedJson'
174-
)->setDispatched(
175-
false
176-
);
158+
$request->setForwarded(true)
159+
->setRouteName('adminhtml')
160+
->setControllerName('auth')
161+
->setActionName('deniedJson')
162+
->setDispatched(false);
177163
} else {
178-
$request->setParam(
179-
'forwarded',
180-
true
181-
)->setRouteName(
182-
'adminhtml'
183-
)->setControllerName(
184-
'auth'
185-
)->setActionName(
186-
'login'
187-
)->setDispatched(
188-
false
189-
);
164+
$request->setForwarded(true)
165+
->setRouteName('adminhtml')
166+
->setControllerName('auth')
167+
->setActionName('login')
168+
->setDispatched(false);
190169
}
191170
}
192171
}

‎app/code/Magento/Backend/Test/Unit/App/Action/Plugin/AuthenticationTest.php

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,4 +84,79 @@ public function testAroundDispatchProlongStorage()
8484

8585
$this->assertEquals($expectedResult, $this->plugin->aroundDispatch($subject, $proceed, $request));
8686
}
87+
88+
/**
89+
* Calls aroundDispatch to access protected method _processNotLoggedInUser
90+
*
91+
* Data provider supplies different possibilities of request parameters and properties
92+
* @dataProvider userNotLoggedInRequest
93+
*/
94+
public function testProcessNotLoggedInUser($isIFrameParam, $isAjaxParam, $isForwardedFlag)
95+
{
96+
$subject = $this->getMockBuilder('Magento\Backend\Controller\Adminhtml\Index')
97+
->disableOriginalConstructor()
98+
->getMock();
99+
$request = $this->getMockBuilder('Magento\Framework\App\Request\Http')
100+
->disableOriginalConstructor()
101+
->getMock();
102+
$storage = $this->getMockBuilder('Magento\Backend\Model\Auth\Session')
103+
->disableOriginalConstructor()
104+
->getMock();
105+
106+
// Stubs to control the flow of execution in aroundDispatch
107+
$this->auth->expects($this->any())->method('getAuthStorage')->will($this->returnValue($storage));
108+
$request->expects($this->once())->method('getActionName')->will($this->returnValue('non/open/action/name'));
109+
$this->auth->expects($this->any())->method('getUser')->willReturn(false);
110+
$this->auth->expects($this->once())->method('isLoggedIn')->will($this->returnValue(false));
111+
$request->expects($this->any())->method('getPost')->willReturn(false);
112+
113+
// Test cases and expectations based on provided data
114+
$request->expects($this->once())->method('isForwarded')->willReturn($isForwardedFlag);
115+
$getParamCalls = 0;
116+
$actionName = '';
117+
118+
// If forwarded flag is set, getParam never gets called
119+
if (!$isForwardedFlag) {
120+
if ($isIFrameParam) {
121+
$getParamCalls = 1;
122+
$actionName = 'deniedIframe';
123+
} else if ($isAjaxParam) {
124+
$getParamCalls = 2;
125+
$actionName = 'deniedJson';
126+
} else {
127+
$getParamCalls = 2;
128+
$actionName = 'login';
129+
}
130+
}
131+
132+
$requestParams = [
133+
['isIframe', null, $isIFrameParam],
134+
['isAjax', null, $isAjaxParam]
135+
];
136+
137+
$setterCalls = $isForwardedFlag ? 0 : 1;
138+
$request->expects($this->exactly($getParamCalls))->method('getParam')->willReturnMap($requestParams);
139+
$request->expects($this->exactly($setterCalls))->method('setForwarded')->with(true)->willReturnSelf();
140+
$request->expects($this->exactly($setterCalls))->method('setRouteName')->with('adminhtml')->willReturnSelf();
141+
$request->expects($this->exactly($setterCalls))->method('setControllerName')->with('auth')->willReturnSelf();
142+
$request->expects($this->exactly($setterCalls))->method('setActionName')->with($actionName)->willReturnSelf();
143+
$request->expects($this->exactly($setterCalls))->method('setDispatched')->with(false)->willReturnSelf();
144+
145+
$expectedResult = 'expectedResult';
146+
$proceed = function ($request) use ($expectedResult)
147+
{
148+
return $expectedResult;
149+
};
150+
$this->assertEquals($expectedResult, $this->plugin->aroundDispatch($subject, $proceed, $request));
151+
}
152+
153+
public function userNotLoggedInRequest()
154+
{
155+
return [
156+
'iFrame' => [true, false, false],
157+
'Ajax' => [false, true, false],
158+
'Neither iFrame nor Ajax' => [false, false, false],
159+
'Forwarded request' => [true, true, true]
160+
];
161+
}
87162
}

‎app/code/Magento/Captcha/Controller/Adminhtml/Refresh/Refresh.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,4 +27,14 @@ public function execute()
2727
$this->getResponse()->representJson(json_encode(['imgSrc' => $captchaModel->getImgSrc()]));
2828
$this->_actionFlag->set('', self::FLAG_NO_POST_DISPATCH, true);
2929
}
30+
31+
/**
32+
* Check if user has permissions to access this controller
33+
*
34+
* @return bool
35+
*/
36+
protected function _isAllowed()
37+
{
38+
return true;
39+
}
3040
}

‎lib/internal/Magento/Framework/HTTP/PhpEnvironment/Request.php

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,12 @@ class Request extends \Zend\Http\PhpEnvironment\Request
6767
*/
6868
protected $dispatched = false;
6969

70+
/**
71+
* Flag for whether the request is forwarded or not
72+
*
73+
* @var bool
74+
*/
75+
protected $forwarded;
7076

7177
/**
7278
* @var CookieReaderInterface
@@ -690,4 +696,24 @@ public function getBaseUrl()
690696
$url = str_replace('\\', '/', $url);
691697
return $url;
692698
}
699+
700+
/**
701+
* @return bool
702+
* @codeCoverageIgnore
703+
*/
704+
public function isForwarded()
705+
{
706+
return $this->forwarded;
707+
}
708+
709+
/**
710+
* @param bool $forwarded
711+
* @return $this
712+
* @codeCoverageIgnore
713+
*/
714+
public function setForwarded($forwarded)
715+
{
716+
$this->forwarded = $forwarded;
717+
return $this;
718+
}
693719
}

0 commit comments

Comments
 (0)