Skip to content

Preventing user changing own group#10776

Merged
wilsonge merged 7 commits intojoomla:stagingfrom
infograf768:chnageowngroup
Jun 15, 2016
Merged

Preventing user changing own group#10776
wilsonge merged 7 commits intojoomla:stagingfrom
infograf768:chnageowngroup

Conversation

@infograf768
Copy link
Copy Markdown
Member

@infograf768 infograf768 commented Jun 10, 2016

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.
screen shot 2016-06-10 at 12 39 34

@ghost
Copy link
Copy Markdown

ghost commented Jun 10, 2016

I have tested this item ✅ successfully on 3ec40bf


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

@andrepereiradasilva
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on 3ec40bf

works as described.


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

@brianteeman
Copy link
Copy Markdown
Contributor

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 10, 2016
<?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) : ?>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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?
  2. 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?

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Jun 11, 2016

This is an admin template and not a site template.
But thanks. I had forgotten to modify Hathor too. Doing now.
There is no reason for a Super User to change its group and he should not anyway.

@joomla-cms-bot
Copy link
Copy Markdown

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.

@infograf768
Copy link
Copy Markdown
Member Author

We could indeed also simplify by adding the code in view.html.php, which would prevent adding it in both admin templates.

        if ((int) JFactory::getUser()->id == (int) $this->item->id)
        {
            $this->grouplist = null;
        }

Not a big deal imho...

@izharaazmi
Copy link
Copy Markdown
Contributor

We could indeed also simplify by adding the code in view.html.php, which would prevent adding it in both admin templates.

Correct, that's is what I meant 👍

@izharaazmi
Copy link
Copy Markdown
Contributor

Or better, call model only when needed.

if ((int) JFactory::getUser()->id != (int) $this->item->id)
{
    $this->grouplist = $this->get('Groups');
}

@joomla-cms-bot
Copy link
Copy Markdown

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.

@infograf768
Copy link
Copy Markdown
Member Author

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

@infograf768
Copy link
Copy Markdown
Member Author

Can the milestone be added to 3.6.0 please?
@wilsonge @brianteeman

@izharaazmi
Copy link
Copy Markdown
Contributor

@infograf768 You need to remove line 42. Without it the grouplist is anyway already populated.

@joomla-cms-bot
Copy link
Copy Markdown

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.

@infograf768
Copy link
Copy Markdown
Member Author

done. It was working here though. Maybe a session issue

@izharaazmi
Copy link
Copy Markdown
Contributor

@infograf768 Accept my apologies for coming in small bits.
I just noticed $this->groups = $this->get('AssignedGroups'); at now line 42 (43 previously) is unused when not editing groups. Would you please move that too inside the recently added if conditional!

@infograf768
Copy link
Copy Markdown
Member Author

I should have stayed with the simple ones I had at the beginning.. :)
No time now.

@izharaazmi
Copy link
Copy Markdown
Contributor

izharaazmi commented Jun 11, 2016

I have tested this item ✅ successfully on d08924b
Thanks!


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

@andrepereiradasilva
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on d08924b


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

@ggppdk
Copy link
Copy Markdown
Contributor

ggppdk commented Jun 11, 2016

I have tested this item 🔴 unsuccessfully on d08924b

I have applied the patch,

  1. logged as "Administrator"
  2. then editted the DOM to copy-paste the usergroups form fields from another form
  3. modified the user-groups, and then clicking save has saved the usergroups
    This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776.

@ggppdk
Copy link
Copy Markdown
Contributor

ggppdk commented Jun 11, 2016

At the top of the save() method
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/user/user.php#L706

I have tested the following code (and seems to solve the issue with server-side validation of enforcing the new policy):

    // Enforce security policy, user can not modify its own user-groups
    if ( $this->id == JFactory::getUser()->id )
    {
        if ($this->groups != null)
        {
            // Form was probably tampered with
            JFactory::getApplication()->enqueueMessage(
                'You can not edit your own user groups, usergroups saving was skipped',
                'warning'
            );
            $this->groups = null;
        }
    }

@izharaazmi
Copy link
Copy Markdown
Contributor

izharaazmi commented Jun 11, 2016

Nice catch. The save method in the model must also be taken care of to
ignore/warn about $data['group']

On 12-Jun-2016 2:48 AM, "Georgios Papadakis" notifications@github.com
wrote:

I have tested this item 🔴 unsuccessfully on d08924b
d08924b

I have applied the patch,

  1. logged as "Administrator"
  2. then editted the DOM to copy-paste the usergroups form fields from
    another form
  3. modified the user-groups, and then clicking save has saved the
    usergroups

    This comment was created with the J!Tracker Application
    https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/10776
    https://issues.joomla.org/tracker/joomla-cms/10776.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#10776 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGZUDS4s_wP927AXkgD7c9dd72GvUSNfks5qKyZAgaJpZM4Iy1Pp
.

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Jun 13, 2016

@ggppdk

Can you propose a patch against my branch? With a new lang string if necessary.

@infograf768
Copy link
Copy Markdown
Member Author

As SuperAdmin (as defined in Global Configuration) has all permissions, he is afaik de facto member of all groups.

@infograf768
Copy link
Copy Markdown
Member Author

SuperAdmin => Super User

// User cannot modify own user groups
if ((int) $user->id == (int) $my->id)
{
if ($data['groups'] != null)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to have very clear code 😃 I prefer as it is.

@izharaazmi
Copy link
Copy Markdown
Contributor

Not necessary, but if you want you can also exempt super users from this enforcement to make things obvious.

@infograf768
Copy link
Copy Markdown
Member Author

Not necessary, but if you want you can also exempt super users from this enforcement to make things obvious.

No use imho.

@izharaazmi
Copy link
Copy Markdown
Contributor

izharaazmi commented Jun 14, 2016

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.

@infograf768
Copy link
Copy Markdown
Member Author

Indeed, it is failing when superuser wants to modify himself
I get the warning
"Save failed with the following error: You can't remove your own Super User permissions."

@izharaazmi
Copy link
Copy Markdown
Contributor

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)

@joomla-cms-bot
Copy link
Copy Markdown

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.

@infograf768
Copy link
Copy Markdown
Member Author

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'))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll ignore that you call getUser twice and not use a variable 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@izharaazmi
Copy link
Copy Markdown
Contributor

Great! You exempted the Super user. I'll now set my test success in few minutes. 👍

@izharaazmi
Copy link
Copy Markdown
Contributor

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?

@joomla-cms-bot
Copy link
Copy Markdown

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.

@izharaazmi
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on a3a9afa

1. Without any hacks for each registered and administrator (normal) users - does not allow saving own groups, allows for other than own.
2. With DOM manipulation to force submit user groups for normal users - shows warning and groups not changed for own, allowed for others.
3. For Super user allows all groups changes. Also warns when user tries to remove own super user rights (everything same as without this patch)
4. Non super user cannot edit super user (less relevant, but important)

👍


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

@izharaazmi
Copy link
Copy Markdown
Contributor

That you said about J3.6 aiming, can you please have a look at #10657 too!

@infograf768
Copy link
Copy Markdown
Member Author

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?

Indeed unrelated. We should imho in that case display a message telling that a user should be assigned to at least one user group.
Further test show that one could create a new user without any user group... In that case when the user tries to login he is treated as Public/Guest when trying to login in frontend.
Both these situations should be taken care of I guess. Best would be to discuss first this on list.

@ggppdk
Copy link
Copy Markdown
Contributor

ggppdk commented Jun 14, 2016

I have tested this item ✅ successfully on a3a9afa

Thanks for the super admin update

  • various software, including mine use use-groups assignments not only for ACL

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

  • Works when editing other user
  • Works when editing self, (user-groups do not appear)
  • Works when editing self, with DOM manipulation to inject the groups, form is saved without user-groups being changed
  • Works for super users, when editing self, they can modify the usergroups
    This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10776.

@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented Jun 14, 2016

RTC based on testing.


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

@wilsonge wilsonge merged commit da79cb0 into joomla:staging Jun 15, 2016
@wilsonge wilsonge added this to the Joomla 3.6.0 milestone Jun 15, 2016
@wojsmol
Copy link
Copy Markdown
Contributor

wojsmol commented Jun 15, 2016

@joomla-cms-bot Please remove RTC label as this is merged.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 15, 2016
@wilsonge
Copy link
Copy Markdown
Contributor

Thanks for the work on this everyone!

@Devportobello
Copy link
Copy Markdown
Contributor

Why not adding a new permission to grant access ? by checking only "core.admin" this introduce B/C

@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented May 17, 2018

This is a issue closed on 16 Jun 2016 please open a new issue as this message is going to be missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants