This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new a244ced8f0 Fixes from code review
a244ced8f0 is described below
commit a244ced8f074b98bf7e26ea888cee32bc32140ae
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 3e74619e33..6c32c49927 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);
}
@@ -1062,9 +1059,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) {
@@ -1235,9 +1240,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();
@@ -1248,9 +1286,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;
@@ -1281,23 +1321,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
@@ -1321,10 +1360,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
@@ -1340,13 +1378,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);
@@ -1358,10 +1395,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()) {
@@ -1396,22 +1432,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
@@ -1435,7 +1470,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()) {
@@ -1445,11 +1480,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
@@ -1468,14 +1502,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()) {
@@ -1485,11 +1519,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();
}
}
@@ -1541,7 +1574,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) {
@@ -1549,17 +1582,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
@@ -1585,7 +1617,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();
@@ -1596,7 +1628,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);
}
}
}
@@ -1609,12 +1641,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
@@ -1639,14 +1670,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) {
@@ -1654,7 +1685,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) {
@@ -1666,7 +1697,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);
}
}
}
@@ -1679,14 +1710,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
@@ -1694,6 +1725,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 10b33f67ff..2c890b1fe5 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 339cc2bf4d..378e46590c 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 8e8a1b6cac..00e52569b2 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 3d17b40b87..b8898b36a8 100644
--- a/java/org/apache/catalina/users/LocalStrings.properties
+++ b/java/org/apache/catalina/users/LocalStrings.properties
@@ -18,6 +18,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 5853f11c46..82d6a50536 100644
--- a/test/org/apache/catalina/users/TestDataSourceUserDatabase.java
+++ b/test/org/apache/catalina/users/TestDataSourceUserDatabase.java
@@ -233,6 +233,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 1fa7afa519..13809502f7 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -122,6 +122,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]
