Merge pull request #1928 from vladak/plugins_session_invalidate

Plugins session invalidate

Author: tarzanek

build.xml

@@ -1128,7 +1128,7 @@ Portions Copyright (c) 2017, Chris Fraire <[email protected]>.
     </target>
 
     <!-- Build the plugins. -->
-    <target name="plugins" depends="jar">
+    <target name="plugins" depends="jar" description="create jar of all authorization plugins for distribution">
         <mkdir dir="${build.dir}/plugins"/>
         <javac srcdir="plugins/"
                destdir="${build.dir}/plugins"

plugins/LdapPlugin/src/opengrok/auth/plugin/AbstractLdapPlugin.java

@@ -35,14 +35,12 @@
 import org.opensolaris.opengrok.authorization.IAuthorizationPlugin;
 import org.opensolaris.opengrok.configuration.Group;
 import org.opensolaris.opengrok.configuration.Project;
-import org.opensolaris.opengrok.configuration.RuntimeEnvironment;
 
 /**
  * Abstract class for all plug-ins working with LDAP. Takes care of
  * <ul>
  * <li>controlling the established session</li>
  * <li>controlling if the session belongs to the user</li>
- * <li>controlling plug-in version</li>
  * </ul>
  *
  * <p>
@@ -63,9 +61,9 @@ abstract public class AbstractLdapPlugin implements IAuthorizationPlugin {
     protected static final String CONFIGURATION_PARAM = "configuration";
     protected static final String FAKE_PARAM = "fake";
 
-    protected String SESSION_USERNAME = "opengrok-group-plugin-username";
-    protected String SESSION_ESTABLISHED = "opengrok-group-plugin-session-established";
-    protected String SESSION_VERSION = "opengrok-group-plugin-session-version";
+    private final static String SESSION_PREFIX = "opengrok-abstract-ldap-plugin-";
+    protected String SESSION_USERNAME = SESSION_PREFIX + "username";
+    protected String SESSION_ESTABLISHED = SESSION_PREFIX + "session-established";
 
     /**
      * Configuration for the LDAP servers.
@@ -86,7 +84,6 @@ abstract public class AbstractLdapPlugin implements IAuthorizationPlugin {
     public AbstractLdapPlugin() {
         SESSION_USERNAME += "-" + nextId;
         SESSION_ESTABLISHED += "-" + nextId;
-        SESSION_VERSION += "-" + nextId;
         nextId++;
     }
 
@@ -197,11 +194,8 @@ public AbstractLdapProvider getLdapProvider() {
      * @return true if it does; false otherwise
      */
     protected boolean isSameUser(String sessionUsername, String authUser) {
-        if (sessionUsername != null
-                && sessionUsername.equals(authUser)) {
-            return true;
-        }
-        return false;
+        return sessionUsername != null
+                && sessionUsername.equals(authUser);
     }
 
     /**
@@ -214,7 +208,6 @@ protected boolean isSameUser(String sessionUsername, String authUser) {
     protected boolean sessionExists(HttpServletRequest req) {
         return req != null && req.getSession() != null
                 && req.getSession().getAttribute(SESSION_ESTABLISHED) != null
-                && req.getSession().getAttribute(SESSION_VERSION) != null
                 && req.getSession().getAttribute(SESSION_USERNAME) != null;
     }
 
@@ -234,39 +227,40 @@ protected boolean sessionExists(HttpServletRequest req) {
     @SuppressWarnings("unchecked")
     private void ensureSessionExists(HttpServletRequest req) {
         User user;
+        
         if (req.getSession() == null) {
             // old/invalid request (should not happen)
             return;
         }
-
+        
+        // The cast to User should not be problem as this object is stored
+        // in the request itself (as opposed to in the session).
         if ((user = (User) req.getAttribute(UserPlugin.REQUEST_ATTR)) == null) {
-            updateSession(req, null, false, getPluginVersion());
+            updateSession(req, null, false);
             return;
         }
 
         if (sessionExists(req)
                 // we've already filled the groups and projects
                 && (boolean) req.getSession().getAttribute(SESSION_ESTABLISHED)
                 // the session belongs to the user from the request
-                && isSameUser((String) req.getSession().getAttribute(SESSION_USERNAME), user.getUsername())
-                // and this is not a case when we want to renew all sessions
-                && !isSessionInvalidated(req)) {
+                && isSameUser((String) req.getSession().getAttribute(SESSION_USERNAME), user.getUsername())) {
             /**
              * The session is already filled so no need to
              * {@link #updateSession()}
              */
             return;
         }
 
-        updateSession(req, user.getUsername(), false, getPluginVersion());
+        updateSession(req, user.getUsername(), false);
 
         if (ldap == null) {
             return;
         }
 
         fillSession(req, user);
 
-        updateSession(req, user.getUsername(), true, getPluginVersion());
+        updateSession(req, user.getUsername(), true);
     }
 
     /**
@@ -275,44 +269,12 @@ && isSameUser((String) req.getSession().getAttribute(SESSION_USERNAME), user.get
      * @param req the request
      * @param username new username
      * @param established new value for established
-     * @param sessionV new value for session version
      */
     protected void updateSession(HttpServletRequest req,
             String username,
-            boolean established,
-            int sessionV) {
+            boolean established) {
         setSessionEstablished(req, established);
         setSessionUsername(req, username);
-        setSessionVersion(req, sessionV);
-    }
-
-    /**
-     * Is this session marked as invalid?
-     *
-     * @param req the request
-     * @return true if it is; false otherwise
-     */
-    protected boolean isSessionInvalidated(HttpServletRequest req) {
-        Integer version;
-        if ((version = (Integer) req.getAttribute(SESSION_VERSION)) != null) {
-            return version != getPluginVersion();
-        }
-        if ((version = (Integer) req.getSession().getAttribute(SESSION_VERSION)) != null) {
-            req.setAttribute(SESSION_VERSION, version);
-            return version != getPluginVersion();
-        }
-        return true;
-    }
-
-    /**
-     * Set session version into the session.
-     *
-     * @param req request containing the session
-     * @param value the value
-     */
-    protected void setSessionVersion(HttpServletRequest req, Integer value) {
-        req.getSession().setAttribute(SESSION_VERSION, value);
-        req.setAttribute(SESSION_VERSION, value);
     }
 
     /**
@@ -335,15 +297,6 @@ protected void setSessionUsername(HttpServletRequest req, String value) {
         req.getSession().setAttribute(SESSION_USERNAME, value);
     }
 
-    /**
-     * Return the current plug-in version tracked by the authorization framework.
-     *
-     * @return the version
-     */
-    protected static int getPluginVersion() {
-        return RuntimeEnvironment.getInstance().getPluginVersion();
-    }
-
     @Override
     public boolean isAllowed(HttpServletRequest request, Project project) {
         ensureSessionExists(request);

plugins/LdapPlugin/src/opengrok/auth/plugin/LdapAttrPlugin.java

@@ -127,7 +127,7 @@ public void fillSession(HttpServletRequest req, User user) {
     protected void updateSession(HttpServletRequest req, boolean allowed) {
         req.getSession().setAttribute(SESSION_ALLOWED, allowed);
     }
-
+    
     @Override
     public boolean checkEntity(HttpServletRequest request, Project project) {
         return ((Boolean) request.getSession().getAttribute(SESSION_ALLOWED));

plugins/LdapPlugin/test/opengrok/auth/plugin/LdapAttrTest.java renamed to plugins/LdapPlugin/test/opengrok/auth/plugin/LdapAttrPluginTest.java

@@ -33,6 +33,7 @@
 import java.util.TreeMap;
 import java.util.TreeSet;
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpSession;
 import opengrok.auth.entity.LdapUser;
 import opengrok.auth.plugin.entity.User;
 import opengrok.auth.plugin.util.DummyHttpServletRequestLdap;
@@ -45,7 +46,7 @@
 import org.opensolaris.opengrok.configuration.Group;
 import org.opensolaris.opengrok.configuration.Project;
 
-public class LdapAttrTest {
+public class LdapAttrPluginTest {
 
     private HttpServletRequest dummyRequest;
     private LdapAttrPlugin plugin;
@@ -79,8 +80,8 @@ public void setUp() {
 
         plugin.load(parameters);
 
-        framework = new AuthorizationFramework(null);
-        framework.setPluginVersion(1);
+        framework = new AuthorizationFramework();
+        framework.reload();
     }
 
     private void prepareRequest(String username, String mail, String... ous) {
@@ -90,7 +91,6 @@ private void prepareRequest(String username, String mail, String... ous) {
                 new TreeSet<>(Arrays.asList(ous))));
         plugin.setSessionEstablished(dummyRequest, true);
         plugin.setSessionUsername(dummyRequest, username);
-        plugin.setSessionVersion(dummyRequest, 1);
     }
 
     private Project makeProject(String name) {
@@ -136,36 +136,6 @@ public void testIsAllowed() {
 
         prepareRequest("00A", "[email protected]", "MI6", "MI7");
 
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Random Project")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Project 1")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeGroup("Group 1")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeGroup("Group 2")));
-
-    }
-
-    @Test
-    public void testInvalidateSession() {
-        /**
-         * whitelist[mail] => [[email protected], [email protected], just_a_text]
-         */
-        prepareRequest("007", "[email protected]", "MI6", "MI7");
-        
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Random Project")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Project 1")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeGroup("Group 1")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeGroup("Group 2")));
-
-        framework.increasePluginVersion();
-        prepareRequest("007", "[email protected]", "MI6", "MI7");
-
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Random Project")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Project 1")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeGroup("Group 1")));
-        Assert.assertTrue(plugin.isAllowed(dummyRequest, makeGroup("Group 2")));
-
-        plugin.setSessionVersion(dummyRequest, 2);
-        prepareRequest("007", "[email protected]", "MI6", "MI7");
-        
         Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Random Project")));
         Assert.assertTrue(plugin.isAllowed(dummyRequest, makeProject("Project 1")));
         Assert.assertTrue(plugin.isAllowed(dummyRequest, makeGroup("Group 1")));

plugins/LdapPlugin/test/opengrok/auth/plugin/LdapFilterTest.java renamed to plugins/LdapPlugin/test/opengrok/auth/plugin/LdapFilterPluginTest.java

@@ -31,7 +31,7 @@
 
 import static org.junit.Assert.assertEquals;
 
-public class LdapFilterTest {
+public class LdapFilterPluginTest {
 
     private LdapFilterPlugin plugin;
 

plugins/LdapPlugin/test/opengrok/auth/plugin/util/DummyHttpServletRequestLdap.java

@@ -40,6 +40,7 @@
 import javax.servlet.http.HttpSessionContext;
 import opengrok.auth.plugin.UserPlugin;
 import opengrok.auth.plugin.entity.User;
+import org.opensolaris.opengrok.util.RandomString;
 
 public class DummyHttpServletRequestLdap implements HttpServletRequest {
 
@@ -60,7 +61,7 @@ public String getId() {
             if ((user = (User) getAttribute(UserPlugin.REQUEST_ATTR)) != null) {
                 return user.getUsername();
             }
-            return Strings.generate(5);
+            return RandomString.generate(5);
         }
 
         @Override