The GUI already forbid you to create a second root node or to move a node to root at least there is no b/c break.

if you have a better solution I'm happy to see it

The GUI's limitations are artificial at best, https://developer.joomla.org/joomlacode-archive/issue-20528.html is the issue that introduced them (relevant commit) and honestly just masked the actual described issue instead of fixing it.

The security patch (which given the lack of information conveyed by Joomla in the issues actually being fixed, I cannot ascertain what the root issue that was trying to be addressed is) complements the current com_users artificial limitations, but that's really all I can say just from analyzing code diffs. If the GUI limitations were removed (which, they should be, again to my knowledge there is nothing in how Joomla is built that mandates that the usergroups table can only have one root node) and the details of the security fix causing that check to be added disclosed, then a proper patch for the table class could be discussed. Until then, this issue is mostly hypothetical on an out-of-the-box installation and only evident if you overload the UsersModelGroup class to remove this form definition change, or possibly alter the form definition using a plugin. I could imagine that being a legitimate scenario in a multi-site type installation if you're creating a separate root node for each site.

Select list doesn't render in the UI for the public group... https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_users/models/group.php#L121-L125 So you don't - that's not been an issue for at least 8 of the ten years.

basically after this the server side validation now matches the GUI. And if anyone has been using this behaviour it's not been effectively broken in the GUI for at least 10 years as you say. I'd say that's long enough to probably judge that no one is doing it and formally disallowing it is ok.

the server side validation now matches the GUI

The library class should not be aware of the GUI implementation. That's a design flaw introduced with this patch.

I'm not following why specifically the "cannot demote root node to child nor cannot promote child node to root" check is actually necessary. The only check I can think should be necessary is a prohibition on moving the only root node to be a child of anything or flat out removing the only root node, which with how com_users presents things would effectively be the same type of fix without introducing the hardcoded "cannot move existing node in these ways" condition introduced here.

Actually, I’m just closing. The feeling I’m getting is that since the com_users interface presents the data in a very specific way that the API design should only support the com_users way. Nevermind that plugins can extend core and alter it to undo the check coded into the model and correctly handle the data integrity issue the right way instead of monkey patching around it as was done all those years ago.

I do believe this is a backwards compatibility break in that it makes an incompatible change to the data processing workflow of the Joomla\CMS\Table\Usergroup class and introduces an unintended data model restriction that did not previously exist, but since you can’t actually trigger it with an out of the box installation thanks to the decade old patch it might as well be as though I never said anything.

I'm not following why specifically the "cannot demote root node to child nor cannot promote child node to root" check is actually necessary.

Well when you set the public usergroup's parent ID to an non existing value the ACL is broken for that reason we introduced that check. Can you think of a way to do such checks it in a better way? I'm happy to test such a patch against the issue that got reported to the JSST when you whish i can also share the report with you in private so we can discuss an better patch that still support the features mention here.

The only check I can think should be necessary is a prohibition on moving the only root node to be a child of anything or flat out removing the only root node

Not knowing the full details of the issue, I can only guess, but what I mentioned before still seems like a better solution than what was implemented. Basically, the engine should not allow someone to move the only root node of the #__usergroups table to be a child of another node in the tree, and the engine should not allow deletion of the only root node in the #__usergroups table. Those types of checks retain the required data integrity of that table and the ACL system as a whole. Otherwise, it needs to be clearly documented why specifically the table class prohibits the creation of new root nodes, prohibits promoting any child node to a root node, and prohibits demoting any root node to be a child node (beyond the existing behavior in com_users which looks to be more of a hack than a fix if I am being blunt about things). And if Joomla is really so fragile that it mandates that ID 1 be a root node, then that should be documented as well. All of this should be technically focused documentation explaining the rationale behind these design decisions, not just a page on the docs wiki that says "you can't do this", because that technical documentation is going to help people later on down the line.

Well when you set the public usergroup's parent ID to an non existing value the ACL is broken

Then ACL has been broken since 1.6.0 because there is no #__usergroups.id record with ID 0. Also notice that I said nothing about the part of the patch that validates that the parent ID maps to an existing record (actually this is a perfect use case for that exists form rule I added in 3.9 which would add validation to the form input instead of letting things go as deep into the system as you did by patching the table class).

IIRC, there is no configuration anywhere setting the default "Public" group as the system default group, the only configuration that exists is the guest group parameter in com_users which falls back to 1 if that isn't set, which in 99.9% of cases will exist. Which means if my memory is right then things are actually only working correctly by happenstance and not by the code having been written in a way to support whatever it is that people are trying to accomplish.

That's why I think the issue needs better clarity about what exactly was trying to be fixed and that if necessary additional checks specific to that group (or rather groups in that position of the nested tree model) need to be made, not trying to enforce a rule in a part of the code base that is unaware of the global configuration or what a "public" usergroup is. All I know is by looking at the code, adding the same artificial limitation to Joomla\CMS\Table\Usergroup that exists in com_users is bad and to me can be considered a B/C break if not using the com_users web interface and programmatically using the class to manage groups, or utilizing plugins to extend the com_users interface and reverse the changes applied by the UsersModelGroup class.

If someone decides that the artificial limitation of com_users is going to be a permanent fixture of Joomla, then the B/C break should be adequately documented and any documentation pertaining to the usergroups data model and the ACL system should indicate that the tree is required to have a record ID of 1 as the sole root node of the data structure.

Ok will send you the deails when i get back to my PC. Let's reopen this issue here while we check this.

Cc @SniperSister

@mbabker i have just sended you an mail with the details feel free to let me know when you have any questions.

Close, at this point damage is done.