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

Fix CMS Admin access issues #115

Merged
merged 3 commits into from
Oct 22, 2013
Merged

Conversation

mateusz
Copy link
Contributor

@mateusz mateusz commented Oct 16, 2013

This fixes issues with Subsites where an CMS Administrator is denied access in some situations:

  • if he doesn't have access to SubsiteID=0, but we are operating from one master domain.
  • if he doesn't have access to SubsiteAdmin and cannot execute the SubsiteList PJAX request

Currently the request cannot be made if one doesn't have access to the
SubsiteAdmin section, which often happens with subsite-specific admins.
Tries to find an accessible section in the current site, falls back to
searching across all sites and all sections.

Also adds more powerful and generic functionss:
Subsites::all_sites - get the full list
Subsites::all_accessible_sites - get Member accessible list
LeftAndMainExtension::sectionSites - get section-specific list
@mateusz
Copy link
Contributor Author

mateusz commented Oct 18, 2013

Yay, it passes! :shipit:

@chillu
Copy link
Member

chillu commented Oct 22, 2013

Tested a couple of variations, with a CMS sub-admin (access to subsite1 and "Pages" section only).
Each request was performed in a new browser/PHP session.

So I think it works better than before, but the two failures should be fixed as well, right? I can have a look, but wanted to check if you have any ideas first.

@mateusz
Copy link
Contributor Author

mateusz commented Oct 22, 2013

The idea was to first redirect to the same site, different section, and then if not possible to another site, any section. So on your list, assuming that you only have access to subsite1/Pages, the results for all above should be redirection to subsite1 > admin/pages (admin/pages URL).

I guess it would be worth to check why the tests are not catching this situation - I thought I have tested for this. That could highlight a gap in my reasoning.


// Check forbidden site.
$this->getAndFollowAll("admin/pages/?SubsiteID=0");
$this->assertEquals(Subsite::currentSubsiteID(), $subsite1->ID, 'Is redirected to permitted subsite.');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh here, a test case seems to be missing. This one here only tries to land on a section that's permitted on another subsite. We'd need another one trying to access a section that's not accessible on any subsite with an account that can access something.

$this->getAndFollowAll("admin/inaccessible-existing-section/?SubsiteID=0");
$this->assertEquals(Subsite::currentSubsiteID(), $subsite1->ID, 'Is redirected to first permitted subsite.');
$this->assertNotRegExp('#^Security/login.*#', $this->mainSession->lastUrl(), 'Is not denied access');

chillu added a commit that referenced this pull request Oct 22, 2013
@chillu chillu merged commit d85412a into silverstripe:master Oct 22, 2013
@chillu
Copy link
Member

chillu commented Oct 22, 2013

Fixed those edge cases mentioned above, and committed your (modified) test from the comment above. Even though it doesn't catch the edge case I've found, its a valueable test.

@mateusz
Copy link
Contributor Author

mateusz commented Oct 23, 2013

Thanks!!!

jedateach pushed a commit to dnadesign/silverstripe-subsites that referenced this pull request Dec 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants