Bug #90989
closedLookup of access restricted records throws 404
100%
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
Updated by Markus Klein about 4 years ago
- Related to Bug #90070: isLoggedIn not set in PSR-15 Middleware added
Updated by Reiner Kempkes about 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?
Updated by Benni Mack about 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.
Updated by Benni Mack about 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.
Updated by Reiner Kempkes about 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?
Updated by Gerrit Code Review about 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
Updated by Gerrit Code Review about 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
Updated by Gerrit Code Review almost 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
Updated by Christian Kuhn almost 4 years ago
Reviewing the patch currently.
Special thanks go to Reiner Kempkes for an excellent issue report and analysis. Great job!
Updated by Christian Kuhn almost 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
Updated by Christian Kuhn almost 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?
Updated by Reiner Kempkes almost 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.
Updated by Christian Kuhn over 3 years ago
- Status changed from Needs Feedback to Closed
Closing issue for now. It's very likely solved.
Updated by Gerrit Code Review over 3 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
Updated by Gerrit Code Review over 3 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
Updated by Gerrit Code Review over 3 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
Updated by Gerrit Code Review over 3 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
Updated by Gerrit Code Review over 3 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
Updated by Benni Mack over 3 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 07d186716224bd92fd44342fbc217e5dc9293142.