]> source.dussan.org Git - gitblit.git/commitdiff
issue-361: Reset user cookie after administrative password change
authorJames Moger <james.moger@gitblit.com>
Tue, 28 Jan 2014 18:16:37 +0000 (13:16 -0500)
committerJames Moger <james.moger@gitblit.com>
Tue, 28 Jan 2014 18:16:37 +0000 (13:16 -0500)
Cookies were not reset on administrative password change of a user
account. This allowed accounts with changed passwords to continue
authenticating.  Cookies are now reset on password changes, they are
validated on each page request, AND they will now expire 7 days after
generation.

releases.moxie
src/main/java/com/gitblit/ConfigUserService.java
src/main/java/com/gitblit/client/EditUserDialog.java
src/main/java/com/gitblit/manager/AuthenticationManager.java
src/main/java/com/gitblit/manager/GitblitManager.java
src/main/java/com/gitblit/manager/IAuthenticationManager.java
src/main/java/com/gitblit/wicket/pages/EditUserPage.java
src/main/java/com/gitblit/wicket/pages/SessionPage.java

index d646bc8122d785e960079a39c974b73798a96371..116fca950c6d76a210eb950c4e9bdbe2bd2b4cb1 100644 (file)
@@ -8,7 +8,12 @@ r20: {
     note: "The default access restriction has been elevated from NONE to PUSH and anonymous push access has been disabled."
     html: ~
     text: ~
-    security: ~
+    security:
+       - ''issue-361: Cookies were not reset on administrative password change of a user account.
+        This allowed accounts with changed passwords to continue authenticating.
+        Cookies are now reset on password changes, they are validated on each page request,
+        AND they will now expire 7 days after generation.
+        ''
     fixes:
        - Fixed incorrect tagger attribution in the dashboard (issue-276)
        - Fixed support for implied SSH urls in web.otherUrls (issue-311)
index 19e4736a8750c6c09e315e51d4de0b9845f96534..e8652252e2d5ee17ec12966ea0fc548dd8c1534d 100644 (file)
@@ -272,6 +272,9 @@ public class ConfigUserService implements IUserService {
                        }\r
                        read();\r
                        originalUser = users.remove(username.toLowerCase());\r
+                       if (originalUser != null) {\r
+                               cookies.remove(originalUser.cookie);\r
+                       }\r
                        users.put(model.username.toLowerCase(), model);\r
                        // null check on "final" teams because JSON-sourced UserModel\r
                        // can have a null teams object\r
index 2936a29d88661f2cef654dce0e34228f4b1b6364..ab3ea67e30f27b272e4231d81f0eecadb16ad2cc 100644 (file)
@@ -325,6 +325,9 @@ public class EditUserDialog extends JDialog {
                                return false;\r
                        }\r
 \r
+                       // change the cookie\r
+                       user.cookie = StringUtils.getSHA1(user.username + password);\r
+\r
                        String type = settings.get(Keys.realm.passwordStorage).getString("md5");\r
                        if (type.equalsIgnoreCase("md5")) {\r
                                // store MD5 digest of password\r
index eef675b25d519c9df6d1f4f371b683f0d2f3d230..cd4a258fad9647be79a1cd01e1d3c7bc0175cf8a 100644 (file)
@@ -22,6 +22,7 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
 
 import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
@@ -235,13 +236,18 @@ public class AuthenticationManager implements IAuthenticationManager {
                        return null;
                }
 
+               UserModel user = null;
+
                // try to authenticate by cookie
-               UserModel user = authenticate(httpRequest.getCookies());
-               if (user != null) {
-                       flagWicketSession(AuthenticationType.COOKIE);
-                       logger.debug(MessageFormat.format("{0} authenticated by cookie from {1}",
+               String cookie = getCookie(httpRequest);
+               if (!StringUtils.isEmpty(cookie)) {
+                       user = userManager.getUserModel(cookie.toCharArray());
+                       if (user != null) {
+                               flagWicketSession(AuthenticationType.COOKIE);
+                               logger.debug(MessageFormat.format("{0} authenticated by cookie from {1}",
                                        user.username, httpRequest.getRemoteAddr()));
-                       return user;
+                               return user;
+                       }
                }
 
                // try to authenticate by BASIC
@@ -272,26 +278,6 @@ public class AuthenticationManager implements IAuthenticationManager {
                return null;
        }
 
-       /**
-        * Authenticate a user based on their cookie.
-        *
-        * @param cookies
-        * @return a user object or null
-        */
-       protected UserModel authenticate(Cookie[] cookies) {
-               if (settings.getBoolean(Keys.web.allowCookieAuthentication, true)) {
-                       if (cookies != null && cookies.length > 0) {
-                               for (Cookie cookie : cookies) {
-                                       if (cookie.getName().equals(Constants.NAME)) {
-                                               String value = cookie.getValue();
-                                               return userManager.getUserModel(value.toCharArray());
-                                       }
-                               }
-                       }
-               }
-               return null;
-       }
-
        protected void flagWicketSession(AuthenticationType authenticationType) {
                RequestCycle requestCycle = RequestCycle.get();
                if (requestCycle != null) {
@@ -364,6 +350,28 @@ public class AuthenticationManager implements IAuthenticationManager {
                return user;
        }
 
+       /**
+        * Returns the Gitlbit cookie in the request.
+        *
+        * @param request
+        * @return the Gitblit cookie for the request or null if not found
+        */
+       @Override
+       public String getCookie(HttpServletRequest request) {
+               if (settings.getBoolean(Keys.web.allowCookieAuthentication, true)) {
+                       Cookie[] cookies = request.getCookies();
+                       if (cookies != null && cookies.length > 0) {
+                               for (Cookie cookie : cookies) {
+                                       if (cookie.getName().equals(Constants.NAME)) {
+                                               String value = cookie.getValue();
+                                               return value;
+                                       }
+                               }
+                       }
+               }
+               return null;
+       }
+
        /**
         * Sets a cookie for the specified user.
         *
@@ -390,7 +398,8 @@ public class AuthenticationManager implements IAuthenticationManager {
                                        } else {
                                                // create real cookie
                                                userCookie = new Cookie(Constants.NAME, cookie);
-                                               userCookie.setMaxAge(Integer.MAX_VALUE);
+                                               // expire the cookie in 7 days
+                                               userCookie.setMaxAge((int) TimeUnit.DAYS.toSeconds(7));
                                        }
                                }
                                userCookie.setPath("/");
index 7e788361568f8c9d2c13cf21b14444d996a0342d..95d50ac1e317a8ea15bb770a0781fac6d703d34f 100644 (file)
@@ -633,6 +633,11 @@ public class GitblitManager implements IGitblit {
                return user;
        }
 
+       @Override
+       public String getCookie(HttpServletRequest request) {
+               return authenticationManager.getCookie(request);
+       }
+
        @Override
        public void setCookie(HttpServletResponse response, UserModel user) {
                authenticationManager.setCookie(response, user);
index 093f44d972e75b84132dec64fcd526f61034d6f4..3007a303c48b55851e11a7001459eb3383243203 100644 (file)
@@ -55,6 +55,14 @@ public interface IAuthenticationManager extends IManager {
         */
        UserModel authenticate(String username, char[] password);
 
+       /**
+        * Returns the Gitlbit cookie in the request.
+        *
+        * @param request
+        * @return the Gitblit cookie for the request or null if not found
+        */
+       String getCookie(HttpServletRequest request);
+
        /**
         * Sets a cookie for the specified user.
         *
index 62a8ea5a71573e03e129223674852bd6f288aff4..15c35fa6aee9441d546a873c866d37fd7cb490c1 100644 (file)
@@ -154,6 +154,9 @@ public class EditUserPage extends RootSubPage {
                                                        return;\r
                                                }\r
 \r
+                                               // change the cookie\r
+                                               userModel.cookie = StringUtils.getSHA1(userModel.username + password);\r
+\r
                                                // Optionally store the password MD5 digest.\r
                                                String type = app().settings().getString(Keys.realm.passwordStorage, "md5");\r
                                                if (type.equalsIgnoreCase("md5")) {\r
index d2fcfa0da59c6c823228e048699c65eeb2f9bef6..22ae6e2ea242974103a4372da67e93a06f8314e2 100644 (file)
@@ -16,6 +16,7 @@
 package com.gitblit.wicket.pages;\r
 \r
 import javax.servlet.http.HttpServletRequest;\r
+import javax.servlet.http.HttpServletResponse;\r
 \r
 import org.apache.wicket.PageParameters;\r
 import org.apache.wicket.markup.html.WebPage;\r
@@ -24,6 +25,7 @@ import org.apache.wicket.protocol.http.WebResponse;
 \r
 import com.gitblit.Keys;\r
 import com.gitblit.models.UserModel;\r
+import com.gitblit.utils.StringUtils;\r
 import com.gitblit.wicket.GitBlitWebApp;\r
 import com.gitblit.wicket.GitBlitWebSession;\r
 \r
@@ -53,6 +55,24 @@ public abstract class SessionPage extends WebPage {
                        // already have a session, refresh usermodel to pick up\r
                        // any changes to permissions or roles (issue-186)\r
                        UserModel user = app().users().getUserModel(session.getUser().username);\r
+\r
+                       // validate cookie during session (issue-361)\r
+                       if (app().settings().getBoolean(Keys.web.allowCookieAuthentication, true)) {\r
+                               HttpServletRequest request = ((WebRequest) getRequestCycle().getRequest())\r
+                                               .getHttpServletRequest();\r
+                               String requestCookie = app().authentication().getCookie(request);\r
+                               if (!StringUtils.isEmpty(requestCookie) && !StringUtils.isEmpty(user.cookie)) {\r
+                                       if (!requestCookie.equals(user.cookie)) {\r
+                                               // cookie was changed during our session\r
+                                               HttpServletResponse response = ((WebResponse) getRequestCycle().getResponse())\r
+                                                               .getHttpServletResponse();\r
+                                               app().authentication().logout(response, user);\r
+                                               session.setUser(null);\r
+                                               session.invalidateNow();\r
+                                               return;\r
+                                       }\r
+                               }\r
+                       }\r
                        session.setUser(user);\r
                        return;\r
                }\r