michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654963957
##########
File path: java/org/apache/catalina/TomcatPrincipal.java
##########
@@ -47,4 +48,58 @@
* 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. May also
+ * return <code>null</code>, if the named attribute exists but cannot be
+ * returned in a way that ensures that changes made to the returned object
+ * are not reflected by objects returned by subsequent calls to this
method.
+ * <p>
+ * 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 <i>user database</i>, which then are provided
+ * through the Principal's attributes map.
+ * <p>
+ * In order to keep the attributes map <i>immutable</i>, the objects
+ * returned by this method should always be <i>defensive copies</i> of the
+ * objects contained in the attributes map. Any changes applied to these
+ * objects must not be reflected by objects returned by subsequent calls to
+ * this method. If that cannot be guaranteed (e. g. there is no way to copy
+ * the object), the object's string representation (or even
+ * <code>null</code>) shall be returned.
+ * <p>
+ * Attribute names and naming conventions are maintained by the Tomcat
+ * components that contribute to this map, like some of the Realm
+ * implementations.
+ *
+ * @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 the
+ * object's string representation or <code>null</code> if its value
+ * cannot be copied in order to keep the attributes immutable
+ */
+ 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();
Review comment:
Is the `Enumeration` still used these days in new APIs?
##########
File path: java/org/apache/catalina/realm/GenericPrincipal.java
##########
@@ -304,10 +333,165 @@ public void logout() throws Exception {
}
+ @Override
+ public Object getAttribute(String name) {
+ if (attributes == null) {
+ return null;
+ }
+ Object value = attributes.get(name);
+ if (value == null) {
+ return null;
+ }
+ Object copy = copyObject(value);
+ return copy != null ? copy : value.toString();
+ }
+
+
+ @Override
+ public Enumeration<String> getAttributeNames() {
+ if (attributes == null) {
+ return Collections.emptyEnumeration();
+ }
+ return Collections.enumeration(attributes.keySet());
+ }
+
+
+ @Override
+ public boolean isAttributesCaseIgnored() {
+ return (attributes instanceof CaseInsensitiveKeyMap<?>);
+ }
+
+
+ /**
+ * Creates and returns a deep copy of the specified object. Deep-copying
+ * works only for objects of a couple of <i>hard coded</i> types or, if the
+ * object implements <code>java.io.Serializable</code>. In all other cases,
+ * this method returns <code>null</code>.
+ *
+ * @param obj the object to copy
+ *
+ * @return a deep copied clone of the specified object, or
<code>null</code>
+ * if deep-copying was not possible
+ */
+ private Object copyObject(Object obj) {
+
+ // first, try some commonly used object types
+ if (obj instanceof String) {
+ return new String((String) obj);
+
+ } else if (obj instanceof Integer) {
+ return Integer.valueOf((int) obj);
+
+ } else if (obj instanceof Long) {
+ return Long.valueOf((long) obj);
+
+ } else if (obj instanceof Boolean) {
+ return Boolean.valueOf((boolean) obj);
+
+ } else if (obj instanceof Double) {
+ return Double.valueOf((double) obj);
+
+ } else if (obj instanceof Float) {
+ return Float.valueOf((float) obj);
+
+ } else if (obj instanceof Character) {
+ return Character.valueOf((char) obj);
+
+ } else if (obj instanceof Byte) {
+ return Byte.valueOf((byte) obj);
+
+ } else if (obj instanceof Short) {
+ return Short.valueOf((short) obj);
+
+ } else if (obj instanceof BigDecimal) {
+ return new BigDecimal(((BigDecimal) obj).toString());
+
+ } else if (obj instanceof BigInteger) {
+ return new BigInteger(((BigInteger) obj).toString());
+
+ }
+
+ // Date and JDBC date and time types
+ else if (obj instanceof java.sql.Date) {
+ return ((java.sql.Date) obj).clone();
+
+ } else if (obj instanceof java.sql.Time) {
+ return ((java.sql.Time) obj).clone();
+
+ } else if (obj instanceof java.sql.Timestamp) {
+ return ((java.sql.Timestamp) obj).clone();
+
+ } else if (obj instanceof Date) {
+ return ((Date) obj).clone();
+
+ }
+
+ // these types may come up as well
+ else if (obj instanceof URI) {
+ try {
+ return new URI(((URI) obj).toString());
+ } catch (URISyntaxException e) {
+ // not expected
+ }
+ } else if (obj instanceof URL) {
+ try {
+ return new URI(((URL) obj).toString());
+ } catch (URISyntaxException e) {
+ // not expected
+ }
+ } else if (obj instanceof UUID) {
+ return new UUID(((UUID) obj).getMostSignificantBits(),
+ ((UUID) obj).getLeastSignificantBits());
+
+ }
Review comment:
Why?
Most of the are immutable anyway. Why not check for `Cloneable`?
##########
File path:
java/org/apache/tomcat/util/collections/CaseInsensitiveKeyLinkedMap.java
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomcat.util.collections;
+
+import java.util.LinkedHashMap;
+import java.util.Locale;
+
+/**
+ * A Map implementation that uses case-insensitive (using
+ * {@link Locale#ENGLISH}) strings as keys. This class uses a
+ * <code>LinkedHashMap</code> as backing store. So this Map's behavior, except
+ * for case-insensitive key handling, is comparable with
+ * <code>LinkedHashMap</code>. Most notably is its predictable iteration order.
+ * <p>
+ * Keys must be instances of {@link String}. Note that this means that
+ * <code>null</code> keys are not permitted.
+ * <p>
+ * This implementation is not thread-safe.
+ *
+ * @param <V> Type of values placed in this Map.
+ * @see LinkedHashMap
+ */
+public class CaseInsensitiveKeyLinkedMap<V> extends CaseInsensitiveKeyMap<V> {
Review comment:
I think this is redudant, but solely my opinion.
##########
File path: java/org/apache/catalina/TomcatPrincipal.java
##########
@@ -47,4 +48,58 @@
* 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. May also
+ * return <code>null</code>, if the named attribute exists but cannot be
+ * returned in a way that ensures that changes made to the returned object
+ * are not reflected by objects returned by subsequent calls to this
method.
+ * <p>
+ * 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 <i>user database</i>, which then are provided
+ * through the Principal's attributes map.
+ * <p>
+ * In order to keep the attributes map <i>immutable</i>, the objects
+ * returned by this method should always be <i>defensive copies</i> of the
+ * objects contained in the attributes map. Any changes applied to these
+ * objects must not be reflected by objects returned by subsequent calls to
+ * this method. If that cannot be guaranteed (e. g. there is no way to copy
+ * the object), the object's string representation (or even
+ * <code>null</code>) shall be returned.
+ * <p>
+ * Attribute names and naming conventions are maintained by the Tomcat
+ * components that contribute to this map, like some of the Realm
+ * implementations.
+ *
+ * @param name a <code>String</code> specifying the name of the attribute
Review comment:
You should specify that the name must not be null and will throw NPE.
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -572,6 +801,21 @@ protected void startInternal() throws LifecycleException {
temp.append(" = ?");
preparedCredentials = temp.toString();
+ // Create the user attributes PreparedStatement string (only its tail
w/o SELECT
+ // clause)
+ temp = new StringBuilder(" FROM ");
+ temp.append(userTable);
+ temp.append(" WHERE ");
+ temp.append(userNameCol);
+ temp.append(" = ?");
+ preparedAttributesTail = temp.toString();
+
+ // Create the available user attributes PreparedStatement string
+ temp = new StringBuilder("SELECT * FROM ");
+ temp.append(userTable);
+ temp.append(" WHERE FALSE");
Review comment:
Who guarantees that `false` is portable across databases?
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -510,21 +582,22 @@ protected Principal getPrincipal(String username) {
return null;
}
- ArrayList<String> list = null;
+ // Using a Set removes duplicate roles
+ Set<String> roles = null;
try (PreparedStatement stmt =
dbConnection.prepareStatement(preparedRoles)) {
stmt.setString(1, username);
try (ResultSet rs = stmt.executeQuery()) {
- list = new ArrayList<>();
+ roles = new HashSet<>();
Review comment:
Don't change this here. It has nothing to do with the new feature.
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -223,6 +266,34 @@ public void setUserTable( String userTable ) {
this.userTable = userTable;
}
+ /**
+ * @return the comma separated names of user attributes to additionally
query
+ * from the user table
+ */
+ public String getUserAttributes() {
Review comment:
Why not a `String[]` or `Collection<String>`?
##########
File path: java/org/apache/catalina/TomcatPrincipal.java
##########
@@ -47,4 +48,58 @@
* 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. May also
+ * return <code>null</code>, if the named attribute exists but cannot be
+ * returned in a way that ensures that changes made to the returned object
+ * are not reflected by objects returned by subsequent calls to this
method.
+ * <p>
+ * 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 <i>user database</i>, which then are provided
+ * through the Principal's attributes map.
+ * <p>
+ * In order to keep the attributes map <i>immutable</i>, the objects
+ * returned by this method should always be <i>defensive copies</i> of the
+ * objects contained in the attributes map. Any changes applied to these
+ * objects must not be reflected by objects returned by subsequent calls to
+ * this method. If that cannot be guaranteed (e. g. there is no way to copy
+ * the object), the object's string representation (or even
+ * <code>null</code>) shall be returned.
+ * <p>
+ * Attribute names and naming conventions are maintained by the Tomcat
+ * components that contribute to this map, like some of the Realm
+ * implementations.
+ *
+ * @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 the
+ * object's string representation or <code>null</code> if its value
+ * cannot be copied in order to keep the attributes immutable
+ */
+ 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();
+
+ /**
+ * Determines whether attribute names are case-insensitive. May be
+ * <code>true</code> if using <em>JNDIRealm</em> and then, depends on the
+ * configured directory server.
+ *
+ * @return <code>true</code>, if attribute names are case-insensitive;
+ * <code>false</code> otherwise
+ */
+ boolean isAttributesCaseIgnored();
Review comment:
I would drop this since you modeled it after `ServletRequest` and alike,
they do not offer case-insensitive attributes. This should be completely at the
descretion of an implementation.
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -107,6 +134,22 @@
private volatile boolean connectionSuccess = true;
+ /**
+ * The comma separated names of user attributes to additionally query from
the
+ * user table. These will be provided to the user through the created
+ * Principal's <i>attributes</i> map.
+ */
+ protected String userAttributes;
Review comment:
Most implementations use an array internally and do a split in the
setter.
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -539,6 +612,162 @@ private boolean isRoleStoreDefined() {
}
+ /**
+ * Return the specified user's requested user attributes as a map.
+ *
+ * @param dbConnection The database connection to be used
+ * @param username User name for which to return user attributes
+ *
+ * @return a map containing the specified user's requested user attributes
+ */
+ protected Map<String, Object> getUserAttributesMap(Connection
dbConnection, String username) {
+
+ String preparedAttributes = getUserAttributesStatement(dbConnection);
+ if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
+ // The above reference comparison is intentional.
USER_ATTRIBUTES_NONE_REQUESTED
+ // is a tag object (empty String) to distinguish between null (not
yet
+ // initialized) and empty (no attributes requested).
+ // TODO Could as well be changed to `preparedAttributes.lenghth()
= 0`
+
+ // Return null if no user attributes are requested (or if the
statement was not
+ // yet built successfully)
+ return null;
+ }
+
+ try (PreparedStatement stmt =
dbConnection.prepareStatement(preparedAttributes)) {
+ stmt.setString(1, username);
+
+ try (ResultSet rs = stmt.executeQuery()) {
+
+ if (rs.next()) {
+ Map<String, Object> attrs = new LinkedHashMap<>();
Review comment:
Why a linked hash map? Does it matter to the client to retain the order?
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -539,6 +612,162 @@ private boolean isRoleStoreDefined() {
}
+ /**
+ * Return the specified user's requested user attributes as a map.
+ *
+ * @param dbConnection The database connection to be used
+ * @param username User name for which to return user attributes
+ *
+ * @return a map containing the specified user's requested user attributes
+ */
+ protected Map<String, Object> getUserAttributesMap(Connection
dbConnection, String username) {
+
+ String preparedAttributes = getUserAttributesStatement(dbConnection);
+ if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
Review comment:
Why not `preparedAttributes.isEmpty()`?
##########
File path: java/org/apache/catalina/realm/MemoryRealm.java
##########
@@ -47,6 +51,18 @@
private static final Log log = LogFactory.getLog(MemoryRealm.class);
+ /**
+ * Contains the names of all user attributes available for this Realm.
+ */
+ private static final List<String> USER_ATTRIBUTES_AVAILABLE =
+ new ArrayList<>(Arrays.asList("username", "fullname", "roles"));
Review comment:
Why the `new` wrapper?
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -539,6 +612,162 @@ private boolean isRoleStoreDefined() {
}
+ /**
+ * Return the specified user's requested user attributes as a map.
+ *
+ * @param dbConnection The database connection to be used
+ * @param username User name for which to return user attributes
+ *
+ * @return a map containing the specified user's requested user attributes
+ */
+ protected Map<String, Object> getUserAttributesMap(Connection
dbConnection, String username) {
+
+ String preparedAttributes = getUserAttributesStatement(dbConnection);
+ if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
+ // The above reference comparison is intentional.
USER_ATTRIBUTES_NONE_REQUESTED
+ // is a tag object (empty String) to distinguish between null (not
yet
+ // initialized) and empty (no attributes requested).
+ // TODO Could as well be changed to `preparedAttributes.lenghth()
= 0`
+
+ // Return null if no user attributes are requested (or if the
statement was not
+ // yet built successfully)
+ return null;
+ }
+
+ try (PreparedStatement stmt =
dbConnection.prepareStatement(preparedAttributes)) {
+ stmt.setString(1, username);
+
+ try (ResultSet rs = stmt.executeQuery()) {
+
+ if (rs.next()) {
+ Map<String, Object> attrs = new LinkedHashMap<>();
+ ResultSetMetaData md = rs.getMetaData();
+ int ncols = md.getColumnCount();
+ for (int columnIndex = 1; columnIndex <= ncols;
columnIndex++) {
+ String columnName = md.getColumnName(columnIndex);
+ // Ignore case, database may have case-insensitive
field names
+ if (columnName.equalsIgnoreCase(userCredCol)) {
+ // Always skip userCredCol (must be there if all
columns
+ // have been requested)
+ continue;
+ }
+ attrs.put(columnName, rs.getObject(columnIndex));
+ }
+ return attrs.size() > 0 ? attrs : null;
Review comment:
Some user stores may have multivalued attributes. For this realm you
should mention that it only supports single-valued ones.
##########
File path: java/org/apache/catalina/realm/GenericPrincipal.java
##########
@@ -283,10 +306,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:
Even if this a good change, it should be separate PR.
##########
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##########
@@ -539,6 +612,162 @@ private boolean isRoleStoreDefined() {
}
+ /**
+ * Return the specified user's requested user attributes as a map.
+ *
+ * @param dbConnection The database connection to be used
+ * @param username User name for which to return user attributes
+ *
+ * @return a map containing the specified user's requested user attributes
+ */
+ protected Map<String, Object> getUserAttributesMap(Connection
dbConnection, String username) {
+
+ String preparedAttributes = getUserAttributesStatement(dbConnection);
+ if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
+ // The above reference comparison is intentional.
USER_ATTRIBUTES_NONE_REQUESTED
+ // is a tag object (empty String) to distinguish between null (not
yet
+ // initialized) and empty (no attributes requested).
+ // TODO Could as well be changed to `preparedAttributes.lenghth()
= 0`
+
+ // Return null if no user attributes are requested (or if the
statement was not
+ // yet built successfully)
+ return null;
+ }
+
+ try (PreparedStatement stmt =
dbConnection.prepareStatement(preparedAttributes)) {
+ stmt.setString(1, username);
+
+ try (ResultSet rs = stmt.executeQuery()) {
+
+ if (rs.next()) {
+ Map<String, Object> attrs = new LinkedHashMap<>();
+ ResultSetMetaData md = rs.getMetaData();
+ int ncols = md.getColumnCount();
+ for (int columnIndex = 1; columnIndex <= ncols;
columnIndex++) {
+ String columnName = md.getColumnName(columnIndex);
+ // Ignore case, database may have case-insensitive
field names
+ if (columnName.equalsIgnoreCase(userCredCol)) {
+ // Always skip userCredCol (must be there if all
columns
+ // have been requested)
+ continue;
+ }
+ attrs.put(columnName, rs.getObject(columnIndex));
+ }
+ return attrs.size() > 0 ? attrs : null;
+ }
+ }
+ } catch (SQLException e) {
+ containerLog.error(
+
sm.getString("dataSourceRealm.getUserAttributes.exception", username), e);
+ }
+
+ return null;
+ }
+
+
+ /**
+ * Return the SQL statement for querying additional user attributes. The
+ * statement is lazily initialized (<i>lazily initialized singleton</i>
with
+ * <i>double-checked locking, DCL</i>) since building it may require an
extra
+ * database query under some conditions.
+ *
+ * @param dbConnection connection for accessing the database
+ */
+ private String getUserAttributesStatement(Connection dbConnection) {
+ // DCL so userAttributesStatement MUST be volatile
+ if (userAttributesStatement == null) {
+ synchronized (userAttributesStatementLock) {
+ if (userAttributesStatement == null) {
+ List<String> requestedAttributes =
parseUserAttributes(userAttributes);
+ if (requestedAttributes == null) {
+ return USER_ATTRIBUTES_NONE_REQUESTED;
+ }
+ if (requestedAttributes.size() > 0
+ &&
requestedAttributes.get(0).equals(USER_ATTRIBUTES_WILDCARD)) {
+ userAttributesStatement = "SELECT *" +
preparedAttributesTail;
Review comment:
Space after asterisk?
##########
File path: java/org/apache/catalina/realm/JNDIRealm.java
##########
@@ -1419,18 +1469,27 @@ protected User getUser(JNDIConnection connection,
String username, String creden
User user = null;
// Get attributes to retrieve from user entry
- List<String> list = new ArrayList<>();
- if (userPassword != null) {
- list.add(userPassword);
- }
- if (userRoleName != null) {
- list.add(userRoleName);
- }
- if (userRoleAttribute != null) {
- list.add(userRoleAttribute);
+ // Includes attributes required for authentication and authorization
as well as
+ // user attributes to additionally query from the user's directory
entry
+ String[] attrIds = null;
+ if (userAttributesList == null
+ ||
!userAttributesList.get(0).equals(USER_ATTRIBUTES_WILDCARD)) {
+ Set<String> list = new HashSet<>();
Review comment:
A hash set is not a list. You should use consistent collections
throughout your PR.
##########
File path: java/org/apache/catalina/realm/MemoryRealm.java
##########
@@ -47,6 +51,18 @@
private static final Log log = LogFactory.getLog(MemoryRealm.class);
+ /**
+ * Contains the names of all user attributes available for this Realm.
+ */
+ private static final List<String> USER_ATTRIBUTES_AVAILABLE =
+ new ArrayList<>(Arrays.asList("username", "fullname", "roles"));
+
+ /**
+ * Contains the names of user attributes for which access is denied.
+ */
+ private static final List<String> USER_ATTRIBUTES_ACCESS_DENIED =
+ new ArrayList<>(Arrays.asList("password"));
Review comment:
Why the `new` wrapper?
##########
File path: java/org/apache/catalina/realm/MemoryRealm.java
##########
@@ -167,23 +246,46 @@ public Principal authenticate(String username, String
credentials) {
* @param password User's password (clear text)
* @param roles Comma-delimited set of roles associated with this user
*/
- void addUser(String username, String password, String roles) {
+ void addUser(String username, String password, String roles, String
fullname) {
// Accumulate the list of roles for this user
- List<String> list = new ArrayList<>();
+ Set<String> roleSet = new LinkedHashSet<>();
roles += ",";
while (true) {
int comma = roles.indexOf(',');
if (comma < 0) {
break;
}
String role = roles.substring(0, comma).trim();
- list.add(role);
+ roleSet.add(role);
roles = roles.substring(comma + 1);
}
+ // Create the user attributes map for this user's principal
+ Map<String, Object> attributes = null;
+ if (userAttributesList != null) {
+ attributes = new LinkedHashMap<>();
+ for (String name : userAttributesList) {
+ switch (name) {
+ case "username":
+ case "name":
+ attributes.put(name, new String(username));
Review comment:
Why the unnecessary object allocation?
##########
File path: java/org/apache/catalina/realm/MemoryRealm.java
##########
@@ -167,23 +246,46 @@ public Principal authenticate(String username, String
credentials) {
* @param password User's password (clear text)
* @param roles Comma-delimited set of roles associated with this user
*/
- void addUser(String username, String password, String roles) {
+ void addUser(String username, String password, String roles, String
fullname) {
// Accumulate the list of roles for this user
- List<String> list = new ArrayList<>();
+ Set<String> roleSet = new LinkedHashSet<>();
Review comment:
Same as before, not related to this change.
##########
File path: java/org/apache/catalina/realm/MemoryRealm.java
##########
@@ -167,23 +246,46 @@ public Principal authenticate(String username, String
credentials) {
* @param password User's password (clear text)
* @param roles Comma-delimited set of roles associated with this user
*/
- void addUser(String username, String password, String roles) {
+ void addUser(String username, String password, String roles, String
fullname) {
// Accumulate the list of roles for this user
- List<String> list = new ArrayList<>();
+ Set<String> roleSet = new LinkedHashSet<>();
roles += ",";
while (true) {
int comma = roles.indexOf(',');
if (comma < 0) {
break;
}
String role = roles.substring(0, comma).trim();
- list.add(role);
+ roleSet.add(role);
roles = roles.substring(comma + 1);
}
+ // Create the user attributes map for this user's principal
+ Map<String, Object> attributes = null;
+ if (userAttributesList != null) {
+ attributes = new LinkedHashMap<>();
+ for (String name : userAttributesList) {
+ switch (name) {
+ case "username":
+ case "name":
+ attributes.put(name, new String(username));
+ break;
+
+ case "fullname":
+ attributes.put(name, new String(fullname));
+ break;
+
+ case "roles":
+ attributes.put(name, StringUtils.join(roleSet));
Review comment:
Why not retain a read-only collection?
##########
File path: java/org/apache/catalina/realm/JNDIRealm.java
##########
@@ -469,6 +474,19 @@
*/
protected boolean useContextClassLoader = true;
+ /**
+ * The comma separated names of user attributes to additionally query from
the
+ * user's directory entry. These will be provided to the user through the
+ * created Principal's <i>attributes</i> map.
+ */
+ protected String userAttributes;
Review comment:
Same as in the `DataSourceRealm`
--
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.
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
