Bug 64442 - Re-use roles and groups defined on users on MemoryUserDatabase creation
Summary: Re-use roles and groups defined on users on MemoryUserDatabase creation
Status: NEW
Alias: None
Product: Tomcat 10
Classification: Unclassified
Component: Catalina (show other bugs)
Version: unspecified
Hardware: All All
: P2 minor (vote)
Target Milestone: ------
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-14 18:34 UTC by Felix Schumacher
Modified: 2020-05-30 13:02 UTC (History)
0 users



Attachments
Re-use roles and groups defined on users on MemoryUserDatabase creation (2.37 KB, patch)
2020-05-14 18:34 UTC, Felix Schumacher
Details | Diff
Re-use roles and groups defined on users on MemoryUserDatabase creation (3.30 KB, patch)
2020-05-20 15:24 UTC, Felix Schumacher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Felix Schumacher 2020-05-14 18:34:41 UTC
Created attachment 37246 [details]
Re-use roles and groups defined on users on MemoryUserDatabase creation

When the XML file for MemoryUserDatabse is digested, the order of the
elements was important. It had to be roles, groups and than users.
With this patch the order of the elements is not important any more.
If a user element defined a role or group before the corresponding
role or group element, we now will re-use that element and add a
possibly missing description.
Comment 1 Christopher Schultz 2020-05-14 21:05:31 UTC
What's the desire, here? Making the XML more readable or avoiding redefining roles or groups?

If you are going to modify the code, you could also modify the XML Schema to suit (/conf/tomcat-users.xsd).
Comment 2 Felix Schumacher 2020-05-15 04:55:25 UTC
The trigger here was, that we tried to add a role without a XML-aware editor and placed the role definition behind the user, that also defined the role. The result of such a misconfiguration is, a user that is not member of that role. This is confusing. And the patch tries to be more forgiving with misconfiguration.

Hadn't thought about adapting the XSD, though. Do you think we should adapt it?

Another possible way to let users know the misconfiguration is actually checking the conformance of the XML file, but I think the more forgiving approach is nicer (to the user).
Comment 3 mgrigorov 2020-05-15 10:30:59 UTC
Is the XSD actually being used by Tomcat to validate the .xml when parsing ?
I guess it is not, otherwise Felix would have seen an error in the logs explaining that the order is not correct.
Comment 4 Christopher Schultz 2020-05-15 14:24:25 UTC
I think if we are going to provide an XSD for the file, our code should match it. Or the other way around. In any case, they should agree. :)

Tomcat does not actually bother to enforce the schema's rules when parsing the file. But historically, there was no schema; that's a (relatively) late addition (2019, I think). Now that it exists, it /could/ be used for validation during database-loading.

Maybe for Tomcat 10?
Comment 5 Felix Schumacher 2020-05-15 16:02:15 UTC
I think the code should either work without surprises and I was surprised, that adding a role (even at the wrong place) led to a user without a role, or fail fast and log a warning.

My preference would be to be forgiving, but if you think we should be more strict, than I could try to see, if I can enable the loader to enforce the schema.
Comment 6 Christopher Schultz 2020-05-17 18:45:52 UTC
No, I'm okay with both the relaxed behavior and not requiring the XSD validation. I just want the XSD to reflect what the code is willing to accept.
Comment 7 Mark Thomas 2020-05-20 07:58:25 UTC
The XSD was added to document the requirements / allow XML aware editors to produce valid files.

I'm happy with relaxing the restrictions.

With the relaxation in place, we need to think about what duplicate definition of roles / groups / users means and how to handle it.
Comment 8 Remy Maucherat 2020-05-20 08:10:09 UTC
Did I miss something, or is a random element order much harder to write in a xsd ? The sequence is very easy otoh, but the ordering is fixed.

I'm ok as well for allowing creativity here, it doesn't hurt. People can fully respect the schema if they like it better.
Comment 9 Felix Schumacher 2020-05-20 15:13:19 UTC
OK, so I will try to relax the schema.

Currently a role, that is defined after a user has "defined" a role, will reset the membership and therefore loose the connection to the user. That has been fixed with the patch. Another problem was, that a role - again defined through an user entry - had no description. That has been fixed with the patch, too.

Now, the only part undefined would be, when a xml file contained a role (or user or group) twice (with different descriptions). Then only the first definition would be used (loosing the second one). But the users would still be connected to a role with the correct name.

So given a xml structure like

 <user name="u1" roles="r1"/>
 <role name="r1" description="first"/>
 <user name="u2" roles="r1"/>
 <role name="r1" description="second"/>

would lead to one role (r1 with description "first"), that is connected to the users u1 and u2.

Currently this would lead to two users u1 and u2 that are not connected to any role and one role (r1 with description "second").

And yes, I know, that the xml structure was not valid according to our xsd :)
Comment 10 Felix Schumacher 2020-05-20 15:24:45 UTC
Created attachment 37260 [details]
Re-use roles and groups defined on users on MemoryUserDatabase creation

Re-use roles that may have been created earlier through user entries and relax the schema to allow such constellations.
Comment 11 Christopher Schultz 2020-05-24 13:51:58 UTC
(In reply to Remy Maucherat from comment #8)
> Did I miss something, or is a random element order much harder to write in a
> xsd ? The sequence is very easy otoh, but the ordering is fixed.

Nope. Felix's latest proposal uses <xs:all> as a wrapper (which I've never used before), but you could also do this, which I think is clearer:

<xs:choose minOccurs="0" maxOccurs="unbounded">
  // element role
  // element group
  // element user
  ...
</xs:choose>
Comment 12 Felix Schumacher 2020-05-25 14:34:23 UTC
I didn't find anything about xs:choose. Did you mean xs:choice? (That would not work, as it would allow only one of the three elements).
Comment 13 Mark Thomas 2020-05-26 09:41:27 UTC
The way the servlet XSD does things is with a nested choice.
Comment 14 Felix Schumacher 2020-05-26 10:31:20 UTC
Yes, you can use a choice-element, if it is used together with the attributes minOccurs and maxOccurs, but I fail to see, how it is clearer than using the all-element.

But as it is clearer for Chris, it is used in the servlet xsd and I used all only because I found it first while researching a replacement, I will use choice :)
Comment 15 Christopher Schultz 2020-05-26 15:45:20 UTC
(In reply to Felix Schumacher from comment #12)
> I didn't find anything about xs:choose. Did you mean xs:choice? (That would
> not work, as it would allow only one of the three elements).

Yes, I meant "choice". The unbounded choice allows any number of any of the items contained within he choice (not just any number of whichever one you choose).
Comment 16 Mark Thomas 2020-05-29 14:37:19 UTC
Do you want to commit this change before I tag the next release next week?
Comment 17 Felix Schumacher 2020-05-30 13:02:24 UTC
I commited it to trunk and to the 9.0.x tree. Should I backport it to 8.5.x, too?