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

Render the Contao login page even if the current route is not a Contao page #7077

Open
wants to merge 12 commits into
base: 4.13
Choose a base branch
from

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Apr 3, 2024

Fixed point 1 of #6805 for Contao 4.13

This must not be merged upstream!

@aschempp aschempp added the bug label Apr 3, 2024
@aschempp aschempp added this to the 4.13 milestone Apr 3, 2024
@aschempp aschempp requested a review from a team April 3, 2024 10:15
@aschempp aschempp self-assigned this Apr 3, 2024
@leofeyer leofeyer requested review from ausi and fritzmg and removed request for a team April 9, 2024 07:45
@fritzmg
Copy link
Contributor

fritzmg commented Apr 9, 2024

I am not quite sure what to test here. Do you mean a custom controller that has a route starting with %contao.backend.route_prefix% but has no backend _scope?

@fritzmg
Copy link
Contributor

fritzmg commented Apr 9, 2024

I tried with

namespace App\Controller;

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;

#[Route(path: '%contao.backend.route_prefix%/test')]
class TestController
{
    public function __invoke(): Response
    {
        return new Response('Hello World!');
    }
}

But this controller is always rendered, regardless of whether you have a valid back end login or not (as it should, I think?).

@aschempp
Copy link
Member Author

This is not about the back end. I have a custom controller on a custom route, that has scope = frontend. The controller checks permissions using Symfony, which works fine because the front end firewall is active. But because it does not have a pageModel in the request attributes, the Contao 401 login page is not rendered if access is denied.

@aschempp aschempp requested a review from a team May 7, 2024 08:16
@fritzmg
Copy link
Contributor

fritzmg commented May 7, 2024

Ah, I see. I have the same issue within fritzmg/contao-file-access - I hacked it in by executing

$root = $this->findFirstPublishedRootByHostAndLanguage($request->getHost(), $request->getLocale());

if (null !== $root) {
    $root->loadDetails();
    $request->attributes->set('pageModel', $root);
    $GLOBALS['objPage'] = $root;
}

This means after this PR this should not be necessary anymore, correct?

Copy link
Contributor

@fritzmg fritzmg left a comment

Choose a reason for hiding this comment

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

I tested it - but it does not work if you set contao.legacy_routing: false. When Legacy Routing is disabled, Frontend::getRootPageFromUrl() will simply check for the pageModel attribute in the current request, which is of course missing.

public static function getRootPageFromUrl()
{
if (!System::getContainer()->getParameter('contao.legacy_routing'))
{
$objRequest = System::getContainer()->get('request_stack')->getCurrentRequest();
if ($objRequest instanceof Request)
{
$objPage = $objRequest->attributes->get('pageModel');
if ($objPage instanceof PageModel)
{
$objPage->loadDetails();
return PageModel::findByPk($objPage->rootId);
}
}
throw new NoRootPageFoundException('No root page found');
}

@aschempp
Copy link
Member Author

aschempp commented May 7, 2024

Thanks @fritzmg. I have updated the PR to use other means to find the root page. The used method is deprecated, but since this is not going to be merged upstream I think that should be fine.

@fritzmg
Copy link
Contributor

fritzmg commented May 7, 2024

The used method is deprecated

Btw. which method do you mean? As far as I can see you did not use any deprecated methods 🤔

@leofeyer
Copy link
Member

leofeyer commented May 8, 2024

Wouldn‘t it be easier to backport the PageFinder service? This would also prevent conflicts when merging upstream.

@aschempp
Copy link
Member Author

Wouldn‘t it be easier to backport the PageFinder service? This would also prevent conflicts when merging upstream.

I don‘t think its a good idea to backport this so late in the patch process…

@leofeyer
Copy link
Member

Why not? It wouldn‘t break anything and it would ensure upstream mergeability, which is worth much more than following the "no new features in a bugfix release" rule. We could even make the service @internal in Contao 4.13, so that nobody uses it, if that is what you‘re worried about.

@fritzmg
Copy link
Contributor

fritzmg commented May 17, 2024

To be fair, it could also reduce duplicate code in Contao 4.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants