[4.0] new approach prevent registration as super user

[brianteeman]

The code was already present to not display super user in the list of available usergroups. But the logic was not correct. With this PR the list of available user groups in the com_users options correctly excludes super users

Pull Request for Issue #27629

(Just noticed this should have been done against staging. I am offline for the next few hours but this can still be tested in j4 and then I can backport it when confirmed it works)

[joomdonation]

If I'm not mistaken, your change will always remove user groups which has core.admin permission, even if the current user is Super Admin, so it changes existing behavior and it's wrong.

I think we can:

• Add a new attribute to the field https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_users/config.xml#L30, like exclude_permissions="core.admin"
• Modify code the file libraries/src/Form/Field/UsergrouplistField.php, method

protected function getOptions()
	{
		// Hash for caching
		$hash = spl_object_id($this->element);

		if (!isset(static::$options[$hash]))
		{
			static::$options[$hash] = parent::getOptions();

			$groups             = UserGroupsHelper::getInstance()->getAll();
			$checkSuperUser     = (int) $this->getAttribute('checksuperusergroup', 0);
			
			if ($this->getAttribute('exclude_permissions'))
			{
				$excludePermissions = explode(',', $this->getAttribute('exclude_permissions'));
			}
			else
			{
				$excludePermissions = array();
			}



			$isSuperUser        = Factory::getUser()->authorise('core.admin');
			$options            = [];



			foreach ($groups as $group)
			{
				// Don't show the user groups which is set in exclude_permissions property
				foreach ($excludePermissions as $permission)
				{
					if (Access::checkGroup($group->id, $permission))
					{
						continue 2;
					}
				}

				// Don't show super user groups to non super users.
				if ($checkSuperUser && !$isSuperUser && Access::checkGroup($group->id, 'core.admin'))
				{
					continue;
				}

				$options[] = (object) array(
					'text'  => str_repeat('- ', $group->level) . $group->title,
					'value' => $group->id,
					'level' => $group->level
				);
			}

			static::$options[$hash] = array_merge(static::$options[$hash], $options);
		}

		return static::$options[$hash];
	}

That code will allows us to prevent user groups with any permissions we want from being shown in the field.

[brianteeman]

If I'm not mistaken, your change will always remove user groups which has core.admin permission, even if the current user is Super Admin, so it changes existing behavior and it's wrong.

You are misunderstanding the point of the PR. The aim of the PR is prevent the guest and registered user groups being set in com_users as superuser. That is exactly what it does. There is never a use case where you should be able to set either of those as a superuser. The code already exists to prevent it from happening during registration but there was a missed case of the guest user. Instead of leaving the situation where a site admin can set the user group to be a value that the registration forbids and then giving an error this PR correctly prevents that scenario from ever happening.

[joomdonation]

I understand the point of the PR and the code you changed. However, you changed code of a form field type here and with that change, if someone uses that field type (for example, in third party extensions), they won't be able to list Super Users group (for Super Admin) and don't show that user group (for none Super Admin users) on the same form. That's a backward in-compatible change. That's the reason I suggested adding exclude_permissions attribute

(I hope it's clear now as I don't know how to explain it better, maybe someone with better English can explain more clearly)

[brianteeman]

That's a backward in-compatible change.

This is Joomla 4 :)

[brianteeman]

Also if you look in the xml you will see that the attribute had already been added - its just the logic for the attribute that was not working as intended

[joomdonation]

Even with Joomla 4, it limits the usage of the form field, make it less flexible than how it was designed.

[brianteeman]

This is NOT breaking anything. The field is working exactly as it was before. It is ONLY if you have the additional paramater in the xml that any usergroup is removed. That paramater existed before - it just didnt work

[joomdonation]

As I said, it changes the behavior of the field, make it less flexible. I don't know how to explain it more clear, so I will leave it to someone else.

[joomdonation]

Maybe an example would help. Assume that in Mass Mail Users feature of Joomla, a user from Manage User group is not allowed to send Mass Mail to Super User group. At the moment, it could be done by change this field https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_users/forms/mail.xml#L26 , add checksuperusergroup="1" to it

However, with your change, even users from Super Users user group won't be able to see and send mail to Super Users (or Administrator) group.

Hope it's more clear now.

[Bakual]

@joomdonation Brian is right here. The $checkSuperUser is only true if the checksuperusergroup attribute is set in the XML. That attribute is only used for the new_usertype and guest_usergroup fields.
It was added as part a security hardening in 3.6.5. See https://developer.joomla.org/security-centre/667-20161204-misc-security-hardening.html

So it indeed looks like an oversight that you can set it to a superuser group if you're a super user yourself. It never makes sense to be able to set that.

@brianteeman You can also remove the line 84 $isSuperUser = Factory::getUser()->authorise('core.admin'); since $isSuperUser isn't used anymore after you removed the only use of it.

[joomdonation]

@Bakual I don't know the reason/history of checksuperusergroup attribute, so I commented base on reading the code. And from reading the code, the original behavior is that:

• If checksuperusergroup is not set, the field will show all user groups.
• If checksuperusergroup is set and the current user does not have core.admin permission, hide all user groups which are having core.admin permission
• If checksuperusergroup is set and the current user has core.admin permission, all user groups still be displayed

So this PR, as I mentioned, change behavior of a form field type which could be used by third party extensions (the sample user case which mentioned in Mass Mail example could be valid)

And if you are correct that checksuperusergroup is added as part of 3.6.5 security release, why we can still set Guest User Group to Super Users now? So the link you sent https://developer.joomla.org/security-centre/667-20161204-misc-security-hardening.html could be additional code added elsewhere.

[Bakual]

If checksuperusergroup is set and the current user has core.admin permission, all user groups still be displayed

Yep, and this is basically a bug as it doesn't make sense. It also doesn't make sense to behave like this for 3rd parties. So the changed behavior is fine.

And if you are correct that checksuperusergroup is added as part of 3.6.5 security release, why we can still set Guest User Group to Super Users now?

Which only proves my point that it is a bug 😄
I think what happend back then was that the one who wrote the security fix mixed the part for the "Guest User Group" and "New User Registration Group" with the part "restricting the ability for a lesser privileged user to make user group assignment changes to users in a Super User group". First one shouldn't be allowed for Super Users as well, and the second part should be allowed for Super Users, hence the conditional. But instead of adding that conditional only for the second part of the fix, it got added to both of them.

[alikon]

I have tested this item ✅ successfully on 7d6f427

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28011.}

[alikon]

guess should be backported on 3.x

[jwaisner]

I have tested this item ✅ successfully on 7d6f427

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28011.}

[jwaisner]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28011.}

[joomdonation]

@brianteeman Please remove this line of code https://github.com/brianteeman/joomla-cms/blob/superuser/libraries/src/Form/Field/UsergrouplistField.php#L81 as it's not needed with your change anymore.

[Quy]

@brianteeman Please delete obsolete variable on line 81.

[richard67]

@brianteeman Could you solve conflicts? Or shall I do it for you to save you some time?

[richard67]

Conflicts are coming from recently merged #28021 .

[brianteeman]

not at my laptop right now.not ging to try doing it on my phone. so feel free to give it a go

[richard67]

@brianteeman Done. Was possible in the GitHub UI, but you are right, that should not be done on a mobile phone.

[brianteeman]

thx

[richard67]

@alikon @jwaisner Could you test again?

[richard67]

I have tested this item ✅ successfully on e2e0434

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28011.}

[chmst]

I have tested this item ✅ successfully on e2e0434

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28011.}

[richard67]

So we have 2 good tests again and can keep the RTC ;-)

[brianteeman]

Thanks