Merge lp:~nwilliams/akiban-server/security-service-login-service into lp:~akiban-technologies/akiban-server/trunk

[Nathan Williams]

Replace SingleThreadedJDBCService with new one backed directly by the SecurityService.

The JDBCService is racey and doesn't work with the requirements of our EmbeddedJDBCService so just ditch it.

Expose more of the information off of the User (password hashes, all roles). It wasn't a direct concern, but the new LoginService also has a cache time so the performance should be identical.

The test failed quite consistently for the 5, 10, and 20 thread cases before the fix. Now passes as desired.

[lp:~nwilliams/akiban-server/security-service-login-service]

2638. By Nathan Williams on 2013-04-22

Remove unneeded import

[Thomas Jones-Low (tjoneslo)]

Well, this will certainly make user process tracking simpler.
Otherwise looks as described.

review: Approve

=== modified file 'src/main/java/com/akiban/http/HttpConductorImpl.java'
--- src/main/java/com/akiban/http/HttpConductorImpl.java	2013-04-14 16:11:04 +0000
+++ src/main/java/com/akiban/http/HttpConductorImpl.java	2013-04-22 23:01:29 +0000
@@ -17,19 +17,18 @@
 
 package com.akiban.http;
 
-import com.akiban.server.error.AkibanInternalException;
 import com.akiban.server.service.Service;
 import com.akiban.server.service.config.ConfigurationService;
 import com.akiban.server.service.monitor.MonitorService;
 import com.akiban.server.service.monitor.ServerMonitor;
 import com.akiban.server.service.security.SecurityService;
+import com.akiban.sql.embedded.EmbeddedJDBCService;
 import com.google.inject.Inject;
 import org.eclipse.jetty.servlets.CrossOriginFilter;
 import org.eclipse.jetty.util.security.Constraint;
 import org.eclipse.jetty.security.Authenticator;
 import org.eclipse.jetty.security.ConstraintMapping;
 import org.eclipse.jetty.security.ConstraintSecurityHandler;
-import org.eclipse.jetty.security.JDBCLoginService;
 import org.eclipse.jetty.security.LoginService;
 import org.eclipse.jetty.security.authentication.BasicAuthenticator;
 import org.eclipse.jetty.security.authentication.DigestAuthenticator;
@@ -45,8 +44,6 @@
 
 import javax.servlet.FilterRegistration;
 import javax.servlet.ServletException;
-import java.io.IOException;
-import java.lang.reflect.Method;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
@@ -59,6 +56,7 @@
     private static final String CONFIG_PORT_PROPERTY = CONFIG_HTTP_PREFIX + "port";
     private static final String CONFIG_SSL_PROPERTY = CONFIG_HTTP_PREFIX + "ssl";
     private static final String CONFIG_LOGIN_PROPERTY = CONFIG_HTTP_PREFIX + "login";
+    private static final String CONFIG_LOGIN_CACHE_SECONDS = CONFIG_HTTP_PREFIX + "login_cache_seconds";
     private static final String CONFIG_XORIGIN_PREFIX = CONFIG_HTTP_PREFIX + "cross_origin.";
     private static final String CONFIG_XORIGIN_ENABLED = CONFIG_XORIGIN_PREFIX + "enabled";
     private static final String CONFIG_XORIGIN_ORIGINS = CONFIG_XORIGIN_PREFIX + "allowed_origins";
@@ -152,6 +150,7 @@
         String portProperty = configurationService.getProperty(CONFIG_PORT_PROPERTY);
         String sslProperty = configurationService.getProperty(CONFIG_SSL_PROPERTY);
         String loginProperty = configurationService.getProperty(CONFIG_LOGIN_PROPERTY);
+        int loginCacheSeconds = Integer.parseInt(configurationService.getProperty(CONFIG_LOGIN_CACHE_SECONDS));
         int portLocal;
         boolean ssl;
         AuthenticationType login;
@@ -210,20 +209,20 @@
                 localServer.setHandler(localHandlerList);
             }
             else {
-                String resource;
+                SecurityServiceLoginService.CredentialType credentialType;
                 Authenticator authenticator;
                 switch (login) {
                 case BASIC:
-                    resource = "basic.properties";
+                    credentialType = SecurityServiceLoginService.CredentialType.BASIC;
                     authenticator = new BasicAuthenticator();
                     break;
                 case DIGEST:
-                    resource = "digest.properties";
+                    credentialType = SecurityServiceLoginService.CredentialType.DIGEST;
                     authenticator = new DigestAuthenticator();
                     break;
                 default:
                     assert false : "Unexpected authentication type " + login;
-                    resource = null;
+                    credentialType = null;
                     authenticator = null;
                 }
                 Constraint constraint = new Constraint(authenticator.getAuthMethod(),
@@ -238,9 +237,7 @@
                 sh.setAuthenticator(authenticator);
                 sh.setConstraintMappings(Collections.singletonList(cm));
 
-                LoginService loginService =
-                    new SingleThreadJDBCLoginService(SecurityService.REALM, 
-                                                     HttpConductorImpl.class.getResource(resource).toString());
+                LoginService loginService = new SecurityServiceLoginService(securityService, credentialType, loginCacheSeconds);
                 sh.setLoginService(loginService);
 
                 sh.setHandler(localHandlerList);
@@ -315,42 +312,6 @@
         return result;
     }
 
-    // Embedded JDBC is single-threaded, but login service assumes it
-    // is thread-safe.  Also, it does not close its ResultSet, which
-    // leaves a transaction active. So just close connection around
-    // each use.  Login service is already
-    // synchronized. Unfortunately, this needs a private method.
-    static class SingleThreadJDBCLoginService extends JDBCLoginService {
-        private Method closeMethod;
-
-        public SingleThreadJDBCLoginService(String name, String config)
-                throws IOException {
-            super(name, config);
-            try {
-                closeMethod = JDBCLoginService.class.getDeclaredMethod("closeConnection", null);
-                closeMethod.setAccessible(true);
-            }
-            catch (Exception ex) {
-                throw new AkibanInternalException("Cannot get JDBC close method", ex);
-            }
-        }
-
-        @Override
-        protected org.eclipse.jetty.server.UserIdentity loadUser(String username) {
-            try {
-                return super.loadUser(username);
-            }
-            finally {
-                try {
-                    closeMethod.invoke(this);
-                }
-                catch (Exception ex) {
-                    logger.warn("Cannot call JDBC close method", ex);
-                }
-            }
-        }
-    }
-    
     private class ConnectionMonitor implements ServerMonitor {
         public static final String SERVER_TYPE = "REST";
         private final SelectChannelConnector connector;

=== added file 'src/main/java/com/akiban/http/SecurityServiceLoginService.java'
--- src/main/java/com/akiban/http/SecurityServiceLoginService.java	1970-01-01 00:00:00 +0000
+++ src/main/java/com/akiban/http/SecurityServiceLoginService.java	2013-04-22 23:01:29 +0000
@@ -0,0 +1,72 @@
+/**
+ * Copyright (C) 2009-2013 Akiban Technologies, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package com.akiban.http;
+
+import java.util.List;
+
+import com.akiban.server.service.security.SecurityService;
+import com.akiban.server.service.security.User;
+import com.akiban.util.ArgumentValidation;
+import org.eclipse.jetty.security.MappedLoginService;
+import org.eclipse.jetty.server.UserIdentity;
+import org.eclipse.jetty.util.security.Credential;
+
+public class SecurityServiceLoginService extends MappedLoginService
+{
+    public enum CredentialType { BASIC, DIGEST }
+
+    private final SecurityService securityService;
+    private final CredentialType credentialType;
+    private final long cacheMillis;
+    private volatile long lastCachePurge;
+
+    public SecurityServiceLoginService(SecurityService securityService, CredentialType credentialType, int cacheSeconds) {
+        ArgumentValidation.isGTE("cacheSeconds", cacheSeconds, 0);
+        if(credentialType != CredentialType.BASIC && credentialType != CredentialType.DIGEST) {
+            throw new IllegalArgumentException("Unknown credential: " + credentialType);
+        }
+        this.securityService = securityService;
+        this.credentialType = credentialType;
+        this.cacheMillis = cacheSeconds * 1000;
+    }
+
+    @Override
+    public UserIdentity login(String username, Object credentials) {
+        long now = System.currentTimeMillis();
+        if((now - lastCachePurge) > cacheMillis) {
+            super._users.clear();
+            lastCachePurge = now;
+        }
+        return super.login(username, credentials);
+    }
+
+    @Override
+    protected void loadUsers() {
+    }
+
+    @Override
+    protected UserIdentity loadUser(String username) {
+        User user = securityService.getUser(username);
+        if(user != null) {
+            String password = (credentialType == CredentialType.BASIC) ? user.getBasicPassword() : user.getDigestPassword();
+            List<String> roles = user.getRoles();
+            return putUser(username, Credential.getCredential(password), roles.toArray(new String[roles.size()]));
+        }
+        return null;
+    }
+}

=== modified file 'src/main/java/com/akiban/server/service/security/SecurityServiceImpl.java'
--- src/main/java/com/akiban/server/service/security/SecurityServiceImpl.java	2013-03-27 10:45:17 +0000
+++ src/main/java/com/akiban/server/service/security/SecurityServiceImpl.java	2013-04-22 23:01:29 +0000
@@ -204,7 +204,7 @@
             roles.add(rs1.getString(2));
         }
         rs1.close();
-        return new User(rs.getInt(1), rs.getString(2), roles);
+        return new User(rs.getInt(1), rs.getString(2), rs.getString(3), rs.getString(4), roles);
     }
 
     @Override
@@ -212,12 +212,14 @@
         int id;
         Connection conn = null;
         PreparedStatement stmt = null;
+        String basicPassword = basicPassword(password);
+        String digestPassword = digestPassword(name, password);
         try {
             conn = openConnection();
             stmt = conn.prepareStatement(ADD_USER_SQL);
             stmt.setString(1, name);
-            stmt.setString(2, basicPassword(password));
-            stmt.setString(3, digestPassword(name, password));
+            stmt.setString(2, basicPassword);
+            stmt.setString(3, digestPassword);
             stmt.setString(4, md5Password(name, password));
             int nrows = stmt.executeUpdate();
             if (nrows != 1) {
@@ -250,7 +252,7 @@
         finally {
             cleanup(conn, stmt);
         }
-        return new User(id, name, new ArrayList<>(roles));
+        return new User(id, name, basicPassword, digestPassword, new ArrayList<>(roles));
     }
 
     @Override

=== modified file 'src/main/java/com/akiban/server/service/security/User.java'
--- src/main/java/com/akiban/server/service/security/User.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/server/service/security/User.java	2013-04-22 23:01:29 +0000
@@ -25,11 +25,15 @@
 {
     private final int id;
     private final String name;
+    private final String basicPassword;
+    private final String digestPassword;
     private final List<String> roles;
 
-    protected User(int id, String name, List<String> roles) {
+    protected User(int id, String name, String basicPassword, String digestPassword, List<String> roles) {
         this.id = id;
         this.name = name;
+        this.basicPassword = basicPassword;
+        this.digestPassword = digestPassword;
         this.roles = roles;
     }
 
@@ -42,6 +46,18 @@
         return name;
     }
 
+    public String getBasicPassword() {
+        return basicPassword;
+    }
+
+    public String getDigestPassword() {
+        return digestPassword;
+    }
+
+    public List<String> getRoles() {
+        return roles;
+    }
+
     public boolean hasRole(String role) {
         return roles.contains(role);
     }

=== removed file 'src/main/resources/com/akiban/http/basic.properties'
--- src/main/resources/com/akiban/http/basic.properties	2013-03-22 20:05:57 +0000
+++ src/main/resources/com/akiban/http/basic.properties	1970-01-01 00:00:00 +0000
@@ -1,33 +0,0 @@
-#
-# Copyright (C) 2009-2013 Akiban Technologies, Inc.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Affero General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU Affero General Public License for more details.
-#
-# You should have received a copy of the GNU Affero General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-
-# Driver doesn't matter (already loaded), but must have no-arg ctor.
-jdbcdriver=java.lang.Object
-url=jdbc:default:connection
-username=akiban
-password=akiban
-usertable=security_schema.users
-usertablekey=id
-usertableuserfield=name
-usertablepasswordfield=password_basic
-roletable=security_schema.roles
-roletablekey=id
-roletablerolefield=name
-userroletable=security_schema.user_roles
-userroletableuserkey=user_id
-userroletablerolekey=role_id
-cachetime=300

=== removed file 'src/main/resources/com/akiban/http/digest.properties'
--- src/main/resources/com/akiban/http/digest.properties	2013-03-22 20:05:57 +0000
+++ src/main/resources/com/akiban/http/digest.properties	1970-01-01 00:00:00 +0000
@@ -1,33 +0,0 @@
-#
-# Copyright (C) 2009-2013 Akiban Technologies, Inc.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Affero General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU Affero General Public License for more details.
-#
-# You should have received a copy of the GNU Affero General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-
-# Driver doesn't matter (already loaded), but must have no-arg ctor.
-jdbcdriver=java.lang.Object
-url=jdbc:default:connection
-username=akiban
-password=akiban
-usertable=security_schema.users
-usertablekey=id
-usertableuserfield=name
-usertablepasswordfield=password_digest
-roletable=security_schema.roles
-roletablekey=id
-roletablerolefield=name
-userroletable=security_schema.user_roles
-userroletableuserkey=user_id
-userroletablerolekey=role_id
-cachetime=300

=== modified file 'src/main/resources/com/akiban/server/service/config/configuration-defaults.properties'
--- src/main/resources/com/akiban/server/service/config/configuration-defaults.properties	2013-04-15 16:20:25 +0000
+++ src/main/resources/com/akiban/server/service/config/configuration-defaults.properties	2013-04-22 23:01:29 +0000
@@ -82,6 +82,9 @@
 akserver.http.ssl=false
 akserver.http.login=none
 
+# How long to save credential look-ups
+akserver.http.login_cache_seconds = 300
+
 # If enabled, use a CrossOriginFilter and construct it with these defaults.
 akserver.http.cross_origin.enabled = true
 akserver.http.cross_origin.allowed_origins = *

=== added file 'src/test/java/com/akiban/http/HttpThreadedLoginIT.java'
--- src/test/java/com/akiban/http/HttpThreadedLoginIT.java	1970-01-01 00:00:00 +0000
+++ src/test/java/com/akiban/http/HttpThreadedLoginIT.java	2013-04-22 23:01:29 +0000
@@ -0,0 +1,147 @@
+/**
+ * Copyright (C) 2009-2013 Akiban Technologies, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package com.akiban.http;
+
+import com.akiban.rest.RestService;
+import com.akiban.rest.RestServiceImpl;
+import com.akiban.server.service.servicemanager.GuicedServiceManager;
+import com.akiban.server.test.it.ITBase;
+import com.akiban.sql.embedded.EmbeddedJDBCService;
+import com.akiban.sql.embedded.EmbeddedJDBCServiceImpl;
+
+import org.apache.http.HttpResponse;
+import org.apache.http.HttpStatus;
+import org.apache.http.client.HttpClient;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.DefaultHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.junit.Assert.*;
+
+import java.net.URI;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+public class HttpThreadedLoginIT extends ITBase
+{
+    private static final Logger LOG = LoggerFactory.getLogger(HttpThreadedLoginIT.class);
+
+    @Override
+    protected GuicedServiceManager.BindingsConfigurationProvider serviceBindingsProvider() {
+        return super.serviceBindingsProvider()
+            .bindAndRequire(RestService.class, RestServiceImpl.class);
+    }
+
+    @Override
+    protected Map<String, String> startupConfigProperties() {
+        Map<String, String> properties = new HashMap<>();
+        properties.put("akserver.http.login", "basic");
+        return properties;
+    }
+
+    private static int openRestURL(String userInfo, int port, String path) throws Exception {
+        HttpClient client = new DefaultHttpClient();
+        URI uri = new URI("http", userInfo, "localhost", port, path, null, null);
+        HttpGet get = new HttpGet(uri);
+        HttpResponse response = client.execute(get);
+        int code = response.getStatusLine().getStatusCode();
+        EntityUtils.consume(response.getEntity());
+        client.getConnectionManager().shutdown();
+        return code;
+    }
+
+    @Test
+    public void oneThread() throws InterruptedException {
+        run(1);
+    }
+
+    @Test
+    public void fiveThreads() throws InterruptedException {
+        run(5);
+    }
+
+    @Test
+    public void tenThreads() throws InterruptedException {
+        run(10);
+    }
+
+    @Test
+    public void twentyThreads() throws InterruptedException {
+        run(20);
+    }
+
+
+    private void run(int count) throws InterruptedException {
+        final int port = serviceManager().getServiceByClass(HttpConductor.class).getPort();
+        final String context = serviceManager().getServiceByClass(RestService.class).getContextPath();
+        final String path = context + "/version";
+        final UncaughtHandler uncaughtHandler = new UncaughtHandler();
+
+        Thread threads[] = new Thread[count];
+        for(int i = 0; i < count; ++i) {
+            threads[i] = new Thread(new TestRunnable(port, path, i), "Thread"+i);
+            threads[i].setUncaughtExceptionHandler(uncaughtHandler);
+        }
+        for(int i = 0; i < count; ++i) {
+            threads[i].start();
+        }
+        for(int i = 0; i < count; ++i) {
+            threads[i].join();
+        }
+
+        for(Throwable entry : uncaughtHandler.uncaught.values()) {
+            LOG.error("Uncaught exception", entry);
+        }
+        assertEquals("uncaught exception count", 0, uncaughtHandler.uncaught.size());
+    }
+
+    private static class UncaughtHandler implements Thread.UncaughtExceptionHandler {
+        public final Map<Thread,Throwable> uncaught = Collections.synchronizedMap(new HashMap<Thread,Throwable>());
+
+        @Override
+        public void uncaughtException(Thread t, Throwable e) {
+            uncaught.put(t, e);
+        }
+    }
+
+    private static class TestRunnable implements Runnable {
+        private final int port;
+        private final String url;
+        private final int userNum;
+
+        public TestRunnable(int port, String url, int userNum) {
+            this.port = port;
+            this.url = url;
+            this.userNum = userNum;
+        }
+
+        @Override
+        public void run() {
+            String userInfo = String.format("user_%d:password", userNum);
+            try {
+                assertEquals(userInfo, HttpStatus.SC_UNAUTHORIZED, openRestURL(userInfo, port, url));
+            } catch(Exception e) {
+                throw new RuntimeException(e);
+            }
+        }
+    }
+}

=== modified file 'src/test/java/com/akiban/server/service/security/SecurityServiceIT.java'
--- src/test/java/com/akiban/server/service/security/SecurityServiceIT.java	2013-04-07 07:28:15 +0000
+++ src/test/java/com/akiban/server/service/security/SecurityServiceIT.java	2013-04-22 23:01:29 +0000
@@ -101,6 +101,9 @@
         assertNotNull("user found", user);
         assertTrue("user has role", user.hasRole("rest-user"));
         assertFalse("user does not have role", user.hasRole("admin"));
+        assertEquals("users roles", "[rest-user]", user.getRoles().toString());
+        assertEquals("user password basic", "MD5:5F4DCC3B5AA765D61D8327DEB882CF99", user.getBasicPassword());
+        assertEquals("user password digest", "MD5:740C53C6C7EE87EEEE489E8CE4116048", user.getDigestPassword());
     }
 
     @Test