I have tested this item ✅ successfully on 3ec40bf

I have tested this item ✅ successfully on 3ec40bf

works as described.

RTC

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.

This PR has received new commits.

CC: @andrepereiradasilva, @bertmert

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...

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 ????

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

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?
@wilsonge @brianteeman

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

This PR has received new commits.

CC: @andrepereiradasilva, @bertmert

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

@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!

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

I have tested this item ✅ successfully on d08924b
Thanks!

I have tested this item ✅ successfully on d08924b

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.}

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;
        }
    }

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
.

@ggppdk

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

This PR has received new commits.

CC: @andrepereiradasilva, @bertmert, @ggppdk, @izharaazmi

Folks, I have implemented the check in model. Is'nt that sufficient? Do we really need to also add it to the library?

I think it would not be appropriate to enforce this in the library. Checks
in the model should be enough here.

I am assuming the case when certain component such as subscription will try
to manipulate the usergroups of the current user when they purchase a plan,
it would be a trouble.

OK, therefore please set test as successful (after testing ? )

@izharaazmi @infograf768

i was about to comment on the same thing,

• i am wondering if the library or the model is best place to add this check

and thinking if it is good to enforce this on 3rd party,

now after the comment of @izharaazmi, i see that indeed it is better to add it to the model

@izharaazmi @ggppdk
Please test and mark your test on https://issues.joomla.org/tracker/joomla-cms/10776
We need this in 3.6.0
Thanks

I have tested

• editing different user, all good
• editing self without form tampering to inject groups, all good
• editing self with form tampered to inject groups, all good, we get warning + form is saved without changing the groups

but before marking this successful, a question

what about a super admin ? should a super admin be allowed to change its own groups ?

• currently the form and the model will not allow a super admin user to change its own groups

[EDIT]
i know it was mentioned above, but a super admin may want to add its account to a group ? (i think)

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

SuperAdmin => Super User

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

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 get the warning
"Save failed with the following error: You can't remove your own Super User permissions."

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

I used another way. Please test.

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

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)

?

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

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.

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.}

RTC based on testing.

@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.