Hi Mark,
thanks for sharing your ideas :)
On 26/05/2021 19:56, Mark Thomas wrote:
Given that the attributes may well be security related, you would need
to make sure neither the Map nor any of the keys/values could be
modified. Protecting the Map is easy. Protecting the keys/values is a
little trickier. For that reason I'd lean towards the solution below.
Oh yes, these attributes should likely be immutable. Since I still
believe that Enumerations are kind of uncomfortable (and outdated?),
what about strictly relying on Collections.unmodifiableMap?
But, I'm ok with getAttribute(String name) and getAttributeNames() pair
as well. You decide :)
My first idea was to allow users to put their own attributes as well.
But then, we'd need to track Realm-sourced attribute names and make
those read-only in a setAttribute method. Since users could as well put
their stuff into session attributes, making the whole map immutable is
possible and clearly much simpler.
2. Should I add attributes-related public methods to the
TomcatPrincipal interface as well?
Yes. So far the only Tomcat specific extensions are for SPNEGO but this
is in the same category. The other option would be a new interface but I
don't see a need for that.
Since UserDatabasePrincipal is a private inner class of
UserDatabaseRealm, no instanceof checks are possible to determine
whether attributes may be available or not. So, I was thinking of a new
interface
public interface AttributedPrincipal extends Principal {
public Object getAttribute(String name);
Enumeration<String> getAttributeNames();
}
That interface has a slightly more meaningful name for consumers of
additional user attributes. But adding these methods to TomcatPrincipal
is good as well.
3. Class UserDatabasePrincipal in UserDatabaseRealm
Why does UserDatabaseRealm pass a userPrincipal of type
UserDatabasePrincipal? Can't we just drop that and do it like
JNDIRealm or DataSourceRealm?
I don't see any obvious reason. I'll do some digging in the source
history to see if I can find out why. Absent a good reason, I'd say
drop it.
There is a good reason for it, but I think it should be possible to drop
it.
It is there because the UserDatabaseRealm supports the concepts of
groups. Users can have roles assigned directly or users can be assigned
to a group and inherit the roles of the group. This means hasRole() is a
little more complicated and the UserDatabasePrincipal is used to
determine if this additional processing is required.
I think this could be replaced by a
"org.apache.catalina.realm.UserDatabaseRealm.groups" attribute which
would remove the need for the dedicated UserDatabasePrincipal
Honestly, I don't see that in the code. What I see is, that
UserDatabaseRealm resolves roles coming from groups up front in its
getPrincipal method. After that, it just creates an ordinary
GenericPrincipal instance with a single list of effective roles.
The associated UserDatabasePrincipal does nothing special with
hasRole(). Its only method is getName(). So, it just removes or "hides"
any special GenericPrincipal methods like hasRole() and getRoles() from
the user (aka from the Servlet code, internally Tomcat still uses the
surrounding GenericPrincipal, e. g. for authorization).
So, the only thing UserDatabasePrincipal does, is to hide the fact that
groups have already been resolved to a single list of effective roles,
the Principal is working with during its lifetime. Did I overlook something?
Actually, when declaring the attributes-related methods in a public
interface (either TomcatPrincipal or AttributedPrincipal), adding that
interface to UserDatabasePrincipal is not expensive. So, we could always
decide to leave that UserDatabasePrincipal in place and let it implement
that interface.
However, in order to not "pollute" UserDatabasePrincipal with unneeded
methods, I'd vote for using a new
org.apache.catalina.AttributedPrincipal interface.
Additionally, class UserDatabasePrincipal is NOT serializable. That
means, it gets dropped when sessions and principals get persisted and
reloaded. That applies to session persistence across restarts (with
persistAuthentication set to true) and likely to clustering/HA. That's
not fatal, but after a restart or when running on a different cluster
node, users suddenly get a full-blown GenericPrincipal instance when
they call Request.getUserPrincipal(). However, since nobody complained
about it, the UserDatabasePrincipal is likely not so important :-p
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
