issue-361: Reset user cookie after administrative password change

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

@@ -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)

src/main/java/com/gitblit/ConfigUserService.java

@@ -272,6 +272,9 @@ public class ConfigUserService implements IUserService {
 			}
 			read();
 			originalUser = users.remove(username.toLowerCase());
+			if (originalUser != null) {
+				cookies.remove(originalUser.cookie);
+			}
 			users.put(model.username.toLowerCase(), model);
 			// null check on "final" teams because JSON-sourced UserModel
 			// can have a null teams object

src/main/java/com/gitblit/client/EditUserDialog.java

@@ -325,6 +325,9 @@ public class EditUserDialog extends JDialog {
 				return false;
 			}
 
+			// change the cookie
+			user.cookie = StringUtils.getSHA1(user.username + password);
+
 			String type = settings.get(Keys.realm.passwordStorage).getString("md5");
 			if (type.equalsIgnoreCase("md5")) {
 				// store MD5 digest of password

src/main/java/com/gitblit/manager/AuthenticationManager.java

@@ -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("/");

src/main/java/com/gitblit/manager/GitblitManager.java

@@ -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);

src/main/java/com/gitblit/manager/IAuthenticationManager.java

@@ -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.
 	 *

src/main/java/com/gitblit/wicket/pages/EditUserPage.java

@@ -154,6 +154,9 @@ public class EditUserPage extends RootSubPage {
 							return;
 						}
 
+						// change the cookie
+						userModel.cookie = StringUtils.getSHA1(userModel.username + password);
+
 						// Optionally store the password MD5 digest.
 						String type = app().settings().getString(Keys.realm.passwordStorage, "md5");
 						if (type.equalsIgnoreCase("md5")) {

src/main/java/com/gitblit/wicket/pages/SessionPage.java

@@ -16,6 +16,7 @@
 package com.gitblit.wicket.pages;
 
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
 
 import org.apache.wicket.PageParameters;
 import org.apache.wicket.markup.html.WebPage;
@@ -24,6 +25,7 @@ import org.apache.wicket.protocol.http.WebResponse;
 
 import com.gitblit.Keys;
 import com.gitblit.models.UserModel;
+import com.gitblit.utils.StringUtils;
 import com.gitblit.wicket.GitBlitWebApp;
 import com.gitblit.wicket.GitBlitWebSession;
 
@@ -53,6 +55,24 @@ public abstract class SessionPage extends WebPage {
 			// already have a session, refresh usermodel to pick up
 			// any changes to permissions or roles (issue-186)
 			UserModel user = app().users().getUserModel(session.getUser().username);
+
+			// validate cookie during session (issue-361)
+			if (app().settings().getBoolean(Keys.web.allowCookieAuthentication, true)) {
+				HttpServletRequest request = ((WebRequest) getRequestCycle().getRequest())
+						.getHttpServletRequest();
+				String requestCookie = app().authentication().getCookie(request);
+				if (!StringUtils.isEmpty(requestCookie) && !StringUtils.isEmpty(user.cookie)) {
+					if (!requestCookie.equals(user.cookie)) {
+						// cookie was changed during our session
+						HttpServletResponse response = ((WebResponse) getRequestCycle().getResponse())
+								.getHttpServletResponse();
+						app().authentication().logout(response, user);
+						session.setUser(null);
+						session.invalidateNow();
+						return;
+					}
+				}
+			}
 			session.setUser(user);
 			return;
 		}