You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Christopher Schultz <ch...@christopherschultz.net> on 2018/09/27 15:57:17 UTC
Proposed patch for o.a.c.users.MemoryUserDatabase
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
All,
I have a proposed patch to MemoryUserDatabase that changes the
behavior when a triggered-reload fails. Recently, markt added code to
allow database reloads, but if there is an error reloading the
database, the database is emptied and perhaps an administrator can no
longer make e.g. calls to the manager.
This patch makes an open-failure into a no-op: the user database will
not be changed unless there is a successful load from the file.
This patch changes the way that data is loaded by the Digester.
Instead of modifying the role/group/user maps directly, the data are
loaded into new maps and then all maps are updated atomically.
This patch removes a bunch of code from this class, and I have a unit
test (not attached) which demonstrates that (a) it works and (b)
thread-safety is maintained.
Thanks,
- -chris
For review:
### Eclipse Workspace Patch 1.0
#P tomcat-trunk
Index: java/org/apache/catalina/users/MemoryUserDatabase.java
===================================================================
- --- java/org/apache/catalina/users/MemoryUserDatabase.java (revision
1842017)
+++ java/org/apache/catalina/users/MemoryUserDatabase.java (working copy)
@@ -31,6 +31,7 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.regex.Pattern;
import org.apache.catalina.Globals;
import org.apache.catalina.Group;
@@ -39,11 +40,10 @@
import org.apache.catalina.UserDatabase;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
- -import org.apache.tomcat.util.digester.AbstractObjectCreationFactory;
+import org.apache.tomcat.util.digester.CallParamRule;
import org.apache.tomcat.util.digester.Digester;
import org.apache.tomcat.util.file.ConfigFileLoader;
import org.apache.tomcat.util.res.StringManager;
- -import org.xml.sax.Attributes;
/**
* Concrete implementation of {@link UserDatabase} that loads all
defined users,
@@ -107,7 +107,7 @@
/**
* The set of {@link Group}s defined in this database, keyed by
group name.
*/
- - protected final Map<String, Group> groups = new
ConcurrentHashMap<>();
+ protected Map<String, Group> groups = new ConcurrentHashMap<>();
/**
* The unique global identifier of this user database.
@@ -140,12 +140,12 @@
/**
* The set of {@link Role}s defined in this database, keyed by
role name.
*/
- - protected final Map<String, Role> roles = new ConcurrentHashMap<>();
+ protected Map<String, Role> roles = new ConcurrentHashMap<>();
/**
* The set of {@link User}s defined in this database, keyed by
user name.
*/
- - protected final Map<String, User> users = new ConcurrentHashMap<>();
+ protected Map<String, User> users = new ConcurrentHashMap<>();
private final ReentrantReadWriteLock dbLock = new
ReentrantReadWriteLock();
private final Lock readLock = dbLock.readLock();
@@ -415,54 +415,139 @@
*/
@Override
public void open() throws Exception {
+ String pathName = getPathname();
+ URI uri = ConfigFileLoader.getURI(pathName);
+ URL url = uri.toURL();
+ URLConnection uConn = url.openConnection();
- - writeLock.lock();
- - try {
- - // Erase any previous groups and users
- - users.clear();
- - groups.clear();
- - roles.clear();
+ try (InputStream is = uConn.getInputStream()) {
+ this.lastModified = uConn.getLastModified();
- - String pathName = getPathname();
- - URI uri = ConfigFileLoader.getURI(pathName);
- - URL url = uri.toURL();
- - URLConnection uConn = url.openConnection();
+ // Construct a digester to read the XML input file
+ Digester digester = new Digester();
+ try {
+ digester.setFeature(
+
"http://apache.org/xml/features/allow-java-encodings", true);
+ } catch (Exception e) {
+
log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
+ }
- - try (InputStream is = uConn.getInputStream()) {
- - this.lastModified = uConn.getLastModified();
+ Bundle bundle = new Bundle();
+ digester.push(bundle);
+ digester.addCallMethod("tomcat-users/role", "addRole", 2);
+ digester.addRule("tomcat-users/role", new
CallParamRule(0, "rolename"));
+ digester.addRule("tomcat-users/role", new
CallParamRule(1, "description"));
+ digester.addCallMethod("tomcat-users/group", "addGroup", 3);
+ digester.addRule("tomcat-users/group", new
CallParamRule(0, "groupname"));
+ digester.addRule("tomcat-users/group", new
CallParamRule(1, "description"));
+ digester.addRule("tomcat-users/group", new
CallParamRule(2, "roles"));
+ digester.addCallMethod("tomcat-users/user", "addUser", 5);
+ digester.addRule("tomcat-users/user", new
CallParamRule(0, "username"));
+ digester.addRule("tomcat-users/user", new
CallParamRule(1, "fullname"));
+ digester.addRule("tomcat-users/user", new
CallParamRule(2, "password"));
+ digester.addRule("tomcat-users/user", new
CallParamRule(3, "roles"));
+ digester.addRule("tomcat-users/user", new
CallParamRule(4, "groups"));
- - // Construct a digester to read the XML input file
- - Digester digester = new Digester();
- - try {
- - digester.setFeature(
- -
"http://apache.org/xml/features/allow-java-encodings", true);
- - } catch (Exception e) {
- -
log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
+ // Parse the XML input to load this database
+ digester.parse(is);
+
+ // Update all maps simultaneously
+ writeLock.lock();
+ try {
+ roles = bundle.getRoles();
+ groups = bundle.getGroups();
+ users = bundle.getUsers();
+ } finally {
+ writeLock.unlock();
+ }
+ } catch (IOException ioe) {
+ log.error(sm.getString("memoryUserDatabase.fileNotFound",
pathName));
+ }
+ }
+
+ private static final Pattern CSV = Pattern.compile("\\s*,\\s*");
+ /**
+ * A wrapper around the role/group/user maps managed by the
MemoryUserDatabase,
+ * used for loading the database
+ */
+ public class Bundle {
+ ConcurrentHashMap<String,User> users = new ConcurrentHashMap<>();
+ ConcurrentHashMap<String,Group> groups = new
ConcurrentHashMap<>();
+ ConcurrentHashMap<String,Role> roles = new ConcurrentHashMap<>();
+
+ public void addRole(Role role) {
+ roles.put(role.getName(), role);
+ }
+
+ public void addRole(String name, String description) {
+ addRole(new MemoryRole(MemoryUserDatabase.this, name,
description));
+ }
+
+ public void addGroup(Group group) {
+ groups.put(group.getName(), group);
+ }
+
+ public void addGroup(String name, String description, String
roles) {
+ Group group = new MemoryGroup(MemoryUserDatabase.this,
name, description);
+
+ if(null != roles) {
+ for(String roleName : CSV.split(roles)) {
+ Role role = getRole(roleName);
+ if(null == role) {
+ role = new
MemoryRole(MemoryUserDatabase.this, roleName, null);
+ addRole(role);
+ }
+ group.addRole(role);
+ }
+ }
+
+ addGroup(group);
+ }
+
+ public void addUser(String name, String fullname, String
password, String roles, String groups) {
+ User user = new MemoryUser(MemoryUserDatabase.this, name,
password, fullname);
+
+ if (groups != null) {
+ for(String groupName : CSV.split(groups)) {
+ Group group = getGroup(groupName);
+ if(null == group) {
+ group = new
MemoryGroup(MemoryUserDatabase.this, groupName, null);
+ addGroup(group);
+ }
+
+ user.addGroup(group);
}
- - digester.addFactoryCreate("tomcat-users/group",
- - new MemoryGroupCreationFactory(this), true);
- - digester.addFactoryCreate("tomcat-users/role",
- - new MemoryRoleCreationFactory(this), true);
- - digester.addFactoryCreate("tomcat-users/user",
- - new MemoryUserCreationFactory(this), true);
+ }
+
+ if (roles != null) {
+ for(String roleName : CSV.split(roles)) {
+ Role role = getRole(roleName);
+ if(null == role) {
+ role = new
MemoryRole(MemoryUserDatabase.this, roleName, null);
+ addRole(role);
+ }
- - // Parse the XML input to load this database
- - digester.parse(is);
- - } catch (IOException ioe) {
- -
log.error(sm.getString("memoryUserDatabase.fileNotFound", pathName));
+ user.addRole(role);
+ }
}
- - } catch (Exception e) {
- - // Fail safe on error
- - users.clear();
- - groups.clear();
- - roles.clear();
- - throw e;
- - } finally {
- - writeLock.unlock();
+ addUser(user);
}
- - }
+
+ public void addUser(User user) {
+ users.put(user.getName(), user);
+ }
+
+ public Role getRole(String name) {
+ return roles.get(name);
+ }
+ public Group getGroup(String name) { return groups.get(name); }
+ public User getUser(String name) { return users.get(name); }
+ public Map<String,User> getUsers() { return users; }
+ public Map<String,Group> getGroups() { return groups; }
+ public Map<String,Role> getRoles() { return roles; }
+ }
/**
* Remove the specified {@link Group} from this user database.
@@ -706,145 +791,3 @@
return sb.toString();
}
}
- -
- -
- -/**
- - * Digester object creation factory for group instances.
- - */
- -class MemoryGroupCreationFactory extends AbstractObjectCreationFactory {
- -
- - public MemoryGroupCreationFactory(MemoryUserDatabase database) {
- - this.database = database;
- - }
- -
- -
- - @Override
- - public Object createObject(Attributes attributes) {
- - String groupname = attributes.getValue("groupname");
- - if (groupname == null) {
- - groupname = attributes.getValue("name");
- - }
- - String description = attributes.getValue("description");
- - String roles = attributes.getValue("roles");
- - Group group = database.createGroup(groupname, description);
- - if (roles != null) {
- - while (roles.length() > 0) {
- - String rolename = null;
- - int comma = roles.indexOf(',');
- - if (comma >= 0) {
- - rolename = roles.substring(0, comma).trim();
- - roles = roles.substring(comma + 1);
- - } else {
- - rolename = roles.trim();
- - roles = "";
- - }
- - if (rolename.length() > 0) {
- - Role role = database.findRole(rolename);
- - if (role == null) {
- - role = database.createRole(rolename, null);
- - }
- - group.addRole(role);
- - }
- - }
- - }
- - return group;
- - }
- -
- - private final MemoryUserDatabase database;
- -}
- -
- -
- -/**
- - * Digester object creation factory for role instances.
- - */
- -class MemoryRoleCreationFactory extends AbstractObjectCreationFactory {
- -
- - public MemoryRoleCreationFactory(MemoryUserDatabase database) {
- - this.database = database;
- - }
- -
- -
- - @Override
- - public Object createObject(Attributes attributes) {
- - String rolename = attributes.getValue("rolename");
- - if (rolename == null) {
- - rolename = attributes.getValue("name");
- - }
- - String description = attributes.getValue("description");
- - Role role = database.createRole(rolename, description);
- - return role;
- - }
- -
- - private final MemoryUserDatabase database;
- -}
- -
- -
- -/**
- - * Digester object creation factory for user instances.
- - */
- -class MemoryUserCreationFactory extends AbstractObjectCreationFactory {
- -
- - public MemoryUserCreationFactory(MemoryUserDatabase database) {
- - this.database = database;
- - }
- -
- -
- - @Override
- - public Object createObject(Attributes attributes) {
- - String username = attributes.getValue("username");
- - if (username == null) {
- - username = attributes.getValue("name");
- - }
- - String password = attributes.getValue("password");
- - String fullName = attributes.getValue("fullName");
- - if (fullName == null) {
- - fullName = attributes.getValue("fullname");
- - }
- - String groups = attributes.getValue("groups");
- - String roles = attributes.getValue("roles");
- - User user = database.createUser(username, password, fullName);
- - if (groups != null) {
- - while (groups.length() > 0) {
- - String groupname = null;
- - int comma = groups.indexOf(',');
- - if (comma >= 0) {
- - groupname = groups.substring(0, comma).trim();
- - groups = groups.substring(comma + 1);
- - } else {
- - groupname = groups.trim();
- - groups = "";
- - }
- - if (groupname.length() > 0) {
- - Group group = database.findGroup(groupname);
- - if (group == null) {
- - group = database.createGroup(groupname, null);
- - }
- - user.addGroup(group);
- - }
- - }
- - }
- - if (roles != null) {
- - while (roles.length() > 0) {
- - String rolename = null;
- - int comma = roles.indexOf(',');
- - if (comma >= 0) {
- - rolename = roles.substring(0, comma).trim();
- - roles = roles.substring(comma + 1);
- - } else {
- - rolename = roles.trim();
- - roles = "";
- - }
- - if (rolename.length() > 0) {
- - Role role = database.findRole(rolename);
- - if (role == null) {
- - role = database.createRole(rolename, null);
- - }
- - user.addRole(role);
- - }
- - }
- - }
- - return user;
- - }
- -
- - private final MemoryUserDatabase database;
- -}
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlus/d0ACgkQHPApP6U8
pFjMLRAArZ6ysuGmiqCrb5JVSL0PH7zsaHXkUKWtGym0xvGEBtn6tbPMPg35DPwB
VVBFfoSBhv4jJfp9gyB/u41DDVp3rVWc3Fr/F8gNvAffh4MIJ/8aQW1MsPaUp9Nr
/8Upcx6B6ZhkIC/wfyTDUfhPSUalT72D8XCTqFkGZExNd1rySG42dRVRZc5CUQ/F
lppWJe3fnxBn5C8UGdaz1rO/T213NQPBLe8KzBVEOCiPPlqO3BrlAH252wZFtrmz
/I0grytvbMCJggeHGdDpnb8KRI6XSF7jgARNPK+cROnoVQF1XisvItXW2ErT3ghv
up+p4e0Q1Pcx5wcXOnRuERGv5Fcnvlcs90OmHFHjAbbKpTQhlBMg121GgivjHY4W
btTRepXG/v2rVJyfxdumRbeSnQRYNtUyp4hsQ333eMV8eaSRwKhUvMxLniw57YKJ
Nvtqd9BVv3SCMVK0p2g0P8EdYVrIm3tUfsZJtUdPaVI4+ekNkxuVQurimY3y8Tzj
uMadmUZiOMfUR05s76DBhDcn2u0lZbfjCcJLNx6XPM0Rm/8r/MLGMlrbQ4gM9P93
Hi3XJtKpoQC+my7gO6huYwntX3Yd2R/5XqKkenHrFt9CL6qZDiH+D6c1V7CHMx3t
JswHSsA6w+ARxyNsIpYLsvPRIzt8BF90060xuxvxwvRjckpFaEA=
=zlBU
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Proposed patch for o.a.c.users.MemoryUserDatabase
Posted by Igal Sapir <is...@apache.org>.
On 10/1/2018 12:08 PM, Mark Thomas wrote:
> On 27/09/18 16:57, Christopher Schultz wrote:
>> All,
>>
>> I have a proposed patch to MemoryUserDatabase that changes the
>> behavior when a triggered-reload fails. Recently, markt added code to
>> allow database reloads, but if there is an error reloading the
>> database, the database is emptied and perhaps an administrator can no
>> longer make e.g. calls to the manager.
>>
>> This patch makes an open-failure into a no-op: the user database will
>> not be changed unless there is a successful load from the file.
> Seems reasonable to me.
>
>> This patch changes the way that data is loaded by the Digester.
>> Instead of modifying the role/group/user maps directly, the data are
>> loaded into new maps and then all maps are updated atomically.
>>
>> This patch removes a bunch of code from this class, and I have a unit
>> test (not attached) which demonstrates that (a) it works and (b)
>> thread-safety is maintained.
> Woot for less code ;)
+1
Igal
> Mark
>
>
>> Thanks,
>> -chris
>>
>> For review:
>>
>> ### Eclipse Workspace Patch 1.0
>> #P tomcat-trunk
>> Index: java/org/apache/catalina/users/MemoryUserDatabase.java
>> ===================================================================
>> --- java/org/apache/catalina/users/MemoryUserDatabase.java (revision
>> 1842017)
>> +++ java/org/apache/catalina/users/MemoryUserDatabase.java (working copy)
>> @@ -31,6 +31,7 @@
>> import java.util.concurrent.ConcurrentHashMap;
>> import java.util.concurrent.locks.Lock;
>> import java.util.concurrent.locks.ReentrantReadWriteLock;
>> +import java.util.regex.Pattern;
>>
>> import org.apache.catalina.Globals;
>> import org.apache.catalina.Group;
>> @@ -39,11 +40,10 @@
>> import org.apache.catalina.UserDatabase;
>> import org.apache.juli.logging.Log;
>> import org.apache.juli.logging.LogFactory;
>> -import org.apache.tomcat.util.digester.AbstractObjectCreationFactory;
>> +import org.apache.tomcat.util.digester.CallParamRule;
>> import org.apache.tomcat.util.digester.Digester;
>> import org.apache.tomcat.util.file.ConfigFileLoader;
>> import org.apache.tomcat.util.res.StringManager;
>> -import org.xml.sax.Attributes;
>>
>> /**
>> * Concrete implementation of {@link UserDatabase} that loads all
>> defined users,
>> @@ -107,7 +107,7 @@
>> /**
>> * The set of {@link Group}s defined in this database, keyed by
>> group name.
>> */
>> - protected final Map<String, Group> groups = new
>> ConcurrentHashMap<>();
>> + protected Map<String, Group> groups = new ConcurrentHashMap<>();
>>
>> /**
>> * The unique global identifier of this user database.
>> @@ -140,12 +140,12 @@
>> /**
>> * The set of {@link Role}s defined in this database, keyed by
>> role name.
>> */
>> - protected final Map<String, Role> roles = new ConcurrentHashMap<>();
>> + protected Map<String, Role> roles = new ConcurrentHashMap<>();
>>
>> /**
>> * The set of {@link User}s defined in this database, keyed by
>> user name.
>> */
>> - protected final Map<String, User> users = new ConcurrentHashMap<>();
>> + protected Map<String, User> users = new ConcurrentHashMap<>();
>>
>> private final ReentrantReadWriteLock dbLock = new
>> ReentrantReadWriteLock();
>> private final Lock readLock = dbLock.readLock();
>> @@ -415,54 +415,139 @@
>> */
>> @Override
>> public void open() throws Exception {
>> + String pathName = getPathname();
>> + URI uri = ConfigFileLoader.getURI(pathName);
>> + URL url = uri.toURL();
>> + URLConnection uConn = url.openConnection();
>>
>> - writeLock.lock();
>> - try {
>> - // Erase any previous groups and users
>> - users.clear();
>> - groups.clear();
>> - roles.clear();
>> + try (InputStream is = uConn.getInputStream()) {
>> + this.lastModified = uConn.getLastModified();
>>
>> - String pathName = getPathname();
>> - URI uri = ConfigFileLoader.getURI(pathName);
>> - URL url = uri.toURL();
>> - URLConnection uConn = url.openConnection();
>> + // Construct a digester to read the XML input file
>> + Digester digester = new Digester();
>> + try {
>> + digester.setFeature(
>> +
>> "http://apache.org/xml/features/allow-java-encodings", true);
>> + } catch (Exception e) {
>> +
>> log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
>> + }
>>
>> - try (InputStream is = uConn.getInputStream()) {
>> - this.lastModified = uConn.getLastModified();
>> + Bundle bundle = new Bundle();
>> + digester.push(bundle);
>> + digester.addCallMethod("tomcat-users/role", "addRole", 2);
>> + digester.addRule("tomcat-users/role", new
>> CallParamRule(0, "rolename"));
>> + digester.addRule("tomcat-users/role", new
>> CallParamRule(1, "description"));
>> + digester.addCallMethod("tomcat-users/group", "addGroup", 3);
>> + digester.addRule("tomcat-users/group", new
>> CallParamRule(0, "groupname"));
>> + digester.addRule("tomcat-users/group", new
>> CallParamRule(1, "description"));
>> + digester.addRule("tomcat-users/group", new
>> CallParamRule(2, "roles"));
>> + digester.addCallMethod("tomcat-users/user", "addUser", 5);
>> + digester.addRule("tomcat-users/user", new
>> CallParamRule(0, "username"));
>> + digester.addRule("tomcat-users/user", new
>> CallParamRule(1, "fullname"));
>> + digester.addRule("tomcat-users/user", new
>> CallParamRule(2, "password"));
>> + digester.addRule("tomcat-users/user", new
>> CallParamRule(3, "roles"));
>> + digester.addRule("tomcat-users/user", new
>> CallParamRule(4, "groups"));
>>
>> - // Construct a digester to read the XML input file
>> - Digester digester = new Digester();
>> - try {
>> - digester.setFeature(
>> -
>> "http://apache.org/xml/features/allow-java-encodings", true);
>> - } catch (Exception e) {
>> -
>> log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
>> + // Parse the XML input to load this database
>> + digester.parse(is);
>> +
>> + // Update all maps simultaneously
>> + writeLock.lock();
>> + try {
>> + roles = bundle.getRoles();
>> + groups = bundle.getGroups();
>> + users = bundle.getUsers();
>> + } finally {
>> + writeLock.unlock();
>> + }
>> + } catch (IOException ioe) {
>> + log.error(sm.getString("memoryUserDatabase.fileNotFound",
>> pathName));
>> + }
>> + }
>> +
>> + private static final Pattern CSV = Pattern.compile("\\s*,\\s*");
>> + /**
>> + * A wrapper around the role/group/user maps managed by the
>> MemoryUserDatabase,
>> + * used for loading the database
>> + */
>> + public class Bundle {
>> + ConcurrentHashMap<String,User> users = new ConcurrentHashMap<>();
>> + ConcurrentHashMap<String,Group> groups = new
>> ConcurrentHashMap<>();
>> + ConcurrentHashMap<String,Role> roles = new ConcurrentHashMap<>();
>> +
>> + public void addRole(Role role) {
>> + roles.put(role.getName(), role);
>> + }
>> +
>> + public void addRole(String name, String description) {
>> + addRole(new MemoryRole(MemoryUserDatabase.this, name,
>> description));
>> + }
>> +
>> + public void addGroup(Group group) {
>> + groups.put(group.getName(), group);
>> + }
>> +
>> + public void addGroup(String name, String description, String
>> roles) {
>> + Group group = new MemoryGroup(MemoryUserDatabase.this,
>> name, description);
>> +
>> + if(null != roles) {
>> + for(String roleName : CSV.split(roles)) {
>> + Role role = getRole(roleName);
>> + if(null == role) {
>> + role = new
>> MemoryRole(MemoryUserDatabase.this, roleName, null);
>> + addRole(role);
>> + }
>> + group.addRole(role);
>> + }
>> + }
>> +
>> + addGroup(group);
>> + }
>> +
>> + public void addUser(String name, String fullname, String
>> password, String roles, String groups) {
>> + User user = new MemoryUser(MemoryUserDatabase.this, name,
>> password, fullname);
>> +
>> + if (groups != null) {
>> + for(String groupName : CSV.split(groups)) {
>> + Group group = getGroup(groupName);
>> + if(null == group) {
>> + group = new
>> MemoryGroup(MemoryUserDatabase.this, groupName, null);
>> + addGroup(group);
>> + }
>> +
>> + user.addGroup(group);
>> }
>> - digester.addFactoryCreate("tomcat-users/group",
>> - new MemoryGroupCreationFactory(this), true);
>> - digester.addFactoryCreate("tomcat-users/role",
>> - new MemoryRoleCreationFactory(this), true);
>> - digester.addFactoryCreate("tomcat-users/user",
>> - new MemoryUserCreationFactory(this), true);
>> + }
>> +
>> + if (roles != null) {
>> + for(String roleName : CSV.split(roles)) {
>> + Role role = getRole(roleName);
>> + if(null == role) {
>> + role = new
>> MemoryRole(MemoryUserDatabase.this, roleName, null);
>> + addRole(role);
>> + }
>>
>> - // Parse the XML input to load this database
>> - digester.parse(is);
>> - } catch (IOException ioe) {
>> -
>> log.error(sm.getString("memoryUserDatabase.fileNotFound", pathName));
>> + user.addRole(role);
>> + }
>> }
>> - } catch (Exception e) {
>> - // Fail safe on error
>> - users.clear();
>> - groups.clear();
>> - roles.clear();
>>
>> - throw e;
>> - } finally {
>> - writeLock.unlock();
>> + addUser(user);
>> }
>> - }
>> +
>> + public void addUser(User user) {
>> + users.put(user.getName(), user);
>> + }
>> +
>> + public Role getRole(String name) {
>> + return roles.get(name);
>> + }
>> + public Group getGroup(String name) { return groups.get(name); }
>> + public User getUser(String name) { return users.get(name); }
>>
>> + public Map<String,User> getUsers() { return users; }
>> + public Map<String,Group> getGroups() { return groups; }
>> + public Map<String,Role> getRoles() { return roles; }
>> + }
>>
>> /**
>> * Remove the specified {@link Group} from this user database.
>> @@ -706,145 +791,3 @@
>> return sb.toString();
>> }
>> }
>> -
>> -
>> -/**
>> - * Digester object creation factory for group instances.
>> - */
>> -class MemoryGroupCreationFactory extends AbstractObjectCreationFactory {
>> -
>> - public MemoryGroupCreationFactory(MemoryUserDatabase database) {
>> - this.database = database;
>> - }
>> -
>> -
>> - @Override
>> - public Object createObject(Attributes attributes) {
>> - String groupname = attributes.getValue("groupname");
>> - if (groupname == null) {
>> - groupname = attributes.getValue("name");
>> - }
>> - String description = attributes.getValue("description");
>> - String roles = attributes.getValue("roles");
>> - Group group = database.createGroup(groupname, description);
>> - if (roles != null) {
>> - while (roles.length() > 0) {
>> - String rolename = null;
>> - int comma = roles.indexOf(',');
>> - if (comma >= 0) {
>> - rolename = roles.substring(0, comma).trim();
>> - roles = roles.substring(comma + 1);
>> - } else {
>> - rolename = roles.trim();
>> - roles = "";
>> - }
>> - if (rolename.length() > 0) {
>> - Role role = database.findRole(rolename);
>> - if (role == null) {
>> - role = database.createRole(rolename, null);
>> - }
>> - group.addRole(role);
>> - }
>> - }
>> - }
>> - return group;
>> - }
>> -
>> - private final MemoryUserDatabase database;
>> -}
>> -
>> -
>> -/**
>> - * Digester object creation factory for role instances.
>> - */
>> -class MemoryRoleCreationFactory extends AbstractObjectCreationFactory {
>> -
>> - public MemoryRoleCreationFactory(MemoryUserDatabase database) {
>> - this.database = database;
>> - }
>> -
>> -
>> - @Override
>> - public Object createObject(Attributes attributes) {
>> - String rolename = attributes.getValue("rolename");
>> - if (rolename == null) {
>> - rolename = attributes.getValue("name");
>> - }
>> - String description = attributes.getValue("description");
>> - Role role = database.createRole(rolename, description);
>> - return role;
>> - }
>> -
>> - private final MemoryUserDatabase database;
>> -}
>> -
>> -
>> -/**
>> - * Digester object creation factory for user instances.
>> - */
>> -class MemoryUserCreationFactory extends AbstractObjectCreationFactory {
>> -
>> - public MemoryUserCreationFactory(MemoryUserDatabase database) {
>> - this.database = database;
>> - }
>> -
>> -
>> - @Override
>> - public Object createObject(Attributes attributes) {
>> - String username = attributes.getValue("username");
>> - if (username == null) {
>> - username = attributes.getValue("name");
>> - }
>> - String password = attributes.getValue("password");
>> - String fullName = attributes.getValue("fullName");
>> - if (fullName == null) {
>> - fullName = attributes.getValue("fullname");
>> - }
>> - String groups = attributes.getValue("groups");
>> - String roles = attributes.getValue("roles");
>> - User user = database.createUser(username, password, fullName);
>> - if (groups != null) {
>> - while (groups.length() > 0) {
>> - String groupname = null;
>> - int comma = groups.indexOf(',');
>> - if (comma >= 0) {
>> - groupname = groups.substring(0, comma).trim();
>> - groups = groups.substring(comma + 1);
>> - } else {
>> - groupname = groups.trim();
>> - groups = "";
>> - }
>> - if (groupname.length() > 0) {
>> - Group group = database.findGroup(groupname);
>> - if (group == null) {
>> - group = database.createGroup(groupname, null);
>> - }
>> - user.addGroup(group);
>> - }
>> - }
>> - }
>> - if (roles != null) {
>> - while (roles.length() > 0) {
>> - String rolename = null;
>> - int comma = roles.indexOf(',');
>> - if (comma >= 0) {
>> - rolename = roles.substring(0, comma).trim();
>> - roles = roles.substring(comma + 1);
>> - } else {
>> - rolename = roles.trim();
>> - roles = "";
>> - }
>> - if (rolename.length() > 0) {
>> - Role role = database.findRole(rolename);
>> - if (role == null) {
>> - role = database.createRole(rolename, null);
>> - }
>> - user.addRole(role);
>> - }
>> - }
>> - }
>> - return user;
>> - }
>> -
>> - private final MemoryUserDatabase database;
>> -}
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Proposed patch for o.a.c.users.MemoryUserDatabase
Posted by Mark Thomas <ma...@apache.org>.
On 27/09/18 16:57, Christopher Schultz wrote:
> All,
>
> I have a proposed patch to MemoryUserDatabase that changes the
> behavior when a triggered-reload fails. Recently, markt added code to
> allow database reloads, but if there is an error reloading the
> database, the database is emptied and perhaps an administrator can no
> longer make e.g. calls to the manager.
>
> This patch makes an open-failure into a no-op: the user database will
> not be changed unless there is a successful load from the file.
Seems reasonable to me.
> This patch changes the way that data is loaded by the Digester.
> Instead of modifying the role/group/user maps directly, the data are
> loaded into new maps and then all maps are updated atomically.
>
> This patch removes a bunch of code from this class, and I have a unit
> test (not attached) which demonstrates that (a) it works and (b)
> thread-safety is maintained.
Woot for less code ;)
Mark
>
> Thanks,
> -chris
>
> For review:
>
> ### Eclipse Workspace Patch 1.0
> #P tomcat-trunk
> Index: java/org/apache/catalina/users/MemoryUserDatabase.java
> ===================================================================
> --- java/org/apache/catalina/users/MemoryUserDatabase.java (revision
> 1842017)
> +++ java/org/apache/catalina/users/MemoryUserDatabase.java (working copy)
> @@ -31,6 +31,7 @@
> import java.util.concurrent.ConcurrentHashMap;
> import java.util.concurrent.locks.Lock;
> import java.util.concurrent.locks.ReentrantReadWriteLock;
> +import java.util.regex.Pattern;
>
> import org.apache.catalina.Globals;
> import org.apache.catalina.Group;
> @@ -39,11 +40,10 @@
> import org.apache.catalina.UserDatabase;
> import org.apache.juli.logging.Log;
> import org.apache.juli.logging.LogFactory;
> -import org.apache.tomcat.util.digester.AbstractObjectCreationFactory;
> +import org.apache.tomcat.util.digester.CallParamRule;
> import org.apache.tomcat.util.digester.Digester;
> import org.apache.tomcat.util.file.ConfigFileLoader;
> import org.apache.tomcat.util.res.StringManager;
> -import org.xml.sax.Attributes;
>
> /**
> * Concrete implementation of {@link UserDatabase} that loads all
> defined users,
> @@ -107,7 +107,7 @@
> /**
> * The set of {@link Group}s defined in this database, keyed by
> group name.
> */
> - protected final Map<String, Group> groups = new
> ConcurrentHashMap<>();
> + protected Map<String, Group> groups = new ConcurrentHashMap<>();
>
> /**
> * The unique global identifier of this user database.
> @@ -140,12 +140,12 @@
> /**
> * The set of {@link Role}s defined in this database, keyed by
> role name.
> */
> - protected final Map<String, Role> roles = new ConcurrentHashMap<>();
> + protected Map<String, Role> roles = new ConcurrentHashMap<>();
>
> /**
> * The set of {@link User}s defined in this database, keyed by
> user name.
> */
> - protected final Map<String, User> users = new ConcurrentHashMap<>();
> + protected Map<String, User> users = new ConcurrentHashMap<>();
>
> private final ReentrantReadWriteLock dbLock = new
> ReentrantReadWriteLock();
> private final Lock readLock = dbLock.readLock();
> @@ -415,54 +415,139 @@
> */
> @Override
> public void open() throws Exception {
> + String pathName = getPathname();
> + URI uri = ConfigFileLoader.getURI(pathName);
> + URL url = uri.toURL();
> + URLConnection uConn = url.openConnection();
>
> - writeLock.lock();
> - try {
> - // Erase any previous groups and users
> - users.clear();
> - groups.clear();
> - roles.clear();
> + try (InputStream is = uConn.getInputStream()) {
> + this.lastModified = uConn.getLastModified();
>
> - String pathName = getPathname();
> - URI uri = ConfigFileLoader.getURI(pathName);
> - URL url = uri.toURL();
> - URLConnection uConn = url.openConnection();
> + // Construct a digester to read the XML input file
> + Digester digester = new Digester();
> + try {
> + digester.setFeature(
> +
> "http://apache.org/xml/features/allow-java-encodings", true);
> + } catch (Exception e) {
> +
> log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
> + }
>
> - try (InputStream is = uConn.getInputStream()) {
> - this.lastModified = uConn.getLastModified();
> + Bundle bundle = new Bundle();
> + digester.push(bundle);
> + digester.addCallMethod("tomcat-users/role", "addRole", 2);
> + digester.addRule("tomcat-users/role", new
> CallParamRule(0, "rolename"));
> + digester.addRule("tomcat-users/role", new
> CallParamRule(1, "description"));
> + digester.addCallMethod("tomcat-users/group", "addGroup", 3);
> + digester.addRule("tomcat-users/group", new
> CallParamRule(0, "groupname"));
> + digester.addRule("tomcat-users/group", new
> CallParamRule(1, "description"));
> + digester.addRule("tomcat-users/group", new
> CallParamRule(2, "roles"));
> + digester.addCallMethod("tomcat-users/user", "addUser", 5);
> + digester.addRule("tomcat-users/user", new
> CallParamRule(0, "username"));
> + digester.addRule("tomcat-users/user", new
> CallParamRule(1, "fullname"));
> + digester.addRule("tomcat-users/user", new
> CallParamRule(2, "password"));
> + digester.addRule("tomcat-users/user", new
> CallParamRule(3, "roles"));
> + digester.addRule("tomcat-users/user", new
> CallParamRule(4, "groups"));
>
> - // Construct a digester to read the XML input file
> - Digester digester = new Digester();
> - try {
> - digester.setFeature(
> -
> "http://apache.org/xml/features/allow-java-encodings", true);
> - } catch (Exception e) {
> -
> log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
> + // Parse the XML input to load this database
> + digester.parse(is);
> +
> + // Update all maps simultaneously
> + writeLock.lock();
> + try {
> + roles = bundle.getRoles();
> + groups = bundle.getGroups();
> + users = bundle.getUsers();
> + } finally {
> + writeLock.unlock();
> + }
> + } catch (IOException ioe) {
> + log.error(sm.getString("memoryUserDatabase.fileNotFound",
> pathName));
> + }
> + }
> +
> + private static final Pattern CSV = Pattern.compile("\\s*,\\s*");
> + /**
> + * A wrapper around the role/group/user maps managed by the
> MemoryUserDatabase,
> + * used for loading the database
> + */
> + public class Bundle {
> + ConcurrentHashMap<String,User> users = new ConcurrentHashMap<>();
> + ConcurrentHashMap<String,Group> groups = new
> ConcurrentHashMap<>();
> + ConcurrentHashMap<String,Role> roles = new ConcurrentHashMap<>();
> +
> + public void addRole(Role role) {
> + roles.put(role.getName(), role);
> + }
> +
> + public void addRole(String name, String description) {
> + addRole(new MemoryRole(MemoryUserDatabase.this, name,
> description));
> + }
> +
> + public void addGroup(Group group) {
> + groups.put(group.getName(), group);
> + }
> +
> + public void addGroup(String name, String description, String
> roles) {
> + Group group = new MemoryGroup(MemoryUserDatabase.this,
> name, description);
> +
> + if(null != roles) {
> + for(String roleName : CSV.split(roles)) {
> + Role role = getRole(roleName);
> + if(null == role) {
> + role = new
> MemoryRole(MemoryUserDatabase.this, roleName, null);
> + addRole(role);
> + }
> + group.addRole(role);
> + }
> + }
> +
> + addGroup(group);
> + }
> +
> + public void addUser(String name, String fullname, String
> password, String roles, String groups) {
> + User user = new MemoryUser(MemoryUserDatabase.this, name,
> password, fullname);
> +
> + if (groups != null) {
> + for(String groupName : CSV.split(groups)) {
> + Group group = getGroup(groupName);
> + if(null == group) {
> + group = new
> MemoryGroup(MemoryUserDatabase.this, groupName, null);
> + addGroup(group);
> + }
> +
> + user.addGroup(group);
> }
> - digester.addFactoryCreate("tomcat-users/group",
> - new MemoryGroupCreationFactory(this), true);
> - digester.addFactoryCreate("tomcat-users/role",
> - new MemoryRoleCreationFactory(this), true);
> - digester.addFactoryCreate("tomcat-users/user",
> - new MemoryUserCreationFactory(this), true);
> + }
> +
> + if (roles != null) {
> + for(String roleName : CSV.split(roles)) {
> + Role role = getRole(roleName);
> + if(null == role) {
> + role = new
> MemoryRole(MemoryUserDatabase.this, roleName, null);
> + addRole(role);
> + }
>
> - // Parse the XML input to load this database
> - digester.parse(is);
> - } catch (IOException ioe) {
> -
> log.error(sm.getString("memoryUserDatabase.fileNotFound", pathName));
> + user.addRole(role);
> + }
> }
> - } catch (Exception e) {
> - // Fail safe on error
> - users.clear();
> - groups.clear();
> - roles.clear();
>
> - throw e;
> - } finally {
> - writeLock.unlock();
> + addUser(user);
> }
> - }
> +
> + public void addUser(User user) {
> + users.put(user.getName(), user);
> + }
> +
> + public Role getRole(String name) {
> + return roles.get(name);
> + }
> + public Group getGroup(String name) { return groups.get(name); }
> + public User getUser(String name) { return users.get(name); }
>
> + public Map<String,User> getUsers() { return users; }
> + public Map<String,Group> getGroups() { return groups; }
> + public Map<String,Role> getRoles() { return roles; }
> + }
>
> /**
> * Remove the specified {@link Group} from this user database.
> @@ -706,145 +791,3 @@
> return sb.toString();
> }
> }
> -
> -
> -/**
> - * Digester object creation factory for group instances.
> - */
> -class MemoryGroupCreationFactory extends AbstractObjectCreationFactory {
> -
> - public MemoryGroupCreationFactory(MemoryUserDatabase database) {
> - this.database = database;
> - }
> -
> -
> - @Override
> - public Object createObject(Attributes attributes) {
> - String groupname = attributes.getValue("groupname");
> - if (groupname == null) {
> - groupname = attributes.getValue("name");
> - }
> - String description = attributes.getValue("description");
> - String roles = attributes.getValue("roles");
> - Group group = database.createGroup(groupname, description);
> - if (roles != null) {
> - while (roles.length() > 0) {
> - String rolename = null;
> - int comma = roles.indexOf(',');
> - if (comma >= 0) {
> - rolename = roles.substring(0, comma).trim();
> - roles = roles.substring(comma + 1);
> - } else {
> - rolename = roles.trim();
> - roles = "";
> - }
> - if (rolename.length() > 0) {
> - Role role = database.findRole(rolename);
> - if (role == null) {
> - role = database.createRole(rolename, null);
> - }
> - group.addRole(role);
> - }
> - }
> - }
> - return group;
> - }
> -
> - private final MemoryUserDatabase database;
> -}
> -
> -
> -/**
> - * Digester object creation factory for role instances.
> - */
> -class MemoryRoleCreationFactory extends AbstractObjectCreationFactory {
> -
> - public MemoryRoleCreationFactory(MemoryUserDatabase database) {
> - this.database = database;
> - }
> -
> -
> - @Override
> - public Object createObject(Attributes attributes) {
> - String rolename = attributes.getValue("rolename");
> - if (rolename == null) {
> - rolename = attributes.getValue("name");
> - }
> - String description = attributes.getValue("description");
> - Role role = database.createRole(rolename, description);
> - return role;
> - }
> -
> - private final MemoryUserDatabase database;
> -}
> -
> -
> -/**
> - * Digester object creation factory for user instances.
> - */
> -class MemoryUserCreationFactory extends AbstractObjectCreationFactory {
> -
> - public MemoryUserCreationFactory(MemoryUserDatabase database) {
> - this.database = database;
> - }
> -
> -
> - @Override
> - public Object createObject(Attributes attributes) {
> - String username = attributes.getValue("username");
> - if (username == null) {
> - username = attributes.getValue("name");
> - }
> - String password = attributes.getValue("password");
> - String fullName = attributes.getValue("fullName");
> - if (fullName == null) {
> - fullName = attributes.getValue("fullname");
> - }
> - String groups = attributes.getValue("groups");
> - String roles = attributes.getValue("roles");
> - User user = database.createUser(username, password, fullName);
> - if (groups != null) {
> - while (groups.length() > 0) {
> - String groupname = null;
> - int comma = groups.indexOf(',');
> - if (comma >= 0) {
> - groupname = groups.substring(0, comma).trim();
> - groups = groups.substring(comma + 1);
> - } else {
> - groupname = groups.trim();
> - groups = "";
> - }
> - if (groupname.length() > 0) {
> - Group group = database.findGroup(groupname);
> - if (group == null) {
> - group = database.createGroup(groupname, null);
> - }
> - user.addGroup(group);
> - }
> - }
> - }
> - if (roles != null) {
> - while (roles.length() > 0) {
> - String rolename = null;
> - int comma = roles.indexOf(',');
> - if (comma >= 0) {
> - rolename = roles.substring(0, comma).trim();
> - roles = roles.substring(comma + 1);
> - } else {
> - rolename = roles.trim();
> - roles = "";
> - }
> - if (rolename.length() > 0) {
> - Role role = database.findRole(rolename);
> - if (role == null) {
> - role = database.createRole(rolename, null);
> - }
> - user.addRole(role);
> - }
> - }
> - }
> - return user;
> - }
> -
> - private final MemoryUserDatabase database;
> -}
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org