From: James Moger Date: Tue, 28 Jan 2014 18:16:37 +0000 (-0500) Subject: issue-361: Reset user cookie after administrative password change X-Git-Tag: v1.4.0~121 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=7ab32b65fcb20ca68d7afc357befb3a34de662bf;p=gitblit.git 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. --- diff --git a/releases.moxie b/releases.moxie index d646bc81..116fca95 100644 --- a/releases.moxie +++ b/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) diff --git a/src/main/java/com/gitblit/ConfigUserService.java b/src/main/java/com/gitblit/ConfigUserService.java index 19e4736a..e8652252 100644 --- a/src/main/java/com/gitblit/ConfigUserService.java +++ b/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 diff --git a/src/main/java/com/gitblit/client/EditUserDialog.java b/src/main/java/com/gitblit/client/EditUserDialog.java index 2936a29d..ab3ea67e 100644 --- a/src/main/java/com/gitblit/client/EditUserDialog.java +++ b/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 diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index eef675b2..cd4a258f 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/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("/"); diff --git a/src/main/java/com/gitblit/manager/GitblitManager.java b/src/main/java/com/gitblit/manager/GitblitManager.java index 7e788361..95d50ac1 100644 --- a/src/main/java/com/gitblit/manager/GitblitManager.java +++ b/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); diff --git a/src/main/java/com/gitblit/manager/IAuthenticationManager.java b/src/main/java/com/gitblit/manager/IAuthenticationManager.java index 093f44d9..3007a303 100644 --- a/src/main/java/com/gitblit/manager/IAuthenticationManager.java +++ b/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. * diff --git a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java index 62a8ea5a..15c35fa6 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java +++ b/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")) { diff --git a/src/main/java/com/gitblit/wicket/pages/SessionPage.java b/src/main/java/com/gitblit/wicket/pages/SessionPage.java index d2fcfa0d..22ae6e2e 100644 --- a/src/main/java/com/gitblit/wicket/pages/SessionPage.java +++ b/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; }