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: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
