michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655173166
##########
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 return `null`? Is is necessary for string concat?
##########
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:
Agreed.
##########
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:
I have no strong opinion about it. I cannot also judge about the
overhead when there are high load. Maybe Mark and others can share their
opinion.
##########
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:
BTW, such a "static" string builder gives you zero benefit compared over
`+=`. You can also use `+=` and the compliler will transform to string builder
automatically. String builders make sense when you have dynamic data like loops.
##########
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:
Not array-typed columns. A query might return more than one line per
user attribute which would actually override all previous ones with the last
one. I don't expect support for this here, but just like to mention it. If you
don't want to support that is perfectly fine, but it should simply be
documented that's it.
##########
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:
> What other types of multi-valued attributes are you thinking of?
I haven't use SQL databases for any user store for the last 15 years, but
only Active Directory so I am used to multi-valued values.
##########
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:
That's fine.
##########
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:
OK, I see. It is a decent tradeoff.
##########
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:
Hmm...hard nut to crack. At the end the developer decides which
attributes are required to expose to this map. Maybe I am lacking the
expertise, but using reflection is a quite constructed case given that Java 16+
is getting stricter. I still think that one does not need copies of immutable
objects because what you at most can modify is the in-memory representation of
an attribute, but never in the user store. I might be wrong. Gürtel und
Hosenträger!
##########
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:
I see it now. Looked like a bug from first glance.
##########
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:
Yes, that's a problem. I agree. Since I cannot provide anything better I
accept this.
##########
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:
Yes, I completely forgot the row orientation on the database. No need to
add support since this would require a schema change. Don't bother, you already
did a lot.
--
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
