FIX CMS permission checks for subsite are now handled in the state context #388
Conversation
…ntext We now check the subsite state for the context and validate it against the current member's group permissions using the SilverStripe ORM relationships instead of using SQL queries. More granular permission checks e.g. canView etc are still up to data models to define and handle.
| */ | ||
| public function canAccess() | ||
| { | ||
| // Allow us to accept a Member object passed in as an argument without breaking semver | ||
| $passedMember = func_get_args() ? func_get_arg(0) : null; |
| } | ||
| // Check whether we have any subsites | ||
| if (!$subsites->exists()) { | ||
| return $accessibleSubsites; |
There was a problem hiding this comment.
Should this be an empty set here or return $subsites?
There was a problem hiding this comment.
Also, won't it always return true for ->exists when including the main site?
There was a problem hiding this comment.
Yeah you're right since it returns an ArrayList which includes a stub for the main site. I think I wrote this when it was a DataList (which wouldn't). I've removed it anyway, it's not needed
|
|
||
| if ($canAccess === false) { | ||
| // Explicitly denied | ||
| continue; |
There was a problem hiding this comment.
We spoke about this - but I would consider the idea of looping groups here. Pseudo:
foreach ($groups as $group) {
if ($group->accessAllSubsites) {
return $allSites;
}
$accessibleSites->merge($group->accessibleSites)
}
The only reason I suggest it is that I feel it's likely there's less groups than subsites and therefore less loops.
There was a problem hiding this comment.
This part is a wrapper that's calling deeper logic in LeftAndMainSubsites::canAccess so it's not dealing with groups directly at this point
| // Check if any of the current user's groups have been given explicit access to the current subsite | ||
| $groupSubsiteIds = $group->Subsites()->column('ID'); | ||
| if (in_array($currentSubsiteId, $groupSubsiteIds)) { | ||
| $allowedInSubsite = true; |
There was a problem hiding this comment.
Also the breaks could be just return null at this point - although might be confusing in the future.
There was a problem hiding this comment.
Yeah I'd considered that. I think the reason I left it like this is because the two if statements contribute to the same criteria of "is allowed in subsite" so I left them separate for readability. That OK?
There was a problem hiding this comment.
Sure - I'm just happy the break is in now :)
|
I put "Requested Changes" but you could probably convince me there's no changes needed... Except that last one. |
…nd add break to loop
|
Do you want to ask for any additional testing by the original reporter? |
|
Nope, I'd rather merge and make follow ups as necessary |
|
@ScopeyNZ FYI I've restored the original branch for next time we pick this issue up |
We now check the subsite state for the context and validate it against the current member's group permissions using the SilverStripe ORM relationships instead of using SQL queries.
More granular permission checks e.g. canView etc are still up to data models to define and handle.
I think this is semver patch safe. I've used a workaround in
LeftAndMainSubsites::canAccessto allow a passed member argument without adding it to the method signature (not semver safe).Also the removal of
SubsiteXHRControllerTestmay be a little semver unsafe, but it falls back to calling the method fromLeftAndMainSubsitesso I'm OK with it.Fixes #358