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

Revert "ENH Make CMSProfileController use required_permission_codes" #1555

Closed
wants to merge 1 commit into from

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Aug 22, 2023

Issue silverstripe/.github#99

Reverting #1027

This caused the following failures in https://github.com/silverstripe/recipe-kitchen-sink/actions/runs/5892574918/job/15998642199 (recipe-cms builds)

  1. SilverStripe\Admin\Tests\CMSProfileControllerTest::testMemberEditsOwnProfile
  2. SilverStripe\Admin\Tests\LeftAndMainTest::testCanView

I didn't investigate it too deeply, though seems that the reason is that Permission::checkMember() will (I think) count any permission code that starts with CMS_ACCESS_ e.g. CMS_ACCESS_CMSMain as CMS_ACCESS, whereas as I'm assuming required_permission_codes does not?

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 22, 2023

Broken behat test is existing

@GuySartorelli
Copy link
Member

Broken behat test is existing

Is there a PR for it? This issue is explicitly about fixing broken tests.

@GuySartorelli
Copy link
Member

I didn't investigate it too deeply, though seems that the reason is that Permission::checkMember() will (I think) count any permission code that starts with CMS_ACCESS_ e.g. CMS_ACCESS_CMSMain as CMS_ACCESS, whereas as I'm assuming required_permission_codes does not?

If you click through parent::canView() to LeftAndMain::canView(), you will see that it does in fact use Permission::checkMember which immediately makes me think reverting this outright is not the right move. I'll investigate further to see if I can find the actual root cause of the unit test failure.

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 22, 2023

After investigation, it seems this is not caused by any problem with this change, but rather with inappropriate permissions checking in the subsites module (specifically in the LeftAndMainSubsites extension, either in sectionSites(), accessible_sites(), hasMainSitePermission(), or canAccess() - I'd probably look at hasMainSitePermission() first) which explains why it's failing in kitchen sink and not anywhere else.

The only reason this wasn't failing before is because there are extension hooks in LeftAndMain that weren't being called previously. But the LeftAndMainSubsites is meant to work for subclasses of LeftAndMain so it should be made to work for this one too.

Please close this PR and resolve the problem in the subsites module.

@emteknetnz
Copy link
Member Author

Is there a PR for it? This issue is explicitly about fixing broken tests.

silverstripe/silverstripe-frameworktest#147

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 23, 2023

I'll close this PR and split off a separate card to investigate subsites

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