On Thu, Feb 10, 2022 at 1:01 PM Rémy Maucherat <[email protected]> wrote:
>
> On Tue, Feb 8, 2022 at 2:13 PM <[email protected]> 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 <[email protected]>
> > 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 <code>Object</code>,
> > or
> > + * <code>null</code> if no attribute of the given name exists, or if
> > + * <code>null</code> has been specified as the attribute's name.
> > + * <p>
> > + * Only the servlet container may set attributes to make available
> > custom
> > + * information about a Principal or the user it represents.
> > + *
> > + * @param name a <code>String</code> specifying the name of the
> > attribute
> > + * @return an <code>Object</code> containing the value of the
> > attribute, or
> > + * <code>null</code> if the attribute does not exist, or if
> > + * <code>null</code> has been specified as the attribute's name
> > + */
> > + Object getAttribute(String name);
> > +
> > + /**
> > + * Returns an <code>Enumeration</code> containing the names of the
> > + * attributes available to this Principal. This method returns an empty
> > + * <code>Enumeration</code> if the Principal has no attributes
> > available to
> > + * it.
> > + *
> > + * @return an <code>Enumeration</code> of strings containing the names
> > of
> > + * the Principal's attributes
> > + */
> > + Enumeration<String> 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<String> 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<String>
> > 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<String> roles,
> > Principal userPrincipal, LoginContext loginContext,
> > - GSSCredential gssCredential) {
> > + GSSCredential gssCredential, Map<String, Object> 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;
> > }
> >
> >
> > @@ -193,7 +199,7 @@ public class GenericPrincipal implements
> > TomcatPrincipal, Serializable {
> > public GenericPrincipal(String name, String password, List<String>
> > roles,
> > Principal userPrincipal, LoginContext loginContext,
> > GSSCredential gssCredential) {
> > - this(name, roles, userPrincipal, loginContext, gssCredential);
> > + this(name, roles, userPrincipal, loginContext, gssCredential,
> > null);
> > }
> >
> >
> > @@ -254,6 +260,11 @@ public class GenericPrincipal implements
> > TomcatPrincipal, Serializable {
> > this.gssCredential = gssCredential;
> > }
> >
> > + /**
> > + * The additional attributes associated with this Principal.
> > + */
> > + protected final Map<String, Object> attributes;
> > +
> >
> > // ---------------------------------------------------------- Public
> > Methods
> >
> > @@ -304,10 +315,28 @@ public class GenericPrincipal implements
> > TomcatPrincipal, Serializable {
> > }
> >
> >
> > + @Override
> > + public Object getAttribute(String name) {
> > + if (attributes == null || name == null) {
> > + return null;
> > + }
> > + return attributes.get(name);
> > + }
> > +
> > +
> > + @Override
> > + public Enumeration<String> getAttributeNames() {
> > + if (attributes == null) {
> > + return Collections.emptyEnumeration();
> > + }
> > + return Collections.enumeration(attributes.keySet());
> > + }
> > +
> > +
> > // -----------------------------------------------------------
> > Serialization
> >
> > private Object writeReplace() {
> > - return new SerializablePrincipal(name, roles, userPrincipal);
> > + return new SerializablePrincipal(name, roles, userPrincipal,
> > attributes);
> > }
> >
> > private static class SerializablePrincipal implements Serializable {
> > @@ -316,9 +345,10 @@ public class GenericPrincipal implements
> > TomcatPrincipal, Serializable {
> > private final String name;
> > private final String[] roles;
> > private final Principal principal;
> > + private final Map<String, Object> attributes;
> >
> > public SerializablePrincipal(String name, String[] roles,
> > - Principal principal) {
> > + Principal principal, Map<String, Object> attributes) {
> > this.name = name;
> > this.roles = roles;
> > if (principal instanceof Serializable) {
> > @@ -326,10 +356,12 @@ public class GenericPrincipal implements
> > TomcatPrincipal, Serializable {
> > } else {
> > this.principal = null;
> > }
> > + this.attributes = attributes;
> > }
> >
> > private Object readResolve() {
> > - return new GenericPrincipal(name, Arrays.asList(roles),
> > principal);
> > + return new GenericPrincipal(name, Arrays.asList(roles),
> > principal, null, null,
> > + attributes);
> > }
> > }
> > }
> > diff --git a/java/org/apache/catalina/realm/JNDIRealm.java
> > b/java/org/apache/catalina/realm/JNDIRealm.java
> > index a9dac4a..e971439 100644
> > --- a/java/org/apache/catalina/realm/JNDIRealm.java
> > +++ b/java/org/apache/catalina/realm/JNDIRealm.java
> > @@ -2516,7 +2516,7 @@ public class JNDIRealm extends RealmBase {
> > }
> >
> > if (user != null) {
> > - return new GenericPrincipal(user.getUserName(), roles, null,
> > null, gssCredential);
> > + return new GenericPrincipal(user.getUserName(), roles, null,
> > null, gssCredential, null);
> > }
> >
> > return null;
> > diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> > index 3a22409..bd2a124 100644
> > --- a/webapps/docs/changelog.xml
> > +++ b/webapps/docs/changelog.xml
> > @@ -123,6 +123,11 @@
> > is not supported by the configured providers as will be the case
> > for a
> > FIPS compliant configuration. (markt)
> > </fix>
> > + <add>
> > + <pr>463</pr>: Add support for additional user attributes to
> > + <code>TomcatPrincipal</code> and <code>GenericPrincipal</code>.
> > + Patch provided by Carsten Klein. (michaelo)
> > + </add>
> > <fix>
> > <pr>469</pr>: Include the Jakarata Annotations API in the classes
> > that
> > Tomcat will not load from web applications. Pull request provided
> > by
> > diff --git a/webapps/examples/jsp/security/protected/index.jsp
> > b/webapps/examples/jsp/security/protected/index.jsp
> > index 31122eb..75ac4bc 100644
> > --- a/webapps/examples/jsp/security/protected/index.jsp
> > +++ b/webapps/examples/jsp/security/protected/index.jsp
> > @@ -15,6 +15,8 @@
> > limitations under the License.
> > --%>
> > <%@ page import="java.util.Enumeration" %>
> > +<%@ page import="java.security.Principal" %>
> > +<%@ page import="org.apache.catalina.TomcatPrincipal" %>
> > <%
> > if (request.getParameter("logoff") != null) {
> > session.invalidate();
> > @@ -73,6 +75,48 @@ enter it here:
> > </form>
> > <br><br>
> >
> > +<%
> > + Principal p = request.getUserPrincipal();
> > + if (!(p instanceof TomcatPrincipal)) {
> > +%>
> > +<p>The principal does not support attributes.</p>
> > +<%
> > + } else {
> > + TomcatPrincipal principal = (TomcatPrincipal) p;
> > +%>
> > +<p>The principal contains the following attributes:</p>
> > +<table>
> > +<tr><th>Name</th><th>Value</th><th>Type</th></tr>
> > +<%
> > + Enumeration<String> names = principal.getAttributeNames();
> > + while (names.hasMoreElements()) {
> > + String name = names.nextElement();
> > + Object value = principal.getAttribute(name);
> > + String type = value != null ? value.getClass().getName() : "unknown";
> > + if (value instanceof Object[]) {
> > + Object[] values = (Object[]) value;
> > + value = "";
> > + for (int i = 0; i < values.length; i++) {
> > + value += values[i] + "<br/>";
> > + }
> > + if (values.length > 0) {
> > + type = values[0].getClass().getName() + "[]";
> > + } else {
> > + type = "unknown";
> > + }
> > + }
> > + type = type.replaceFirst("^java\\.lang\\.", "");
> > +%>
> > +<tr><td><%= name %></td><td><%= value %></td><td><%= type %></td>
> > +<%
> > + }
> > +%>
> > +</table>
> > +<%
> > + }
> > +%>
> > +<br><br>
> > +
> > To add some data to the authenticated session, enter it here:
> > <form method="GET" action='<%= response.encodeURL("index.jsp") %>'>
> > <input type="text" name="dataName">
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [email protected]
> > For additional commands, e-mail: [email protected]
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
