User tests: Successful: Unsuccessful:
During an investigation for another bug, I came upon the code for the usergroup handling in Joomla.
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.
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.
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().
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())
Labels |
Added:
?
|
Category | ⇒ | ACL |
Status | Pending | ⇒ | Information Required |
It has been almost a year since this was submitted - is it still relevant etc. Is it right to make this change before J4?
I still plan on doing this, so I would ask you to keep this open.
Status | Information Required | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-11-13 09:46:39 |
Closed_By | ⇒ | brianteeman |
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
Labels |
Added:
?
|
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...