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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to