Add support for additional user attributes in TomcatPrincipal

2022-02-10 Thread Carsten Klein

Hello Rémy, Mark and Michael,

I'm new to the dev list and did not get your recent mails and didn't 
figure out how to get them in order to answer. So, I decided to start a 
new thread (sorry for that).


I guess, having those attributes with the Principal (aka Principal 
metadata) does not make the Principal a data storage. For my mind, even 
the Principal's getName() returns kind of metadata, since an application 
typically does not really need the logged on user's name in order to 
function properly. Much more important are the user's roles, for example.


So, the new read-only(!) attributes map is just a way of dynamically 
adding more of such metadata fields. Another way would be to add a 
getUserDisplayName() and a new Realm config attribute 
'userDisplayNameCol' (e.g. for the DataSourceRealm), and maybe a 
getUserMailAddress() method later. However, that's not flexible at all 
and many requests from people to extend this scheme may be the result.


So, the dynamic attributes map is the better choice, right? As long as 
it remains read-only, also no one can abuse the Principal as data 
storage. Also, there is really no need to ever request that, since an 
application already has a fully featured data storage around: the 
Session's attributes list. It is primarily intended for exactly that: 
storing the application's data. So, you could always deny any upcoming 
PRs adding write support to the Principal's attributes by referring to 
the Session's attributes map.


Providing such metadata through the Principal is new and was not done 
before. However, since, more or less, it's just an extension to the 
already available getName() method, I guess, it's a quite good idea.


In the Javadoc, of course, we could emphasize more, that this attributes 
map is intentionally read-only and will never be writable.


Michael-o and I agreed on having individual PRs for the three parts of 
the initial enhancement (TomcatPrincipal/GenericPrincipal, 
DataSourceRealm, JNDIRealm). So, I will provide a third PR for the 
JNDIRealm after the DataSourceRealm has been merged.


@Rémy: That was the deal/agreement. We do not touch UserDatabaseRealm 
and you do not vote against DataSourceRealm and JNDIRealm.



@Remy: Maybe you would feel better about this, if we use a completely 
different approach?


We could use the Realm for technically querying those attributes, but 
provide them to the application through the Session's attributes map?


We could either put each single attribute directly into the Session's 
attributes using a prefixed name like "userAttribute.user_display_name", 
or we could add a UserAttributesStore instance (would be a new Tomcat 
specific class) under a "userAttributes" name, which has 
getAttribute(String name) and getAttributeNames() methods.


However, I guess, implementing this approach is a bit more complicated 
than the current one.


Carsten

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: Add support for additional user attributes in TomcatPrincipal

2022-02-11 Thread Carsten Klein





On Fri, Feb 11, 2022 at 10:10 AM Rémy Maucherat wrote


If we get there, it could include mail addresses, ssn, payment info,
user profile pictures (binary), etc etc.
Also one thing I don't get now is how this became Object
getAttribute(String name) instead of String getAttribute(String name)
? The legitimate examples you gave are text, not binary objects.

Ultimately, all of this is still application data ... Moving it in the
Tomcat realm creates a proprietary behavior, the application is no
longer portable while at the same time the benefit is minimal (saving
a query ?). When logging in, the app should pull all the metadata it
needs, store it in the session.

Yes, it could be that much metadata, so what? And yes, it's all 
application data. But that must not be something negative. You may say, 
this is up to the application but, isn't it convenient if the realm does 
it for you? Tomcat's Request (e.g. HTTPServletRequest) returns all data 
POSTed from a client; all that data may be application specific data as 
well. Nobody cares...


Again, it's not about saving one query while logging in a user. It's 
primarily about


1. not needing to configure an extra connection to the user database
2. not needing much custom code an knowledge/skills of the user database 
(SQL, LDAP etc.)
3. getting extra attributes from Tomcat "for free" with 'any' Realm 
simply by configuring a list of fields uniformly


As you are correctly saying, point 1. is not that complicated for 
DataSourceRealm, as there is a JDBC pool around. Nevertheless, the 
application still needs to know the name of the connection (e. g. 
"jdbc/userdatabase") and the name of the user table, the column 
containing the logon name (what Principal.getName() returns) and the 
extra columns to query.


With my solution, all that configuration is "hidden" in the Realm's 
configuration. The only new thing is the "userAttributes" config 
attribute. No duplicated configuration is required.


What do you mean with 'proprietary behavior'? Why shouldn't the 
application be no longer portable? The Realm's configuration always 
contains any access data (JNDI resource names, URLs and passwords in 
case of JNDIRealm), so it wasn't ever portable at all. If you think 
about moving the application between different servers on the same site 
(targeting the same user database), the new "userAttributes" should work 
from any Tomcat server you use.


Moving between different user databases is also much simpler, since the 
whole configuration is centralized at the Realm.


Why do I store Object and not String attributes? Because I can... When 
querying a JDBC database or a LDAP server dynamically, you must expect 
various different types being returned. We could add a rule, that all 
must be Strings or we call the toString() method on each value. But why?




Yes, well, unfortunately, due to more background thinking ...
The purpose of the UserDatabase is to be able to write, so given the
API it is an object database at this point.



In my recent implementation, the AbstractUser got a

/**
 * Additional attributes of this user.
 */
protected Map attributes = new HashMap<>();

so, the UserDatabase does not store Object typed attributes, but only 
Strings (since XML attributes are strings only). So, I don't understand 
why you consider it an object database.





Ok, but ... Your actual use case is the DataSourceRealm, which uses a
DataSource. That DataSource is a JNDI resource which is also available
to the application. Getting a connection from the pool is not
expensive at all, and running an extra query from a prepared statement
is just that. If more state is needed (I believe that will always be
the case), then the difference becomes minimal. Also, the whole data
layout is in the hands of the developer, who then chooses to abuse the
realm backend. So overall in that case, all that you mention is still
best done in the application, replacing the API with something
different (like storing in the session) does not change that and this
is simply about moving a small piece of code from the application to
the container.


Yes, with JDBC and a DataSource it's quite simple to do that in the 
application. However, this ends in much custom code required to run 
after login. Actually this makes the application not portable (or at 
least hard to port).


Believe me, I know what I'm saying. I'm running a software company with 
< 20 customers, each having different user databases and needs. Having 
tailored code for each of them is something you could call `JAR-hell` 
(you know Windows DLL-hell?).





Although I heavily changed my mind on the rest, JNDI/LDAP always
looked to me like the legitimate use case. There's indeed metadata in
there. It could be more difficult to get to it from the app. Maybe
it's less scalable than a DB, and there's no shared connection pool
with the app. So it's always going to be significantly less efficient
to get them from the application.


Yes, LDAP i