Skip to content

Commit

Permalink
BUG Fix SiteTree / SiteConfig permissions
Browse files Browse the repository at this point in the history
  • Loading branch information
Damian Mooyman committed Mar 19, 2015
1 parent c238e1e commit 3df41e1
Show file tree
Hide file tree
Showing 12 changed files with 553 additions and 304 deletions.
82 changes: 46 additions & 36 deletions code/controllers/CMSMain.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
'treeview',
'listview',
'ListViewForm',
'childfilter',
);

public function init() {
Expand Down Expand Up @@ -409,60 +410,38 @@ public function SiteTreeHints() {

// Contains all possible classes to support UI controls listing them all,
// such as the "add page here" context menu.
$def['All'] = array();
$def['All'] = array();

// Identify disallows and set globals
$globalDisallowed = array();
foreach($classes as $class) {
$obj = singleton($class);
$needsPerm = $obj->stat('need_permission');
if($obj instanceof HiddenClass) continue;

if(!($obj instanceof HiddenClass)) {
$def['All'][$class] = array(
'title' => $obj->i18n_singular_name()
);
}
// Name item
$def['All'][$class] = array(
'title' => $obj->i18n_singular_name()
);

if(!$obj->stat('can_be_root')) {
$def['Root']['disallowedChildren'][] = $class;
}

// Check if can be created at the root
$needsPerm = $obj->stat('need_permission');
if(
($obj instanceof HiddenClass)
!$obj->stat('can_be_root')
|| (!array_key_exists($class, $cacheCanCreate) || !$cacheCanCreate[$class])
|| ($needsPerm && !$this->can($needsPerm))
) {
$globalDisallowed[] = $class;
$def['Root']['disallowedChildren'][] = $class;
}
}

// Set disallows by class
foreach($classes as $class) {
$obj = singleton($class);
if($obj instanceof HiddenClass) continue;

// Hint data specific to the class
$def[$class] = array();

$allowed = $obj->allowedChildren();
if($pos = array_search('SiteTree', $allowed)) unset($allowed[$pos]);

// Start by disallowing all classes which aren't specifically allowed,
// then add the ones which are globally disallowed.
$disallowed = array_diff($classes, (array)$allowed);
$disallowed = array_unique(array_merge($disallowed, $globalDisallowed));
// Re-index the array for JSON non sequential key issue
if($disallowed) $def[$class]['disallowedChildren'] = array_values($disallowed);

$defaultChild = $obj->defaultChild();
if($defaultChild != 'Page' && $defaultChild != null) {
if($defaultChild !== 'Page' && $defaultChild !== null) {
$def[$class]['defaultChild'] = $defaultChild;
}

$defaultParent = $obj->defaultParent();
$parent = SiteTree::get_by_link($defaultParent);
$id = $parent ? $parent->id : null;
if ($defaultParent != 1 && $defaultParent != null) {
if ($defaultParent !== 1 && $defaultParent !== null) {
$def[$class]['defaultParent'] = $defaultParent;
}
}
Expand Down Expand Up @@ -491,8 +470,6 @@ public function PageTypes() {

if($instance instanceof HiddenClass) continue;

if(!$instance->canCreate()) continue;

// skip this type if it is restricted
if($instance->stat('need_permission') && !$this->can(singleton($class)->stat('need_permission'))) continue;

Expand Down Expand Up @@ -705,6 +682,39 @@ public function treeview($request) {
public function listview($request) {
return $this->renderWith($this->getTemplatesWithSuffix('_ListView'));
}

/**
* Callback to request the list of page types allowed under a given page instance.
* Provides a slower but more precise response over SiteTreeHints
*
* @param SS_HTTPRequest $request
* @return SS_HTTPResponse
*/
public function childfilter($request) {
// Check valid parent specified
$parentID = $request->requestVar('ParentID');
$parent = SiteTree::get()->byID($parentID);
if(!$parent || !$parent->exists()) return $this->httpError(404);

// Build hints specific to this class
// Identify disallows and set globals
$classes = SiteTree::page_type_classes();
$disallowedChildren = array();
foreach($classes as $class) {
$obj = singleton($class);
if($obj instanceof HiddenClass) continue;

if(!$obj->canCreate(null, array('Parent' => $parent))) {
$disallowedChildren[] = $class;
}
}

$this->extend('updateChildFilter', $disallowedChildren, $parentID);
return $this
->response
->addHeader('Content-Type', 'application/json; charset=utf-8')
->setBody(Convert::raw2json($disallowedChildren));
}

/**
* Safely reconstruct a selected filter from a given set of query parameters
Expand Down
22 changes: 8 additions & 14 deletions code/controllers/CMSPageAddController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class CMSPageAddController extends CMSPageEditController {
/**
* @return Form
*/
function AddForm() {
public function AddForm() {
$pageTypes = array();
foreach($this->PageTypes() as $type) {
$html = sprintf('<span class="page-icon class-%s"></span><strong class="title">%s</strong><span class="description">%s</span>',
Expand All @@ -38,11 +38,6 @@ function AddForm() {
$childTitle = _t('CMSPageAddController.ParentMode_child', 'Under another page');

$fields = new FieldList(
// TODO Should be part of the form attribute, but not possible in current form API
$hintsField = new LiteralField(
'Hints',
sprintf('<span class="hints" data-hints="%s"></span>', Convert::raw2xml($this->SiteTreeHints()))
),
new LiteralField('PageModeHeader', sprintf($numericLabelTmpl, 1, _t('CMSMain.ChoosePageParentMode', 'Choose where to create this page'))),
$parentModeField = new SelectionGroup(
"ParentModeField",
Expand All @@ -68,7 +63,7 @@ function AddForm() {
$typeField = new OptionsetField(
"PageType",
sprintf($numericLabelTmpl, 2, _t('CMSMain.ChoosePageType', 'Choose page type')),
$pageTypes,
$pageTypes,
'Page'
),
new LiteralField(
Expand All @@ -78,7 +73,7 @@ function AddForm() {
_t(
'CMSMain.AddPageRestriction',
'Note: Some page types are not allowed for this selection'
)
)
)
)
);
Expand Down Expand Up @@ -122,6 +117,9 @@ function AddForm() {
$form = CMSForm::create(
$this, "AddForm", $fields, $actions
)->setHTMLID('Form_AddForm');
$form->setAttribute('data-hints', $this->SiteTreeHints());
$form->setAttribute('data-childfilter', $this->Link('childfilter'));

$form->setResponseNegotiator($this->getResponseNegotiator());
$form->addExtraClass('cms-add-form stacked cms-content center cms-edit-form ' . $this->BaseCSSClasses());
$form->setTemplate($this->getTemplatesWithSuffix('_EditForm'));
Expand All @@ -145,12 +143,8 @@ public function doAdd($data, $form) {

if(!$parentObj || !$parentObj->ID) $parentID = 0;

if($parentObj) {
if(!$parentObj->canAddChildren()) return Security::permissionFailure($this);
if(!singleton($className)->canCreate()) return Security::permissionFailure($this);
} else {
if(!SiteConfig::current_site_config()->canCreateTopLevel())
return Security::permissionFailure($this);
if(!singleton($className)->canCreate(Member::currentUser(), array('Parent' => $parentObj))) {
return Security::permissionFailure($this);
}

$record = $this->getNewItem("new-$className-$parentID".$suffix, false);
Expand Down
87 changes: 65 additions & 22 deletions code/model/SiteConfig.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,26 @@ class SiteConfig extends DataObject implements PermissionProvider, TemplateGloba
"EditorGroups" => "Group",
"CreateTopLevelGroups" => "Group"
);

private static $defaults = array(
"CanViewType" => "Anyone",
"CanEditType" => "LoggedInUsers",
"CanCreateTopLevelType" => "LoggedInUsers",
);

/**
* @config
* @var array
*/
private static $disabled_themes = array();

/**
* Default permission to check for 'LoggedInUsers' to create or edit pages
*
* @var array
* @config
*/
private static $required_permission = array('CMS_ACCESS_CMSMain', 'CMS_ACCESS_LeftAndMain');

This comment has been minimized.

Sign in to view

Copy link
@stevie-mayhew

stevie-mayhew Mar 24, 2015

Contributor

I'm wondering if the default permissions for this are correct.

We are now allowing people to create or edit pages based on their permission to "Access to 'Pages' section" - access doesn't imply creation or editing ability.

Instead, should this not be based on the content permission "Edit any page" (SITETREEEDITALL) instead, which makes more sense as its related to the editing of page, rather than the ability to view a section of the CMS.

ping @tractorcow

This comment has been minimized.

Sign in to view

Copy link
@tractorcow

tractorcow Mar 24, 2015

Contributor

These are the default permissions in the SiteTree class. Check out some of the permissions in SiteTree::batch_permission_check. Essentially these permissions were decided a long time ago, they were just never put in place in SiteConfig. :)

The dropdown on the siteconfig says "anyone who can access the CMS can edit pages". This is just the check for who can see the pages section in the CMS. If you don't want users to edit pages, but still see the pages section, then you need to change your setting in "SiteConfig" to "only these groups".

And, they can still be overridden in your custom project if you feel they need to be otherwise. :)

This comment has been minimized.

Sign in to view

Copy link
@stevie-mayhew

stevie-mayhew Mar 24, 2015

Contributor

Sorry scratch this - CMS_ACCESS_LeftAndMain isn't what I thought it was.

I think the permissions here are wrong, but I'm not sure what they should be - probably an independent new configuration as this doesn't make sense in the wider scheme of the SS environment.

Basically, the pages section shouldn't influence editing or viewing of page content, page access, edit and viewing rights should.

This comment has been minimized.

Sign in to view

Copy link
@tractorcow

tractorcow Mar 24, 2015

Contributor

Problem is that the user could have CMS_ACCESS_LeftAndMain and thus won't have the CMSMain permission. CMSMain is the "global" permission you can apply to automatically grant permission to all sections, and some people will need to pass the check who don't meet that.

This comment has been minimized.

Sign in to view

Copy link
@stevie-mayhew

stevie-mayhew Mar 24, 2015

Contributor

Yeah, but I still believe that the wording/permission is wrong then. Access to a section of the CMS shouldn't influence the actions available to that user once in the section.

We have multiple instances where users can edit pages outside of the Pages section, so we don't give them access to that section. I think its a miscommunication of intent, even if it suits the purposes of the "default" SilverStripe installation.

This comment has been minimized.

Sign in to view

Copy link
@tractorcow

tractorcow Mar 24, 2015

Contributor

I think the logic this code implements is:

  • User has access to pages section (via one of the two permissions above) but not write.
  • Site config says "if a user has access to pages, let them also write to that section"
  • Thus those users inherit write access to the pages

If the process was simply step 1 & 3 I would agree that's wrong... but the site config step explicitly promotes those users. It's only right because that's the specific option set in siteconfig. You can disable this option to set to another behaviour.

  • User has access to pages section (via one of the two permissions above) but not write.
  • Site config says "only users in these groups can write pages" and a group is selected those users do not belong to.
  • Thus those users can only view pages but can't write, because they are not in the selected groups.

In your case, you should change that option to a default that makes sense to your application. The problem seems to be relying on the default option in siteconfig.

Would you suggest another default for CanEditType perhaps? :) Perhaps that's the solution we need?

Also it would be better to show on the member's permission tabs any permissions they inherit from the siteconfig. I think that "creating pages in the root" isn't actually shown, and should be. Would that solve the miscommunication problem you mentioned?

This comment has been minimized.

Sign in to view

Copy link
@stevie-mayhew

stevie-mayhew Mar 24, 2015

Contributor

My issue is that the ability to create and edit and create pages is now by default entirely predicated by the fact that users have to have access to the pages section.

All I'm saying is that having "Access to the pages section" now is the base of being able to edit and create pages within the CMS, which isn't what that permission is for - its for access to a specific section of the CMS and shouldn't be the base check to see if a user can edit and create pages.

If the wording of that option were instead "Access to the pages section and the base ability to edit and create pages" then it would make sense. I just think that the configuration of access shouldn't be the basis for the ability to administer.

This comment has been minimized.

Sign in to view

Copy link
@tractorcow

tractorcow Mar 24, 2015

Contributor

All I'm saying is that having "Access to the pages section" now is the base of being able to edit and create pages

That functionality was designed and implemented way back in 2.4. All we've done is fix is so it worked as it was intended. We can change that behaviour, just not in 3.x.

/**
* @deprecated 3.2 Use the "SiteConfig.disabled_themes" config setting instead
Expand Down Expand Up @@ -230,48 +244,70 @@ static public function make_site_config() {
* called if a page is set to Inherit, but there is nothing
* to inherit from.
*
* @param mixed $member
* @param Member $member
* @return boolean
*/
public function canView($member = null) {
public function canViewPages($member = null) {
if(!$member) $member = Member::currentUserID();
if($member && is_numeric($member)) $member = DataObject::get_by_id('Member', $member);

if ($member && Permission::checkMember($member, "ADMIN")) return true;

$extended = $this->extendedCan('canViewPages', $member);
if($extended !== null) return $extended;

if (!$this->CanViewType || $this->CanViewType == 'Anyone') return true;

// check for any logged-in users
if($this->CanViewType == 'LoggedInUsers' && $member) return true;
if($this->CanViewType === 'LoggedInUsers' && $member) return true;

// check for specific groups
if($this->CanViewType == 'OnlyTheseUsers' && $member && $member->inGroups($this->ViewerGroups())) return true;
if($this->CanViewType === 'OnlyTheseUsers' && $member && $member->inGroups($this->ViewerGroups())) return true;

return false;
}

/**
* Can a user edit pages on this site? This method is only
* called if a page is set to Inherit, but there is nothing
* to inherit from.
* to inherit from, or on new records without a parent.
*
* @param mixed $member
* @param Member $member
* @return boolean
*/
public function canEdit($member = null) {
public function canEditPages($member = null) {
if(!$member) $member = Member::currentUserID();
if($member && is_numeric($member)) $member = DataObject::get_by_id('Member', $member);

if ($member && Permission::checkMember($member, "ADMIN")) return true;

// check for any logged-in users
if(!$this->CanEditType || $this->CanEditType == 'LoggedInUsers' && $member) return true;
$extended = $this->extendedCan('canEditPages', $member);
if($extended !== null) return $extended;

// check for any logged-in users with CMS access
if( $this->CanEditType === 'LoggedInUsers'
&& Permission::checkMember($member, $this->config()->required_permission)
) {
return true;
}

// check for specific groups
if($this->CanEditType == 'OnlyTheseUsers' && $member && $member->inGroups($this->EditorGroups())) return true;
if($this->CanEditType === 'OnlyTheseUsers' && $member && $member->inGroups($this->EditorGroups())) {
return true;
}

return false;
}

public function canEdit($member = null) {
if(!$member) $member = Member::currentUserID();
if($member && is_numeric($member)) $member = DataObject::get_by_id('Member', $member);

$extended = $this->extendedCan('canEdit', $member);
if($extended !== null) return $extended;

return Permission::checkMember($member, "EDIT_SITECONFIG");
}

public function providePermissions() {
return array(
Expand All @@ -287,25 +323,32 @@ public function providePermissions() {
/**
* Can a user create pages in the root of this site?
*
* @param mixed $member
* @param Member $member
* @return boolean
*/
public function canCreateTopLevel($member = null) {
if(!$member || !(is_a($member, 'Member')) || is_numeric($member)) {
$member = Member::currentUserID();
}

if (Permission::check('ADMIN')) return true;
if(!$member) $member = Member::currentUserID();
if($member && is_numeric($member)) $member = DataObject::get_by_id('Member', $member);

if ($member && Permission::checkMember($member, "ADMIN")) return true;

$extended = $this->extendedCan('canCreateTopLevel', $member);
if($extended !== null) return $extended;

// check for any logged-in users
if($this->CanCreateTopLevelType == 'LoggedInUsers' && $member) return true;
// check for any logged-in users with CMS permission
if( $this->CanCreateTopLevelType === 'LoggedInUsers'
&& Permission::checkMember($member, $this->config()->required_permission)
) {
return true;
}

// check for specific groups
if($member && is_numeric($member)) $member = DataObject::get_by_id('Member', $member);
if($this->CanCreateTopLevelType == 'OnlyTheseUsers' && $member && $member->inGroups($this->CreateTopLevelGroups())) return true;

if( $this->CanCreateTopLevelType === 'OnlyTheseUsers'
&& $member
&& $member->inGroups($this->CreateTopLevelGroups())
) {
return true;
}

return false;
}
Expand Down
Loading

0 comments on commit 3df41e1

Please sign in to comment.