Preventing user changing own group#10776
Conversation
|
I have tested this item ✅ successfully on 3ec40bf This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
I have tested this item ✅ successfully on 3ec40bf This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
| <?php echo JHtml::_('bootstrap.addTab', 'myTab', 'groups', JText::_('COM_USERS_ASSIGNED_GROUPS')); ?> | ||
| <?php echo $this->loadTemplate('groups'); ?> | ||
| <?php echo JHtml::_('bootstrap.endTab'); ?> | ||
| <?php if ((int) JFactory::getUser()->id != (int) $this->item->id) : ?> |
There was a problem hiding this comment.
- Shouldn't this kind of logical test be kept independent of layout as much as possible? Most templates have overrides for
com_users(such as), right? - Shouldn't the Super User still be allowed to do this?
IMO, This can be achieved by setting $this->grouplist to empty or similar in the view or the model wherever applicable.
I see this is already RTC, but can we still consider this for once?
|
This is an admin template and not a site template. |
|
This PR has received new commits. CC: @andrepereiradasilva, @bertmert This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
We could indeed also simplify by adding the code in view.html.php, which would prevent adding it in both admin templates. Not a big deal imho... |
Correct, that's is what I meant 👍 |
|
Or better, call model only when needed. if ((int) JFactory::getUser()->id != (int) $this->item->id)
{
$this->grouplist = $this->get('Groups');
} |
|
This PR has received new commits. CC: @andrepereiradasilva, @bertmert This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
Done, please test and set test as success ful in https://issues.joomla.org/tracker/joomla-cms/10776 We need this in 3.6.0. Thanks |
|
Can the milestone be added to 3.6.0 please? |
|
@infograf768 You need to remove line 42. Without it the |
|
This PR has received new commits. CC: @andrepereiradasilva, @bertmert This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
done. It was working here though. Maybe a session issue |
|
@infograf768 Accept my apologies for coming in small bits. |
|
I should have stayed with the simple ones I had at the beginning.. :) |
|
I have tested this item ✅ successfully on d08924b This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
I have tested this item ✅ successfully on d08924b This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
I have tested this item 🔴 unsuccessfully on d08924b
|
|
At the top of the save() method I have tested the following code (and seems to solve the issue with server-side validation of enforcing the new policy): |
|
Nice catch. The save method in the model must also be taken care of to
|
|
Can you propose a patch against my branch? With a new lang string if necessary. |
|
As SuperAdmin (as defined in Global Configuration) has all permissions, he is afaik de facto member of all groups. |
|
SuperAdmin => Super User |
| // User cannot modify own user groups | ||
| if ((int) $user->id == (int) $my->id) | ||
| { | ||
| if ($data['groups'] != null) |
There was a problem hiding this comment.
Do we need this nested if? if ((int) $user->id == (int) $my->id && isset($data['groups'])) should do fine.
If you are changing this then I'll wait to set my test success, or let me know otherwise.
There was a problem hiding this comment.
It's nice to have very clear code 😃 I prefer as it is.
|
Not necessary, but if you want you can also exempt super users from this enforcement to make things obvious. |
No use imho. |
|
I see it would be necessary to exempt the Super User from this rule OR remove the checks at line 231, starting with if ($iAmSuperAdmin && $my->get('id') == $pk)
...It is anyway useless to check self demote enforcement (but keep the comment note) when we never allow group change. Test this patch for super user and you'll see it failing. |
|
Indeed, it is failing when superuser wants to modify himself |
|
I think the simple and yet reliable way would be to change if ($iAmSuperAdmin && $my->get('id') == $pk)to if (isset($data['groups']) && $iAmSuperAdmin && $my->get('id') == $pk) |
|
This PR has received new commits. CC: @andrepereiradasilva, @bertmert, @ggppdk, @izharaazmi This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
I used another way. Please test. |
| } | ||
|
|
||
| // Prevent user from modifying own group(s) | ||
| if ((int) JFactory::getUser()->id != (int) $this->item->id || JFactory::getUser()->authorise('core.admin')) |
There was a problem hiding this comment.
I'll ignore that you call getUser twice and not use a variable 😄
|
Great! You exempted the Super user. I'll now set my test success in few minutes. 👍 |
|
One thing which is not related to this PR anyway. Have you tried removing a user from ALL user groups? It doesn't update. Because $data['groups'] is not set in that case. What do you suggest? |
|
This PR has received new commits. CC: @andrepereiradasilva, @bertmert, @ggppdk, @izharaazmi This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
I have tested this item ✅ successfully on a3a9afa 👍 This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
That you said about J3.6 aiming, can you please have a look at #10657 too! |
Indeed unrelated. We should imho in that case display a message telling that a user should be assigned to at least one user group. |
|
I have tested this item ✅ successfully on a3a9afa Thanks for the super admin update
e.g. they use them to select users to which to send notifications thus super users should be able to edit their own user-groups and assign them-selves to such user-goups
|
|
RTC based on testing. This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776. |
|
@joomla-cms-bot Please remove RTC label as this is merged. |
|
Thanks for the work on this everyone! |
|
Why not adding a new permission to grant access ? by checking only "core.admin" this introduce B/C |
|
This is a issue closed on 16 Jun 2016 please open a new issue as this message is going to be missed. |
Pull Request for Issue #10775
Create a subgroup of administrator
for example "testgroup"
Deny some permissions for that group, for example Create, Edit, etc. (anything for the sake of the demonstration) globally or for any component except com_users.
Log with a user member of the "testgroup"
Go to User Manager, edit the user, change his group to Administrator.
Before patch this is possible.
Patch and test again: the Assigned User Groups tab does not display anymore.
