Bug 64442

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

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

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.

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?

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.

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.

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.

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.

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 :)

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.

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

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

The way the servlet XSD does things is with a nested choice.

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 :)

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

Do you want to commit this change before I tag the next release next week?

I commited it to trunk and to the 9.0.x tree. Should I backport it to 8.5.x, too?

I don't see why not.

Backported to 8.5.x