Skip to content
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

#10611 Ignore request key check for error page to avoid redirect loop #10921

Merged
merged 1 commit into from
Sep 29, 2017
Merged

#10611 Ignore request key check for error page to avoid redirect loop #10921

merged 1 commit into from
Sep 29, 2017

Conversation

lbajsarowicz
Copy link
Contributor

@lbajsarowicz lbajsarowicz commented Sep 17, 2017

Description

Problems described in the issue was caused by redirect occuring when request made to the application had incorrect URL key. This specific situation took place only when user had no access to any part of application. So finally trying to get access to adminhtml/index controller, he got redirected to adminhtml/noroute/denied that he also have no access because of incorrect URL key.

Fixed Issues (if relevant)

  1. Magento admin gets in redirect loop when I login with a role that has no resources assigned #10611 Magento admin gets in redirect loop when I login with a role that has no resources assigned

Manual testing scenarios

  1. Create a user role with no resources assigned to it.
  2. Create an admin user with that role.
  3. Log in with that user.

Contribution checklist

  • [ x ] Pull request has a meaningful description of its purpose
  • [ x ] All commits are accompanied by meaningful commit messages
  • [ - ] All new or changed code is covered with unit/integration tests (if applicable)
  • [ x ] All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 17, 2017

CLA assistant check
All committers have signed the CLA.

@ishakhsuvarov ishakhsuvarov added this to the September 2017 milestone Sep 17, 2017
@vkublytskyi vkublytskyi self-assigned this Sep 27, 2017
*
* @return bool
*/
protected function _validateSecretKey()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done in more consistent with other controllers way.

Magento\Backend\App\AbstractAction already provides a mechanism for disabling secret key validation: \Magento\Backend\App\AbstractAction::$_publicActions. Setting this property will be more correct and consistent fix:

$_publicActions = ['index']

However, both solution is fixing the only symptom of an issue, not a root cause. Secret check failed because of invalid value for action (key generated for denied and checked for index). That should be investigated and fixed as well. Another one issue is why request redirected to */*/denied path which actually does not match any action controller class and fall back to noroute controller which does not display own content but has special handling in Magento\Backend\App\AbstractAction to render adminhtml_denied. In addition Magento\Backend\Controller\Adminhtml\Denied action controller is not in proper place and seems should be moved to Magento\Backend\Controller\Adminhtml\Index namespace.

As full fix of the issue require deep refactoring of Magento_Backend module this fix will be accepted. We will be thankful if you continue your work and fix legacy implementation to achieve more straightforward and predictable routing in admin.

@magento-team magento-team merged commit 2f422ad into magento:develop Sep 29, 2017
magento-team pushed a commit that referenced this pull request Sep 29, 2017
convenient added a commit to convenient/mtwo that referenced this pull request Oct 4, 2017
Similar to 2f422ad but using the framework.

Fixes issue magento#10611 for the `2.2-develop` branch.

Doesn't address the route cause, but only fixes the symptom as per magento#10921 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants