joomla/joomla-cms — pull request #5434 by Hackwar (2014-12-15 19:27:25+00:00)
Title: Refactoring group handling in JUser and JUserHelper

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())