tomcat-dev mailing list archives

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


Mime
View raw message