[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655188601
##
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 getUserAttributesMap(Connection
dbConnection, String username) {
+
+String preparedAttributes = getUserAttributesStatement(dbConnection);
+if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
Review comment:
With DataSourceRealm, parsing and validating is done lazily, so `null`
means _still uninitialized_. When not using a value different from `null` for
_empty/none required_, I end with constantly parsing and validating in method
`getUserAttributesStatement`. I could have uses another boolean
`userAttributesNoneRequested`, however I like that _3-state value approach_
some more.
##
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 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 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:
I'm about to implement support for (at least) arrays, BLOBS and CLOBS.
Likely it is sufficient to support one-dimensional arrays only. Everything
_not_ supported will be documented (both in Javadoc comments as well as in
Tomcat's configuration reference).
##
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:
I know. However, the contributing guideline recommends to _copy_ the
coding style of the modified source file. That's what I did: the _roles_ and
_credentials_ statements have already been there, using the `StringBuilder`.
##
File path:
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655239478
##
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 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 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:
I'm almost done with support for SQL-Arrays, BLOBs and CLOBS. I thought
that these come with `getObject()` for free. But, a couple of lines of code are
required to get these types as well.
Arrays are a quite cool feature and provide multi-valued attribute support.
BLOBS may be quite useful to provide binary data, like images and similar stuff.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655222693
##
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 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 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:
Ah, OK, multiple records for a user... Since by definition, the
attributes are extra fields of the _user table_ (that has logon name and
password), there is generally no support for user tables that contain the user
more than once. That's just not allowed. In method `getPassword` credentials
are read from the first record returned (there's no `while (rs.hasNext())`
loop).
So generally, having a user more than once in the user table may cause
problems with attributes: in `getUserAttributesMap` the multiple rows for that
username could come in a different order. Than, the Principal will get the
wrong attributes.
We could generally deny authenticating in a user, for which more than one
row is present in the user table (as done in JNDIRealm, AFAIK). However, that
has nothing to do with the enhancement and should probably go into a new PR.
As an alternative, we could add an ORDER BY to both the
_credential_ and the _attributes_ statements in order to force the same order
of rows.
So, since the _multiple rows for one user_ case should actually not occur,
it is no proper way to add multi-valued attributes by having multiple records.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655222693
##
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 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 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:
Ah, OK, multiple records for a user... Since by definition, the
attributes are extra fields of the _user table_ (that has logon name and
password), there is generally no support for user tables that contain the user
more than once. That's just not allowed. In method `getPassword` credentials
are read from the first record returned (there's no `while (rs.hasNext())`
loop).
So generally, having a user more than once in the user table may cause
problems with attributes: in `getUserAttributesMap` the multiple rows for that
username could come in a different order. Than, the Principal will get the
wrong attributes.
We could generally deny authenticating in a user, for which more than one
row is present in the user table (as done in JNDIRealm, AFAIK). However, that
has nothing to do with the enhancement and should probably got to a new PR.
As an alternative, we could add an ORDER BY to both the
_credential_ and the _attributes_ statements in order to force the same order
of rows.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655202844
##
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 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 hard coded types or, if the
+ * object implements java.io.Serializable. In all other cases,
+ * this method returns null.
+ *
+ * @param obj the object to copy
+ *
+ * @return a deep copied clone of the specified object, or
null
+ * 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:
I also mentioned that in this PR's initial comment under _Defensive
copies of attribute values_. So, consider my solution as a starting point (e.
g. for a in-deep discussion about that). There was not yet any agreement for a
strategy so, I started off with a working solution that should be safe. Now we
see, that it might even by over-engineered or paranoid.
When modifying the final 'value' field in an `Integer`, I'm quite sure that
Java uses that new value subsequently. So, It's not only the in-memory
representation.
(Das Sprichwort kannte ich bisher nicht. Sehr treffend :)
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655192559
##
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:
I know. However, the contributing guideline recommends to _copy_ the
coding style of the modified source file. That's what I did: the _roles_ and
_credentials_ statements have already been there, using the `StringBuilder`.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655190479
##
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 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 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:
I'm about to implement support for (at least) arrays, BLOBS and CLOBS.
Likely it is sufficient to support one-dimensional arrays only. Everything
_not_ supported will be documented (both in Javadoc comments as well as in
Tomcat's configuration reference).
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655188601
##
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 getUserAttributesMap(Connection
dbConnection, String username) {
+
+String preparedAttributes = getUserAttributesStatement(dbConnection);
+if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
Review comment:
With DataSourceRealm, parsing and validating is done lazily, so `null`
means _still uninitialized_. When not using a value different from `null` for
_empty/none required_, I end with constantly parsing and validating in method
`getUserAttributesStatement`. I could have uses another boolean
`userAttributesNoneRequested`, however I like that _3-state value approach_
some more.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655088342
##
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 getUserAttributesMap(Connection
dbConnection, String username) {
+
+String preparedAttributes = getUserAttributesStatement(dbConnection);
+if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
Review comment:
Guten Morgen :-)
Yes, that were my doubts, too. So, I changed that to `isEmpty()`. My
question was whether I should keep the constant when setting
`preparedAttributes`to the empty string by returning it method
`getUserAttributesStatement`. Here `return USER_ATTRIBUTES_NONE_REQUESTED;`is
likely more descriptive than `return "";`.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655092739
##
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:
I will add a comment and change to `1 = 2`
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655092412
##
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 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 attrs = new LinkedHashMap<>();
Review comment:
I don't know, but I guess the answer is no. AFAIK, using order
preserving maps is a more recent thing (e. g. Google Chrome preserves insertion
order of JavaScript Objects, which are HashMaps as well. That's also not
required by the ECMA standards, but it's a neat feature, especially with
debugging etc.)
> Maybe we could apply the same behavior.
Why not? But that's always a feature, users can't rely on. In fact, it's
just _nice to have_, However, since `LinkedHashMap`is not much more costly than
a classic `HashMap` (especially with only few entries), there is no pain with
using 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.
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 #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428: URL: https://github.com/apache/tomcat/pull/428#discussion_r655089215 ## 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 attributes map. + */ +protected String userAttributes; Review comment: Same solution 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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655088342
##
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 getUserAttributesMap(Connection
dbConnection, String username) {
+
+String preparedAttributes = getUserAttributesStatement(dbConnection);
+if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
Review comment:
Guten Morgen :-)
Yes, that wwere my doubts, too. So, I changed that to `isEmpty()`. My
question was whether I should keep the constant when setting
`preparedAttributes`to the empty string by returning it method
`getUserAttributesStatement`. Here `return USER_ATTRIBUTES_NONE_REQUESTED;`is
likely more descriptive than `return "";`.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655079698
##
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 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 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:
Ah, applies as well to BLOBs, CLOBs and the like.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655077759
##
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 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 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:
You're thinking of array-typed columns? Actually, these don't work as
expected. `getObject(int)` only returns a string representation of the array.
Maybe that's what the documentation means by _materialized_
> In the JDBC 2.0 API, the behavior of method `getObject` is extended to
materialize data of SQL user-defined types.
I'm trying to find a way to get real arrays from the database with standard
JDBC (in contrast to using driver proprietary types and methods).
What other types of multi-valued attributes are you thinking of?
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655075152
##
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 Object, or
+ * null if no attribute of the given name exists. May also
+ * return null, 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.
+ *
+ * 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.
+ *
+ * In order to keep the attributes map immutable, the objects
+ * returned by this method should always be defensive copies 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
+ * null) shall be returned.
+ *
+ * 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 String specifying the name of the attribute
Review comment:
Now, I understand... It only NPEs, if `CaseInsensitiveKeyMap` is used,
which does not support `null` as a key (and that's documented). I will add an
explicit check
```
if (name == null) {
return null;
}
```
to method `getAttribute(String)` and mention that in this method's Javadoc
comment accordingly.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654984693
##
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 list = new ArrayList<>();
+Set 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 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:
A read-only collection does not make the collection's items immutable.
Class `org.apache.catalina.users.MemoryRole` is neither `Cloneable` nor
`Serializable`. So, creating a defensive copy is quite costly. Also,
`MemoryRole` objects contain a list of `User` objects, which encapsulate the
user's password. No good candidate for being exposed as a user attribute.
So, I decided to just mimic the plain `role` XML attribute specified in file
tomcat-users.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.
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 #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654983295
##
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 list = new ArrayList<>();
+Set 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 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:
Copy 'n' paste bug. In UserDatabaseRealm the `new String(xxx)` is for
creating a _defensive copy_. But here, of course, attributes are put into a
`GenericPrincipal`, which defensively copies values in `getAttribute(String)`.
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.
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 #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654982860
##
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 list = new ArrayList<>();
+Set roleSet = new LinkedHashSet<>();
Review comment:
OK
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654982159
##
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 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 list = new HashSet<>();
Review comment:
Ah... just reused an existing variable and changed from List to Set (in
order to remove duplicates). Forgot to rename that accordingly.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654981981
##
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 USER_ATTRIBUTES_AVAILABLE =
+new ArrayList<>(Arrays.asList("username", "fullname", "roles"));
Review comment:
Oh... kind of a typo :)
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654981760
##
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 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 USER_ATTRIBUTES_ACCESS_DENIED =
+new ArrayList<>(Arrays.asList("password"));
Review comment:
Oh... kind of a typo :)
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654980982
##
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 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 hard coded types or, if the
+ * object implements java.io.Serializable. In all other cases,
+ * this method returns null.
+ *
+ * @param obj the object to copy
+ *
+ * @return a deep copied clone of the specified object, or
null
+ * 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:
I was discussing that with Mark and Chris. Both are sure that we need to
go with _defensive copies_. Chris was suggesting, that `Cloneable` is not the
best choice. Also, with reflection (and also with JNI), it is still possible to
change the value of a final field, so there are no really immutable objects.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654979259
##
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:
OK
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654979229
##
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:
Maybe `1 = 2` is a better choice?
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654978856
##
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 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 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 (lazily initialized singleton
with
+ * double-checked locking, DCL) 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 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:
There's a leading space in `preparedAttributesTail`.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654977455
##
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 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 attrs = new LinkedHashMap<>();
Review comment:
Since it's a map, it's not essential. But `getAttributeNames()`
returning the names in the order the attributes have been requested is simple
an not expensive with a linked hash map.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654976453
##
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 getUserAttributesMap(Connection
dbConnection, String username) {
+
+String preparedAttributes = getUserAttributesStatement(dbConnection);
+if (preparedAttributes == null || preparedAttributes ==
USER_ATTRIBUTES_NONE_REQUESTED) {
Review comment:
Ah, `isEmpty()` is clearly better. Didn't have that method in mind (only
thought of `preparedAttributes.length() == 0` and wanted something more
_speaking_). Would you keep the `USER_ATTRIBUTES_NONE_REQUESTED` constant
anyway?
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654975865
##
File path: java/org/apache/catalina/realm/DataSourceRealm.java
##
@@ -510,21 +582,22 @@ protected Principal getPrincipal(String username) {
return null;
}
-ArrayList list = null;
+// Using a Set removes duplicate roles
+Set 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:
OK
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428: URL: https://github.com/apache/tomcat/pull/428#discussion_r654975580 ## 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 attributes map. + */ +protected String userAttributes; Review comment: This implementation parses and validates _lazily_ on the first authentication attempt. With DataSourceRealm, I actually do not need an array or a collection but only a proper SQL statement. Of course, that is built from a parsed and validated (temporary) list (using `split()`...). However, I validate requested attributes and emit log messages for every invalid attribute. I can't do that in the setter, since `containerLog` is still null when the setter is invoked while the component gets initialized. So I would end with only parsing in the setter and still need to validate lazily on the first authentication attempt. -- 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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r654974276
##
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:
That is the getter method for the Realm's `userAttributes` configuration
attribute. It's configured by means of an XML attribute in the ``
component. It can also be read and written by through JMX. AFAIK, there should
always be a getter/setter pair for every configuration property using the
String type.
--
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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428: URL: https://github.com/apache/tomcat/pull/428#discussion_r654973642 ## 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 Object, or + * null if no attribute of the given name exists. May also + * return null, 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. + * + * 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. + * + * In order to keep the attributes map immutable, the objects + * returned by this method should always be defensive copies 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 + * null) shall be returned. + * + * 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 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 the + * object's string representation or null if its value + * cannot be copied in order to keep the attributes immutable + */ +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(); + +/** + * Determines whether attribute names are case-insensitive. May be + * true if using JNDIRealm and then, depends on the + * configured directory server. + * + * @return true, if attribute names are case-insensitive; + * false otherwise + */ +boolean isAttributesCaseIgnored(); Review comment: Yes, I wasn't sure with that. I will remove that method. -- 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
[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428: URL: https://github.com/apache/tomcat/pull/428#discussion_r654973423 ## 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 Object, or + * null if no attribute of the given name exists. May also + * return null, 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. + * + * 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. + * + * In order to keep the attributes map immutable, the objects + * returned by this method should always be defensive copies 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 + * null) shall be returned. + * + * 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 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 the + * object's string representation or null if its value + * cannot be copied in order to keep the attributes immutable + */ +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(); Review comment: It's not my first choice as well. However, to be consistent with e. g. `jakarta.servlet.ServletRequest.getAttributeNames()` and other Servlet Spec _attributes collections_, Mark Thomas (and I) agreed on using these classical 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. 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 #428: Enhancement: Additional user attributes queried by (some) realms
cklein05 commented on a change in pull request #428: URL: https://github.com/apache/tomcat/pull/428#discussion_r654973412 ## 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 Object, or + * null if no attribute of the given name exists. May also + * return null, 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. + * + * 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. + * + * In order to keep the attributes map immutable, the objects + * returned by this method should always be defensive copies 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 + * null) shall be returned. + * + * 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 String specifying the name of the attribute Review comment: Ah, ok. Basically, I was just copying from `jakarta.servlet.ServletRequest#getAttribute(String)`. You prefer a real `@exception` tag? None of these many `getAttribute(String)` used in Servlets mention that explicitly. -- 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
