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<String, Object> 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<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:
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: 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:
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 :)
##########
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:
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 <credential_field> to both the
_credential_ and the _attributes_ statements in order to force the same order
of rows.
##########
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:
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 <credential_field> 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.
##########
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:
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
