From 7ab32b65fcb20ca68d7afc357befb3a34de662bf Mon Sep 17 00:00:00 2001
From: James Moger <james.moger@gitblit.com>
Date: Tue, 28 Jan 2014 13:16:37 -0500
Subject: [PATCH] issue-361: Reset user cookie after administrative password change
---
src/main/java/com/gitblit/wicket/pages/EditUserPage.java | 3 +
src/main/java/com/gitblit/manager/IAuthenticationManager.java | 8 ++++
src/main/java/com/gitblit/wicket/pages/SessionPage.java | 20 ++++++++++
src/main/java/com/gitblit/manager/GitblitManager.java | 5 ++
src/main/java/com/gitblit/manager/AuthenticationManager.java | 61 +++++++++++++++++-------------
releases.moxie | 7 +++
src/main/java/com/gitblit/client/EditUserDialog.java | 3 +
src/main/java/com/gitblit/ConfigUserService.java | 3 +
8 files changed, 83 insertions(+), 27 deletions(-)
diff --git a/releases.moxie b/releases.moxie
index d646bc8..116fca9 100644
--- a/releases.moxie
+++ b/releases.moxie
@@ -8,7 +8,12 @@
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 19e4736..e865225 100644
--- a/src/main/java/com/gitblit/ConfigUserService.java
+++ b/src/main/java/com/gitblit/ConfigUserService.java
@@ -272,6 +272,9 @@
}
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 2936a29..ab3ea67 100644
--- a/src/main/java/com/gitblit/client/EditUserDialog.java
+++ b/src/main/java/com/gitblit/client/EditUserDialog.java
@@ -325,6 +325,9 @@
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 eef675b..cd4a258 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.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 @@
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
@@ -266,26 +272,6 @@
} else {
logger.warn(MessageFormat.format("Failed login attempt for {0}, invalid credentials from {1}",
username, httpRequest.getRemoteAddr()));
- }
- }
- }
- 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());
- }
}
}
}
@@ -365,6 +351,28 @@
}
/**
+ * 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.
*
* @param response
@@ -390,7 +398,8 @@
} 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 7e78836..95d50ac 100644
--- a/src/main/java/com/gitblit/manager/GitblitManager.java
+++ b/src/main/java/com/gitblit/manager/GitblitManager.java
@@ -634,6 +634,11 @@
}
@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 093f44d..3007a30 100644
--- a/src/main/java/com/gitblit/manager/IAuthenticationManager.java
+++ b/src/main/java/com/gitblit/manager/IAuthenticationManager.java
@@ -56,6 +56,14 @@
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.
*
* @param response
diff --git a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java
index 62a8ea5..15c35fa 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 @@
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 d2fcfa0..22ae6e2 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 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 @@
// 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;
}
--
Gitblit v1.9.1