Add support for additional user attributes in TomcatPrincipal

[Carsten Klein <c....@datagis.com>]

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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Add support for additional user attributes in TomcatPrincipal

[Rémy Maucherat <re...@apache.org>]

On Fri, Feb 11, 2022 at 11:17 AM Carsten Klein <c....@datagis.com> wrote:
>
>
>
>
> 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

Maybe you should consider some bespoke technology like for example
using JPA instead ? Or any other object persistence framework or
object database obviously.

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

Tomcat is a Servlet container implementing the EE specifications. You
are supposed to be able to take your webapp and be able to run it
unchanged on, for example, Wildfly (with just configuration for the
DataSource resource in the JNDI environment). This, however, is a
proprietary feature that makes the webapp non portable as other
containers don't have TomcatPrincipal with attributes pulled from some
location. So how would the same app retrieve the data on Wildfly ?

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

Because not having that limitation opens the possibility of having to
implement automatic type mapping. Coincidentally, Michael stated he
wanted List<Object> to be handled. A String is better from the Tomcat
perspective because the implementation provided can be simple (then,
let's say it's really a binary, it has to be encoded maybe with base64
- extended custom realm here - and then handled by the application).

>
> >
> > 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<String, String> 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.

So it's fine to replace Object with String then. More seriously, this
means the data is best stored somewhere else instead and that it's not
a good fit for this realm.

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

JNDI, DataSource and JDBC are the basics that allow portable EE apps
that interact with databases. I don't understand.

> 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 is much more complicated and there is no pool... However, is
> that a reason for not providing that new feature for DataSourceRealm?
> Someone who's fit in LDAP and has no glue to SQL may see that different.
>
> Also, for the sake of uniformity, I plead for adding this to most of the
> Realms (all that really access the user database, JSAPIC and JAAS are
> just wrappers for external libraries).

I don't see any uniformity as the functionality is completely
different ... It is best not to add anything.

> >
> > Ok that there's an agreement on javadoc clarifications (which I'll do).
>
> OK

This then leads to:
- Keep the TomcatPrincipal changes (with javadoc added explaining the API)
- Add later a simple implementation for JNDIRealm that exposes the
LDAP attributes (this will be the "reference implementation" of the
feature, you could say); looking at the PR, the typing is rather
horrible but that's a Sun JNDI thing
- To go further, this would be in a subproject or third party project
that implements custom extended realms (changes can be made to make
the DataSourceRealm more extensible, as needed)

Rémy

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

Re: Add support for additional user attributes in TomcatPrincipal

[Carsten Klein <c....@datagis.com>]

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<String, String> 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 is much more complicated and there is no pool... However, is 
that a reason for not providing that new feature for DataSourceRealm? 
Someone who's fit in LDAP and has no glue to SQL may see that different.

Also, for the sake of uniformity, I plead for adding this to most of the 
Realms (all that really access the user database, JSAPIC and JAAS are 
just wrappers for external libraries).

> 
> Ok that there's an agreement on javadoc clarifications (which I'll do).

OK

Carsten

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

Re: Add support for additional user attributes in TomcatPrincipal

[Michael Osipov <mi...@apache.org>]

wrong list

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

Re: Re: Add support for additional user attributes in TomcatPrincipal

[Michael Osipov <mi...@apache.org>]

> On Fri, Feb 11, 2022 at 8:10 AM Carsten Klein <c....@datagis.com> wrote:
>>
>> Hello R=C3=A9my, 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.
> 
> 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.

It must be Object and nothing else. You cannot assume it just to be 
string. Binary attributes, e.g., certificates for encryption. 
Multivalued attributes, e.g. List<Object>.

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

No, this is user data, the source is opaque to the application and that 
is good, you can exchange for testing purposes. Never assume that you 
have sessions. Minimal? How do you know that the retrieval time is 
minimal? Why are you making these assumptions? REST APIs are stateless, 
you will pay for every additional query.

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

This is a UserDatabase problem, not a Realm one. You can easily modify 
it to be read only at config time if the code allows it. Maybe that 
should even be the default. Least priviledge principal.

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

Again, you are making wrong assumptions from your personal usecases.

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

...and here you finally understand it is not just a pool to get a 
connection. We don't something like a PoolingContextSource like Spring 
has. Even if, queries still require time. Believe me, I work with it in 
the largest AD setup on the planet, likely.

M

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

Re: Add support for additional user attributes in TomcatPrincipal

[Rémy Maucherat <re...@apache.org>]

On Fri, Feb 11, 2022 at 8:10 AM Carsten Klein <c....@datagis.com> wrote:
>
> 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.

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.

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

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.

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

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.

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.

Ok that there's an agreement on javadoc clarifications (which I'll do).

Rémy

>
> Carsten
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>

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