This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 8120a12a0d Fixes from code review
8120a12a0d is described below
commit 8120a12a0dbbaafc104fa0d61a319d480b7ff92c
Author: remm <[email protected]>
AuthorDate: Fri Jul 10 14:17:44 2026 +0200
Fixes from code review
Take advantage of rollback to improve save reliability.
Clear out transient memory changes only if save is successful,
---
.../catalina/users/DataSourceUserDatabase.java | 156 +++++++++++++--------
java/org/apache/catalina/users/GenericGroup.java | 2 +-
java/org/apache/catalina/users/GenericRole.java | 2 +-
java/org/apache/catalina/users/GenericUser.java | 2 +-
.../apache/catalina/users/LocalStrings.properties | 1 +
.../catalina/users/TestDataSourceUserDatabase.java | 3 +
webapps/docs/changelog.xml | 5 +
7 files changed, 110 insertions(+), 61 deletions(-)
diff --git a/java/org/apache/catalina/users/DataSourceUserDatabase.java
b/java/org/apache/catalina/users/DataSourceUserDatabase.java
index 119d36a5b4..6412161d18 100644
--- a/java/org/apache/catalina/users/DataSourceUserDatabase.java
+++ b/java/org/apache/catalina/users/DataSourceUserDatabase.java
@@ -153,13 +153,13 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
/**
- * The generated string for the groups PreparedStatement
+ * The generated string for the user groups relationship PreparedStatement
*/
private String preparedUserGroups = null;
/**
- * The generated string for the groups PreparedStatement
+ * The generated string for the group roles relationship PreparedStatement
*/
private String preparedGroupRoles = null;
@@ -231,13 +231,13 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
/**
- * The table that holds user data.
+ * The table that holds group data.
*/
protected String groupTable = null;
/**
- * The table that holds user data.
+ * The table that holds role data.
*/
protected String roleTable = null;
@@ -785,25 +785,22 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
if (rs.getString(1) != null) {
String description = (roleAndGroupDescriptionCol !=
null) ? rs.getString(2) : null;
ArrayList<Role> groupRoles = new ArrayList<>();
- if (groupName != null) {
- groupName = groupName.trim();
- try (PreparedStatement stmt2 =
dbConnection.prepareStatement(preparedGroupRoles)) {
- stmt2.setString(1, groupName);
- try (ResultSet rs2 = stmt2.executeQuery()) {
- while (rs2.next()) {
- String roleName = rs2.getString(1);
- if (roleName != null) {
- Role groupRole =
findRoleInternal(dbConnection, roleName);
- if (groupRole != null) {
- groupRoles.add(groupRole);
- }
+ try (PreparedStatement stmt2 =
dbConnection.prepareStatement(preparedGroupRoles)) {
+ stmt2.setString(1, groupName);
+ try (ResultSet rs2 = stmt2.executeQuery()) {
+ while (rs2.next()) {
+ String roleName = rs2.getString(1);
+ if (roleName != null) {
+ Role groupRole =
findRoleInternal(dbConnection, roleName);
+ if (groupRole != null) {
+ groupRoles.add(groupRole);
}
}
}
- } catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
- return null;
}
+ } catch (SQLException e) {
+
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ return null;
}
group = new GenericGroup<>(this, groupName,
description, groupRoles);
}
@@ -1059,9 +1056,17 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
Boolean.toString(isRoleStoreDefined()),
Boolean.toString(isGroupStoreDefined())));
}
+ if (!isUserStoreDefined()) {
+ throw new
IllegalArgumentException(sm.getString("dataSourceUserDatabase.noUserConfiguration",
+ userTable, userNameCol, userCredCol, userRoleTable,
roleNameCol));
+ }
+
dbWriteLock.lock();
try {
+ // Basic user functionality is mandatory
+ // This includes storing user name, credential, and a list of roles
+
StringBuilder temp = new StringBuilder("SELECT ");
temp.append(userCredCol);
if (userFullNameCol != null) {
@@ -1232,9 +1237,42 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
dbWriteLock.lock();
try {
try {
- saveInternal(dbConnection);
+ dbConnection.setAutoCommit(false);
+ Exception exception = saveInternal(dbConnection);
+ if (exception.getSuppressed().length > 0) {
+ // Some exception occurred so rollback everything
+ try {
+ dbConnection.rollback();
+ } catch (SQLException e) {
+ exception.addSuppressed(e);
+ }
+
log.error(sm.getString("dataSourceUserDatabase.exception"), exception);
+ throw exception;
+ }
+ // Commit all changes to the database
+ dbConnection.commit();
+ // Everything is committed successfully, we can now clear all
un-persisted changes
+ removedRoles.clear();
+ if (isRoleStoreDefined()) {
+ createdRoles.clear();
+ modifiedRoles.clear();
+ }
+ if (isGroupStoreDefined()) {
+ removedGroups.clear();
+ createdGroups.clear();
+ modifiedGroups.clear();
+ }
+ removedUsers.clear();
+ createdUsers.clear();
+ modifiedUsers.clear();
} finally {
- closeConnection(dbConnection);
+ try {
+ dbConnection.setAutoCommit(true);
+ } catch (SQLException e) {
+ // Ignore, this is for cleanup
+ } finally {
+ closeConnection(dbConnection);
+ }
}
} finally {
dbWriteLock.unlock();
@@ -1245,9 +1283,11 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
* Save all pending changes to the database using the provided connection.
*
* @param dbConnection The database connection to use
+ * @return an Exception containing any error that occurred while saving
the database as suppressed exception
*/
- protected void saveInternal(Connection dbConnection) {
+ protected Exception saveInternal(Connection dbConnection) {
+ Exception mainException = new
Exception(sm.getString("dataSourceUserDatabase.exception"));
StringBuilder temp = null;
StringBuilder tempRelation;
StringBuilder tempRelationDelete = null;
@@ -1278,23 +1318,22 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(1, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
try (PreparedStatement stmt =
dbConnection.prepareStatement(tempRelationDelete2.toString())) {
stmt.setString(1, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
try (PreparedStatement stmt =
dbConnection.prepareStatement(temp.toString())) {
stmt.setString(1, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
- removedRoles.clear();
}
// Created roles
@@ -1318,10 +1357,9 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
}
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
- createdRoles.clear();
}
// Modified roles
@@ -1337,13 +1375,12 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
- modifiedRoles.clear();
}
- } else if (userRoleTable != null && roleNameCol != null) {
+ } else {
// Only remove role from users
tempRelationDelete = new StringBuilder("DELETE FROM ");
tempRelationDelete.append(userRoleTable);
@@ -1355,10 +1392,9 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(1, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
- removedRoles.clear();
}
if (isGroupStoreDefined()) {
@@ -1393,22 +1429,21 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(1, group.getGroupname());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
try (PreparedStatement stmt =
dbConnection.prepareStatement(tempRelationDelete2.toString())) {
stmt.setString(1, group.getGroupname());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
try (PreparedStatement stmt =
dbConnection.prepareStatement(temp.toString())) {
stmt.setString(1, group.getGroupname());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
- removedGroups.clear();
}
// Created groups
@@ -1432,7 +1467,7 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
}
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
Iterator<Role> roles = group.getRoles();
while (roles.hasNext()) {
@@ -1442,11 +1477,10 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
}
- createdGroups.clear();
}
// Modified groups
@@ -1465,14 +1499,14 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, group.getGroupname());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
try (PreparedStatement stmt =
dbConnection.prepareStatement(groupRoleRelationDelete)) {
stmt.setString(1, group.getGroupname());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
Iterator<Role> roles = group.getRoles();
while (roles.hasNext()) {
@@ -1482,11 +1516,10 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
}
- modifiedGroups.clear();
}
}
@@ -1538,7 +1571,7 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(1, user.getUsername());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
if (userGroupRelationDelete != null) {
@@ -1546,17 +1579,16 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(1, user.getUsername());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
try (PreparedStatement stmt =
dbConnection.prepareStatement(temp.toString())) {
stmt.setString(1, user.getUsername());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
- removedUsers.clear();
}
// Created users
@@ -1582,7 +1614,7 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
}
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
if (userRoleRelation != null) {
Iterator<Role> roles = user.getRoles();
@@ -1593,7 +1625,7 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
}
@@ -1606,12 +1638,11 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, group.getGroupname());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
}
}
- createdUsers.clear();
}
// Modified users
@@ -1636,14 +1667,14 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
}
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
if (userRoleRelationDelete != null) {
try (PreparedStatement stmt =
dbConnection.prepareStatement(userRoleRelationDelete)) {
stmt.setString(1, user.getUsername());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
if (userGroupRelationDelete != null) {
@@ -1651,7 +1682,7 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(1, user.getUsername());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
if (userRoleRelation != null) {
@@ -1663,7 +1694,7 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, role.getRolename());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
}
@@ -1676,14 +1707,14 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
stmt.setString(2, group.getGroupname());
stmt.executeUpdate();
} catch (SQLException e) {
-
log.error(sm.getString("dataSourceUserDatabase.exception"), e);
+ mainException.addSuppressed(e);
}
}
}
}
- modifiedUsers.clear();
}
+ return mainException;
}
@Override
@@ -1691,6 +1722,15 @@ public class DataSourceUserDatabase extends
SparseUserDatabase {
return connectionSuccess;
}
+ /**
+ * User storage is mandatory.
+ *
+ * @return true when users are properly configured
+ */
+ protected boolean isUserStoreDefined() {
+ return userTable != null && userNameCol != null && userCredCol != null
&& userRoleTable != null && roleNameCol != null;
+ }
+
/**
* Only use groups if the tables are fully defined.
*
diff --git a/java/org/apache/catalina/users/GenericGroup.java
b/java/org/apache/catalina/users/GenericGroup.java
index 1d314a14e0..e0b6c9b926 100644
--- a/java/org/apache/catalina/users/GenericGroup.java
+++ b/java/org/apache/catalina/users/GenericGroup.java
@@ -32,7 +32,7 @@ import org.apache.catalina.UserDatabase;
* Concrete implementation of {@link org.apache.catalina.Group} for a {@link
UserDatabase}.
* </p>
*
- * @param <UD> The specific type of UserDase with which this group is
associated
+ * @param <UD> The specific type of UserDatabase with which this group is
associated
*/
public class GenericGroup<UD extends UserDatabase> extends AbstractGroup {
diff --git a/java/org/apache/catalina/users/GenericRole.java
b/java/org/apache/catalina/users/GenericRole.java
index 793c5c480a..22f2f28d53 100644
--- a/java/org/apache/catalina/users/GenericRole.java
+++ b/java/org/apache/catalina/users/GenericRole.java
@@ -25,7 +25,7 @@ import org.apache.catalina.UserDatabase;
* Concrete implementation of {@link org.apache.catalina.Role} for a {@link
UserDatabase}.
* </p>
*
- * @param <UD> The specific type of UserDase with which this role is associated
+ * @param <UD> The specific type of UserDatabase with which this role is
associated
*/
public class GenericRole<UD extends UserDatabase> extends AbstractRole {
diff --git a/java/org/apache/catalina/users/GenericUser.java
b/java/org/apache/catalina/users/GenericUser.java
index ca3724a213..6f0eee236b 100644
--- a/java/org/apache/catalina/users/GenericUser.java
+++ b/java/org/apache/catalina/users/GenericUser.java
@@ -30,7 +30,7 @@ import org.apache.catalina.UserDatabase;
* Concrete implementation of {@link org.apache.catalina.User} for a {@link
UserDatabase}.
* </p>
*
- * @param <UD> The specific type of UserDase with which this role is associated
+ * @param <UD> The specific type of UserDatabase with which this user is
associated
*/
public class GenericUser<UD extends UserDatabase> extends AbstractUser {
diff --git a/java/org/apache/catalina/users/LocalStrings.properties
b/java/org/apache/catalina/users/LocalStrings.properties
index 56bea7c03f..902730208b 100644
--- a/java/org/apache/catalina/users/LocalStrings.properties
+++ b/java/org/apache/catalina/users/LocalStrings.properties
@@ -15,6 +15,7 @@
dataSourceUserDatabase.exception=Exception accessing the database
dataSourceUserDatabase.features=DataSource UserDatabase features: User <->
Role [{0}], Roles [{1}], Groups [{2}]
+dataSourceUserDatabase.noUserConfiguration=DataSource UserDatabase has no
properly configured user storage defined: userTable [{0}] userNameCol [{1}]
userCredCol [{2}] userRoleTable [{3}] roleNameCol [{4}]
memoryUserDatabase.fileClose=Failed to close [{0}]
memoryUserDatabase.fileDelete=Failed to delete [{0}]
diff --git a/test/org/apache/catalina/users/TestDataSourceUserDatabase.java
b/test/org/apache/catalina/users/TestDataSourceUserDatabase.java
index d3908d9b51..326bf45bd2 100644
--- a/test/org/apache/catalina/users/TestDataSourceUserDatabase.java
+++ b/test/org/apache/catalina/users/TestDataSourceUserDatabase.java
@@ -228,6 +228,9 @@ public class TestDataSourceUserDatabase extends
LoggingBaseTest {
randomUser = db.findUser("random");
Assert.assertTrue("No group for user",
randomUser.isInGroup(userGroup));
+ Iterator<Role> roles = db.getRoles();
+ Assert.assertTrue("No roles found", roles.hasNext());
+
db.close();
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index a30d7270c8..54bceec742 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -243,6 +243,11 @@
Extend <code>sessionAttributeValueClassNameFilter</code> to include
filtering of dynamic proxy interface classes. (markt)
</fix>
+ <fix>
+ Attempt to use rollback when persisting user data to the
+ <code>DataSourceUserDatabase</code> fails and improve error reporting.
+ (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
