On Fri, Feb 11, 2022 at 8:10 AM Carsten Klein <c.kl...@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-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
