Re: [tomcat] branch main updated: Add support for additional user attributes in TomcatPrincipal
On Thu, Feb 10, 2022 at 1:01 PM Rémy Maucherat wrote:
>
> On Tue, Feb 8, 2022 at 2:13 PM wrote:
> >
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > michaelo pushed a commit to branch main
> > in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >
> >
> > The following commit(s) were added to refs/heads/main by this push:
> > new c3edf43 Add support for additional user attributes in
> > TomcatPrincipal
> > c3edf43 is described below
> >
> > commit c3edf437da20af0f11edc0ad6d893399b01e6287
> > Author: Carsten Klein
> > AuthorDate: Wed Jan 12 15:06:42 2022 +0100
> >
> > Add support for additional user attributes in TomcatPrincipal
> >
> > This closes #463
>
> -1 (veto)
The javadoc that has been agreed upon has been added (the wording can
be refined later), so the veto regarding this commit is addressed and
thus dropped.
Rémy
> Unfortunately, the more I think about this, the more it seems wrong to me.
>
> This makes the realm an object store, which is not a good idea at all,
> as it will:
> - Encourage all sorts of bad hacks and practices for users
> - Force us to gradually add more and more features to this generic
> object store feature
> - Have massive discrepancies between realms, with the users trying to
> figure out pros and cons between realms
> - Is a proprietary feature which ties up users to Tomcat
>
> More importantly: application data storage should remain the
> prerogative of the application, Tomcat should not attempt to do that.
> The only thing Tomcat should provide is a principal name, which is
> then the primary key for the third party object store. There is zero
> reason to integrate this within the realm or principal API. It also
> feels like it can never be complete or appropriate.
>
> I apologize for having to do this, after initially helping out with
> this PR, but this is one of those situations where the more I look
> about it, the worse I feel about it.
>
> Rémy
>
> > ---
> > java/org/apache/catalina/TomcatPrincipal.java | 28 +
> > .../apache/catalina/realm/GenericPrincipal.java| 46
> > ++
> > java/org/apache/catalina/realm/JNDIRealm.java | 2 +-
> > webapps/docs/changelog.xml | 5 +++
> > webapps/examples/jsp/security/protected/index.jsp | 44
> > +
> > 5 files changed, 117 insertions(+), 8 deletions(-)
> >
> > diff --git a/java/org/apache/catalina/TomcatPrincipal.java
> > b/java/org/apache/catalina/TomcatPrincipal.java
> > index 83f9035..1e3d9f6 100644
> > --- a/java/org/apache/catalina/TomcatPrincipal.java
> > +++ b/java/org/apache/catalina/TomcatPrincipal.java
> > @@ -17,6 +17,8 @@
> > package org.apache.catalina;
> >
> > import java.security.Principal;
> > +import java.util.Collections;
> > +import java.util.Enumeration;
> >
> > import org.ietf.jgss.GSSCredential;
> >
> > @@ -47,4 +49,30 @@ public interface TomcatPrincipal extends Principal {
> > * exception to LoginContext
> > */
> > void logout() throws Exception;
> > +
> > +/**
> > + * Returns the value of the named attribute as an Object,
> > or
> > + * null if no attribute of the given name exists, or if
> > + * null has been specified as the attribute's name.
> > + *
> > + * Only the servlet container may set attributes to make available
> > custom
> > + * information about a Principal or the user it represents.
> > + *
> > + * @param name a String specifying the name of the
> > attribute
> > + * @return an Object containing the value of the
> > attribute, or
> > + * null if the attribute does not exist, or if
> > + * null has been specified as the attribute's name
> > + */
> > +Object getAttribute(String name);
> > +
> > +/**
> > + * Returns an Enumeration containing the names of the
> > + * attributes available to this Principal. This method returns an empty
> > + * Enumeration if the Principal has no attributes
> > available to
> > + * it.
> > + *
> > + * @return an Enumeration of strings containing the names
> > of
> > + * the Principal's attributes
> > + */
> > +Enumeration getAttributeNames();
> > }
> > diff --git a/java/org/apache/catalina/realm/GenericPrincipal.java
> > b/java/org/apache/catalina/realm/GenericPrincip
Re: Add support for additional user attributes in TomcatPrincipal
On Fri, Feb 11, 2022 at 11:17 AM Carsten Klein 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 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 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,
Re: [tomcat] branch main updated: Add support for additional user, attributes in TomcatPrincipal
Am 2022-02-11 um 11:16 schrieb Rémy Maucherat: On Fri, Feb 11, 2022 at 10:47 AM Michael Osipov wrote: I cannot share your further explanations as well. The Realm *is* already an object store. Principal name and roles are attributes on that object, so is password. Without this possibility you have the following problems: That statement is just weird. The realm stores credentials associated with a principal name, as well as a list of roles (other names) associated with that name. That's it. An object store can store objects (like if there is a get/setAttribute that deals with Object - and that's my problem now, as it turns out ...). No, the realm stores the principal and can additionally store crdentials. You have at least two mechanism which do not verify credentials: SPNEGO and TLS mutual auth. I use both, authentication is completely decoupled from the Realm. Does this onw disqualify? The realm is always a read-only view on a store. Tomcat's intention is not to provide any read/write interface to that store. Strictly reading only. * You cannot abstract from the persistence layer in your application code, at worst you need to double code for retrieval * You double access time since you need to query the principal store again. A performance penalty in a stateless application, e.g. APIs * You maybe even not have access to the realm. Consider the identity provider gives you some form of token containing the attributes which you CANNOT retreive yourself. How to solve? You can't. I don't believe the penalty exists for a DataSource backend. Most likely for LDAP. Either way, the application is now non portable. Given that the benefit for DataSource does not seem large, this simply sounds bad in that case. For LDAP, it sounds like a tradeoff. I disagree here also. Two options: * The Realm database is decoupled form the app database, the app shall not have access to it. What now? * Consider the database is not in the same rack as the app server is. I have cases where it is 500 km away, you instantly feel that the roundtrip matters. Sometimes you simply don't have control over the environment. Yet another example: I have written code to locate the closest domain controller, if I connect to that one I have response times < 10 ms, if I connect to a DC in Munich, I suddenly have > 100 ms. Scale here. No one is aiming here for an ORM layer. One of my points is that this is really far from obvious in the API, so it needs a round of javadoc tightening up. Please do so. M
Re: Add support for additional user attributes in TomcatPrincipal
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
Re: Re: [tomcat] branch main updated: Add support for additional user, attributes in TomcatPrincipal
On Fri, Feb 11, 2022 at 10:47 AM Michael Osipov wrote: > > Rémy, > > really, I don't understand your work attitude. Carsten contacted us on > the mailing list before providing the PR. Then he provided the PR. It > was open for EIGHT months. We discussed this in and out, with you and > Mark. Everyone expressed the conditions he is willing to accept the PR. > I have even asked Carsten to break up the PR to please you. The purpose > of RTC/PR is to express your opinion BEFORE it gets merged to avoid > post-mortem discussions like this. You expressed your conditions and now > you object? This is absolutely not fair and unprofessional. I apologized for this already. I am really sorry about this but background thinking led to different conclusions over time. > I cannot share your further explanations as well. The Realm *is* already > an object store. Principal name and roles are attributes on that object, > so is password. Without this possibility you have the following problems: That statement is just weird. The realm stores credentials associated with a principal name, as well as a list of roles (other names) associated with that name. That's it. An object store can store objects (like if there is a get/setAttribute that deals with Object - and that's my problem now, as it turns out ...). > * You cannot abstract from the persistence layer in your application > code, at worst you need to double code for retrieval > * You double access time since you need to query the principal store > again. A performance penalty in a stateless application, e.g. APIs > * You maybe even not have access to the realm. Consider the identity > provider gives you some form of token containing the attributes which > you CANNOT retreive yourself. How to solve? You can't. I don't believe the penalty exists for a DataSource backend. Most likely for LDAP. Either way, the application is now non portable. Given that the benefit for DataSource does not seem large, this simply sounds bad in that case. For LDAP, it sounds like a tradeoff. > No one is aiming here for an ORM layer. One of my points is that this is really far from obvious in the API, so it needs a round of javadoc tightening up. Rémy > > > Mark, > > you have even assisted on how to backport to previous versions and > object somewhat as well? I don't understand that. > > > From my PoV Carsten did everything we requested over a very long period > of time. > > M - 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
On Fri, Feb 11, 2022 at 8:10 AM Carsten Klein 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. 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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Re: [tomcat] branch main updated: Add support for additional user, attributes in TomcatPrincipal
Rémy, really, I don't understand your work attitude. Carsten contacted us on the mailing list before providing the PR. Then he provided the PR. It was open for EIGHT months. We discussed this in and out, with you and Mark. Everyone expressed the conditions he is willing to accept the PR. I have even asked Carsten to break up the PR to please you. The purpose of RTC/PR is to express your opinion BEFORE it gets merged to avoid post-mortem discussions like this. You expressed your conditions and now you object? This is absolutely not fair and unprofessional. I cannot share your further explanations as well. The Realm *is* already an object store. Principal name and roles are attributes on that object, so is password. Without this possibility you have the following problems: * You cannot abstract from the persistence layer in your application code, at worst you need to double code for retrieval * You double access time since you need to query the principal store again. A performance penalty in a stateless application, e.g. APIs * You maybe even not have access to the realm. Consider the identity provider gives you some form of token containing the attributes which you CANNOT retreive yourself. How to solve? You can't. No one is aiming here for an ORM layer. Mark, you have even assisted on how to backport to previous versions and object somewhat as well? I don't understand that. From my PoV Carsten did everything we requested over a very long period of time. M
Re: Add support for additional user attributes in TomcatPrincipal
On Fri, Feb 11, 2022 at 8:10 AM Carsten Klein 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
Add support for additional user attributes in TomcatPrincipal
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: [tomcat] branch main updated: Add support for additional user attributes in TomcatPrincipal
On Thu, Feb 10, 2022 at 2:48 PM Mark Thomas wrote: > > On 10/02/2022 12:01, Rémy Maucherat wrote: > > On Tue, Feb 8, 2022 at 2:13 PM wrote: > >> > >> This is an automated email from the ASF dual-hosted git repository. > >> > >> michaelo pushed a commit to branch main > >> in repository https://gitbox.apache.org/repos/asf/tomcat.git > >> > >> > >> The following commit(s) were added to refs/heads/main by this push: > >> new c3edf43 Add support for additional user attributes in > >> TomcatPrincipal > >> c3edf43 is described below > >> > >> commit c3edf437da20af0f11edc0ad6d893399b01e6287 > >> Author: Carsten Klein > >> AuthorDate: Wed Jan 12 15:06:42 2022 +0100 > >> > >> Add support for additional user attributes in TomcatPrincipal > >> > >> This closes #463 > > > > -1 (veto) > > > > Unfortunately, the more I think about this, the more it seems wrong to me. > > ACK. I won't tag until this has been reverted or you retract the veto. > > I will note that if I get to the point where I am ready to tag and this > is the only issue remaining to resolve I will revert the change prior to > the tag - although I'd prefer that michaelo did that before then. > > > This makes the realm an object store, which is not a good idea at all, > > as it will: > > - Encourage all sorts of bad hacks and practices for users > > - Force us to gradually add more and more features to this generic > > object store feature > > - Have massive discrepancies between realms, with the users trying to > > figure out pros and cons between realms > > - Is a proprietary feature which ties up users to Tomcat > > Just trying to understand the concern at this point. > > You think the risk is that developers will push data in the Realm and > then access from the authenticated principal? This will seem convenient. If building an e-commerce thingie, you could imagine storing payment info, all misc info, shopping cart. The only thing preventing this is that there is no setAttribute(String name, Object value), but what if someone comes along with a PR later ? Is that a "no" then ? Why would this be more a "no" than for this one ? This is clearly my concern now [following lingering thoughts about how this could be (ab)used]. Again I would like to apologize for taking so long until reaching this position, I know this wastes a lot of time ... My wider concerns were initially triggered when there was suddenly some intent to add the feature to the UserDatabase (which is a R/W database for users, so there the feature clearly becomes general object storage - even though the PR only contained a mock impl). > > More importantly: application data storage should remain the > > prerogative of the application, Tomcat should not attempt to do that. > > The only thing Tomcat should provide is a principal name, which is > > then the primary key for the third party object store. There is zero > > reason to integrate this within the realm or principal API. It also > > feels like it can never be complete or appropriate. > > Looking at the proposed change for the DataSourceRealm I can see it has > clearly defined limits. I can see how we might get requests to push back > those limits in the future. > > I have a general concern / unease that doing this in an application > specific manner would be relatively easy but doing it generically could > get complex. > > > I apologize for having to do this, after initially helping out with > > this PR, but this is one of those situations where the more I look > > about it, the worse I feel about it. > > I do recognise the use case of wanting to expose data that is already in > the Realm to the application. Yes, that was what I was thinking about too. But then it still encourages putting more stuff in the realm backend (since now you can, you probably know how it works by now). The use case in the PR is the DataSourceRealm (not the LDAP one), so let's assume it is a JDBC thing. Retrieving the data from another data source connecting to the same backend in the application might (supposedly) be less efficient (but if you pull a lot of data from the DB, it will suddenly be impossible to tell the difference), but is portable and you could store things properly (and more importantly avoid asking us to add code to automagically map to Java collections ... that's just for starters obviously). > I wonder whether this is a case where we can just provide some hooks > and, if the developer chooses to (ab)use them, that is up to them. > > I'm thinking something like a getUserAttributes() method on Realm that > re
Re: [tomcat] branch main updated: Add support for additional user attributes in TomcatPrincipal
On 10/02/2022 12:01, Rémy Maucherat wrote: On Tue, Feb 8, 2022 at 2:13 PM wrote: This is an automated email from the ASF dual-hosted git repository. michaelo pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/main by this push: new c3edf43 Add support for additional user attributes in TomcatPrincipal c3edf43 is described below commit c3edf437da20af0f11edc0ad6d893399b01e6287 Author: Carsten Klein AuthorDate: Wed Jan 12 15:06:42 2022 +0100 Add support for additional user attributes in TomcatPrincipal This closes #463 -1 (veto) Unfortunately, the more I think about this, the more it seems wrong to me. ACK. I won't tag until this has been reverted or you retract the veto. I will note that if I get to the point where I am ready to tag and this is the only issue remaining to resolve I will revert the change prior to the tag - although I'd prefer that michaelo did that before then. This makes the realm an object store, which is not a good idea at all, as it will: - Encourage all sorts of bad hacks and practices for users - Force us to gradually add more and more features to this generic object store feature - Have massive discrepancies between realms, with the users trying to figure out pros and cons between realms - Is a proprietary feature which ties up users to Tomcat Just trying to understand the concern at this point. You think the risk is that developers will push data in the Realm and then access from the authenticated principal? More importantly: application data storage should remain the prerogative of the application, Tomcat should not attempt to do that. The only thing Tomcat should provide is a principal name, which is then the primary key for the third party object store. There is zero reason to integrate this within the realm or principal API. It also feels like it can never be complete or appropriate. Looking at the proposed change for the DataSourceRealm I can see it has clearly defined limits. I can see how we might get requests to push back those limits in the future. I have a general concern / unease that doing this in an application specific manner would be relatively easy but doing it generically could get complex. I apologize for having to do this, after initially helping out with this PR, but this is one of those situations where the more I look about it, the worse I feel about it. I do recognise the use case of wanting to expose data that is already in the Realm to the application. I wonder whether this is a case where we can just provide some hooks and, if the developer chooses to (ab)use them, that is up to them. I'm thinking something like a getUserAttributes() method on Realm that returns null by default. If an application wants to populate additional attributes, then it needs to provide a custom realm that overrides that method. The issue with this approach is it requires a custom realm. Given your concerns, backing this out while we discuss it some more seems like the right approach to me. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Add support for additional user attributes in TomcatPrincipal
On Tue, Feb 8, 2022 at 2:13 PM wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> michaelo pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new c3edf43 Add support for additional user attributes in
> TomcatPrincipal
> c3edf43 is described below
>
> commit c3edf437da20af0f11edc0ad6d893399b01e6287
> Author: Carsten Klein
> AuthorDate: Wed Jan 12 15:06:42 2022 +0100
>
> Add support for additional user attributes in TomcatPrincipal
>
> This closes #463
-1 (veto)
Unfortunately, the more I think about this, the more it seems wrong to me.
This makes the realm an object store, which is not a good idea at all,
as it will:
- Encourage all sorts of bad hacks and practices for users
- Force us to gradually add more and more features to this generic
object store feature
- Have massive discrepancies between realms, with the users trying to
figure out pros and cons between realms
- Is a proprietary feature which ties up users to Tomcat
More importantly: application data storage should remain the
prerogative of the application, Tomcat should not attempt to do that.
The only thing Tomcat should provide is a principal name, which is
then the primary key for the third party object store. There is zero
reason to integrate this within the realm or principal API. It also
feels like it can never be complete or appropriate.
I apologize for having to do this, after initially helping out with
this PR, but this is one of those situations where the more I look
about it, the worse I feel about it.
Rémy
> ---
> java/org/apache/catalina/TomcatPrincipal.java | 28 +
> .../apache/catalina/realm/GenericPrincipal.java| 46
> ++
> java/org/apache/catalina/realm/JNDIRealm.java | 2 +-
> webapps/docs/changelog.xml | 5 +++
> webapps/examples/jsp/security/protected/index.jsp | 44 +
> 5 files changed, 117 insertions(+), 8 deletions(-)
>
> diff --git a/java/org/apache/catalina/TomcatPrincipal.java
> b/java/org/apache/catalina/TomcatPrincipal.java
> index 83f9035..1e3d9f6 100644
> --- a/java/org/apache/catalina/TomcatPrincipal.java
> +++ b/java/org/apache/catalina/TomcatPrincipal.java
> @@ -17,6 +17,8 @@
> package org.apache.catalina;
>
> import java.security.Principal;
> +import java.util.Collections;
> +import java.util.Enumeration;
>
> import org.ietf.jgss.GSSCredential;
>
> @@ -47,4 +49,30 @@ public interface TomcatPrincipal extends Principal {
> * exception to LoginContext
> */
> void logout() throws Exception;
> +
> +/**
> + * Returns the value of the named attribute as an Object, or
> + * null if no attribute of the given name exists, or if
> + * null has been specified as the attribute's name.
> + *
> + * Only the servlet container may set attributes to make available custom
> + * information about a Principal or the user it represents.
> + *
> + * @param name a String specifying the name of the attribute
> + * @return an Object containing the value of the attribute,
> or
> + * null if the attribute does not exist, or if
> + * null has been specified as the attribute's name
> + */
> +Object getAttribute(String name);
> +
> +/**
> + * Returns an Enumeration containing the names of the
> + * attributes available to this Principal. This method returns an empty
> + * Enumeration if the Principal has no attributes available
> to
> + * it.
> + *
> + * @return an Enumeration of strings containing the names of
> + * the Principal's attributes
> + */
> +Enumeration getAttributeNames();
> }
> diff --git a/java/org/apache/catalina/realm/GenericPrincipal.java
> b/java/org/apache/catalina/realm/GenericPrincipal.java
> index 7260da4..584c104 100644
> --- a/java/org/apache/catalina/realm/GenericPrincipal.java
> +++ b/java/org/apache/catalina/realm/GenericPrincipal.java
> @@ -19,7 +19,10 @@ package org.apache.catalina.realm;
> import java.io.Serializable;
> import java.security.Principal;
> import java.util.Arrays;
> +import java.util.Collections;
> +import java.util.Enumeration;
> import java.util.List;
> +import java.util.Map;
>
> import javax.security.auth.login.LoginContext;
>
> @@ -120,7 +123,7 @@ public class GenericPrincipal implements TomcatPrincipal,
> Serializable {
> */
> public GenericPrincipal(String name, List roles,
> Principal userPrincipal, LoginContext loginContext) {
> -this(name,
[GitHub] [tomcat] michael-o closed pull request #472: Add support for additional user attributes to TomcatPrincipal (9.0.x)
michael-o closed pull request #472: URL: https://github.com/apache/tomcat/pull/472 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #472: Add support for additional user attributes to TomcatPrincipal (9.0.x)
michael-o commented on pull request #472: URL: https://github.com/apache/tomcat/pull/472#issuecomment-1033595371 Merged. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch 9.0.x updated: Add support for additional user attributes in TomcatPrincipal
This is an automated email from the ASF dual-hosted git repository.
michaelo pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 024a495 Add support for additional user attributes in TomcatPrincipal
024a495 is described below
commit 024a4956f0053569beb3f842dc1154c9b7979977
Author: Carsten Klein
AuthorDate: Tue Feb 8 16:53:22 2022 +0100
Add support for additional user attributes in TomcatPrincipal
This closes #472
---
java/org/apache/catalina/TomcatPrincipal.java | 32
.../apache/catalina/realm/GenericPrincipal.java| 42 ++---
java/org/apache/catalina/realm/JNDIRealm.java | 2 +-
webapps/docs/changelog.xml | 5 +++
webapps/examples/jsp/security/protected/index.jsp | 44 ++
5 files changed, 119 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/catalina/TomcatPrincipal.java
b/java/org/apache/catalina/TomcatPrincipal.java
index 83f9035..b74f187 100644
--- a/java/org/apache/catalina/TomcatPrincipal.java
+++ b/java/org/apache/catalina/TomcatPrincipal.java
@@ -17,6 +17,8 @@
package org.apache.catalina;
import java.security.Principal;
+import java.util.Collections;
+import java.util.Enumeration;
import org.ietf.jgss.GSSCredential;
@@ -47,4 +49,34 @@ public interface TomcatPrincipal extends Principal {
* exception to LoginContext
*/
void logout() throws Exception;
+
+/**
+ * Returns the value of the named attribute as an Object, or
+ * null if no attribute of the given name exists, or if
+ * null has been specified as the attribute's name.
+ *
+ * Only the servlet container may set attributes to make available custom
+ * information about a Principal or the user it represents.
+ *
+ * @param name a String specifying the name of the attribute
+ * @return an Object containing the value of the attribute, or
+ * null if the attribute does not exist, or if
+ * null has been specified as the attribute's name
+ */
+default Object getAttribute(String name) {
+return null;
+}
+
+/**
+ * Returns an Enumeration containing the names of the
+ * attributes available to this Principal. This method returns an empty
+ * Enumeration if the Principal has no attributes available to
+ * it.
+ *
+ * @return an Enumeration of strings containing the names of
+ * the Principal's attributes
+ */
+default Enumeration getAttributeNames() {
+return Collections.emptyEnumeration();
+}
}
diff --git a/java/org/apache/catalina/realm/GenericPrincipal.java
b/java/org/apache/catalina/realm/GenericPrincipal.java
index d8a8167..52a039b 100644
--- a/java/org/apache/catalina/realm/GenericPrincipal.java
+++ b/java/org/apache/catalina/realm/GenericPrincipal.java
@@ -19,7 +19,10 @@ package org.apache.catalina.realm;
import java.io.Serializable;
import java.security.Principal;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.Enumeration;
import java.util.List;
+import java.util.Map;
import javax.security.auth.login.LoginContext;
@@ -83,7 +86,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
*/
public GenericPrincipal(String name, String password, List roles,
Principal userPrincipal, LoginContext loginContext) {
-this(name, password, roles, userPrincipal, loginContext, null);
+this(name, password, roles, userPrincipal, loginContext, null, null);
}
/**
@@ -99,10 +102,12 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
* @param loginContext - If provided, this will be used to log out the
user
*at the appropriate time
* @param gssCredential - If provided, the user's delegated credentials
+ * @param attributes - If provided, additional attributes associated with
+ *this Principal
*/
public GenericPrincipal(String name, String password, List roles,
Principal userPrincipal, LoginContext loginContext,
-GSSCredential gssCredential) {
+GSSCredential gssCredential, Map attributes) {
super();
this.name = name;
this.password = password;
@@ -117,6 +122,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
}
this.loginContext = loginContext;
this.gssCredential = gssCredential;
+this.attributes = attributes != null ?
Collections.unmodifiableMap(attributes) : null;
}
@@ -189,6 +195,11 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
this.gssCredential = gssCredential;
}
+/**
+ * The additional attributes associated with this Principal.
+ */
+protected final Map
[GitHub] [tomcat] cklein05 commented on pull request #472: Add support for additional user attributes to TomcatPrincipal (9.0.x)
cklein05 commented on pull request #472: URL: https://github.com/apache/tomcat/pull/472#issuecomment-1033588440 @michael-o Seen this one? Shall we continue with DataSourceRealm after that? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #471: Add support for additional user attributes to TomcatPrincipal (10.0.x)
michael-o commented on pull request #471: URL: https://github.com/apache/tomcat/pull/471#issuecomment-1033534847 Merged -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o closed pull request #471: Add support for additional user attributes to TomcatPrincipal (10.0.x)
michael-o closed pull request #471: URL: https://github.com/apache/tomcat/pull/471 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch 10.0.x updated: Add support for additional user attributes in TomcatPrincipal
This is an automated email from the ASF dual-hosted git repository.
michaelo pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.0.x by this push:
new 8ced4ec Add support for additional user attributes in TomcatPrincipal
8ced4ec is described below
commit 8ced4ec586696db64639de08dad3293611737e32
Author: Carsten Klein
AuthorDate: Wed Jan 12 15:06:42 2022 +0100
Add support for additional user attributes in TomcatPrincipal
This closes #471
---
java/org/apache/catalina/TomcatPrincipal.java | 32 +++
.../apache/catalina/realm/GenericPrincipal.java| 46 ++
java/org/apache/catalina/realm/JNDIRealm.java | 2 +-
webapps/docs/changelog.xml | 5 +++
webapps/examples/jsp/security/protected/index.jsp | 44 +
5 files changed, 121 insertions(+), 8 deletions(-)
diff --git a/java/org/apache/catalina/TomcatPrincipal.java
b/java/org/apache/catalina/TomcatPrincipal.java
index 83f9035..b74f187 100644
--- a/java/org/apache/catalina/TomcatPrincipal.java
+++ b/java/org/apache/catalina/TomcatPrincipal.java
@@ -17,6 +17,8 @@
package org.apache.catalina;
import java.security.Principal;
+import java.util.Collections;
+import java.util.Enumeration;
import org.ietf.jgss.GSSCredential;
@@ -47,4 +49,34 @@ public interface TomcatPrincipal extends Principal {
* exception to LoginContext
*/
void logout() throws Exception;
+
+/**
+ * Returns the value of the named attribute as an Object, or
+ * null if no attribute of the given name exists, or if
+ * null has been specified as the attribute's name.
+ *
+ * Only the servlet container may set attributes to make available custom
+ * information about a Principal or the user it represents.
+ *
+ * @param name a String specifying the name of the attribute
+ * @return an Object containing the value of the attribute, or
+ * null if the attribute does not exist, or if
+ * null has been specified as the attribute's name
+ */
+default Object getAttribute(String name) {
+return null;
+}
+
+/**
+ * Returns an Enumeration containing the names of the
+ * attributes available to this Principal. This method returns an empty
+ * Enumeration if the Principal has no attributes available to
+ * it.
+ *
+ * @return an Enumeration of strings containing the names of
+ * the Principal's attributes
+ */
+default Enumeration getAttributeNames() {
+return Collections.emptyEnumeration();
+}
}
diff --git a/java/org/apache/catalina/realm/GenericPrincipal.java
b/java/org/apache/catalina/realm/GenericPrincipal.java
index 7260da4..584c104 100644
--- a/java/org/apache/catalina/realm/GenericPrincipal.java
+++ b/java/org/apache/catalina/realm/GenericPrincipal.java
@@ -19,7 +19,10 @@ package org.apache.catalina.realm;
import java.io.Serializable;
import java.security.Principal;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.Enumeration;
import java.util.List;
+import java.util.Map;
import javax.security.auth.login.LoginContext;
@@ -120,7 +123,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
*/
public GenericPrincipal(String name, List roles,
Principal userPrincipal, LoginContext loginContext) {
-this(name, roles, userPrincipal, loginContext, null);
+this(name, roles, userPrincipal, loginContext, null, null);
}
/**
@@ -140,7 +143,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
@Deprecated
public GenericPrincipal(String name, String password, List roles,
Principal userPrincipal, LoginContext loginContext) {
-this(name, roles, userPrincipal, loginContext, null);
+this(name, roles, userPrincipal, loginContext, null, null);
}
/**
@@ -154,10 +157,12 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
* @param loginContext - If provided, this will be used to log out the
user
*at the appropriate time
* @param gssCredential - If provided, the user's delegated credentials
+ * @param attributes - If provided, additional attributes associated with
+ *this Principal
*/
public GenericPrincipal(String name, List roles,
Principal userPrincipal, LoginContext loginContext,
-GSSCredential gssCredential) {
+GSSCredential gssCredential, Map attributes) {
super();
this.name = name;
this.userPrincipal = userPrincipal;
@@ -171,6 +176,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
}
this.loginContext = loginContext;
this.gssCredential = gssCredential
[GitHub] [tomcat] cklein05 opened a new pull request #472: Add support for additional user attributes to TomcatPrincipal (9.0.x)
cklein05 opened a new pull request #472: URL: https://github.com/apache/tomcat/pull/472 @michael-o Here's the 9.0.x backport. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #471: Add support for additional user attributes to TomcatPrincipal (10.0.x)
michael-o commented on pull request #471: URL: https://github.com/apache/tomcat/pull/471#issuecomment-1032731167 @markt-asf If you don't object, I will tomorrow and merge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 opened a new pull request #471: Add support for additional user attributes to TomcatPrincipal (10.0.x)
cklein05 opened a new pull request #471: URL: https://github.com/apache/tomcat/pull/471 @michael-o Here's the 10.0.x backport. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch main updated: Add support for additional user attributes in TomcatPrincipal
This is an automated email from the ASF dual-hosted git repository.
michaelo pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new c3edf43 Add support for additional user attributes in TomcatPrincipal
c3edf43 is described below
commit c3edf437da20af0f11edc0ad6d893399b01e6287
Author: Carsten Klein
AuthorDate: Wed Jan 12 15:06:42 2022 +0100
Add support for additional user attributes in TomcatPrincipal
This closes #463
---
java/org/apache/catalina/TomcatPrincipal.java | 28 +
.../apache/catalina/realm/GenericPrincipal.java| 46 ++
java/org/apache/catalina/realm/JNDIRealm.java | 2 +-
webapps/docs/changelog.xml | 5 +++
webapps/examples/jsp/security/protected/index.jsp | 44 +
5 files changed, 117 insertions(+), 8 deletions(-)
diff --git a/java/org/apache/catalina/TomcatPrincipal.java
b/java/org/apache/catalina/TomcatPrincipal.java
index 83f9035..1e3d9f6 100644
--- a/java/org/apache/catalina/TomcatPrincipal.java
+++ b/java/org/apache/catalina/TomcatPrincipal.java
@@ -17,6 +17,8 @@
package org.apache.catalina;
import java.security.Principal;
+import java.util.Collections;
+import java.util.Enumeration;
import org.ietf.jgss.GSSCredential;
@@ -47,4 +49,30 @@ public interface TomcatPrincipal extends Principal {
* exception to LoginContext
*/
void logout() throws Exception;
+
+/**
+ * Returns the value of the named attribute as an Object, or
+ * null if no attribute of the given name exists, or if
+ * null has been specified as the attribute's name.
+ *
+ * Only the servlet container may set attributes to make available custom
+ * information about a Principal or the user it represents.
+ *
+ * @param name a String specifying the name of the attribute
+ * @return an Object containing the value of the attribute, or
+ * null if the attribute does not exist, or if
+ * null has been specified as the attribute's name
+ */
+Object getAttribute(String name);
+
+/**
+ * Returns an Enumeration containing the names of the
+ * attributes available to this Principal. This method returns an empty
+ * Enumeration if the Principal has no attributes available to
+ * it.
+ *
+ * @return an Enumeration of strings containing the names of
+ * the Principal's attributes
+ */
+Enumeration getAttributeNames();
}
diff --git a/java/org/apache/catalina/realm/GenericPrincipal.java
b/java/org/apache/catalina/realm/GenericPrincipal.java
index 7260da4..584c104 100644
--- a/java/org/apache/catalina/realm/GenericPrincipal.java
+++ b/java/org/apache/catalina/realm/GenericPrincipal.java
@@ -19,7 +19,10 @@ package org.apache.catalina.realm;
import java.io.Serializable;
import java.security.Principal;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.Enumeration;
import java.util.List;
+import java.util.Map;
import javax.security.auth.login.LoginContext;
@@ -120,7 +123,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
*/
public GenericPrincipal(String name, List roles,
Principal userPrincipal, LoginContext loginContext) {
-this(name, roles, userPrincipal, loginContext, null);
+this(name, roles, userPrincipal, loginContext, null, null);
}
/**
@@ -140,7 +143,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
@Deprecated
public GenericPrincipal(String name, String password, List roles,
Principal userPrincipal, LoginContext loginContext) {
-this(name, roles, userPrincipal, loginContext, null);
+this(name, roles, userPrincipal, loginContext, null, null);
}
/**
@@ -154,10 +157,12 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
* @param loginContext - If provided, this will be used to log out the
user
*at the appropriate time
* @param gssCredential - If provided, the user's delegated credentials
+ * @param attributes - If provided, additional attributes associated with
+ *this Principal
*/
public GenericPrincipal(String name, List roles,
Principal userPrincipal, LoginContext loginContext,
-GSSCredential gssCredential) {
+GSSCredential gssCredential, Map attributes) {
super();
this.name = name;
this.userPrincipal = userPrincipal;
@@ -171,6 +176,7 @@ public class GenericPrincipal implements TomcatPrincipal,
Serializable {
}
this.loginContext = loginContext;
this.gssCredential = gssCredential;
+this.attributes = attributes != null ?
Collections.unmodifiableMap(attributes) : null
[GitHub] [tomcat] michael-o commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1032593680 @cklein05 Please provide backport PRs for 10.0.x and 9.0.x with default methods. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o closed pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o closed pull request #463: URL: https://github.com/apache/tomcat/pull/463 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-103160 @michael-o Should be fine now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031606851 > @michael-o Will provide a fix for _Travis CI failed_ ASAP. Thanks, will then run it locally. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031603301 @michael-o Will provide a fix for _Travis CI failed_ ASAP. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031481006 Removed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031454181 @cklein05 The default methods are on not required on main since it is still bleeding edge, we can break API here. Default methods are required for backports to 9 and 10 only. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031450526 Done. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031439345 > I'm not talking about removing methods from the interface. In the 2nd paragraph of this comment I was asking whether to include the additions to Tomcat's example JSP application located under `/examples/jsp/security/protected`. The modified example application supports showing any additional user attributes of the authenticated Principal. My bad, yes. Please reuse the example as well. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031432651 I'm not talking about removing methods from the interface. In the 2nd paragraph of this comment I was asking whether to include the additions to Tomcat's example JSP application located under `/examples/jsp/security/protected`. The modified example application supports showing any additional user attributes of the authenticated Principal. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o edited a comment on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o edited a comment on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031428724 > What about the example JSP application? Shouldn't we add support for dumping user attributes with this commit? See [#463 (comment)](https://github.com/apache/tomcat/pull/463#issuecomment-1015511489) (2nd paragraph) Personally, I wouldn't do this. This will limit the semantics of your PR, taking away to work with interface only. My [custom principal](https://michael-o.github.io/tomcatspnegoad/apidocs/net/sf/michaelo/tomcat/realm/ActiveDirectoryPrincipal.html) uses this one as well so Tomcat can internally use the provided methods otherwise it feels foreign to Tomcat. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031428724 > What about the example JSP application? Shouldn't we add support for dumping user attributes with this commit? See [#463 (comment)](https://github.com/apache/tomcat/pull/463#issuecomment-1015511489) (2nd paragraph) Personally, I wouldn't do this. This will limit the semantics of your PR, taking away to work with interface only. My custom principal uses this one as well so Tomcat can internally use the provided methods otherwise it feels foreign to Tomcat. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031426532 What about the example JSP application? Shouldn't we add support for dumping user attributes with this commit? See https://github.com/apache/tomcat/pull/463#issuecomment-1015511489 (2nd paragraph) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1031419530 @cklein05 Please apply all requested changes. I want to merge with main and proceed with Tomcat 9+ as @markt-asf has written. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1017395345 > If we limit the back-port to Tomcat 9+, we are on Java 8 and can use default methods to maintain compatibility. I guess, that's the way to go. Shall I implement these in my branch already? However, I will commit all changes only after we've agreed on all open questions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
markt-asf commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1017381686 If we limit the back-port to Tomcat 9+, we are on Java 8 and can use default methods to maintain compatibility. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1015511489 We could always remove the new _attributes_ methods from the `TomcatPricipal` interface and work with class `GenericPrincipal` only. In other words, having these methods declared on the interface is not crucial for the enhancement. In that case, we just need to update the `instanceof` check in the modified JSP example at /examples/jsp/security/protected as well as the documentation (and example code) under _Common Features / Additional User Attributes_ in realm-howto.xml (both not yet committed). BTW, shouldn't we commit/merge the example mentioned above with this PR? Listing the Principal's additional attributes is completely independent from the Realm implementations. The same is likely true for the new _Common Features / Additional User Attributes_ section in realm-howto.xml. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-101540 > > Is this save to be backported to previous Tomcat versions? > > It should be. I do not see any reasons why this cannot be backported. The big question is here is whether the `TomcatPrincipal` is truly limited to our Realms and not intended to be used in custom Realms. Because if custom Realm will use this interface a mere Tomcat will break their implementations. @markt-asf Can we make this assumption that this is Tomcat private? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on a change in pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on a change in pull request #463: URL: https://github.com/apache/tomcat/pull/463#discussion_r783661408 ## File path: java/org/apache/catalina/TomcatPrincipal.java ## @@ -47,4 +48,37 @@ * exception to LoginContext */ void logout() throws Exception; + +/** + * Returns the value of the named attribute as an Object, or + * null if no attribute of the given name exists, or if + * null has been specified as the attribute's name. + * + * Only the servlet container may set attributes to make available custom + * information about a Principal or the user it represents. For example, some of + * the Realm implementations can be configured to additionally query user + * attributes from the user database, which then are provided through the + * Principal's attributes map. + * + * Attribute names and naming conventions are maintained by the Tomcat + * components that contribute to this map, like some of the Realm + * implementations. Review comment: In other words, when querying an attribute named "displayName", a Realm could add this under a key like "realm.displayName" or "user.displayName". For that, the (maybe custom) Realm may have a config option `userAttributePrefix` or this could even be hard-coded. Such prefixes might make sense, if, in the future, for example, several components could contribute to the Principal's attributes (like the Authenticator or a new component `SocialUserDataProvider` querying user information from Facebook etc.). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on pull request #463: URL: https://github.com/apache/tomcat/pull/463#issuecomment-1011828970 > Is this save to be backported to previous Tomcat versions? It should be. I do not see any reasons why this cannot be backported. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on a change in pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on a change in pull request #463: URL: https://github.com/apache/tomcat/pull/463#discussion_r783215387 ## File path: java/org/apache/catalina/realm/GenericPrincipal.java ## @@ -171,6 +176,7 @@ public GenericPrincipal(String name, List roles, } this.loginContext = loginContext; this.gssCredential = gssCredential; +this.attributes = attributes; Review comment: Class `java.util.Collections.UnmodifiableMap` is private, so we cannot check instance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on a change in pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on a change in pull request #463: URL: https://github.com/apache/tomcat/pull/463#discussion_r783208291 ## File path: java/org/apache/catalina/TomcatPrincipal.java ## @@ -47,4 +48,37 @@ * exception to LoginContext */ void logout() throws Exception; + +/** + * Returns the value of the named attribute as an Object, or + * null if no attribute of the given name exists, or if + * null has been specified as the attribute's name. + * + * Only the servlet container may set attributes to make available custom + * information about a Principal or the user it represents. For example, some of Review comment: Will remove that. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on a change in pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on a change in pull request #463: URL: https://github.com/apache/tomcat/pull/463#discussion_r783207980 ## File path: java/org/apache/catalina/TomcatPrincipal.java ## @@ -47,4 +48,37 @@ * exception to LoginContext */ void logout() throws Exception; + +/** + * Returns the value of the named attribute as an Object, or + * null if no attribute of the given name exists, or if + * null has been specified as the attribute's name. + * + * Only the servlet container may set attributes to make available custom + * information about a Principal or the user it represents. For example, some of + * the Realm implementations can be configured to additionally query user + * attributes from the user database, which then are provided through the + * Principal's attributes map. + * + * Attribute names and naming conventions are maintained by the Tomcat + * components that contribute to this map, like some of the Realm + * implementations. Review comment: Don't know whether it's really the exact word. However, every attribute provider (aka Realm) can use it's own naming schema and conventions. Some may use prefixes (user.displayName) others may not, who knows. We make no rules or assumptions here but only refer to the each attribute provider's documentation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on a change in pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on a change in pull request #463: URL: https://github.com/apache/tomcat/pull/463#discussion_r783204725 ## File path: java/org/apache/catalina/realm/GenericPrincipal.java ## @@ -171,6 +176,7 @@ public GenericPrincipal(String name, List roles, } this.loginContext = loginContext; this.gssCredential = gssCredential; +this.attributes = attributes; Review comment: AFAIK we agreed on not using an unmodifiable map, since there is no write access to this map. However, we could, of course, if you like so. Maybe it's worth checking if it's already an instance of `java.util.Collections.UnmodifiableMap` first. If not, we could wrap it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 commented on a change in pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 commented on a change in pull request #463:
URL: https://github.com/apache/tomcat/pull/463#discussion_r783200198
##
File path: java/org/apache/catalina/realm/GenericPrincipal.java
##
@@ -283,10 +294,16 @@ public boolean hasRole(String role) {
@Override
public String toString() {
StringBuilder sb = new StringBuilder("GenericPrincipal[");
+boolean first = true;
sb.append(this.name);
sb.append('(');
for (String role : roles) {
-sb.append(role).append(',');
+if (first) {
+first = false;
+} else {
+sb.append(',');
+}
+sb.append(role);
Review comment:
Will remove change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on a change in pull request #463: Add support for additional user attributes to TomcatPrincipal
michael-o commented on a change in pull request #463:
URL: https://github.com/apache/tomcat/pull/463#discussion_r783189035
##
File path: java/org/apache/catalina/realm/GenericPrincipal.java
##
@@ -171,6 +176,7 @@ public GenericPrincipal(String name, List roles,
}
this.loginContext = loginContext;
this.gssCredential = gssCredential;
+this.attributes = attributes;
Review comment:
Should we wrap with an unmodifiable map or do we expect the caller to do
this?
##
File path: java/org/apache/catalina/TomcatPrincipal.java
##
@@ -47,4 +48,37 @@
* exception to LoginContext
*/
void logout() throws Exception;
+
+/**
+ * Returns the value of the named attribute as an Object, or
+ * null if no attribute of the given name exists, or if
+ * null has been specified as the attribute's name.
+ *
+ * Only the servlet container may set attributes to make available custom
+ * information about a Principal or the user it represents. For example,
some of
Review comment:
I think the For example is not ncessary, since we don't want to incline
to any actual implementation.
##
File path: java/org/apache/catalina/realm/GenericPrincipal.java
##
@@ -283,10 +294,16 @@ public boolean hasRole(String role) {
@Override
public String toString() {
StringBuilder sb = new StringBuilder("GenericPrincipal[");
+boolean first = true;
sb.append(this.name);
sb.append('(');
for (String role : roles) {
-sb.append(role).append(',');
+if (first) {
+first = false;
+} else {
+sb.append(',');
+}
+sb.append(role);
Review comment:
While this is correct, it should be in separate PR.
##
File path: java/org/apache/catalina/TomcatPrincipal.java
##
@@ -47,4 +48,37 @@
* exception to LoginContext
*/
void logout() throws Exception;
+
+/**
+ * Returns the value of the named attribute as an Object, or
+ * null if no attribute of the given name exists, or if
+ * null has been specified as the attribute's name.
+ *
+ * Only the servlet container may set attributes to make available custom
+ * information about a Principal or the user it represents. For example,
some of
+ * the Realm implementations can be configured to additionally query user
+ * attributes from the user database, which then are provided
through the
+ * Principal's attributes map.
+ *
+ * Attribute names and naming conventions are maintained by the Tomcat
+ * components that contribute to this map, like some of the Realm
+ * implementations.
Review comment:
Are those really maintained? I thought the dev/admin requests the realm
to load attribute values. So the attribute names are not necessarily mandated?!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] cklein05 opened a new pull request #463: Add support for additional user attributes to TomcatPrincipal
cklein05 opened a new pull request #463: URL: https://github.com/apache/tomcat/pull/463 Add support for additional user attributes to the `TomcatPrincipal` interface and the `GenericPrincipal` class. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
