This closes #427

artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/CertificateLoginModule.java

@@ -44,7 +44,6 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
 
    private X509Certificate[] certificates;
    private String username;
-   private Set<String> roles;
    private Set<Principal> principals = new HashSet<>();
 
    /**
@@ -82,8 +81,6 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
          throw new FailedLoginException("No user for client certificate: " + getDistinguishedName(certificates));
       }
 
-      roles = getUserRoles(username);
-
       if (debug) {
          ActiveMQServerLogger.LOGGER.debug("Certificate for user: " + username);
       }
@@ -97,7 +94,7 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
    public boolean commit() throws LoginException {
       principals.add(new UserPrincipal(username));
 
-      for (String role : roles) {
+      for (String role : getUserRoles(username)) {
          principals.add(new RolePrincipal(role));
       }
 
@@ -142,8 +139,8 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
     * Helper method.
     */
    private void clear() {
-      roles.clear();
       certificates = null;
+      username = null;
    }
 
    /**

artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/PropertiesLoginModule.java

@@ -43,7 +43,7 @@ public class PropertiesLoginModule extends PropertiesLoader implements LoginModu
    private CallbackHandler callbackHandler;
 
    private Properties users;
-   private Properties roles;
+   private Map<String,Set<String>> roles;
    private String user;
    private final Set<Principal> principals = new HashSet<>();
    private boolean loginSucceeded;
@@ -59,7 +59,7 @@ public class PropertiesLoginModule extends PropertiesLoader implements LoginModu
 
       init(options);
       users = load(USER_FILE_PROP_NAME, "user", options).getProps();
-      roles = load(ROLE_FILE_PROP_NAME, "role", options).getProps();
+      roles = load(ROLE_FILE_PROP_NAME, "role", options).invertedPropertiesValuesMap();
    }
 
    @Override
@@ -107,17 +107,10 @@ public class PropertiesLoginModule extends PropertiesLoader implements LoginModu
       if (result) {
          principals.add(new UserPrincipal(user));
 
-         for (Map.Entry<Object, Object> entry : roles.entrySet()) {
+         Set<String> matchedRoles = roles.get(user);
-            String name = (String) entry.getKey();
+         if (matchedRoles != null) {
-            String[] userList = ((String) entry.getValue()).split(",");
+            for (String entry : matchedRoles) {
-            if (debug) {
+               principals.add(new RolePrincipal(entry));
-               ActiveMQServerLogger.LOGGER.debug("Inspecting role '" + name + "' with user(s): " + entry.getValue());
-            }
-            for (int i = 0; i < userList.length; i++) {
-               if (user.equals(userList[i])) {
-                  principals.add(new RolePrincipal(name));
-                  break;
-               }
             }
          }
 

artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/ReloadableProperties.java

@@ -20,8 +20,10 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
 
 import org.apache.activemq.artemis.core.server.ActiveMQServerLogger;
 
@@ -29,6 +31,7 @@ public class ReloadableProperties {
 
    private Properties props = new Properties();
    private Map<String, String> invertedProps;
+   private Map<String, Set<String>> invertedValueProps;
    private long reloadTime = -1;
    private final PropertiesLoader.FileNameKey key;
 
@@ -46,6 +49,7 @@ public class ReloadableProperties {
          try {
             load(key.file(), props);
             invertedProps = null;
+            invertedValueProps = null;
             if (key.isDebug()) {
                ActiveMQServerLogger.LOGGER.debug("Load of: " + key);
             }
@@ -71,6 +75,24 @@ public class ReloadableProperties {
       return invertedProps;
    }
 
+   public synchronized Map<String, Set<String>> invertedPropertiesValuesMap() {
+      if (invertedValueProps == null) {
+         invertedValueProps = new HashMap<>(props.size());
+         for (Map.Entry<Object, Object> val : props.entrySet()) {
+            String[] userList = ((String)val.getValue()).split(",");
+            for (String user : userList) {
+               Set<String> set = invertedValueProps.get(user);
+               if (set == null) {
+                  set = new HashSet<>();
+                  invertedValueProps.put(user, set);
+               }
+               set.add((String)val.getKey());
+            }
+         }
+      }
+      return invertedValueProps;
+   }
+
    private void load(final File source,
                      Properties props) throws IOException {
       try (FileInputStream in = new FileInputStream(source)) {

artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/TextFileCertificateLoginModule.java

@@ -20,10 +20,8 @@ import javax.security.auth.Subject;
 import javax.security.auth.callback.CallbackHandler;
 import javax.security.auth.login.LoginException;
 import javax.security.cert.X509Certificate;
-import java.util.Enumeration;
+import java.util.Collections;
-import java.util.HashSet;
 import java.util.Map;
-import java.util.Properties;
 import java.util.Set;
 
 /**
@@ -42,7 +40,7 @@ public class TextFileCertificateLoginModule extends CertificateLoginModule {
    private static final String USER_FILE_PROP_NAME = "org.apache.activemq.jaas.textfiledn.user";
    private static final String ROLE_FILE_PROP_NAME = "org.apache.activemq.jaas.textfiledn.role";
 
-   private Properties roles;
+   private Map<String, Set<String>> rolesByUser;
    private Map<String, String> usersByDn;
 
    /**
@@ -52,7 +50,7 @@ public class TextFileCertificateLoginModule extends CertificateLoginModule {
    public void initialize(Subject subject, CallbackHandler callbackHandler, Map<String, ?> sharedState, Map<String, ?> options) {
       super.initialize(subject, callbackHandler, sharedState, options);
       usersByDn = load(USER_FILE_PROP_NAME, "", options).invertedPropertiesMap();
-      roles = load(ROLE_FILE_PROP_NAME, "", options).getProps();
+      rolesByUser = load(ROLE_FILE_PROP_NAME, "", options).invertedPropertiesValuesMap();
    }
 
    /**
@@ -84,16 +82,9 @@ public class TextFileCertificateLoginModule extends CertificateLoginModule {
     */
    @Override
    protected Set<String> getUserRoles(String username) throws LoginException {
-      Set<String> userRoles = new HashSet<>();
+      Set<String> userRoles = rolesByUser.get(username);
-      for (Enumeration<Object> enumeration = roles.keys(); enumeration.hasMoreElements(); ) {
+      if (userRoles == null) {
-         String groupName = (String) enumeration.nextElement();
+         userRoles = Collections.emptySet();
-         String[] userList = (roles.getProperty(groupName) + "").split(",");
-         for (int i = 0; i < userList.length; i++) {
-            if (username.equals(userList[i])) {
-               userRoles.add(groupName);
-               break;
-            }
-         }
       }
 
       return userRoles;

artemis-server/src/test/java/org/apache/activemq/artemis/core/security/jaas/PropertiesLoginModuleTest.java

@@ -25,11 +25,13 @@ import javax.security.auth.callback.UnsupportedCallbackException;
 import javax.security.auth.login.FailedLoginException;
 import javax.security.auth.login.LoginContext;
 import javax.security.auth.login.LoginException;
+import java.io.File;
 import java.io.IOException;
 import java.net.URL;
 
 import org.apache.activemq.artemis.spi.core.security.jaas.RolePrincipal;
 import org.apache.activemq.artemis.spi.core.security.jaas.UserPrincipal;
+import org.apache.commons.io.FileUtils;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -48,22 +50,8 @@ public class PropertiesLoginModuleTest extends Assert {
 
    @Test
    public void testLogin() throws LoginException {
-      LoginContext context = new LoginContext("PropertiesLogin", new CallbackHandler() {
+      LoginContext context = new LoginContext("PropertiesLogin", new UserPassHandler("first", "secret"));
-         @Override
+
-         public void handle(Callback[] callbacks) throws IOException, UnsupportedCallbackException {
-            for (int i = 0; i < callbacks.length; i++) {
-               if (callbacks[i] instanceof NameCallback) {
-                  ((NameCallback) callbacks[i]).setName("first");
-               }
-               else if (callbacks[i] instanceof PasswordCallback) {
-                  ((PasswordCallback) callbacks[i]).setPassword("secret".toCharArray());
-               }
-               else {
-                  throw new UnsupportedCallbackException(callbacks[i]);
-               }
-            }
-         }
-      });
       context.login();
 
       Subject subject = context.getSubject();
@@ -77,24 +65,55 @@ public class PropertiesLoginModuleTest extends Assert {
       assertEquals("Should have zero principals", 0, subject.getPrincipals().size());
    }
 
+   @Test
+   public void testLoginReload() throws Exception {
+      File targetPropDir = new File("target/loginReloadTest");
+      File usersFile = new File(targetPropDir, "users.properties");
+      File rolesFile = new File(targetPropDir, "roles.properties");
+
+      //Set up initial properties
+      FileUtils.copyFile(new File(getClass().getResource("/users.properties").toURI()), usersFile);
+      FileUtils.copyFile(new File(getClass().getResource("/roles.properties").toURI()), rolesFile);
+
+      LoginContext context = new LoginContext("PropertiesLoginReload", new UserPassHandler("first", "secret"));
+      context.login();
+      Subject subject = context.getSubject();
+
+      //test initial principals
+      assertEquals("Should have three principals", 3, subject.getPrincipals().size());
+      assertEquals("Should have one user principal", 1, subject.getPrincipals(UserPrincipal.class).size());
+      assertEquals("Should have two group principals", 2, subject.getPrincipals(RolePrincipal.class).size());
+
+      context.logout();
+
+      assertEquals("Should have zero principals", 0, subject.getPrincipals().size());
+
+      //Modify the file and test that the properties are reloaded
+      Thread.sleep(1000);
+      FileUtils.copyFile(new File(getClass().getResource("/usersReload.properties").toURI()), usersFile);
+      FileUtils.copyFile(new File(getClass().getResource("/rolesReload.properties").toURI()), rolesFile);
+      FileUtils.touch(usersFile);
+      FileUtils.touch(rolesFile);
+
+      //Use new password to verify  users file was reloaded
+      context = new LoginContext("PropertiesLoginReload", new UserPassHandler("first", "secrets"));
+      context.login();
+      subject = context.getSubject();
+
+      //Check that the principals changed
+      assertEquals("Should have three principals", 2, subject.getPrincipals().size());
+      assertEquals("Should have one user principal", 1, subject.getPrincipals(UserPrincipal.class).size());
+      assertEquals("Should have one group principals", 1, subject.getPrincipals(RolePrincipal.class).size());
+
+      context.logout();
+
+      assertEquals("Should have zero principals", 0, subject.getPrincipals().size());
+   }
+
    @Test
    public void testBadUseridLogin() throws Exception {
-      LoginContext context = new LoginContext("PropertiesLogin", new CallbackHandler() {
+      LoginContext context = new LoginContext("PropertiesLogin", new UserPassHandler("BAD", "secret"));
-         @Override
+
-         public void handle(Callback[] callbacks) throws IOException, UnsupportedCallbackException {
-            for (int i = 0; i < callbacks.length; i++) {
-               if (callbacks[i] instanceof NameCallback) {
-                  ((NameCallback) callbacks[i]).setName("BAD");
-               }
-               else if (callbacks[i] instanceof PasswordCallback) {
-                  ((PasswordCallback) callbacks[i]).setPassword("secret".toCharArray());
-               }
-               else {
-                  throw new UnsupportedCallbackException(callbacks[i]);
-               }
-            }
-         }
-      });
       try {
          context.login();
          fail("Should have thrown a FailedLoginException");
@@ -106,22 +125,8 @@ public class PropertiesLoginModuleTest extends Assert {
 
    @Test
    public void testBadPWLogin() throws Exception {
-      LoginContext context = new LoginContext("PropertiesLogin", new CallbackHandler() {
+      LoginContext context = new LoginContext("PropertiesLogin", new UserPassHandler("first", "BAD"));
-         @Override
+
-         public void handle(Callback[] callbacks) throws IOException, UnsupportedCallbackException {
-            for (int i = 0; i < callbacks.length; i++) {
-               if (callbacks[i] instanceof NameCallback) {
-                  ((NameCallback) callbacks[i]).setName("first");
-               }
-               else if (callbacks[i] instanceof PasswordCallback) {
-                  ((PasswordCallback) callbacks[i]).setPassword("BAD".toCharArray());
-               }
-               else {
-                  throw new UnsupportedCallbackException(callbacks[i]);
-               }
-            }
-         }
-      });
       try {
          context.login();
          fail("Should have thrown a FailedLoginException");
@@ -130,4 +135,30 @@ public class PropertiesLoginModuleTest extends Assert {
       }
 
    }
+
+   private static class UserPassHandler implements CallbackHandler {
+
+      private final String user;
+      private final String pass;
+
+      public UserPassHandler(final String user, final String pass) {
+         this.user = user;
+         this.pass = pass;
+      }
+
+      @Override
+      public void handle(Callback[] callbacks) throws IOException, UnsupportedCallbackException {
+         for (int i = 0; i < callbacks.length; i++) {
+            if (callbacks[i] instanceof NameCallback) {
+               ((NameCallback) callbacks[i]).setName(user);
+            }
+            else if (callbacks[i] instanceof PasswordCallback) {
+               ((PasswordCallback) callbacks[i]).setPassword(pass.toCharArray());
+            }
+            else {
+               throw new UnsupportedCallbackException(callbacks[i]);
+            }
+         }
+      }
+   }
 }

artemis-server/src/test/resources/login.config

@@ -21,6 +21,15 @@ PropertiesLogin {
         org.apache.activemq.jaas.properties.role="roles.properties";
 };
 
+PropertiesLoginReload {
+    org.apache.activemq.artemis.spi.core.security.jaas.PropertiesLoginModule required
+        debug=true
+        reload=true
+        baseDir="target/loginReloadTest/"
+        org.apache.activemq.jaas.properties.user="users.properties"
+        org.apache.activemq.jaas.properties.role="roles.properties";
+};
+
 LDAPLogin {
     org.apache.activemq.artemis.spi.core.security.jaas.LDAPLoginModule required
         debug=true

artemis-server/src/test/resources/rolesReload.properties

@@ -0,0 +1,19 @@
+## ---------------------------------------------------------------------------
+## Licensed to the Apache Software Foundation (ASF) under one or more
+## contributor license agreements.  See the NOTICE file distributed with
+## this work for additional information regarding copyright ownership.
+## The ASF licenses this file to You 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.
+## ---------------------------------------------------------------------------
+
+programmers=first
+accounting=second

artemis-server/src/test/resources/usersReload.properties

@@ -0,0 +1,20 @@
+## ---------------------------------------------------------------------------
+## Licensed to the Apache Software Foundation (ASF) under one or more
+## contributor license agreements.  See the NOTICE file distributed with
+## this work for additional information regarding copyright ownership.
+## The ASF licenses this file to You 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.
+## ---------------------------------------------------------------------------
+
+#different password than users.properties
+first=secrets
+second=password