Project

General

Profile

Actions
Copy link

Bug #90989

closed

Lookup of access restricted records throws 404

Added by Reiner Kempkes over 4 years ago. Updated almost 4 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Site Handling, Site Sets & Routing
Target version:
-
Start date:
2020-04-09
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
9
PHP Version:
7.2
Tags:
PersistedAliasMapper, Access Restriction, UserAspect, User Authentication
Complexity:
Is Regression:
Sprint Focus:

Description

Preparation
News extension and Frontend user authentication is already set up and running properly (see System Environment section below).
Create a frontend user and a news record, User and news record must be assigned to the same frontend group.
As alternative use "Show at any login" (-2) as access restriction for the news record.

Log in via frontend authentication.
Access a news detail page containing an access restricted news record.

Current Behaviour
A 404 error is thrown.

Expected Behaviour
The news detail page with the given record is shown.

Debugging
I traced the issue down to the UserAspect class, which is not properly initialized.

PersistedAliasMapper->findByRouteFieldValue() is processing the news record lookup via QueryBuilder, which uses a FrontendGroupRestriction. There the 'frontend.user' aspect is utilized to determine valid frontend groups. Therefore UserAspect->isLoggedIn() is called, but the second condition of the first return statement fails, even when the user is logged in properly. This is caused due to an empty $this->user->groupData['uid'] array within the UserAspect->isLoggedIn() method.
Therefore the user lookup fails, which causes the group restriction lookup to fail, and therefore causing the QueryBuilder to fail fetching the news record, which causes the 404.

Solution and Patch
After a search for other usages i found TypoScriptFrontendController->initUserGroups(), which is calling FrontendUserAuthentication->fetchGroupData() to initialize the groupData array before processing user groups.
When i apply the same behaviour via the following patch to the UserAspect class, my issue is resolved properly and i am able to see the news detail page with the access restricted news, as expected.

public function isLoggedIn(): bool
{
    if ($this->user instanceof FrontendUserAuthentication) {
        // PATCH BEGIN
        if (empty($this->user->groupData['uid'])) {
            $this->user->fetchGroupData();
        }
        // PATCH END
        return ($this->user->user[$this->user->userid_column ?? 'uid'] ?? 0) > 0 && !empty($this->user->groupData['uid'] ?? null);
    }
    return ($this->user->user[$this->user->userid_column ?? 'uid'] ?? 0) > 0;
}

I am unable to determine if this kind of patch is best practice or will break anything else.
Furthermore i am not able to determine any performance impacts.
It might have a small performance impact, if groupData will be empty even after fetchGroupData() has been called, because it will then call fetchGroupData() again, every time when UserAspect->isLoggedIn() is called.

#90070 might be related to this.

System Environment
  • Currently only tested with TYPO3 9.5.15
  • News in version 7.3.1 is installed
  • Properly configured site configuration

Site Configuration:

News:
  type: Extbase
  extension: News
  plugin: Pi1
  routes:
    -
      routePath: '/article/{news-title}'
      _controller: 'News::detail'
      _arguments:
        news-title: news
  defaultController: 'News::list'
  aspects:
    news-title:
      type: PersistedAliasMapper
      tableName: tx_news_domain_model_news
      routeFieldName: path_segment

Related issues 2 (0 open — 2 closed)

Related to TYPO3 Core - Bug #90070: isLoggedIn not set in PSR-15 MiddlewareClosed2020-01-08

Actions
Related to TYPO3 Core - Bug #91049: PageResolver has no info about feUserGroup to properly do $site->getRouter()->matchRequest which leads to 404 for records that are fe_group protectedClosed2020-04-15

Actions
Actions
Copy link
 #1

Updated by Reiner Kempkes over 4 years ago

  • Description updated (diff)
Actions
Copy link
 #2

Updated by Markus Klein over 4 years ago

  • Related to Bug #90070: isLoggedIn not set in PSR-15 Middleware added
Actions
Copy link
 #3

Updated by Reiner Kempkes over 4 years ago

  • Description updated (diff)
Actions
Copy link
 #4

Updated by Reiner Kempkes over 4 years ago

I wonder, why the FrontendUserAuthenticator middleware does not initialize frontend user groups by default.
In my opinion it does not make any sense to initalize a user without its group, though the UserAspect depends on a group to verify its login state.
https://github.com/TYPO3/TYPO3.CMS/blob/v10.4.1/typo3/sysext/frontend/Classes/Middleware/FrontendUserAuthenticator.php#L131

As long as there is no official bugfix, i think about the following workaround in my local project:
Would it be possible to inject a group resolution middleware as after-dependency to the Middleware 'typo3/cms-frontend/page-resolver', so that the groups are resolved in the frontend user before the request route is getting parsed?
Do you see any potential issues when i implement such a workaround?

Actions
Copy link
 #5

Updated by Benni Mack over 4 years ago

The reason why the user is initialized at the point where it was/is initialized is for legacy reasons.

TYPO3 v9 behaves the exact same way as TYPO3 v8 when initializing users/groups for frontend to keep all existing hooks intact. These hooks have been deprecated in TYPO3 v9 in favor of PSR-15 middlewares, and for that reason we changed the ordering of the middlewares only until TYPO3 v10 (as this would have affected all other hooks - see https://docs.typo3.org/c/typo3/cms-core/master/en-us/Changelog/10.0/Breaking-88540-ChangedRequestWorkflowForFrontendRequests.html)

Until the user is then fetched (which also depends on settings like in AdminPanel) the "isLoggedIn" state cannot be verified properly.

I am very unhappy about this situation as well, but we tried to make it as backwards-compatible as possible, and this case you described is one of the issues we're facing with that.

Actions
Copy link
 #6

Updated by Benni Mack over 4 years ago

Correction. In TYPO3 v10 we still only initialize the groups in $TSFE->initUserGroups(). Actually this would be beneficial to be called in the middleware of the Frontend User Initialization, but would be again - heavily breaking still. TSFE still holds too much information which should be part of a middleware. We're moving our way towards there.

I will keep this issue open in order to remember what needs to happen in TYPO3 v10 or v11 (= breaking) to make this change happen one way or the other.

Your workaround with a custom middleware seems appropriate and a clean solution for the time being.

Actions
Copy link
 #7

Updated by Reiner Kempkes over 4 years ago

Hi Benny,

thank you.
Wouldn't it be useful to fix the "isLoggedIn" method of the user aspect class?
In my opinion it should validate a login state to true, even when no group is set.
My patch above is just a workaround to not touch the condition.
In the meantime i think it would be better to touch the condition itself, so that isLoggedIn returns true even when there is no user group set.
I also assume that such a change won't break anything, as the UserAspect class is pretty new.

What do you think about this approach?

Actions
Copy link
 #8

Updated by Gerrit Code Review over 4 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64441

Actions
Copy link
 #9

Updated by Gerrit Code Review over 4 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64441

Actions
Copy link
 #10

Updated by Gerrit Code Review over 4 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64441

Actions
Copy link
 #11

Updated by Christian Kuhn over 4 years ago

Reviewing the patch currently.

Special thanks go to Reiner Kempkes for an excellent issue report and analysis. Great job!

Actions
Copy link
 #12

Updated by Christian Kuhn over 4 years ago

  • Related to Bug #91049: PageResolver has no info about feUserGroup to properly do $site->getRouter()->matchRequest which leads to 404 for records that are fe_group protected added
Actions
Copy link
 #13

Updated by Christian Kuhn over 4 years ago

  • Status changed from Under Review to Needs Feedback

We think this issue has been resolved with the patch for #91049. Would you retest if the issue is resolved with recent core releases?

Actions
Copy link
 #14

Updated by Reiner Kempkes over 4 years ago

Hi Christian,

you are welcome, personally i appreciate well documented issues.
After you trained us in our company two years ago, we had a little talk about good issue reports in the evening, so i just followed your recommendations. ;-)
Thanks for your hint about the patch, i will review it soon.
It might take a few days, as i am very busy right now.

Actions
Copy link
 #15

Updated by Christian Kuhn about 4 years ago

  • Status changed from Needs Feedback to Closed

Closing issue for now. It's very likely solved.

Actions
Copy link
 #16

Updated by Gerrit Code Review about 4 years ago

  • Status changed from Closed to Under Review

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64441

Actions
Copy link
 #17

Updated by Gerrit Code Review about 4 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64441

Actions
Copy link
 #18

Updated by Gerrit Code Review about 4 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64441

Actions
Copy link
 #19

Updated by Gerrit Code Review about 4 years ago

Patch set 1 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/66113

Actions
Copy link
 #20

Updated by Gerrit Code Review about 4 years ago

Patch set 2 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/66113

Actions
Copy link
 #21

Updated by Benni Mack about 4 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions
Copy link
 #22

Updated by Benni Mack almost 4 years ago

  • Status changed from Resolved to Closed
Actions
Copy link

Also available in: Atom PDF