Implemented custom request handling for (un)authenticated sessions to workaround Wicket bugs

7 files changed, 84 insertions, 17 deletions

diff --git a/docs/04_releases.mkd b/docs/04_releases.mkd
index 074396c7..21b47bab 100644
--- a/docs/04_releases.mkd
+++ b/docs/04_releases.mkd
@@ -11,6 +11,7 @@ If you are updating from an earlier release AND you have indexed branches with t
 

 #### fixes

 

+- Bypass Wicket's inability to handle direct url addressing of a view-restricted, grouped repository for new, unauthenticated sessions (e.g. click link from email or rss feed without having an active Wicket session)

 - Fixed MailExecutor's failure to cope with mail server connection troubles resulting in 100% CPU usage

 - Fixed generated urls in Groovy *sendmail* hook script for grouped repositories

 - Fixed generated urls in RSS feeds for grouped repositories

diff --git a/src/com/gitblit/wicket/AuthorizationStrategy.java b/src/com/gitblit/wicket/AuthorizationStrategy.java
index 452215a7..16a4ec80 100644
--- a/src/com/gitblit/wicket/AuthorizationStrategy.java
+++ b/src/com/gitblit/wicket/AuthorizationStrategy.java
@@ -16,7 +16,7 @@
 package com.gitblit.wicket;

 

 import org.apache.wicket.Component;

-import org.apache.wicket.RestartResponseAtInterceptPageException;

+import org.apache.wicket.RestartResponseException;

 import org.apache.wicket.authorization.IUnauthorizedComponentInstantiationListener;

 import org.apache.wicket.authorization.strategies.page.AbstractPageAuthorizationStrategy;

 

@@ -49,6 +49,7 @@ public class AuthorizationStrategy extends AbstractPageAuthorizationStrategy imp
 			GitBlitWebSession session = GitBlitWebSession.get();

 			if (authenticateView && !session.isLoggedIn()) {

 				// authentication required

+				session.cacheRequest(pageClass);

 				return false;

 			}

 

@@ -78,7 +79,7 @@ public class AuthorizationStrategy extends AbstractPageAuthorizationStrategy imp
 	@Override

 	public void onUnauthorizedInstantiation(Component component) {

 		if (component instanceof BasePage) {

-			throw new RestartResponseAtInterceptPageException(RepositoriesPage.class);

+			throw new RestartResponseException(RepositoriesPage.class);

 		}

 	}

 }

diff --git a/src/com/gitblit/wicket/GitBlitWebSession.java b/src/com/gitblit/wicket/GitBlitWebSession.java
index 2238660f..7ecc05b2 100644
--- a/src/com/gitblit/wicket/GitBlitWebSession.java
+++ b/src/com/gitblit/wicket/GitBlitWebSession.java
@@ -15,10 +15,16 @@
  */

 package com.gitblit.wicket;

 

+import java.util.Map;

 import java.util.TimeZone;

 

+import org.apache.wicket.Page;

+import org.apache.wicket.PageParameters;

+import org.apache.wicket.RedirectToUrlException;

 import org.apache.wicket.Request;

 import org.apache.wicket.Session;

+import org.apache.wicket.protocol.http.RequestUtils;

+import org.apache.wicket.protocol.http.WebRequestCycle;

 import org.apache.wicket.protocol.http.WebSession;

 import org.apache.wicket.protocol.http.request.WebClientInfo;

 

@@ -33,7 +39,9 @@ public final class GitBlitWebSession extends WebSession {
 	private UserModel user;

 

 	private String errorMessage;

-

+	

+	private String requestUrl;

+	

 	public GitBlitWebSession(Request request) {

 		super(request);

 	}

@@ -42,6 +50,46 @@ public final class GitBlitWebSession extends WebSession {
 		super.invalidate();

 		user = null;

 	}

+	

+	/**

+	 * Cache the requested protected resource pending successful authentication.

+	 * 

+	 * @param pageClass

+	 */

+	public void cacheRequest(Class<? extends Page> pageClass) {

+		// build absolute url with correctly encoded parameters?!

+		Request req = WebRequestCycle.get().getRequest();

+		Map<String, ?> params = req.getRequestParameters().getParameters();

+		PageParameters pageParams = new PageParameters(params);

+		String relativeUrl = WebRequestCycle.get().urlFor(pageClass, pageParams).toString();

+		requestUrl = RequestUtils.toAbsolutePath(relativeUrl);

+		if (isTemporary())

+		{

+			// we must bind the temporary session into the session store

+			// so that we can re-use this session for reporting an error message

+			// on the redirected page and continuing the request after

+			// authentication.

+			bind();

+		}

+	}

+	

+	/**

+	 * Continue any cached request.  This is used when a request for a protected

+	 * resource is aborted/redirected pending proper authentication.  Gitblit

+	 * no longer uses Wicket's built-in mechanism for this because of Wicket's

+	 * failure to properly handle parameters with forward-slashes.  This is a

+	 * constant source of headaches with Wicket.

+	 *  

+	 * @return false if there is no cached request to process

+	 */

+	public boolean continueRequest() {

+		if (requestUrl != null) {

+			String url = requestUrl;

+			requestUrl = null;

+			throw new RedirectToUrlException(url);

+		}

+		return false;

+	}

 

 	public boolean isLoggedIn() {

 		return user != null;

@@ -53,6 +101,10 @@ public final class GitBlitWebSession extends WebSession {
 		}

 		return user.canAdmin;

 	}

+	

+	public String getUsername() {

+		return user == null ? "anonymous" : user.username;

+	}

 

 	public UserModel getUser() {

 		return user;

diff --git a/src/com/gitblit/wicket/pages/BasePage.java b/src/com/gitblit/wicket/pages/BasePage.java
index 82862ae9..234c2a94 100644
--- a/src/com/gitblit/wicket/pages/BasePage.java
+++ b/src/com/gitblit/wicket/pages/BasePage.java
@@ -26,7 +26,7 @@ import javax.servlet.http.HttpServletRequest;
 import org.apache.wicket.Application;

 import org.apache.wicket.MarkupContainer;

 import org.apache.wicket.PageParameters;

-import org.apache.wicket.RestartResponseAtInterceptPageException;

+import org.apache.wicket.RedirectToUrlException;

 import org.apache.wicket.RestartResponseException;

 import org.apache.wicket.markup.html.CSSPackageResource;

 import org.apache.wicket.markup.html.WebPage;

@@ -35,9 +35,11 @@ import org.apache.wicket.markup.html.link.BookmarkablePageLink;
 import org.apache.wicket.markup.html.link.ExternalLink;

 import org.apache.wicket.markup.html.panel.FeedbackPanel;

 import org.apache.wicket.markup.html.panel.Fragment;

+import org.apache.wicket.protocol.http.RequestUtils;

 import org.apache.wicket.protocol.http.WebRequest;

 import org.apache.wicket.protocol.http.WebResponse;

 import org.apache.wicket.protocol.http.servlet.ServletWebRequest;

+import org.apache.wicket.request.RequestParameters;

 import org.slf4j.Logger;

 import org.slf4j.LoggerFactory;

 

@@ -137,7 +139,8 @@ public abstract class BasePage extends WebPage {
 			// Set Cookie

 			WebResponse response = (WebResponse) getRequestCycle().getResponse();

 			GitBlit.self().setCookie(response, user);

-			continueToOriginalDestination();

+			

+			session.continueRequest();

 		}

 	}

 

@@ -229,7 +232,7 @@ public abstract class BasePage extends WebPage {
 		// inject username into repository url if authentication is required

 		if (repository.accessRestriction.exceeds(AccessRestrictionType.NONE)

 				&& GitBlitWebSession.get().isLoggedIn()) {

-			String username = GitBlitWebSession.get().getUser().username;

+			String username = GitBlitWebSession.get().getUsername();

 			sb.insert(sb.indexOf("://") + 3, username + "@");

 		}

 		return sb.toString();

@@ -240,10 +243,13 @@ public abstract class BasePage extends WebPage {
 	}

 

 	public void error(String message, boolean redirect) {

-		logger.error(message);

+		logger.error(message  + " for " + GitBlitWebSession.get().getUsername());

 		if (redirect) {

 			GitBlitWebSession.get().cacheErrorMessage(message);

-			throw new RestartResponseException(getApplication().getHomePage());

+			RequestParameters params = getRequest().getRequestParameters();

+			String relativeUrl = urlFor(RepositoriesPage.class, null).toString();

+			String absoluteUrl = RequestUtils.toAbsolutePath(relativeUrl);

+			throw new RedirectToUrlException(absoluteUrl);

 		} else {

 			super.error(message);

 		}

@@ -260,12 +266,13 @@ public abstract class BasePage extends WebPage {
 	}

 

 	public void authenticationError(String message) {

-		logger.error(message);

-		if (GitBlitWebSession.get().isLoggedIn()) {

-			error(message, true);

-		} else {

-			throw new RestartResponseAtInterceptPageException(RepositoriesPage.class);

+		logger.error(getRequest().getURL() + " for " + GitBlitWebSession.get().getUsername());

+		if (!GitBlitWebSession.get().isLoggedIn()) {

+			// cache the request if we have not authenticated.

+			// the request will continue after authentication.

+			GitBlitWebSession.get().cacheRequest(getClass());

 		}

+		error(message, true);

 	}

 

 	/**

diff --git a/src/com/gitblit/wicket/pages/ChangePasswordPage.java b/src/com/gitblit/wicket/pages/ChangePasswordPage.java
index cbe732ff..5e663006 100644
--- a/src/com/gitblit/wicket/pages/ChangePasswordPage.java
+++ b/src/com/gitblit/wicket/pages/ChangePasswordPage.java
@@ -56,7 +56,7 @@ public class ChangePasswordPage extends RootSubPage {
 					GitBlit.getString(Keys.realm.userService, "users.conf")), true);

 		}

 		

-		setupPage(getString("gb.changePassword"), GitBlitWebSession.get().getUser().username);

+		setupPage(getString("gb.changePassword"), GitBlitWebSession.get().getUsername());

 

 		StatelessForm<Void> form = new StatelessForm<Void>("passwordForm") {

 

diff --git a/src/com/gitblit/wicket/pages/RepositoryPage.java b/src/com/gitblit/wicket/pages/RepositoryPage.java
index 6d33a147..19a5de2c 100644
--- a/src/com/gitblit/wicket/pages/RepositoryPage.java
+++ b/src/com/gitblit/wicket/pages/RepositoryPage.java
@@ -151,7 +151,7 @@ public abstract class RepositoryPage extends BasePage {
 		if (showAdmin

 				|| GitBlitWebSession.get().isLoggedIn()

 				&& (model.owner != null && model.owner.equalsIgnoreCase(GitBlitWebSession.get()

-						.getUser().username))) {

+						.getUsername()))) {

 			pages.put("edit", new PageRegistration("gb.edit", EditRepositoryPage.class, params));

 		}

 		return pages;

@@ -198,7 +198,13 @@ public abstract class RepositoryPage extends BasePage {
 			RepositoryModel model = GitBlit.self().getRepositoryModel(

 					GitBlitWebSession.get().getUser(), repositoryName);

 			if (model == null) {

-				authenticationError(getString("gb.unauthorizedAccessForRepository") + " " + repositoryName);

+				if (GitBlit.self().hasRepository(repositoryName)) {

+					// has repository, but unauthorized

+					authenticationError(getString("gb.unauthorizedAccessForRepository") + " " + repositoryName);

+				} else {

+					// does not have repository

+					error(getString("gb.canNotLoadRepository") + " " + repositoryName, true);

+				}

 				return null;

 			}

 			m = model;

diff --git a/src/com/gitblit/wicket/pages/RootPage.java b/src/com/gitblit/wicket/pages/RootPage.java
index eaa25425..40f7aec4 100644
--- a/src/com/gitblit/wicket/pages/RootPage.java
+++ b/src/com/gitblit/wicket/pages/RootPage.java
@@ -210,7 +210,7 @@ public abstract class RootPage extends BasePage {
 				GitBlit.self().setCookie(response, user);

 			}

 

-			if (!continueToOriginalDestination()) {

+			if (!session.continueRequest()) {

 				PageParameters params = getPageParameters();

 				if (params == null) {

 					// redirect to this page