On Tue, Feb 8, 2022 at 2:13 PM <micha...@apache.org> 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 <c.kl...@datagis.com>
> 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 <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: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
