Skip to content
Permalink
Browse files

Fixed infinite redirect issue for password and web based plugin. (#4853)

  • Loading branch information...
pranavbansod authored and bdpiparva committed Jun 20, 2018
1 parent ebf9017 commit 3a1933d4b43e8c1a1f888623c58b664380929faf
@@ -26,6 +26,8 @@
import com.thoughtworks.go.server.service.SecurityAuthConfigService;
import com.thoughtworks.go.server.service.SecurityService;
import com.thoughtworks.go.server.web.GoVelocityView;
import com.thoughtworks.go.util.Clock;
import com.thoughtworks.go.util.SystemEnvironment;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
@@ -55,16 +57,22 @@
private static final String UNKNOWN_ERROR_WHILE_AUTHENTICATION = "There was an unknown error authenticating you. Please try again after some time, and contact the administrator if the problem persists.";
private final SecurityService securityService;
private final SecurityAuthConfigService securityAuthConfigService;
private final SystemEnvironment systemEnvironment;
private final Clock clock;
private final PasswordBasedPluginAuthenticationProvider passwordBasedPluginAuthenticationProvider;
private final WebBasedPluginAuthenticationProvider webBasedPluginAuthenticationProvider;

@Autowired
public AuthenticationController(SecurityService securityService,
SecurityAuthConfigService securityAuthConfigService,
SystemEnvironment systemEnvironment,
Clock clock,
PasswordBasedPluginAuthenticationProvider passwordBasedPluginAuthenticationProvider,
WebBasedPluginAuthenticationProvider webBasedPluginAuthenticationProvider) {
this.securityService = securityService;
this.securityAuthConfigService = securityAuthConfigService;
this.systemEnvironment = systemEnvironment;
this.clock = clock;
this.passwordBasedPluginAuthenticationProvider = passwordBasedPluginAuthenticationProvider;
this.webBasedPluginAuthenticationProvider = webBasedPluginAuthenticationProvider;
}
@@ -168,7 +176,7 @@ public RedirectView authenticateWithWebBasedPlugin(@PathVariable("pluginId") Str
}

private boolean securityIsDisabledOrAlreadyLoggedIn(HttpServletRequest request) {
return !securityService.isSecurityEnabled() || !isAnonymousAuthenticationToken(request);
return !securityService.isSecurityEnabled() || (!isAnonymousAuthenticationToken(request) && SessionUtils.isAuthenticated(request, clock, systemEnvironment));
}

private RedirectView badAuthentication(HttpServletRequest request, String message) {
@@ -38,7 +38,6 @@ public BasicAuthenticationWithRedirectToLoginFilter(SecurityService securityServ
protected void onAuthenticationFailure(HttpServletRequest request,
HttpServletResponse response,
String errorMessage) throws IOException {
SessionUtils.setAuthenticationError(errorMessage, request);
response.sendRedirect("/go/auth/login");
SessionUtils.redirectToLoginPage(request, response, errorMessage);
}
}
@@ -24,6 +24,7 @@
import com.thoughtworks.go.util.Clock;
import com.thoughtworks.go.util.SystemEnvironment;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.web.savedrequest.SavedRequest;
import org.springframework.stereotype.Component;

import javax.servlet.http.HttpServletRequest;
@@ -46,7 +47,6 @@ public ReAuthenticationWithRedirectToLoginFilter(SecurityService securityService
protected void onAuthenticationFailure(HttpServletRequest request,
HttpServletResponse response,
String errorMessage) throws IOException {
SessionUtils.setAuthenticationError(errorMessage, request);
response.sendRedirect("/go/auth/login");
SessionUtils.redirectToLoginPage(request, response, errorMessage);
}
}
@@ -36,7 +36,6 @@ public UserEnabledCheckFilterWithRedirectToLoginPage(UserService userService, Se

@Override
void handleFailure(HttpServletRequest request, HttpServletResponse response, String errorMessage) throws IOException {
SessionUtils.setAuthenticationError(errorMessage, request);
response.sendRedirect("/go/auth/login");
SessionUtils.redirectToLoginPage(request, response, errorMessage);
}
}
@@ -92,8 +92,6 @@ private void removeAnyAssociatedPluginRolesFor(String username) {

protected abstract List<SecurityAuthConfig> getSecurityAuthConfigsToAuthenticateWith(String pluginId);

// protected abstract AuthenticationToken<T> authenticateUser(T credentials, SecurityAuthConfig authConfig);

protected abstract boolean doesPluginSupportAuthentication(String pluginId);

protected abstract AuthenticationResponse authenticateWithExtension(String pluginId,
@@ -30,6 +30,9 @@
import org.springframework.security.web.savedrequest.SavedRequest;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.IOException;

import static com.thoughtworks.go.domain.PersistentObject.NOT_PERSISTED;
import static com.thoughtworks.go.server.security.GoAuthority.ROLE_ANONYMOUS;
@@ -87,6 +90,15 @@ public static void recreateSessionWithoutCopyingOverSessionState(HttpServletRequ
request.getSession();
}

public static void redirectToLoginPage(HttpServletRequest request, HttpServletResponse response, String errorMessage) throws IOException {
SavedRequest savedRequest = SessionUtils.savedRequest(request);
SessionUtils.recreateSessionWithoutCopyingOverSessionState(request);

SessionUtils.saveRequest(request, savedRequest);
SessionUtils.setAuthenticationError(errorMessage, request);
response.sendRedirect("/go/auth/login");
}

public static void setAuthenticationError(String message, HttpServletRequest request) {
request.getSession().setAttribute(AUTHENTICATION_ERROR, message);
}
@@ -32,9 +32,12 @@
import com.thoughtworks.go.server.service.SecurityAuthConfigService;
import com.thoughtworks.go.server.service.SecurityService;
import com.thoughtworks.go.server.web.GoVelocityView;
import com.thoughtworks.go.util.SystemEnvironment;
import com.thoughtworks.go.util.TestingClock;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.springframework.security.web.savedrequest.SavedRequest;
import org.springframework.web.servlet.ModelAndView;
import org.springframework.web.servlet.view.RedirectView;

@@ -55,7 +58,9 @@
private final SecurityService securityService = mock(SecurityService.class);
private final WebBasedPluginAuthenticationProvider webBasedPluginAuthenticationProvider = mock(WebBasedPluginAuthenticationProvider.class);
private final SecurityAuthConfigService securityAuthConfigService = mock(SecurityAuthConfigService.class);
private final AuthenticationController controller = new AuthenticationController(securityService, securityAuthConfigService, authenticationProvider, webBasedPluginAuthenticationProvider);
private final SystemEnvironment systemEnvironment = mock(SystemEnvironment.class);
private final TestingClock clock = new TestingClock();
private final AuthenticationController controller = new AuthenticationController(securityService, securityAuthConfigService, systemEnvironment, clock, authenticationProvider, webBasedPluginAuthenticationProvider);
private final MockHttpServletRequest request = HttpRequestBuilder.GET("/").build();
private final MockHttpServletResponse response = new MockHttpServletResponse();
private HttpSession originalSession = request.getSession(true);
@@ -206,8 +211,12 @@ void setUp() {

@Test
void shouldRedirectToHomepageIfUserIsAlreadyAuthenticated() {
when(systemEnvironment.isReAuthenticationEnabled()).thenReturn(true);
when(systemEnvironment.getReAuthenticationTimeInterval()).thenReturn(15000L);

final GoUserPrinciple goUserPrinciple = new GoUserPrinciple(BOB, DISPLAY_NAME);
final AuthenticationToken<UsernamePassword> usernamePasswordAuthenticationToken = new AuthenticationToken<>(goUserPrinciple, CREDENTIALS, null, 0, null);
final AuthenticationToken<UsernamePassword> usernamePasswordAuthenticationToken = new AuthenticationToken<>(goUserPrinciple, CREDENTIALS, null, clock.currentTimeMillis(), null);
clock.addMillis(10000);

SessionUtils.setAuthenticationTokenAfterRecreatingSession(usernamePasswordAuthenticationToken, request);
originalSession = request.getSession(false);
@@ -218,6 +227,43 @@ void shouldRedirectToHomepageIfUserIsAlreadyAuthenticated() {
assertThat(originalSession).isSameAs(request.getSession(false));
}

@Test
void shouldRedirectToLoginPageIfAuthenticationTokenIsExpired() {
when(systemEnvironment.isReAuthenticationEnabled()).thenReturn(true);
when(systemEnvironment.getReAuthenticationTimeInterval()).thenReturn(5000L);

final GoUserPrinciple goUserPrinciple = new GoUserPrinciple(BOB, DISPLAY_NAME);
final AuthenticationToken<UsernamePassword> usernamePasswordAuthenticationToken = new AuthenticationToken<>(goUserPrinciple, CREDENTIALS, null, clock.currentTimeMillis(), null);
clock.addMillis(10000);

SessionUtils.setAuthenticationTokenAfterRecreatingSession(usernamePasswordAuthenticationToken, request);
originalSession = request.getSession(false);

final ModelAndView modelAndView = (ModelAndView) controller.renderLoginPage(request, response);

assertThat(modelAndView.getViewName()).isEqualTo("auth/login");
}

@Test
void shouldRememberUrlBeforeLogin() {
when(systemEnvironment.isReAuthenticationEnabled()).thenReturn(true);
when(systemEnvironment.getReAuthenticationTimeInterval()).thenReturn(5000L);
SavedRequest savedRequest = mock(SavedRequest.class);

final GoUserPrinciple goUserPrinciple = new GoUserPrinciple(BOB, DISPLAY_NAME);
final AuthenticationToken<UsernamePassword> usernamePasswordAuthenticationToken = new AuthenticationToken<>(goUserPrinciple, CREDENTIALS, null, clock.currentTimeMillis(), null);
clock.addMillis(10000);

SessionUtils.setAuthenticationTokenAfterRecreatingSession(usernamePasswordAuthenticationToken, request);
originalSession = request.getSession(false);

SessionUtils.saveRequest(request, savedRequest);
final ModelAndView modelAndView = (ModelAndView) controller.renderLoginPage(request, response);

assertThat(modelAndView.getViewName()).isEqualTo("auth/login");
assertThat(SessionUtils.savedRequest(request)).isEqualTo(savedRequest);
}

@Test
void shouldRenderLoginPage() {
authenticateAsAnonymous();
@@ -21,10 +21,13 @@
import com.thoughtworks.go.http.mocks.MockHttpServletResponseAssert;
import com.thoughtworks.go.server.newsecurity.utils.SessionUtils;
import org.junit.jupiter.api.Test;
import org.springframework.security.web.savedrequest.SavedRequest;

import javax.servlet.http.HttpSession;
import java.io.IOException;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

class BasicAuthenticationWithRedirectToLoginFilterTest {
@Test
@@ -34,10 +37,17 @@ void shouldInvokeHandler() throws IOException {
final MockHttpServletRequest request = new MockHttpServletRequest();
final MockHttpServletResponse response = new MockHttpServletResponse();
final String message = "foo";
SavedRequest savedRequest = mock(SavedRequest.class);

SessionUtils.saveRequest(request, savedRequest);
HttpSession originalSession = request.getSession(true);

filter.onAuthenticationFailure(request, response, message);

assertThat(SessionUtils.getAuthenticationError(request)).isEqualTo("foo");
assertThat(request.getSession(false)).isNotSameAs(originalSession);
assertThat(SessionUtils.savedRequest(request)).isSameAs(savedRequest);
assertThat(SessionUtils.hasAuthenticationToken(request)).isFalse();

MockHttpServletResponseAssert.assertThat(response)
.redirectsTo("/go/auth/login");
@@ -21,23 +21,32 @@
import com.thoughtworks.go.http.mocks.MockHttpServletResponseAssert;
import com.thoughtworks.go.server.newsecurity.utils.SessionUtils;
import org.junit.jupiter.api.Test;
import org.springframework.security.web.savedrequest.SavedRequest;

import javax.servlet.http.HttpSession;
import java.io.IOException;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

class ReAuthenticationWithRedirectToLoginFilterTest {
@Test
void shouldInvokeHandler() throws IOException {
final ReAuthenticationWithRedirectToLoginFilter filter = new ReAuthenticationWithRedirectToLoginFilter(null, null, null, null, null, null);

final MockHttpServletRequest request = new MockHttpServletRequest();
final MockHttpServletResponse response = new MockHttpServletResponse();
final String message = "foo";
SavedRequest savedRequest = mock(SavedRequest.class);

SessionUtils.saveRequest(request, savedRequest);
HttpSession originalSession = request.getSession(true);

filter.onAuthenticationFailure(request, response, message);

assertThat(SessionUtils.getAuthenticationError(request)).isEqualTo("foo");
assertThat(request.getSession(false)).isNotSameAs(originalSession);
assertThat(SessionUtils.savedRequest(request)).isSameAs(savedRequest);
assertThat(SessionUtils.hasAuthenticationToken(request)).isFalse();

MockHttpServletResponseAssert.assertThat(response)
.redirectsTo("/go/auth/login");
@@ -22,10 +22,13 @@
import com.thoughtworks.go.server.newsecurity.utils.SessionUtils;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.security.web.savedrequest.SavedRequest;

import javax.servlet.http.HttpSession;
import java.io.IOException;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;


class UserEnabledCheckFilterWithRedirectToLoginPageTest {
@@ -43,8 +46,17 @@ void setUp() {

@Test
void shouldRedirectToLoginPageWithAnErrorMessageInTheSession() throws IOException {
SavedRequest savedRequest = mock(SavedRequest.class);
SessionUtils.saveRequest(request, savedRequest);
HttpSession originalSession = request.getSession(true);

filter.handleFailure(request, response, "something bad happened!");

assertThat(SessionUtils.getAuthenticationError(request)).isEqualTo("something bad happened!");
assertThat(request.getSession(false)).isNotSameAs(originalSession);
assertThat(SessionUtils.savedRequest(request)).isSameAs(savedRequest);
assertThat(SessionUtils.hasAuthenticationToken(request)).isFalse();

MockHttpServletResponseAssert.assertThat(response).redirectsTo("/go/auth/login");
assertThat(SessionUtils.getAuthenticationError(request)).isEqualTo("something bad happened!");
}

0 comments on commit 3a1933d

Please sign in to comment.
You can’t perform that action at this time.