tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: Proposed patch for o.a.c.users.MemoryUserDatabase
Date Mon, 01 Oct 2018 19:08:58 GMT
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


Mime
View raw message