You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@karaf.apache.org by gn...@apache.org on 2014/05/30 16:05:39 UTC

[2/2] git commit: [KARAF-2613] Add support for groups in JDBC login module and backing engine

[KARAF-2613] Add support for groups in JDBC login module and backing engine

Project: http://git-wip-us.apache.org/repos/asf/karaf/repo
Commit: http://git-wip-us.apache.org/repos/asf/karaf/commit/232bf6c0
Tree: http://git-wip-us.apache.org/repos/asf/karaf/tree/232bf6c0
Diff: http://git-wip-us.apache.org/repos/asf/karaf/diff/232bf6c0

Branch: refs/heads/master
Commit: 232bf6c0406d127e705754cf0d0abbcb8389693f
Parents: 2a1ad29
Author: Guillaume Nodet <gn...@gmail.com>
Authored: Fri May 30 15:44:44 2014 +0200
Committer: Guillaume Nodet <gn...@gmail.com>
Committed: Fri May 30 15:44:44 2014 +0200

----------------------------------------------------------------------
 jaas/modules/pom.xml                            |   6 +
 .../jaas/modules/BackingEngineFactory.java      |   7 +-
 .../jaas/modules/jdbc/JDBCBackingEngine.java    | 383 +++++++------------
 .../modules/jdbc/JDBCBackingEngineFactory.java  |   9 +-
 .../jaas/modules/jdbc/JDBCLoginModule.java      | 110 ++----
 .../karaf/jaas/modules/jdbc/JDBCUtils.java      |  95 ++++-
 .../PropertiesBackingEngineFactory.java         |  10 +-
 .../jaas/modules/jdbc/JdbcLoginModuleTest.java  | 236 ++++++++++++
 pom.xml                                         |   1 +
 9 files changed, 497 insertions(+), 360 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/pom.xml
----------------------------------------------------------------------
diff --git a/jaas/modules/pom.xml b/jaas/modules/pom.xml
index 76205ac..7302e99 100644
--- a/jaas/modules/pom.xml
+++ b/jaas/modules/pom.xml
@@ -100,6 +100,12 @@
             <version>${directory-version}</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.apache.derby</groupId>
+            <artifactId>derby</artifactId>
+            <version>${derby-version}</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/BackingEngineFactory.java
----------------------------------------------------------------------
diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/BackingEngineFactory.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/BackingEngineFactory.java
index 16fbb82..c7bb2cf 100644
--- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/BackingEngineFactory.java
+++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/BackingEngineFactory.java
@@ -21,18 +21,13 @@ public interface BackingEngineFactory {
 
     /**
      * Returns the corresponding module class.
-     *
-     * @return
      */
     String getModuleClass();
 
 
     /**
      * Backing engine factory method.
-     *
-     * @param options
-     * @return
      */
-    BackingEngine build(Map options);
+    BackingEngine build(Map<String,?> options);
 
 }

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngine.java
----------------------------------------------------------------------
diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngine.java
index 9723de8..189abd9 100644
--- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngine.java
+++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngine.java
@@ -24,12 +24,14 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.sql.DataSource;
+
 import java.security.Principal;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
@@ -40,8 +42,6 @@ public class JDBCBackingEngine implements BackingEngine {
     private DataSource dataSource;
     private EncryptionSupport encryptionSupport;
 
-    private static final String MSG_CONNECTION_CLOSE_FAILED = "Failed to clearly close connection to the database:";
-
     private String addUserStatement = "INSERT INTO USERS VALUES(?,?)";
     private String addRoleStatement = "INSERT INTO ROLES VALUES(?,?)";
     private String deleteRoleStatement = "DELETE FROM ROLES WHERE USERNAME=? AND ROLE=?";
@@ -52,8 +52,6 @@ public class JDBCBackingEngine implements BackingEngine {
 
     /**
      * Constructor
-     *
-     * @param dataSource
      */
     public JDBCBackingEngine(DataSource dataSource) {
         this.dataSource = dataSource;
@@ -66,203 +64,90 @@ public class JDBCBackingEngine implements BackingEngine {
 
     /**
      * Adds a new user.
-     *
-     * @param username
-     * @param password
      */
     public void addUser(String username, String password) {
-        Connection connection = null;
-        PreparedStatement statement = null;
-
-        String newPassword = password;
-
+        if (username.startsWith(GROUP_PREFIX)) {
+            throw new IllegalArgumentException("Prefix not permitted: " + GROUP_PREFIX);
+        }
         //If encryption support is enabled, encrypt password
         if (encryptionSupport != null && encryptionSupport.getEncryption() != null) {
-            newPassword = encryptionSupport.getEncryption().encryptPassword(password);
+            password = encryptionSupport.getEncryption().encryptPassword(password);
             if (encryptionSupport.getEncryptionPrefix() != null) {
-                newPassword = encryptionSupport.getEncryptionPrefix() + newPassword;
+                password = encryptionSupport.getEncryptionPrefix() + password;
             }
             if (encryptionSupport.getEncryptionSuffix() != null) {
-                newPassword = newPassword + encryptionSupport.getEncryptionSuffix();
+                password = password + encryptionSupport.getEncryptionSuffix();
             }
         }
-
-        if (dataSource != null) {
-
-            try {
-                connection = dataSource.getConnection();
-                statement = connection.prepareStatement(addUserStatement);
-                statement.setString(1, username);
-                statement.setString(2, newPassword);
-                int rows = statement.executeUpdate();
-
-                if (!connection.getAutoCommit()) {
-                    connection.commit();
-                }
-                if (logger.isDebugEnabled()) {
-                    logger.debug(String.format("Executiong [%s], USERNAME=%s, PASSWORD=%s. %i rows affected.", addUserStatement, username, newPassword, rows));
-                }
-            } catch (SQLException e) {
-                logger.error("Error executiong statement", e);
-            } finally {
-                try {
-                    if (statement != null) {
-                        statement.close();
-                    }
-                    if (connection != null) {
-                        connection.close();
-                    }
-                } catch (SQLException e) {
-                    logger.warn(MSG_CONNECTION_CLOSE_FAILED, e);
-                }
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                rawUpdate(connection, addUserStatement, username, password);
             }
+        } catch (SQLException e) {
+            throw new RuntimeException("Error adding user", e);
         }
     }
 
     /**
      * Delete user by username.
-     *
-     * @param username
      */
     public void deleteUser(String username) {
-        Connection connection = null;
-        PreparedStatement userStatement = null;
-        PreparedStatement roleStatement = null;
-
-        if (dataSource != null) {
-
-            try {
-                connection = dataSource.getConnection();
-
-                //Remove from roles
-                roleStatement = connection.prepareStatement(deleteAllUserRolesStatement);
-                roleStatement.setString(1, username);
-                roleStatement.executeUpdate();
-
-                //Remove from users
-                userStatement = connection.prepareStatement(deleteUserStatement);
-                userStatement.setString(1, username);
-                int userRows = userStatement.executeUpdate();
-
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                rawUpdate(connection, deleteAllUserRolesStatement, username);
+                rawUpdate(connection, deleteUserStatement, username);
                 if (!connection.getAutoCommit()) {
                     connection.commit();
                 }
-
-                if (logger.isDebugEnabled()) {
-                    logger.debug(String.format("Executiong [%s], USERNAME=%s. %i userRows affected.", deleteUserStatement, username, userRows));
-                }
-            } catch (SQLException e) {
-                logger.error("Error executiong statement", e);
-            } finally {
-                try {
-                    if (userStatement != null) {
-                        userStatement.close();
-                    }
-                    if (roleStatement != null) {
-                        roleStatement.close();
-                    }
-                    if (connection != null) {
-                        connection.close();
-                    }
-                } catch (SQLException e) {
-                    logger.warn(MSG_CONNECTION_CLOSE_FAILED, e);
-                }
             }
+        } catch (SQLException e) {
+            throw new RuntimeException("Error deleting user", e);
         }
     }
 
     /**
      * List all Users
-     *
-     * @return
      */
     public List<UserPrincipal> listUsers() {
-        List<UserPrincipal> users = new ArrayList<UserPrincipal>();
-
-        Connection connection = null;
-        PreparedStatement listUserStatement = null;
-        ResultSet usersResultSet = null;
-
-
-        if (dataSource != null) {
-
-            try {
-                connection = dataSource.getConnection();
-
-                //Remove from users
-                listUserStatement = connection.prepareStatement(selectUsersQuery);
-                usersResultSet = listUserStatement.executeQuery();
-                while (usersResultSet.next()) {
-                    String username = usersResultSet.getString("USERNAME");
-                    users.add(new UserPrincipal(username));
-                }
-            } catch (SQLException e) {
-                logger.error("Error executiong statement", e);
-            } finally {
-                try {
-                    if (usersResultSet != null) {
-                        usersResultSet.close();
-                    }
-                    if (listUserStatement != null) {
-                        listUserStatement.close();
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                List<UserPrincipal> users = new ArrayList<>();
+                for (String name : rawSelect(connection, selectUsersQuery)) {
+                    if (!name.startsWith(GROUP_PREFIX)) {
+                        users.add(new UserPrincipal(name));
                     }
-                    if (connection != null) {
-                        connection.close();
-                    }
-                } catch (SQLException e) {
-                    logger.warn(MSG_CONNECTION_CLOSE_FAILED, e);
                 }
+                return users;
             }
+        } catch (SQLException e) {
+            throw new RuntimeException("Error listing users", e);
         }
-        return users;
     }
 
     /**
      * List the roles of the {@param principal}.
-     *
-     * @param principal
-     * @return
      */
     public List<RolePrincipal> listRoles(Principal principal) {
-        List<RolePrincipal> roles = new ArrayList<RolePrincipal>();
-
-        Connection connection = null;
-        PreparedStatement listRolesStatement = null;
-        ResultSet rolesResultSet = null;
-
-
-        if (dataSource != null) {
-
-            try {
-                connection = dataSource.getConnection();
-
-                //Remove from roles
-                listRolesStatement = connection.prepareStatement(selectRolesQuery);
-                listRolesStatement.setString(1, principal.getName());
-
-                rolesResultSet = listRolesStatement.executeQuery();
-
-                while (rolesResultSet.next()) {
-                    String role = rolesResultSet.getString(1);
-                    roles.add(new RolePrincipal(role));
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                if (principal instanceof GroupPrincipal) {
+                    return listRoles(connection, GROUP_PREFIX + principal.getName());
+                } else {
+                    return listRoles(connection, principal.getName());
                 }
+            }
+        } catch (SQLException e) {
+            throw new RuntimeException("Error listing roles", e);
+        }
+    }
 
-            } catch (SQLException e) {
-                logger.error("Error executiong statement", e);
-            } finally {
-                try {
-                    if (rolesResultSet != null) {
-                        rolesResultSet.close();
-                    }
-                    if (listRolesStatement != null) {
-                        listRolesStatement.close();
-                    }
-                    if (connection != null) {
-                        connection.close();
-                    }
-                } catch (SQLException e) {
-                    logger.warn(MSG_CONNECTION_CLOSE_FAILED, e);
-                }
+    private List<RolePrincipal> listRoles(Connection connection, String name) throws SQLException {
+        List<RolePrincipal> roles = new ArrayList<>();
+        for (String role : rawSelect(connection, selectRolesQuery, name)) {
+            if (role.startsWith(GROUP_PREFIX)) {
+                roles.addAll(listRoles(connection, role));
+            } else {
+                roles.add(new RolePrincipal(role));
             }
         }
         return roles;
@@ -270,88 +155,118 @@ public class JDBCBackingEngine implements BackingEngine {
 
     /**
      * Add a role to a user.
-     *
-     * @param username
-     * @param role
      */
     public void addRole(String username, String role) {
-        Connection connection = null;
-        PreparedStatement statement = null;
-
-        if (dataSource != null) {
-
-            try {
-                connection = dataSource.getConnection();
-                statement = connection.prepareStatement(addRoleStatement);
-                statement.setString(1, username);
-                statement.setString(2, role);
-                int rows = statement.executeUpdate();
-
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                rawUpdate(connection, addRoleStatement, username, role);
                 if (!connection.getAutoCommit()) {
                     connection.commit();
                 }
-                if (logger.isDebugEnabled()) {
-                    logger.debug(String.format("Executiong [%s], USERNAME=%s, ROLE=%s. %i rows affected.", addRoleStatement, username, role, rows));
-                }
-            } catch (SQLException e) {
-                logger.error("Error executiong statement", e);
-            } finally {
-                try {
-                    if (statement != null) {
-                        statement.close();
-                    }
-                    if (connection != null) {
-                        connection.close();
-                    }
-                } catch (SQLException e) {
-                    logger.warn(MSG_CONNECTION_CLOSE_FAILED, e);
-                }
             }
+        } catch (SQLException e) {
+            throw new RuntimeException("Error adding role", e);
         }
     }
 
     /**
      * Remove role from user.
-     *
-     * @param username
-     * @param role
      */
     public void deleteRole(String username, String role) {
-        Connection connection = null;
-        PreparedStatement statement = null;
-
-        if (dataSource != null) {
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                rawUpdate(connection, deleteRoleStatement, username, role);
+                if (!connection.getAutoCommit()) {
+                    connection.commit();
+                }
+            }
+        } catch (SQLException e) {
+            throw new RuntimeException("Error deleting role", e);
+        }
+    }
 
-            try {
-                connection = dataSource.getConnection();
-                statement = connection.prepareStatement(deleteRoleStatement);
-                statement.setString(1, username);
-                statement.setString(2, role);
-                int rows = statement.executeUpdate();
+    @Override
+    public List<GroupPrincipal> listGroups(UserPrincipal principal) {
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+            List<GroupPrincipal> roles = new ArrayList<>();
+            for (String role : rawSelect(connection, selectRolesQuery, principal.getName())) {
+                if (role.startsWith(GROUP_PREFIX)) {
+                    roles.add(new GroupPrincipal(role.substring(GROUP_PREFIX.length())));
+                }
+            }
+            return roles;
+            }
+        } catch (SQLException e) {
+            throw new RuntimeException("Error deleting role", e);
+        }
+    }
 
+    @Override
+    public void addGroup(String username, String group) {
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                String groupName = GROUP_PREFIX + group;
+                rawUpdate(connection, addRoleStatement, username, groupName);
                 if (!connection.getAutoCommit()) {
                     connection.commit();
                 }
-                if (logger.isDebugEnabled()) {
-                    logger.debug(String.format("Executiong [%s], USERNAME=%s, ROLE=%s. %i rows affected.", deleteRoleStatement, username, role, rows));
-                }
-            } catch (SQLException e) {
-                logger.error("Error executing statement", e);
-            } finally {
-                try {
-                    if (statement != null) {
-                        statement.close();
-                    }
-                    if (connection != null) {
-                        connection.close();
+            }
+        } catch (SQLException e) {
+            logger.error("Error executing statement", e);
+        }
+    }
+
+    @Override
+    public void deleteGroup(String username, String group) {
+        try {
+            try (Connection connection = dataSource.getConnection()) {
+                rawUpdate(connection, deleteRoleStatement, username, GROUP_PREFIX + group);
+                // garbage collection, clean up the groups if needed
+                boolean inUse = false;
+                for (String user : rawSelect(connection, selectUsersQuery)) {
+                    for (String g : rawSelect(connection, selectRolesQuery, user)) {
+                        if (group.equals(g)) {
+                            // there is another user of this group, nothing to clean up
+                            inUse = true;
+                            break;
+                        }
                     }
-                } catch (SQLException e) {
-                    logger.warn(MSG_CONNECTION_CLOSE_FAILED, e);
+                }
+                // nobody is using this group any more, remove it
+                if (!inUse) {
+                    rawUpdate(connection, deleteAllUserRolesStatement, GROUP_PREFIX + group);
+                }
+                if (!connection.getAutoCommit()) {
+                    connection.commit();
                 }
             }
+        } catch (SQLException e) {
+            logger.error("Error executing statement", e);
         }
     }
 
+    @Override
+    public void addGroupRole(String group, String role) {
+        addRole(GROUP_PREFIX + group, role);
+    }
+
+    @Override
+    public void deleteGroupRole(String group, String role) {
+        deleteRole(GROUP_PREFIX + group, role);
+    }
+
+    protected void rawUpdate(Connection connection, String query, String... params) throws SQLException {
+        int rows = JDBCUtils.rawUpdate(connection, query, params);
+        if (logger.isDebugEnabled()) {
+            logger.debug(String.format("Executing [%s], params=%s. %d rows affected.", query, Arrays.toString(params), rows));
+        }
+    }
+
+    protected List<String> rawSelect(Connection connection, String query, String... params) throws SQLException {
+        return JDBCUtils.rawSelect(connection, query, params);
+    }
+
     public String getAddUserStatement() {
         return addUserStatement;
     }
@@ -408,34 +323,4 @@ public class JDBCBackingEngine implements BackingEngine {
         this.selectRolesQuery = selectRolesQuery;
     }
 
-    @Override
-    public List<GroupPrincipal> listGroups(UserPrincipal user) {
-        // TODO support of groups has to be added
-        return Collections.emptyList();
-    }
-
-    @Override
-    public void addGroup(String username, String group) {
-        // TODO support of groups has to be added
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public void deleteGroup(String username, String group) {
-        // TODO support of groups has to be added
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public void addGroupRole(String group, String role) {
-        // TODO support of groups has to be added
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public void deleteGroupRole(String group, String role) {
-        // TODO support of groups has to be added
-        throw new UnsupportedOperationException();
-    }
-
 }

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngineFactory.java
----------------------------------------------------------------------
diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngineFactory.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngineFactory.java
index f6d1e7e..7e0415a 100644
--- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngineFactory.java
+++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCBackingEngineFactory.java
@@ -32,11 +32,8 @@ public class JDBCBackingEngineFactory implements BackingEngineFactory {
 
     /**
      * Build a Backing engine for the JDBCLoginModule.
-     *
-     * @param options
-     * @return
      */
-    public BackingEngine build(Map options) {
+    public BackingEngine build(Map<String, ?> options) {
         JDBCBackingEngine instance = null;
         String datasourceURL = (String) options.get(JDBCUtils.DATASOURCE);
         BundleContext bundleContext = (BundleContext) options.get(BundleContext.class.getName());
@@ -50,7 +47,7 @@ public class JDBCBackingEngineFactory implements BackingEngineFactory {
         String selectRolesQuery = (String) options.get(JDBCLoginModule.ROLE_QUERY);
 
         try {
-            DataSource dataSource = (DataSource) JDBCUtils.createDatasource(bundleContext, datasourceURL);
+            DataSource dataSource = JDBCUtils.createDatasource(bundleContext, datasourceURL);
             EncryptionSupport encryptionSupport = new EncryptionSupport(options);
             instance = new JDBCBackingEngine(dataSource, encryptionSupport);
             if(addUserStatement != null) {
@@ -82,8 +79,6 @@ public class JDBCBackingEngineFactory implements BackingEngineFactory {
 
     /**
      * Returns the login module class, that this factory can build.
-     *
-     * @return
      */
     public String getModuleClass() {
         return JDBCLoginModule.class.getName();

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCLoginModule.java
----------------------------------------------------------------------
diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCLoginModule.java
index ea81859..93255af 100644
--- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCLoginModule.java
+++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCLoginModule.java
@@ -15,9 +15,11 @@
  */
 package org.apache.karaf.jaas.modules.jdbc;
 
+import org.apache.karaf.jaas.boot.principal.GroupPrincipal;
 import org.apache.karaf.jaas.boot.principal.RolePrincipal;
 import org.apache.karaf.jaas.boot.principal.UserPrincipal;
 import org.apache.karaf.jaas.modules.AbstractKarafLoginModule;
+import org.apache.karaf.jaas.modules.BackingEngine;
 import org.apache.karaf.jaas.modules.properties.PropertiesLoginModule;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -34,6 +36,7 @@ import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 
 public class JDBCLoginModule extends AbstractKarafLoginModule {
@@ -56,24 +59,20 @@ public class JDBCLoginModule extends AbstractKarafLoginModule {
     public void initialize(Subject subject, CallbackHandler callbackHandler, Map<String, ?> sharedState, Map<String, ?> options) {
         super.initialize(subject, callbackHandler, options);
         datasourceURL = (String) options.get(JDBCUtils.DATASOURCE);
-        passwordQuery = (String) options.get(PASSWORD_QUERY);
-        roleQuery = (String) options.get(ROLE_QUERY);
         if (datasourceURL == null || datasourceURL.trim().length() == 0) {
             LOGGER.error("No datasource was specified ");
         } else if (!datasourceURL.startsWith(JDBCUtils.JNDI) && !datasourceURL.startsWith(JDBCUtils.OSGI)) {
             LOGGER.error("Invalid datasource lookup protocol");
         }
+        if (options.containsKey(PASSWORD_QUERY)) {
+            passwordQuery = (String) options.get(PASSWORD_QUERY);
+        }
+        if (options.containsKey(ROLE_QUERY)) {
+            roleQuery = (String) options.get(ROLE_QUERY);
+        }
     }
 
     public boolean login() throws LoginException {
-        Connection connection = null;
-
-        PreparedStatement passwordStatement = null;
-        PreparedStatement roleStatement = null;
-
-        ResultSet passwordResultSet = null;
-        ResultSet roleResultSet = null;
-
         Callback[] callbacks = new Callback[2];
         callbacks[0] = new NameCallback("Username: ");
         callbacks[1] = new PasswordCallback("Password: ", false);
@@ -94,75 +93,42 @@ public class JDBCLoginModule extends AbstractKarafLoginModule {
         }
 
         String password = new String(tmpPassword);
-        principals = new HashSet<Principal>();
+        principals = new HashSet<>();
 
         try {
-            Object credentialsDatasource = JDBCUtils.createDatasource(bundleContext, datasourceURL);
-
-            if (credentialsDatasource == null) {
-                throw new LoginException("Cannot obtain data source:" + datasourceURL);
-            } else if (credentialsDatasource instanceof DataSource) {
-                connection = ((DataSource) credentialsDatasource).getConnection();
-            } else if (credentialsDatasource instanceof XADataSource) {
-                connection = ((XADataSource) credentialsDatasource).getXAConnection().getConnection();
-            } else {
-                throw new LoginException("Unknow dataSource type " + credentialsDatasource.getClass());
-            }
-
-            //Retrieve user credentials from database.
-            passwordStatement = connection.prepareStatement(passwordQuery);
-            passwordStatement.setString(1, user);
-            passwordResultSet = passwordStatement.executeQuery();
-
-            if (!passwordResultSet.next()) {
-            	if (!this.detailedLoginExcepion) {
-            		throw new LoginException("login failed");
-            	} else {
-            		throw new LoginException("User " + user + " does not exist");
-            	}
-            } else {
-                String storedPassword = passwordResultSet.getString(1);
-
-                if (!checkPassword(password, storedPassword)) {
-                	if (!this.detailedLoginExcepion) {
-                		throw new LoginException("login failed");
-                	} else {
-                		throw new LoginException("Password for " + user + " does not match");
-                	}
+            DataSource datasource = JDBCUtils.createDatasource(bundleContext, datasourceURL);
+            try (Connection connection = datasource.getConnection()) {
+                List<String> passwords = JDBCUtils.rawSelect(connection, passwordQuery, user);
+                if (passwords.isEmpty()) {
+                    if (!this.detailedLoginExcepion) {
+                        throw new LoginException("login failed");
+                    } else {
+                        throw new LoginException("User " + user + " does not exist");
+                    }
+                }
+                if (!checkPassword(password, passwords.get(0))) {
+                    if (!this.detailedLoginExcepion) {
+                        throw new LoginException("login failed");
+                    } else {
+                        throw new LoginException("Password for " + user + " does not match");
+                    }
                 }
                 principals.add(new UserPrincipal(user));
-            }
 
-            //Retrieve user roles from database
-            roleStatement = connection.prepareStatement(roleQuery);
-            roleStatement.setString(1, user);
-            roleResultSet = roleStatement.executeQuery();
-            while (roleResultSet.next()) {
-                String role = roleResultSet.getString(1);
-                principals.add(new RolePrincipal(role));
-            }
-        } catch (Exception ex) {
-            throw new LoginException("Error has occured while retrieving credentials from database:" + ex.getMessage());
-        } finally {
-            try {
-                if (passwordResultSet != null) {
-                    passwordResultSet.close();
-                }
-                if (passwordStatement != null) {
-                    passwordStatement.close();
+                List<String> roles = JDBCUtils.rawSelect(connection, roleQuery, user);
+                for (String role : roles) {
+                    if (role.startsWith(BackingEngine.GROUP_PREFIX)) {
+                        principals.add(new GroupPrincipal(role.substring(BackingEngine.GROUP_PREFIX.length())));
+                        for (String r : JDBCUtils.rawSelect(connection, roleQuery, role)) {
+                            principals.add(new RolePrincipal(r));
+                        }
+                    } else {
+                        principals.add(new RolePrincipal(role));
+                    }
                 }
-                if (roleResultSet != null) {
-                    roleResultSet.close();
-                }
-                if (roleStatement != null) {
-                    roleStatement.close();
-                }
-                if (connection != null) {
-                    connection.close();
-                }
-            } catch (SQLException ex) {
-                LOGGER.warn("Failed to clearly close connection to the database:", ex);
             }
+        } catch (Exception ex) {
+            throw new LoginException("Error has occurred while retrieving credentials from database:" + ex.getMessage());
         }
         return true;
     }

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCUtils.java
----------------------------------------------------------------------
diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCUtils.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCUtils.java
index eaecfdd..c782abe 100644
--- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCUtils.java
+++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/jdbc/JDBCUtils.java
@@ -16,10 +16,19 @@
 
 package org.apache.karaf.jaas.modules.jdbc;
 
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.ServiceReference;
 
 import javax.naming.InitialContext;
+import javax.sql.DataSource;
 
 public final class JDBCUtils {
 
@@ -33,32 +42,37 @@ public final class JDBCUtils {
 
     /**
      * Looks up a datasource from the url. The datasource can be passed either as jndi name or bundles ldap filter.
-     *
-     * @param url
-     * @return
-     * @throws Exception
      */
-    public static Object createDatasource(BundleContext bc, String url) throws Exception {
-        if (url == null) {
-            throw new Exception("Illegal datasource url format. Datasource URL cannot be null.");
-        } else if (url.trim().length() == 0) {
-            throw new Exception("Illegal datasource url format. Datasource URL cannot be empty.");
+    public static DataSource createDatasource(BundleContext bc, String url) throws Exception {
+        Object ds = doCreateDatasource(bc, url);
+        if (ds == null) {
+            throw new Exception("Unable to create datasource for " + url);
+        }
+        return DataSource.class.cast(ds);
+    }
+
+    protected static Object doCreateDatasource(BundleContext bc, String url) throws Exception {
+        url = (url != null) ? url.trim() : null;
+        if (url == null || url.isEmpty()) {
+            throw new Exception("Illegal datasource url format. Datasource URL cannot be null or empty.");
         } else if (url.startsWith(JNDI)) {
             String jndiName = url.substring(JNDI.length());
             InitialContext ic = new InitialContext();
-            return ic.lookup(jndiName);
+            try {
+                return ic.lookup(jndiName);
+            } finally {
+                ic.close();
+            }
         } else if (url.startsWith(OSGI)) {
             String osgiFilter = url.substring(OSGI.length());
             String clazz = null;
             String filter = null;
             String[] tokens = osgiFilter.split("/", 2);
-            if (tokens != null) {
-                if (tokens.length > 0) {
-                    clazz = tokens[0];
-                }
-                if (tokens.length > 1) {
-                    filter = tokens[1];
-                }
+            if (tokens.length > 0) {
+                clazz = tokens[0];
+            }
+            if (tokens.length > 1) {
+                filter = tokens[1];
             }
             ServiceReference[] references = bc.getServiceReferences(clazz, filter);
             if (references != null) {
@@ -70,8 +84,53 @@ public final class JDBCUtils {
                 throw new Exception("Unable to find service reference for datasource: " + clazz + "/" + filter);
             }
         } else {
-            throw new Exception("Illegal datasource url format");
+            throw new Exception("Illegal datasource url format " + url);
+        }
+    }
+
+    protected static int rawUpdate(DataSource dataSource, String query, String... params) throws SQLException {
+        try (Connection connection = dataSource.getConnection()) {
+            try (PreparedStatement statement = connection.prepareStatement(query)) {
+                for (int i = 0; i < params.length; i++) {
+                    statement.setString(i + 1, params[i]);
+                }
+                int res = statement.executeUpdate();
+                if (!connection.getAutoCommit()) {
+                    connection.commit();
+                }
+                return res;
+            }
+        }
+    }
+
+    protected static int rawUpdate(Connection connection, String query, String... params) throws SQLException {
+        try (PreparedStatement statement = connection.prepareStatement(query)) {
+            for (int i = 0; i < params.length; i++) {
+                statement.setString(i + 1, params[i]);
+            }
+            return statement.executeUpdate();
+        }
+    }
+
+    protected static List<String> rawSelect(DataSource dataSource, String query, String... params) throws SQLException {
+        try (Connection connection = dataSource.getConnection()) {
+            return rawSelect(connection, query, params);
+        }
+    }
+
+    protected static List<String> rawSelect(Connection connection, String query, String... params) throws SQLException {
+        List<String> results = new ArrayList<>();
+        try (PreparedStatement statement = connection.prepareStatement(query)) {
+            for (int i = 0; i < params.length; i++) {
+                statement.setString(i + 1, params[i]);
+            }
+            try (ResultSet resultSet = statement.executeQuery()) {
+                while (resultSet.next()) {
+                    results.add(resultSet.getString(1));
+                }
+            }
         }
+        return results;
     }
 
 }

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineFactory.java
----------------------------------------------------------------------
diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineFactory.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineFactory.java
index df8b5f9..dd583b4 100644
--- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineFactory.java
+++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineFactory.java
@@ -35,11 +35,8 @@ public class PropertiesBackingEngineFactory implements BackingEngineFactory {
 
     /**
      * Builds the Backing Engine
-     *
-     * @param options
-     * @return
      */
-    public BackingEngine build(Map options) {
+    public BackingEngine build(Map<String,?> options) {
         PropertiesBackingEngine engine = null;
         String usersFile = (String) options.get(USER_FILE);
 
@@ -51,15 +48,12 @@ public class PropertiesBackingEngineFactory implements BackingEngineFactory {
             engine = new PropertiesBackingEngine(users, encryptionSupport);
         } catch (IOException ioe) {
             LOGGER.warn("Cannot open users file: {}", usersFile);
-        } finally {
-            return engine;
         }
+        return engine;
     }
 
     /**
      * Returns the login module class, that this factory can build.
-     *
-     * @return
      */
     public String getModuleClass() {
         return PropertiesLoginModule.class.getName();

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/jdbc/JdbcLoginModuleTest.java
----------------------------------------------------------------------
diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/jdbc/JdbcLoginModuleTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/jdbc/JdbcLoginModuleTest.java
new file mode 100644
index 0000000..24634dc
--- /dev/null
+++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/jdbc/JdbcLoginModuleTest.java
@@ -0,0 +1,236 @@
+/*
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ *  under the License.
+ */
+package org.apache.karaf.jaas.modules.jdbc;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.security.auth.Subject;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.sql.DataSource;
+
+import org.apache.derby.jdbc.EmbeddedDataSource40;
+import org.apache.karaf.jaas.boot.principal.GroupPrincipal;
+import org.apache.karaf.jaas.boot.principal.RolePrincipal;
+import org.apache.karaf.jaas.boot.principal.UserPrincipal;
+import org.easymock.EasyMock;
+import org.junit.Before;
+import org.junit.Test;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceReference;
+
+import static org.easymock.EasyMock.expect;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class JdbcLoginModuleTest {
+
+    private EmbeddedDataSource40 dataSource;
+    private Map<String, Object> options;
+
+    @Before
+    public void setUp() throws Exception {
+        System.setProperty("derby.stream.error.file", "target/derby.log");
+
+        // Create datasource
+        dataSource = new EmbeddedDataSource40();
+        dataSource.setDatabaseName("memory:db");
+        dataSource.setCreateDatabase("create");
+
+        // Delete tables
+        try (Connection connection = dataSource.getConnection()) {
+            connection.setAutoCommit(true);
+            try {
+                try (Statement statement = connection.createStatement()) {
+                    statement.execute("drop table USERS");
+                }
+            } catch (SQLException e) {
+                // Ignore
+            }
+            try {
+                try (Statement statement = connection.createStatement()) {
+                    statement.execute("drop table ROLES");
+                }
+            } catch (SQLException e) {
+                // Ignore
+            }
+            connection.commit();
+        }
+
+        // Create tables
+        try (Connection connection = dataSource.getConnection()) {
+            try (Statement statement = connection.createStatement()) {
+                statement.execute("create table USERS (USERNAME VARCHAR(32) PRIMARY KEY, PASSWORD VARCHAR(32))");
+            }
+            try (Statement statement = connection.createStatement()) {
+                statement.execute("create table ROLES (USERNAME VARCHAR(32), ROLE VARCHAR(1024))");
+            }
+            connection.commit();
+        }
+
+        // Mocks
+        BundleContext context = EasyMock.createMock(BundleContext.class);
+        ServiceReference reference = EasyMock.createMock(ServiceReference.class);
+
+        // Create options
+        options = new HashMap<>();
+        options.put(JDBCUtils.DATASOURCE, "osgi:" + DataSource.class.getName());
+        options.put(BundleContext.class.getName(), context);
+
+        expect(context.getServiceReferences(DataSource.class.getName(), null)).andReturn(new ServiceReference[] { reference });
+        expect(context.getService(reference)).andReturn(dataSource);
+        expect(context.ungetService(reference)).andReturn(true);
+
+        EasyMock.replay(context);
+    }
+
+    @Test
+    public void testLoginModule() throws Exception {
+        JDBCBackingEngine engine = new JDBCBackingEngine(dataSource);
+        engine.addUser("abc", "xyz");
+        engine.addRole("abc", "role1");
+
+        JDBCLoginModule module = new JDBCLoginModule();
+
+        Subject subject = new Subject();
+        module.initialize(subject, getCallbackHandler("abc", "xyz"), null, options);
+
+        module.login();
+        module.commit();
+
+        assertFalse(subject.getPrincipals(UserPrincipal.class).isEmpty());
+        assertEquals("abc", subject.getPrincipals(UserPrincipal.class).iterator().next().getName());
+        assertFalse(subject.getPrincipals(RolePrincipal.class).isEmpty());
+        assertEquals("role1", subject.getPrincipals(RolePrincipal.class).iterator().next().getName());
+    }
+
+    @Test
+    public void testLoginModuleWithGroups() throws Exception {
+        JDBCBackingEngine engine = new JDBCBackingEngine(dataSource);
+        engine.addGroupRole("group1", "role2");
+        engine.addUser("abc", "xyz");
+        engine.addRole("abc", "role1");
+        engine.addGroup("abc", "group1");
+
+        JDBCLoginModule module = new JDBCLoginModule();
+
+        Subject subject = new Subject();
+        module.initialize(subject, getCallbackHandler("abc", "xyz"), null, options);
+
+        module.login();
+        module.commit();
+
+        assertTrue(subject.getPrincipals().contains(new UserPrincipal("abc")));
+        assertTrue(subject.getPrincipals().contains(new GroupPrincipal("group1")));
+        assertTrue(subject.getPrincipals().contains(new RolePrincipal("role1")));
+        assertTrue(subject.getPrincipals().contains(new RolePrincipal("role2")));
+    }
+
+    @Test
+    public void testEngine() throws Exception {
+        JDBCBackingEngine engine = new JDBCBackingEngine(dataSource);
+
+        assertTrue(engine.listUsers().isEmpty());
+
+        engine.addUser("abc", "xyz");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).isEmpty());
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).isEmpty());
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).isEmpty());
+
+        engine.addRole("abc", "role1");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).contains(new RolePrincipal("role1")));
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).isEmpty());
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).isEmpty());
+
+        engine.addGroupRole("group1", "role2");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).contains(new RolePrincipal("role1")));
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).contains(new RolePrincipal("role2")));
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).isEmpty());
+
+        engine.addGroup("abc", "group1");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).contains(new RolePrincipal("role1")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).contains(new RolePrincipal("role2")));
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).contains(new RolePrincipal("role2")));
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).contains(new GroupPrincipal("group1")));
+
+        engine.deleteRole("abc", "role1");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).contains(new RolePrincipal("role2")));
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).contains(new RolePrincipal("role2")));
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).contains(new GroupPrincipal("group1")));
+
+        engine.deleteGroupRole("group1", "role2");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).isEmpty());
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).isEmpty());
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).contains(new GroupPrincipal("group1")));
+
+        engine.addGroupRole("group1", "role3");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).contains(new RolePrincipal("role3")));
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).contains(new RolePrincipal("role3")));
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).contains(new GroupPrincipal("group1")));
+
+        engine.deleteGroup("abc", "group1");
+
+        assertTrue(engine.listUsers().contains(new UserPrincipal("abc")));
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).isEmpty());
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).isEmpty());
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).isEmpty());
+
+        engine.deleteUser("abc");
+
+        assertTrue(engine.listUsers().isEmpty());
+        assertTrue(engine.listRoles(new UserPrincipal("abc")).isEmpty());
+        assertTrue(engine.listRoles(new GroupPrincipal("group1")).isEmpty());
+        assertTrue(engine.listGroups(new UserPrincipal("abc")).isEmpty());
+    }
+
+    private CallbackHandler getCallbackHandler(final String user, final String password) {
+        return new CallbackHandler() {
+                @Override
+                public void handle(Callback[] callbacks) throws IOException, UnsupportedCallbackException {
+                    for (Callback cb : callbacks) {
+                        if (cb instanceof NameCallback) {
+                            ((NameCallback) cb).setName(user);
+                        } else if (cb instanceof PasswordCallback) {
+                            ((PasswordCallback) cb).setPassword(password.toCharArray());
+                        }
+                    }
+                }
+            };
+    }
+}

http://git-wip-us.apache.org/repos/asf/karaf/blob/232bf6c0/pom.xml
----------------------------------------------------------------------
diff --git a/pom.xml b/pom.xml
index fe89cb4..3a7747e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -249,6 +249,7 @@
         <spring.security31.version>3.1.4.RELEASE</spring.security31.version>
 
         <sshd.version>0.11.0</sshd.version>
+        <derby-version>10.10.1.1</derby-version>
         <directory-version>2.0.0-M16</directory-version>
         <struts.bundle.version>1.3.10_1</struts.bundle.version>
         <xbean.version>3.16</xbean.version>