As mentioned by andypost on #1938390-62: Convert contact_site_page and contact_person_page to a new-style Controller, there is no access to _account on 403 pages.

The reason for that is, that ExceptionListener creates a new request without the previous _account attribute. This though causes a problem as on our 403/404 pages, we render links blocks, so we need to check access.

Proposed solution

Replace the symfony event listener with one taking care about _account and potentially other internal values.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
3.95 KB

Suppose both changes are needed but no idea about tests

Status: Needs review » Needs work

The last submitted patch, 2057607-exception-user-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Controller/ExceptionController.phpundefined
@@ -92,6 +92,7 @@ public function on403Html(FlattenException $exception, Request $request) {
+      $subrequest->attributes->set('_account', $request->attributes->get('_account'));

@@ -165,6 +166,7 @@ public function on404Html(FlattenException $exception, Request $request) {
+      $subrequest->attributes->set('_account', $request->attributes->get('_account'));

As everything with an underscore is kind of special (_account, _system_path) i am wondering whether we should copy all of them.

andypost’s picture

proper patch, previous was on top of #1938390-67: Convert contact_site_page and contact_person_page to a new-style Controller + debug

do we really need to patch all sub|request creation places to properly pass the account or need same clone method?

dawehner’s picture

FileSize
3.08 KB

Just a rough patch so other people can work on it.

Status: Needs review » Needs work

The last submitted patch, 2057607.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.45 KB

Let's combine them.

andypost’s picture

Issue tags: +Needs tests

new it needs tests, no idea about unit tests

dawehner’s picture

Issue tags: +WSCCI

Adding tag. I will work on some tests

dawehner’s picture

Opened a pull request to make it easier to add additonal parameters, see https://github.com/symfony/symfony/pull/8716

dawehner’s picture

FileSize
8.11 KB

Here is a test.
For some odd reason I had to hack the error log in order to not fail due to a thrown exception.

danylevskyi’s picture

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, routing-2057607-11.patch, failed testing.

yanniboi’s picture

Status: Needs work » Needs review
FileSize
614 bytes
8.43 KB

'Symfony\Component\HttpKernel\Exception\FlattenException' is a deprecated class, so I replaced it with 'Symfony\Component\Debug\Exception\FlattenException' to get rid of most of the test fails.

disasm’s picture

#15: routing-2057607-15.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Needs work

The approach used here can't work anymore, as we should rather rely on the current_user service.

h3rj4n’s picture

Status: Needs work » Needs review

This issue is out of scope. The parent issue is already (#1938390) continued without the fix in this issue. The part that's addressed here is already fixed.

I also tested it and I don't get a exception on 403 pages. I enabled the contact module. Create a new user with no rights for the contact form and try to access the contact form as this user. I got a clean 403 page, without any errors or exceptions.

I set it to 'needs review' because I'm not confident enough to close it.

dawehner’s picture

Status: Needs review » Fixed

The current_user service should totally solve the problem described in the issue, so we can close this now.

Thanks for bringing that issue up again.

Automatically closed -- issue fixed for 2 weeks with no activity.