On 27/05/2021 07:32, Carsten Klein wrote:
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?
The issue while objects can't be added to the Map or removed from the
Map, the objects can still be mutated. For that reason I prefer the
getAttribute() approach where an appropriate defensive copy can be made
before returning any attribute value.
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.
I don't see a need either now or in the future to differentiate between
TomcatPrimcipal and AttributePrincipal. If we can think of a valid use
case for separate interfaces I'd support that - but at the moment I
don't see one.
3. Class UserDatabasePrincipal in UserDatabaseRealm
<snip/>
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?
I don't think so.
I think I have figured it out.
User implements Principal and exposes various fields including password.
User was used as the UserPrincipal stored in GenericPrincipal
Some time ago, getPassword() was removed from GenericPrincipal. In order
to prevent access to User.getPassword(), UserDatabasePrincipal was
introduced.
As far as I can tell, removing UserDatabasePrincipal, relying on
GenericPrincipal and User remaining an internal object not exposed via
the Servlet API would achieve the same result with less code.
At this point I am looking for a reason not to remove
UserDatabasePrincipal and I'm not seeing one.
I think it would be worth handling this is a separate commit to give
folks the chance to review it before proceeding to add attribute support.
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
I suspect the users that worry about that sort of thing aren't using the
UserDatabaseRealm but it would be nice to fix that anyway.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
