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

avoid CMS coupling #9830

Open
wants to merge 1 commit into
base: 4
Choose a base branch
from
Open

avoid CMS coupling #9830

wants to merge 1 commit into from

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Jan 19, 2021

In the current setup, we cannot use a custom security controller without the cms module. This changes allow setting another controller as "page_class".

In the current setup, we cannot use a custom security controller without the cms module. This changes allow setting another controller as "page_class".
@@ -579,8 +579,13 @@ protected function getResponseController($title)
$holderPage->URLSegment = 'Security';
// Disable ID-based caching of the log-in page by making it a random number
$holderPage->ID = -1 * random_int(1, 10000000);

if ($holderPage instanceof Controller) {
$controller = $holderPage;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go a bit higher in the method after the Injector call and move setting the title, url segment and ID into the condition when it's not an instance of a controller? Even though it might work it seems a bit hacky this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Actually I believe that what would be nice is to set some kind of callback to generate the page. This way this code could be shifted into the CMS module altogether and avoid coupling entirely. But that requires coordinated changes between the framework and CMS module. It's not logical that the security class has knowledge of the page class.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

While I don't disagree with the intention of this PR, I really don't think "reusing" the page_class config which is intended for Pages, with a Controller, is a particularly clean solution

Also fair warning, any pull-requests that are security related are difficult to merge due to the potentially nasty downstream regressions they can cause if we get things wrong.

@lekoala
Copy link
Contributor Author

lekoala commented Mar 25, 2021

@emteknetnz yes i fully understand. as i said, the page_class should not be in there in the first place because it relies on the cms module to be installed.
so actually a fully "clean" solution would require much more changes. I just made this pull request because i tried to use the framework as a standalone module and this is how i did it. it should not have any side effect (pages are not instances of controller) and it works rather nicely, but i understand you want to do things the right way :-)

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.

3 participants