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