[GitHub] [tomcat] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


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 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?




-- 
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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655173373



##
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:
   Agreed.




-- 
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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655174381



##
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 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.




-- 
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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655175314



##
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.




-- 
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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655176832



##
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:
   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.




-- 
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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655177819



##
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:
   > 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.




-- 
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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655178151



##
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:
   That's fine.




-- 
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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655182143



##
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:
   I see it now. Looked like a bug from first glance.




-- 
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] michael-o commented on pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-864846238


   > 
   > 
   > > Can you please explain the purpose of the `deniedAttributes`? Why is it 
necessary, what is the usecase for?
   > 
   > _Denied Attributes_ is the internal term of attributes, for which access 
is denied to. Those attributes could never be exposed as _user attributes_ in 
the Principal's attributes map. Basically, this applies to attributes/fields 
that contain the user's password. Requesting such an attribute causes a 
`userAttributeAccessDenied` message to be logged.
   > 
   > Both MemoryRealm and UserDatabaseRealm provide all possible (and actually 
used, MemoryRealm does not use the group attribute) XML attributes of the 
`` entry in tomcat-users.xml. Obviously, the password attribute is 
sensitive and should not be exposed so, these Realms make the `password` 
attribute a _denied_ attribute.
   > 
   > DataSourceRealm makes the column name configured in `userCredCol` a 
_denied attribute_ and JNDIRealm does this for the attribute name configured in 
`userPassword`.
   
   So you basically want to protest the admin/developer to shoot in the foot?


-- 
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] michael-o edited a comment on pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o edited a comment on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-864846238


   > 
   > 
   > > Can you please explain the purpose of the `deniedAttributes`? Why is it 
necessary, what is the usecase for?
   > 
   > _Denied Attributes_ is the internal term of attributes, for which access 
is denied to. Those attributes could never be exposed as _user attributes_ in 
the Principal's attributes map. Basically, this applies to attributes/fields 
that contain the user's password. Requesting such an attribute causes a 
`userAttributeAccessDenied` message to be logged.
   > 
   > Both MemoryRealm and UserDatabaseRealm provide all possible (and actually 
used, MemoryRealm does not use the group attribute) XML attributes of the 
`` entry in tomcat-users.xml. Obviously, the password attribute is 
sensitive and should not be exposed so, these Realms make the `password` 
attribute a _denied_ attribute.
   > 
   > DataSourceRealm makes the column name configured in `userCredCol` a 
_denied attribute_ and JNDIRealm does this for the attribute name configured in 
`userPassword`.
   
   So you basically want to protect the admin/developer to shoot in the foot?


-- 
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

2021-06-21 Thread GitBox


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

2021-06-21 Thread GitBox


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

2021-06-21 Thread GitBox


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

2021-06-21 Thread GitBox


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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655204617



##
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:
   Yes, that's a problem. I agree. Since I cannot provide anything better I 
accept this.




-- 
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 pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


cklein05 commented on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-864868657


   > So you basically want to protect the admin/developer to shoot in the foot?
   
   Yes, as it may hurt badly. Also, only these _denied attributes_ makes using 
the (otherwise quite comfortable) wildcard character * possible/safe.


-- 
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

2021-06-21 Thread GitBox


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

2021-06-21 Thread GitBox


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] michael-o commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


michael-o commented on a change in pull request #428:
URL: https://github.com/apache/tomcat/pull/428#discussion_r655234294



##
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:
   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



[GitHub] [tomcat] cklein05 commented on a change in pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


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



[Bug 65387] JSP compile error after upgrade 10.0.6 -> 10.0.7

2021-06-21 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=65387

--- Comment #18 from Mark Thomas  ---
Just following up in this as we'd really like to catch regressions like this in
the ~3 working day window between the release candidate being available and the
release vote concluding.

It is great that you are using the latest Tomcat release as soon as it as
available but what would we, the Tomcat team, need to do for you to be able to
test the release candidate as soon as it as available rather than you waiting
for the release?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 65390] Problem in the code generated by jasper

2021-06-21 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=65390

--- Comment #6 from Mark Thomas  ---
Just following up in this as we'd really like to catch regressions like this in
the ~3 working day window between the release candidate being available and the
release vote concluding.

It is great that you are using the latest Tomcat release as soon as it as
available but what would we, the Tomcat team, need to do for you to be able to
test the release candidate as soon as it as available rather than you waiting
for the release?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



JSP regressions - do we need a new set of releases

2021-06-21 Thread Mark Thomas

All,

Despite the extra tests I added to improve code coverage to almost 100% 
for JSP code generation, the changes I made to remove unnecessary code 
have triggered a couple of regressions. There is no easy way to avoid 
these regressions.


Given the above do we want to pull forward the July release round and 
start the next set of releases soon / today / this week?


There is a balance to strike between getting fixed releases out that 
address these regressions and allowing enough time for any further 
regressions to be identified.


Thoughts?

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 65390] Problem in the code generated by jasper

2021-06-21 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=65390

--- Comment #7 from tk...@ai-ag.de ---
Hallo Mark,

first I’d like to thank you for your very fast response on our bug report :)

We use tomcat-embedded and pull it from a maven repository.
If you made your release candidates available via a public maven repository
which always hosted the current release or upcoming patch, e.g. 

"org.apache.tomcat:tomcat-catalina:9.0.next_or_rc"

our nightly could pick that up.

Of course you’d need to do that for all artifacts.

Tank you!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[GitHub] [tomcat] rmaucher commented on pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


rmaucher commented on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-865007741


   This looks good enough to put it in. I can probably think about enhancements 
late. The only thing that I'm really wondering about is why the 
UserDatabaseRealm did not get arbitrary attributes.


-- 
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 pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


cklein05 commented on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-865024538


   > The only thing that I'm really wondering about is why the 
UserDatabaseRealm did not get arbitrary attributes.
   
   My first focus was on UserDatabaseRealm and JDNIRealm. Arbitrary user 
attributes for UserDatabaseRealm sounds interesting. However, I did not yet 
think about it in detail. AFAIK, there is a schema file for tomcat-users.xml. 
Some new Digester rules must be defined. Actually, the attributes come from the 
`User` instance. Likely this class should get an attributes map as well (which 
we then can provide through `UserDatabasePrincipal`).
   
   I'm almost finished with all the changes caused by that code review. Also, 
I've added support for SQL types ARRAY, BLOB, and CLOB. I will push my latest 
commits soon.
   
   Still open/pending:
   
   - Using *linked* hash maps (yes/no)
   - Defensive copies (how paranoid shall we go?)
   - Documentation (likely my job)
   
   The latter might be quite challenging. Needs additions in 
`config/realm.html` and likely in `realm-howto.html`. Since, at current, only 
`TomcatPrincipal` supports the attribute stuff, maybe a code example in one of 
the HTML sites will be required.
   
   Has anyone thought about putting these methods to a more _official_ 
interface `jakarta.servlet.Principal`? Since it is now _jakarta_, is Apache or 
Tomcat now responsible for the Servlet Standard?


-- 
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



Re: JSP regressions - do we need a new set of releases

2021-06-21 Thread Rémy Maucherat
On Mon, Jun 21, 2021 at 12:17 PM Mark Thomas  wrote:
>
> All,
>
> Despite the extra tests I added to improve code coverage to almost 100%
> for JSP code generation, the changes I made to remove unnecessary code
> have triggered a couple of regressions. There is no easy way to avoid
> these regressions.
>
> Given the above do we want to pull forward the July release round and
> start the next set of releases soon / today / this week?
>
> There is a balance to strike between getting fixed releases out that
> address these regressions and allowing enough time for any further
> regressions to be identified.
>
> Thoughts?

The rate at which they are reported is not too alarming so far. Maybe
we can keep these as July releases but move them forward (the previous
tags was on 06/08, with further delays for retags and cleanups, so
maybe we could plan a tag on 07/01).

Rémy


>
> Mark
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[GitHub] [tomcat] rmaucher commented on pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


rmaucher commented on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-865049209


   No, the Tomcat project is not responsible for Jakarta and has no control 
over it, so we cannot make that change.
   
   I'm perfectly fine as it is, you're not supposed to do everything by 
yourself and I'm interested in improving things a bit too if I can.


-- 
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 pull request #428: Enhancement: Additional user attributes queried by (some) realms

2021-06-21 Thread GitBox


cklein05 commented on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-865053410


   Please, wait at least until I've pushed my latest changes. Also, JDBC stuff 
in DataSourceRealm needs some adjustments in order not to put arbitrary 
`java.sql` objects into the map. I'd really love to make ends meet for things 
that I've started...


-- 
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



Re: JSP regressions - do we need a new set of releases

2021-06-21 Thread Mark Thomas

On 21/06/2021 14:16, Rémy Maucherat wrote:

On Mon, Jun 21, 2021 at 12:17 PM Mark Thomas  wrote:


All,

Despite the extra tests I added to improve code coverage to almost 100%
for JSP code generation, the changes I made to remove unnecessary code
have triggered a couple of regressions. There is no easy way to avoid
these regressions.

Given the above do we want to pull forward the July release round and
start the next set of releases soon / today / this week?

There is a balance to strike between getting fixed releases out that
address these regressions and allowing enough time for any further
regressions to be identified.

Thoughts?


The rate at which they are reported is not too alarming so far. Maybe
we can keep these as July releases but move them forward (the previous
tags was on 06/08, with further delays for retags and cleanups, so
maybe we could plan a tag on 07/01).


Sounds reasonable. The tag only ever slips as things come in at the last 
minute so I'll start the process shortly of updating the translations, 
checking the dependencies etc and we can see where we are once 
everything is ready to tag.


Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



svn commit: r48450 - /release/tomcat/tomcat-10/v10.0.6/

2021-06-21 Thread markt
Author: markt
Date: Mon Jun 21 16:14:14 2021
New Revision: 48450

Log:
Drop Apache Tomcat 10.0.6 from mirror system

Removed:
release/tomcat/tomcat-10/v10.0.6/


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



svn commit: r48451 - /release/tomcat/tomcat-9/v9.0.46/

2021-06-21 Thread markt
Author: markt
Date: Mon Jun 21 16:14:49 2021
New Revision: 48451

Log:
Drop Apache Tomcat 9.0.46 from mirror system

Removed:
release/tomcat/tomcat-9/v9.0.46/


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[tomcat] branch main updated (98d61c1 -> 2b92581)

2021-06-21 Thread markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a change to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git.


from 98d61c1  Fix typos
 new fa24edd  Improve French translations (remm)
 new 2b92581  Improvements to Korean translations. (woonsan)

The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 java/jakarta/servlet/http/LocalStrings_fr.properties | 1 +
 java/jakarta/servlet/http/LocalStrings_ko.properties | 1 +
 webapps/docs/changelog.xml   | 6 ++
 3 files changed, 8 insertions(+)

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[tomcat] 01/02: Improve French translations (remm)

2021-06-21 Thread markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git

commit fa24eddfc49cc2e19b4cd4606a4eb3b7d55fed98
Author: Mark Thomas 
AuthorDate: Mon Jun 21 17:34:12 2021 +0100

Improve French translations (remm)
---
 java/jakarta/servlet/http/LocalStrings_fr.properties | 1 +
 webapps/docs/changelog.xml   | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/java/jakarta/servlet/http/LocalStrings_fr.properties 
b/java/jakarta/servlet/http/LocalStrings_fr.properties
index 5352f35..bdf7898 100644
--- a/java/jakarta/servlet/http/LocalStrings_fr.properties
+++ b/java/jakarta/servlet/http/LocalStrings_fr.properties
@@ -21,6 +21,7 @@ err.cookie_name_is_token=Le nom de cookie [{0}] est un 
"token" réservé
 err.io.indexOutOfBounds=L''offset [{0}] et/ou la longueur [{1}] spécifiés pour 
la taille du tableau [{2}] sont invalides
 err.io.nullArray=Null a été passée comme tableau d'octets à la méthode 
d'écriture
 err.io.short_read=Lecture partielle
+err.state.commit=Interdit une fois que la réponse a été commitée
 
 http.method_delete_not_supported=La méthode HTTP DELETE n'est pas supportée 
par cette URL
 http.method_get_not_supported=La méthode HTTP GET n'est pas supportée par 
cette URL
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 24ab417..f02d11d 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -217,6 +217,9 @@
 16 onwards to the service.bat script to align it with the
 other start-up scripts. PR provided by MCMicS. (markt)
   
+  
+Improvements to French translations. (remm)
+  
 
   
 

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[tomcat] 02/02: Improvements to Korean translations. (woonsan)

2021-06-21 Thread markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git

commit 2b925814e6af699d4ea6f740d408310389fee03a
Author: Mark Thomas 
AuthorDate: Mon Jun 21 17:37:50 2021 +0100

Improvements to Korean translations. (woonsan)
---
 java/jakarta/servlet/http/LocalStrings_ko.properties | 1 +
 webapps/docs/changelog.xml   | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/java/jakarta/servlet/http/LocalStrings_ko.properties 
b/java/jakarta/servlet/http/LocalStrings_ko.properties
index 13c501d..30cdc43 100644
--- a/java/jakarta/servlet/http/LocalStrings_ko.properties
+++ b/java/jakarta/servlet/http/LocalStrings_ko.properties
@@ -21,6 +21,7 @@ err.cookie_name_is_token=쿠키 이름 [{0}]은(는) 예약된 토큰입니다.
 err.io.indexOutOfBounds=크기 [{2}]인 배열에 대하여, 유효하지 않은 offset [{0}] 그리고/또는 길이 
[{1}].
 err.io.nullArray=write 메소드에 널인 바이트 배열이 전달되었습니다.
 err.io.short_read=Short Read
+err.state.commit=응답이 한번 커밋된 이후에는 허용되지 않습니다.
 
 http.method_delete_not_supported=HTTP 메소드 DELETE는 이 URL에 의해 지원되지 않습니다.
 http.method_get_not_supported=HTTP 메소드 GET은 이 URL에 의해 지원되지 않습니다.
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index f02d11d..d09a26b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -220,6 +220,9 @@
   
 Improvements to French translations. (remm)
   
+  
+Improvements to Korean translations. (woonsan)
+  
 
   
 

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[tomcat] branch main updated (2b92581 -> e40d778)

2021-06-21 Thread markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a change to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git.


from 2b92581  Improvements to Korean translations. (woonsan)
 add e40d778  Make fields accessible to sub-classes of JNDIRealm

No new revisions were added by this update.

Summary of changes:
 java/org/apache/catalina/realm/JNDIRealm.java | 10 +-
 webapps/docs/changelog.xml|  4 
 2 files changed, 9 insertions(+), 5 deletions(-)

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[tomcat] branch 10.0.x updated: Make fields accessible to sub-classes of JNDIRealm

2021-06-21 Thread markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/10.0.x by this push:
 new b651945  Make fields accessible to sub-classes of JNDIRealm
b651945 is described below

commit b651945857d9a47523e336a83cb4024a65f6e518
Author: Mark Thomas 
AuthorDate: Mon Jun 21 18:12:49 2021 +0100

Make fields accessible to sub-classes of JNDIRealm

Access problem for sub-classes reported via users@tomcat.a.o
https://tomcat.markmail.org/thread/l6exgocuy46v75ch
---
 java/org/apache/catalina/realm/JNDIRealm.java | 10 +-
 webapps/docs/changelog.xml|  4 
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/java/org/apache/catalina/realm/JNDIRealm.java 
b/java/org/apache/catalina/realm/JNDIRealm.java
index b6318b4..89564ea 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -3083,29 +3083,29 @@ public class JNDIRealm extends RealmBase {
  * The MessageFormat object associated with the current
  * userSearch.
  */
-protected MessageFormat userSearchFormat = null;
+public MessageFormat userSearchFormat = null;
 
 /**
  * An array of MessageFormat objects associated with the current
  * userPatternArray.
  */
-protected MessageFormat[] userPatternFormatArray = null;
+public MessageFormat[] userPatternFormatArray = null;
 
 /**
  * The MessageFormat object associated with the current
  * roleBase.
  */
-protected MessageFormat roleBaseFormat = null;
+public MessageFormat roleBaseFormat = null;
 
 /**
  * The MessageFormat object associated with the current
  * roleSearch.
  */
-protected MessageFormat roleFormat = null;
+public MessageFormat roleFormat = null;
 
 /**
  * The directory context linking us to our directory server.
  */
-protected DirContext context = null;
+public DirContext context = null;
 }
 }
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index b3b1a15..02973d5 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -111,6 +111,10 @@
 Refactor the RemoteIpValve to use the common utility 
method
 for list to comma separated string conversion. (markt)
   
+  
+Refactor JNDIRealm$JNDIConnection so its fields are
+accessible to sub-classes of JNDIRealm. (markt)
+  
 
   
   

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[tomcat] branch 9.0.x updated: Make fields accessible to sub-classes of JNDIRealm

2021-06-21 Thread markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/9.0.x by this push:
 new 86b9958  Make fields accessible to sub-classes of JNDIRealm
86b9958 is described below

commit 86b9958b191a7cb924a54c86205a5a247174f1be
Author: Mark Thomas 
AuthorDate: Mon Jun 21 18:12:49 2021 +0100

Make fields accessible to sub-classes of JNDIRealm

Access problem for sub-classes reported via users@tomcat.a.o
https://tomcat.markmail.org/thread/l6exgocuy46v75ch
---
 java/org/apache/catalina/realm/JNDIRealm.java | 10 +-
 webapps/docs/changelog.xml|  4 
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/java/org/apache/catalina/realm/JNDIRealm.java 
b/java/org/apache/catalina/realm/JNDIRealm.java
index 315d9e6..7f72ae3 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -3084,29 +3084,29 @@ public class JNDIRealm extends RealmBase {
  * The MessageFormat object associated with the current
  * userSearch.
  */
-protected MessageFormat userSearchFormat = null;
+public MessageFormat userSearchFormat = null;
 
 /**
  * An array of MessageFormat objects associated with the current
  * userPatternArray.
  */
-protected MessageFormat[] userPatternFormatArray = null;
+public MessageFormat[] userPatternFormatArray = null;
 
 /**
  * The MessageFormat object associated with the current
  * roleBase.
  */
-protected MessageFormat roleBaseFormat = null;
+public MessageFormat roleBaseFormat = null;
 
 /**
  * The MessageFormat object associated with the current
  * roleSearch.
  */
-protected MessageFormat roleFormat = null;
+public MessageFormat roleFormat = null;
 
 /**
  * The directory context linking us to our directory server.
  */
-protected DirContext context = null;
+public DirContext context = null;
 }
 }
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 598969e..72db4ee 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -111,6 +111,10 @@
 Refactor the RemoteIpValve to use the common utility 
method
 for list to comma separated string conversion. (markt)
   
+  
+Refactor JNDIRealm$JNDIConnection so its fields are
+accessible to sub-classes of JNDIRealm. (markt)
+  
 
   
   

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org