From 03223516b32758fe9d5602ffb4ce10a8a308f0e9 Mon Sep 17 00:00:00 2001
From: James Moger <james.moger@gmail.com>
Date: Fri, 18 Sep 2015 14:30:07 -0400
Subject: [PATCH] Merge pull request #908 from mrjoel/mrjoel-authrequestnotsession
---
src/main/java/com/gitblit/wicket/pages/SessionPage.java | 68 ++++++++++++++++++++--------------
src/main/java/com/gitblit/Constants.java | 4 +
src/main/java/com/gitblit/manager/AuthenticationManager.java | 35 ++++++++++++-----
src/main/java/com/gitblit/wicket/pages/RootPage.java | 6 +-
4 files changed, 71 insertions(+), 42 deletions(-)
diff --git a/src/main/java/com/gitblit/Constants.java b/src/main/java/com/gitblit/Constants.java
index 8b05f89..787d726 100644
--- a/src/main/java/com/gitblit/Constants.java
+++ b/src/main/java/com/gitblit/Constants.java
@@ -137,7 +137,9 @@
public static final String DEVELOP = "develop";
- public static final String AUTHENTICATION_TYPE = "authentication-type";
+ public static final String ATTRIB_AUTHTYPE = NAME + ":authentication-type";
+
+ public static final String ATTRIB_AUTHUSER = NAME + ":authenticated-user";
public static String getVersion() {
String v = Constants.class.getPackage().getImplementationVersion();
diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java
index 38e45a6..51aa221 100644
--- a/src/main/java/com/gitblit/manager/AuthenticationManager.java
+++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java
@@ -194,6 +194,14 @@
*/
@Override
public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCertificate) {
+
+ // Check if this request has already been authenticated, and trust that instead of re-processing
+ String reqAuthUser = (String) httpRequest.getAttribute(Constants.ATTRIB_AUTHUSER);
+ if (!StringUtils.isEmpty(reqAuthUser)) {
+ logger.warn("Called servlet authenticate when request is already authenticated.");
+ return userManager.getUserModel(reqAuthUser);
+ }
+
// try to authenticate by servlet container principal
if (!requiresCertificate) {
Principal principal = httpRequest.getUserPrincipal();
@@ -204,7 +212,7 @@
UserModel user = userManager.getUserModel(username);
if (user != null) {
// existing user
- flagSession(httpRequest, AuthenticationType.CONTAINER);
+ flagRequest(httpRequest, AuthenticationType.CONTAINER, user.username);
logger.debug(MessageFormat.format("{0} authenticated by servlet container principal from {1}",
user.username, httpRequest.getRemoteAddr()));
return validateAuthentication(user, AuthenticationType.CONTAINER);
@@ -239,7 +247,7 @@
}
userManager.updateUserModel(user);
- flagSession(httpRequest, AuthenticationType.CONTAINER);
+ flagRequest(httpRequest, AuthenticationType.CONTAINER, user.username);
logger.debug(MessageFormat.format("{0} authenticated and created by servlet container principal from {1}",
user.username, httpRequest.getRemoteAddr()));
return validateAuthentication(user, AuthenticationType.CONTAINER);
@@ -260,7 +268,7 @@
UserModel user = userManager.getUserModel(model.username);
X509Metadata metadata = HttpUtils.getCertificateMetadata(httpRequest);
if (user != null) {
- flagSession(httpRequest, AuthenticationType.CERTIFICATE);
+ flagRequest(httpRequest, AuthenticationType.CERTIFICATE, user.username);
logger.debug(MessageFormat.format("{0} authenticated by client certificate {1} from {2}",
user.username, metadata.serialNumber, httpRequest.getRemoteAddr()));
return validateAuthentication(user, AuthenticationType.CERTIFICATE);
@@ -282,7 +290,7 @@
if (!StringUtils.isEmpty(cookie)) {
user = userManager.getUserModel(cookie.toCharArray());
if (user != null) {
- flagSession(httpRequest, AuthenticationType.COOKIE);
+ flagRequest(httpRequest, AuthenticationType.COOKIE, user.username);
logger.debug(MessageFormat.format("{0} authenticated by cookie from {1}",
user.username, httpRequest.getRemoteAddr()));
return validateAuthentication(user, AuthenticationType.COOKIE);
@@ -304,7 +312,7 @@
char[] password = values[1].toCharArray();
user = authenticate(username, password);
if (user != null) {
- flagSession(httpRequest, AuthenticationType.CREDENTIALS);
+ flagRequest(httpRequest, AuthenticationType.CREDENTIALS, user.username);
logger.debug(MessageFormat.format("{0} authenticated by BASIC request header from {1}",
user.username, httpRequest.getRemoteAddr()));
return validateAuthentication(user, AuthenticationType.CREDENTIALS);
@@ -423,8 +431,9 @@
return user;
}
- protected void flagSession(HttpServletRequest httpRequest, AuthenticationType authenticationType) {
- httpRequest.getSession().setAttribute(Constants.AUTHENTICATION_TYPE, authenticationType);
+ protected void flagRequest(HttpServletRequest httpRequest, AuthenticationType authenticationType, String authedUsername) {
+ httpRequest.setAttribute(Constants.ATTRIB_AUTHUSER, authedUsername);
+ httpRequest.setAttribute(Constants.ATTRIB_AUTHTYPE, authenticationType);
}
/**
@@ -545,9 +554,15 @@
@Override
public void setCookie(HttpServletRequest request, HttpServletResponse response, UserModel user) {
if (settings.getBoolean(Keys.web.allowCookieAuthentication, true)) {
- HttpSession session = request.getSession();
- AuthenticationType authenticationType = (AuthenticationType) session.getAttribute(Constants.AUTHENTICATION_TYPE);
- boolean standardLogin = authenticationType.isStandard();
+ boolean standardLogin = true;
+
+ if (null != request) {
+ // Pull the auth type from the request, it is set there if container managed
+ AuthenticationType authenticationType = (AuthenticationType) request.getAttribute(Constants.ATTRIB_AUTHTYPE);
+
+ if (null != authenticationType)
+ standardLogin = authenticationType.isStandard();
+ }
if (standardLogin) {
Cookie userCookie;
diff --git a/src/main/java/com/gitblit/wicket/pages/RootPage.java b/src/main/java/com/gitblit/wicket/pages/RootPage.java
index 6d7ebd8..61d7b75 100644
--- a/src/main/java/com/gitblit/wicket/pages/RootPage.java
+++ b/src/main/java/com/gitblit/wicket/pages/RootPage.java
@@ -279,7 +279,7 @@
request = ((WebRequest) getRequest()).getHttpServletRequest();
response = ((WebResponse) getResponse()).getHttpServletResponse();
- request.getSession().setAttribute(Constants.AUTHENTICATION_TYPE, AuthenticationType.CREDENTIALS);
+ request.getSession().setAttribute(Constants.ATTRIB_AUTHTYPE, AuthenticationType.CREDENTIALS);
// Set Cookie
app().authentication().setCookie(request, response, user);
@@ -608,8 +608,8 @@
UserModel user = session.getUser();
boolean editCredentials = app().authentication().supportsCredentialChanges(user);
HttpServletRequest request = ((WebRequest) getRequest()).getHttpServletRequest();
- AuthenticationType authenticationType = (AuthenticationType) request.getSession().getAttribute(Constants.AUTHENTICATION_TYPE);
- boolean standardLogin = authenticationType.isStandard();
+ AuthenticationType authenticationType = (AuthenticationType) request.getAttribute(Constants.ATTRIB_AUTHTYPE);
+ boolean standardLogin = (null != authenticationType) ? authenticationType.isStandard() : true;
if (app().settings().getBoolean(Keys.web.allowGravatar, true)) {
add(new AvatarImage("username", user, "navbarGravatar", 20, false));
diff --git a/src/main/java/com/gitblit/wicket/pages/SessionPage.java b/src/main/java/com/gitblit/wicket/pages/SessionPage.java
index af7f211..bcf8e97 100644
--- a/src/main/java/com/gitblit/wicket/pages/SessionPage.java
+++ b/src/main/java/com/gitblit/wicket/pages/SessionPage.java
@@ -56,34 +56,50 @@
HttpServletRequest request = ((WebRequest) getRequest()).getHttpServletRequest();
HttpServletResponse response = ((WebResponse) getResponse()).getHttpServletResponse();
- if (session.isLoggedIn() && !session.isSessionInvalidated()) {
- // 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);
+ // If using container/external servlet authentication, use request attribute
+ String authedUser = (String) request.getAttribute(Constants.ATTRIB_AUTHUSER);
- if (user == null || user.disabled) {
- // user was deleted/disabled during session
- app().authentication().logout(request, response, user);
- session.setUser(null);
- session.invalidateNow();
- return;
+ // Default to trusting session authentication if not set in request by external processing
+ if (StringUtils.isEmpty(authedUser) && session.isLoggedIn()) {
+ authedUser = session.getUsername();
+ }
+
+ if (!StringUtils.isEmpty(authedUser)) {
+ // Avoid session fixation for non-session authentication
+ // If the authenticated user is different from the session user, discard
+ // the old session entirely, without trusting any session values
+ if (!authedUser.equals(session.getUsername())) {
+ session.replaceSession();
}
- // validate cookie during session (issue-361)
- if (user != null && app().settings().getBoolean(Keys.web.allowCookieAuthentication, true)) {
- 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
- app().authentication().logout(request, response, user);
- session.setUser(null);
- session.invalidateNow();
- return;
+ if (!session.isSessionInvalidated()) {
+ // Refresh usermodel to pick up any changes to permissions or roles (issue-186)
+ UserModel user = app().users().getUserModel(authedUser);
+
+ if (user == null || user.disabled) {
+ // user was deleted/disabled during session
+ app().authentication().logout(request, response, user);
+ session.setUser(null);
+ session.invalidateNow();
+ return;
+ }
+
+ // validate cookie during session (issue-361)
+ if (app().settings().getBoolean(Keys.web.allowCookieAuthentication, true)) {
+ 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
+ app().authentication().logout(request, response, user);
+ session.setUser(null);
+ session.invalidateNow();
+ return;
+ }
}
}
+ session.setUser(user);
+ return;
}
- session.setUser(user);
- return;
}
// try to authenticate by servlet request
@@ -91,9 +107,7 @@
// Login the user
if (user != null) {
- // preserve the authentication type across session replacement
- AuthenticationType authenticationType = (AuthenticationType) request.getSession()
- .getAttribute(Constants.AUTHENTICATION_TYPE);
+ AuthenticationType authenticationType = (AuthenticationType) request.getAttribute(Constants.ATTRIB_AUTHTYPE);
// issue 62: fix session fixation vulnerability
// but only if authentication was done in the container.
@@ -101,10 +115,8 @@
// don't like
if (AuthenticationType.CONTAINER != authenticationType) {
session.replaceSession();
- }
+ }
session.setUser(user);
-
- request.getSession().setAttribute(Constants.AUTHENTICATION_TYPE, authenticationType);
// Set Cookie
app().authentication().setCookie(request, response, user);
--
Gitblit v1.9.1