? ? Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
15 Dec 2014

During an investigation for another bug, I came upon the code for the usergroup handling in Joomla.

JUser::getAuthorisedGroups()

This method is ok so far, but unnecessarily complexly named. I shortened it to JUser::getGroups(). The old method has been deprecated for Joomla 4.0.

Adding/removing a user to a group

Right now, changes to the group membership of a user all have to happen through a form of calling JUserHelper, which in turn calls JTableUser::save(). This means that checks in JTableUser apply to the method calls in JUserHelper, too.
From my perspective, JTable classes are only meant to check the data to store for sanity, not do any security checks. At the same time, JTableUser handles not only the data for this one table, but for other tables, too, and I would say that this is something that we should change.
Long story short: I added methods to JUser to add and remove groups from this user, which in return for the moment call JUserHelper with its respective methods.
These methods should be part of JUser from my perspective, even if the actual changing of the data is done in another class. In the long term, JUserHelper is the wrong place for that however. I don't know the right solution right now (moving this to a JUsergroups class or putting the SQL directly into JUser or something like that) but I think JUserHelper is the wrong class for this.

$groups member in JUser

Right now we have $groups and $_authorisedGroups in JUser, which both hold the same data. $groups was the one that was modified when a user was added to a group, while $_authorisedGroups was the one that the access rights were checked against. We should make $groups protected from my perspective and deprecate $_authorisedGroups. JUser::getGroups() should be used to retrieve the groups data and not direct access to $groups as way to many seem to do right now. Since $groups is not correctly used right now, it was not cleared properly in the clearAccessRights() method. It was also filled with the wrong data in JUserHelper::addUserToGroup().

Updating a logged in user with the new data

Somehow there was an attempt at updating the current users data in the session with the data that was just stored when a session ID was present. For one this broke our unittests as @wilsonge told me and it also does not work and instead would set the usergroups of the currently logged in user to the usergroups of the one that was just edited if JUserHelper was used to modify the group membership. Fortunately, we are not doing this in Joomla right now. This could however be a security issue if there are extensions out there that let you add other users to a usergroup. By adding a super user to a user group, a user with lower privileges could (in part) gain the higher privileges if another extension only checks for the groups in $groups.
Again, long story short: Replacing that whole code block with a call to JUser::clearAccessRights() and comparing the user IDs before that, solves the whole issue.

I don't have testing instructions right now. I would also ask for feedback where to put the user group membership handling code in order to remove it from JUserHelper and remove that circular reference that we have right now. (JUser::addGroup() calls JUserHelper::addUserToGroup() which again calls JUser::save())

avatar Hackwar Hackwar - open - 15 Dec 2014
avatar jissues-bot jissues-bot - change - 15 Dec 2014
Labels Added: ?
avatar brianteeman brianteeman - change - 16 Dec 2014
Category ACL
avatar Hackwar
Hackwar - comment - 23 Dec 2014

One downside of this rewrite would be, that the save events of the user don't work on the groups in this case. That would be something that we have to cover...

avatar brianteeman brianteeman - change - 12 Nov 2015
Status Pending Information Required
avatar brianteeman
brianteeman - comment - 12 Nov 2015

It has been almost a year since this was submitted - is it still relevant etc. Is it right to make this change before J4?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5434.

avatar Hackwar
Hackwar - comment - 13 Nov 2015

I still plan on doing this, so I would ask you to keep this open.

avatar brianteeman brianteeman - change - 13 Nov 2015
Status Information Required Closed
Closed_Date 0000-00-00 00:00:00 2015-11-13 09:46:39
Closed_By brianteeman
avatar brianteeman
brianteeman - comment - 13 Nov 2015

Personally I dont believe that the issue tracker should be used a personal todo lists.
Having pull requests that are not ready on the tracker just confuses people and wastes time of those who come to volunteer their time to test things.
If it is under active development it might be a little different but this has not been touched for a year now. I am closing it. When you have found the time to do something with it then it can always be re-opened


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5434.

avatar brianteeman brianteeman - close - 13 Nov 2015
avatar joomla-cms-bot joomla-cms-bot - change - 13 Nov 2015
Labels Added: ?

Add a Comment

Login with GitHub to post a comment