Bug #34383
open
Incorrect redirect with multiple user groups
0%
Description
If the "redirectMode" is set to "groupLogin" and there are several groups with "felogin_redirectPid", the redirect should happen to the first or last group (depending on the mode of "redirectFirst" flag. The actual redirect result is undefined because felogin fetches groups from the database without any sorting. Depending on MySQL internal index state, results can come in any order and this order can change over time. In addition felogin uses $GLOBALS['TSFE'->fe_user->groupData['uid'], which contains sorted uid values for groups. This prevents correct redirects for multiple groups too.
Felogin should ensure that it adds redirects URLs in the proper order.
Updated by Gerrit Code Review over 12 years ago
- Status changed from New to Under Review
Patch set 1 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9275
Updated by Jigal van Hemert over 12 years ago
The groupData originates from the authentication service and has not defined order. It also takes into account that groups can have subgroups. This complicates the order a bit.
Subgroups also make it harder to define any order.
If the default authentication service of the core is used we could say that the order in the BE should be the order of the groups. The question is then how the subgroups should fit in this order? First the groups, then the subgroups, then the sub-subgroups, etc.? Or the first group, the first subgroup of the first group, the subgroup of the first subgroup of the first group, the second subgroup of the first group, ...?
In t3lib_userAuthGroup::fetchGroups() there is no defined order (database order) for the group list, subgroups are appended to the main groups (unless the group is already present in the existing list).
The place to set an order would in the first place be the authentication service. I'm not sure if other authentication services will also import the group records to make them available for felogin to query. If this is not the case the code in felogin should be extended to provide a mechanism for the authentication service to provide such redirect information.
Updated by Dmitry Dulepov over 12 years ago
Yes, that's all correct but would you submit a better patch? ;)
Anyway, we solved it for the customer in our local core. The problem is that the customer wants redirects based on the first group. This does not work at all, even though it is advertised by TYPO3 as the official feature. So this feature should be either removed or fixed. I decided to fix it in a simpler way. Felogin should not have really implemented that due to reasons you described so well. But since the feature is implemented, it has to work. So here is the fix.
The proposed solution works in most cases. So if anybody has the same problem, you are welcome to use the patch.
Updated by Gerrit Code Review over 12 years ago
Patch set 2 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9275
Updated by Jigal van Hemert over 12 years ago
I think I found the source of "the official feature": http://wiki.typo3.org/Felogin . This is a very outdated version and doesn't match the manual in the system extension.
One possible solution would be:
- collect groups sorted by ordering in auth service
- collect subgroups sorted by ordering in auth service
- give groups to felogin
- felogin loops through list until one has a redirect (not in a single query)
Updated by Alexander Opitz over 10 years ago
- Status changed from Under Review to New
- Is Regression set to No
Patch was abandoned so reset issue status.
Updated by
Updated by Jigal van Hemert almost 7 years ago
- Has duplicate Feature #72251: Wrong redirect priority with 'redirectMode=groupLogin' at felogin added
Updated by Jan Stockfisch over 6 years ago
- Related to Epic #84262: [FEATURE] Update felogin to extbase added
Updated by Markus Klein about 4 years ago
- Category changed from felogin to Authentication
That's a problem of the FrontendUserAuthentication class, not of felogin.
Updated by Dmitry Dulepov about 4 years ago
Markus Klein wrote:
That's a problem of the FrontendUserAuthentication class, not of felogin.
Are you sure? At the time of reporting redirection was made by the felogin extension. Since redirection functionality is located in the extension, logically the fix should be in the extension as well.
Updated by Markus Klein about 4 years ago
Yes the redirection stems from felogin, but the groups are loaded in FrontendUserAuthentication and order should be handled there, right?
Updated by Dmitry Dulepov about 4 years ago
Markus Klein wrote:
Yes the redirection stems from felogin, but the groups are loaded in FrontendUserAuthentication and order should be handled there, right?
It would not be that easy and useful. Typical goal for loading groups in user auth is to set various permissions. Order does not matter for that. However ordering takes time. Since group loading happens on every request, it is more time to sort them for every request than it is now without sorting. But the result of this new sorting will be useless for 99.999% of cases (no redirection on most pages).
Groups can be nested. For example:
a
b
c -> (a)
d -> (a, c)
e -> (b)
f -> (c, f)
g -> (e, f)
Sorting is not trivial here. Where "c" goes: first or the last? And so on. This all has to be in the algorithm. And if it all works on every request, these are extra calculations that are useless for most pages.
So is it worth to sort groups while loading?
Updated by Markus Klein about 4 years ago
Totally correct. But I remember there were some other cases, which needed sorted groups as well.
Generally: The groups are rather "static data". So actually the order can be cached for a very long time, until some group or the user is modified.
Then group loading (incl. proper ordering) would be linear or constant time.
The algorithm doing the sorting should be a dedicated class covered with a lot of tests, to have this documented "technically".