Merge pull request #647 from rationalrevolt/userservice-tests

Remove dependency on HttpSession in UserService

src/main/java/com/commafeed/CommaFeedApplication.java

@@ -4,7 +4,6 @@ import io.dropwizard.Application;
 import io.dropwizard.assets.AssetsBundle;
 import io.dropwizard.db.DataSourceFactory;
 import io.dropwizard.hibernate.HibernateBundle;
-import io.dropwizard.jersey.sessions.HttpSessionProvider;
 import io.dropwizard.migrations.MigrationsBundle;
 import io.dropwizard.servlets.CacheBustingFilter;
 import io.dropwizard.setup.Bootstrap;
@@ -36,6 +35,7 @@ import com.commafeed.backend.service.UserService;
 import com.commafeed.backend.task.OldStatusesCleanupTask;
 import com.commafeed.backend.task.OrphansCleanupTask;
 import com.commafeed.backend.task.SchedulingService;
+import com.commafeed.frontend.SessionHelperProvider;
 import com.commafeed.frontend.auth.SecurityCheckProvider;
 import com.commafeed.frontend.auth.SecurityCheckProvider.SecurityCheckUserServiceProvider;
 import com.commafeed.frontend.resource.AdminREST;
@@ -108,7 +108,7 @@ public class CommaFeedApplication extends Application<CommaFeedConfiguration> {
 		environment.servlets().setSessionHandler(new SessionHandler(config.getSessionManagerFactory().build()));
 		environment.jersey().register(new SecurityCheckUserServiceProvider(injector.getInstance(UserService.class)));
 		environment.jersey().register(SecurityCheckProvider.class);
-		environment.jersey().register(HttpSessionProvider.class);
+		environment.jersey().register(SessionHelperProvider.class);
 
 		// REST resources
 		environment.jersey().setUrlPattern("/rest/*");

src/main/java/com/commafeed/backend/model/User.java

@@ -15,6 +15,7 @@ import javax.persistence.TemporalType;
 import lombok.Getter;
 import lombok.Setter;
 
+import org.apache.commons.lang.time.DateUtils;
 import org.hibernate.annotations.Cascade;
 
 import com.commafeed.backend.model.UserRole.Role;
@@ -77,5 +78,13 @@ public class User extends AbstractModel {
 		}
 		return false;
 	}
+	
+	public boolean shouldRefreshFeedsAt(Date when) {
+		return (lastFullRefresh == null || lastFullRefreshMoreThan30MinutesBefore(when));
+	}
+
+	private boolean lastFullRefreshMoreThan30MinutesBefore(Date when) {
+		return lastFullRefresh.before(DateUtils.addMinutes(when, -30));
+	}
 
 }

src/main/java/com/commafeed/backend/service/UserService.java

@@ -6,7 +6,6 @@ import java.util.UUID;
 
 import javax.inject.Inject;
 import javax.inject.Singleton;
-import javax.servlet.http.HttpSession;
 
 import lombok.RequiredArgsConstructor;
 
@@ -28,8 +27,6 @@ import com.google.common.base.Preconditions;
 @Singleton
 public class UserService {
 
-	private static final String SESSION_KEY_USER = "user";
-
 	private final FeedCategoryDAO feedCategoryDAO;
 	private final UserDAO userDAO;
 	private final UserSettingsDAO userSettingsDAO;
@@ -54,37 +51,12 @@ public class UserService {
 		if (user != null && !user.isDisabled()) {
 			boolean authenticated = encryptionService.authenticate(password, user.getPassword(), user.getSalt());
 			if (authenticated) {
-				afterLogin(user);
+				performPostLoginActivities(user);
-				return Optional.fromNullable(user);
-			}
-		}
-		return Optional.absent();
-	}
-
-	/**
-	 * try to log in with given credentials and create a session for the user
-	 */
-	public Optional<User> login(String nameOrEmail, String password, HttpSession sessionToFill) {
-		Optional<User> user = login(nameOrEmail, password);
-		if (user.isPresent()) {
-			sessionToFill.setAttribute(SESSION_KEY_USER, user.get());
-		}
-		return user;
-	}
-
-	/**
-	 * try to log in by checking if the user has an active session
-	 */
-	public Optional<User> login(HttpSession session) {
-		if (session != null) {
-			User user = (User) session.getAttribute(SESSION_KEY_USER);
-			if (user != null) {
-				afterLogin(user);
 				return Optional.of(user);
 			}
 		}
 		return Optional.absent();
-	}
+	}	
 
 	/**
 	 * try to log in with given api key
@@ -96,8 +68,8 @@ public class UserService {
 
 		User user = userDAO.findByApiKey(apiKey);
 		if (user != null && !user.isDisabled()) {
-			afterLogin(user);
+			performPostLoginActivities(user);
-			return Optional.fromNullable(user);
+			return Optional.of(user);
 		}
 		return Optional.absent();
 	}
@@ -105,7 +77,7 @@ public class UserService {
 	/**
 	 * should triggers after successful login
 	 */
-	private void afterLogin(User user) {
+	public void performPostLoginActivities(User user) {
 		postLoginActivities.executeFor(user);
 	}
 

src/main/java/com/commafeed/backend/service/internal/PostLoginActivities.java

@@ -28,16 +28,15 @@ public class PostLoginActivities {
 
 		boolean saveUser = false;
 		// only update lastLogin field every hour in order to not
-		// invalidate the cache everytime someone logs in
+		// invalidate the cache every time someone logs in
 		if (lastLogin == null || lastLogin.before(DateUtils.addHours(now, -1))) {
 			user.setLastLogin(now);
 			saveUser = true;
 		}
-		if (config.getApplicationSettings().isHeavyLoad()
+		if (config.getApplicationSettings().isHeavyLoad() && user.shouldRefreshFeedsAt(now)) {
-				&& (user.getLastFullRefresh() == null || user.getLastFullRefresh().before(DateUtils.addMinutes(now, -30)))) {
+			feedSubscriptionService.refreshAll(user);
 			user.setLastFullRefresh(now);
 			saveUser = true;
-			feedSubscriptionService.refreshAll(user);
 		}
 		if (saveUser) {
 			userDAO.merge(user);

src/main/java/com/commafeed/frontend/SessionHelper.java

@@ -0,0 +1,39 @@
+package com.commafeed.frontend;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpSession;
+
+import lombok.RequiredArgsConstructor;
+
+import com.commafeed.backend.model.User;
+import com.google.common.base.Optional;
+
+@RequiredArgsConstructor()
+public class SessionHelper {
+	
+	private static final String SESSION_KEY_USER = "user";
+	
+	private final HttpServletRequest request;
+	
+	public Optional<User> getLoggedInUser() {
+		Optional<HttpSession> session = getSession(false);
+		
+		if (session.isPresent()) {
+			User user = (User) session.get().getAttribute(SESSION_KEY_USER);
+			return Optional.fromNullable(user);
+		}
+		
+		return Optional.absent();
+	}
+	
+	public void setLoggedInUser(User user) {
+		Optional<HttpSession> session = getSession(true);
+		session.get().setAttribute(SESSION_KEY_USER, user);
+	}
+	
+	private Optional<HttpSession> getSession(boolean force) {
+		HttpSession session = request.getSession(force);
+		return Optional.fromNullable(session);
+	}
+
+}

src/main/java/com/commafeed/frontend/SessionHelperProvider.java

@@ -0,0 +1,44 @@
+package com.commafeed.frontend;
+
+import java.lang.reflect.Type;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.ext.Provider;
+
+import com.sun.jersey.core.spi.component.ComponentContext;
+import com.sun.jersey.core.spi.component.ComponentScope;
+import com.sun.jersey.spi.inject.Injectable;
+import com.sun.jersey.spi.inject.InjectableProvider;
+
+@Provider
+public class SessionHelperProvider implements InjectableProvider<Context, Type> {
+	
+    private final ThreadLocal<HttpServletRequest> request;
+
+    public SessionHelperProvider(@Context ThreadLocal<HttpServletRequest> request) {
+        this.request = request;
+    }
+
+    @Override
+    public ComponentScope getScope() {
+        return ComponentScope.PerRequest;
+    }
+
+    @Override
+    public Injectable<?> getInjectable(ComponentContext ic, final Context session, Type type) {
+        if (type.equals(SessionHelper.class)) {
+            return new Injectable<SessionHelper>() {
+                @Override
+                public SessionHelper getValue() {
+                    final HttpServletRequest req = request.get();
+                    if (req != null) {
+                        return new SessionHelper(req);
+                    }
+                    return null;
+                }
+            };
+        }
+        return null;
+    }
+}

src/main/java/com/commafeed/frontend/auth/SecurityCheckProvider.java

@@ -15,6 +15,7 @@ import org.eclipse.jetty.util.StringUtil;
 import com.commafeed.backend.model.User;
 import com.commafeed.backend.model.UserRole.Role;
 import com.commafeed.backend.service.UserService;
+import com.commafeed.frontend.SessionHelper;
 import com.google.common.base.Optional;
 import com.sun.jersey.api.core.HttpContext;
 import com.sun.jersey.api.model.Parameter;
@@ -35,7 +36,7 @@ public class SecurityCheckProvider implements InjectableProvider<SecurityCheck,
 	}
 
 	@RequiredArgsConstructor
-	private static class SecurityCheckInjectable<T> extends AbstractHttpContextInjectable<User> {
+	static class SecurityCheckInjectable<T> extends AbstractHttpContextInjectable<User> {
 		private static final String PREFIX = "Basic";
 
 		private final HttpServletRequest request;
@@ -66,8 +67,13 @@ public class SecurityCheckProvider implements InjectableProvider<SecurityCheck,
 			}
 		}
 
-		private Optional<User> cookieSessionLogin() {
+		Optional<User> cookieSessionLogin() {
-			return userService.login(request.getSession(false));
+			SessionHelper sessionHelper = new SessionHelper(request);
+			Optional<User> loggedInUser = sessionHelper.getLoggedInUser();
+			if (loggedInUser.isPresent()) {
+				userService.performPostLoginActivities(loggedInUser.get());
+			}
+			return loggedInUser;
 		}
 
 		private Optional<User> basicAuthenticationLogin(HttpContext c) {

src/main/java/com/commafeed/frontend/resource/UserREST.java

@@ -1,7 +1,6 @@
 package com.commafeed.frontend.resource;
 
 import io.dropwizard.hibernate.UnitOfWork;
-import io.dropwizard.jersey.sessions.Session;
 import io.dropwizard.jersey.validation.ValidationErrorMessage;
 
 import java.util.Arrays;
@@ -11,7 +10,6 @@ import java.util.UUID;
 
 import javax.inject.Inject;
 import javax.inject.Singleton;
-import javax.servlet.http.HttpSession;
 import javax.validation.ConstraintViolation;
 import javax.validation.Valid;
 import javax.ws.rs.Consumes;
@@ -20,6 +18,7 @@ import javax.ws.rs.POST;
 import javax.ws.rs.Path;
 import javax.ws.rs.Produces;
 import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.Response.Status;
@@ -49,6 +48,7 @@ import com.commafeed.backend.model.UserSettings.ViewMode;
 import com.commafeed.backend.service.MailService;
 import com.commafeed.backend.service.PasswordEncryptionService;
 import com.commafeed.backend.service.UserService;
+import com.commafeed.frontend.SessionHelper;
 import com.commafeed.frontend.auth.SecurityCheck;
 import com.commafeed.frontend.model.Settings;
 import com.commafeed.frontend.model.UserModel;
@@ -223,10 +223,11 @@ public class UserREST {
 	@POST
 	@UnitOfWork
 	@ApiOperation(value = "Register a new account")
-	public Response register(@Valid @ApiParam(required = true) RegistrationRequest req, @Session HttpSession session) {
+	public Response register(@Valid @ApiParam(required = true) RegistrationRequest req, @Context SessionHelper sessionHelper) {
 		try {
-			userService.register(req.getName(), req.getPassword(), req.getEmail(), Arrays.asList(Role.USER));
+			User registeredUser = userService.register(req.getName(), req.getPassword(), req.getEmail(), Arrays.asList(Role.USER));
-			userService.login(req.getName(), req.getPassword(), session);
+			userService.login(req.getName(), req.getPassword());
+			sessionHelper.setLoggedInUser(registeredUser);
 			return Response.ok().build();
 		} catch (final IllegalArgumentException e) {
 			return Response.status(422).entity(new ValidationErrorMessage(Collections.<ConstraintViolation<?>> emptySet()) {
@@ -242,9 +243,10 @@ public class UserREST {
 	@POST
 	@UnitOfWork
 	@ApiOperation(value = "Login and create a session")
-	public Response login(@ApiParam(required = true) LoginRequest req, @Session HttpSession session) {
+	public Response login(@ApiParam(required = true) LoginRequest req, @Context SessionHelper sessionHelper) {
-		Optional<User> user = userService.login(req.getName(), req.getPassword(), session);
+		Optional<User> user = userService.login(req.getName(), req.getPassword());
 		if (user.isPresent()) {
+			sessionHelper.setLoggedInUser(user.get());
 			return Response.ok().build();
 		} else {
 			return Response.status(Response.Status.UNAUTHORIZED).entity("wrong username or password").build();

src/main/java/com/commafeed/frontend/servlet/CustomCssServlet.java

@@ -18,6 +18,7 @@ import com.commafeed.backend.dao.UserSettingsDAO;
 import com.commafeed.backend.model.User;
 import com.commafeed.backend.model.UserSettings;
 import com.commafeed.backend.service.UserService;
+import com.commafeed.frontend.SessionHelper;
 import com.google.common.base.Optional;
 
 @SuppressWarnings("serial")
@@ -36,7 +37,12 @@ public class CustomCssServlet extends HttpServlet {
 		final Optional<User> user = new UnitOfWork<Optional<User>>(sessionFactory) {
 			@Override
 			protected Optional<User> runInSession() throws Exception {
-				return userService.login(req.getSession(false));
+				SessionHelper sessionHelper = new SessionHelper(req);
+				Optional<User> loggedInUser = sessionHelper.getLoggedInUser();
+				if (loggedInUser.isPresent()) {
+					userService.performPostLoginActivities(loggedInUser.get());
+				}
+				return loggedInUser;
 			}
 		}.run();
 		if (!user.isPresent()) {

src/main/java/com/commafeed/frontend/servlet/NextUnreadServlet.java

@@ -26,6 +26,7 @@ import com.commafeed.backend.model.FeedSubscription;
 import com.commafeed.backend.model.User;
 import com.commafeed.backend.model.UserSettings.ReadingOrder;
 import com.commafeed.backend.service.UserService;
+import com.commafeed.frontend.SessionHelper;
 import com.commafeed.frontend.resource.CategoryREST;
 import com.google.common.base.Optional;
 import com.google.common.collect.Iterables;
@@ -53,7 +54,12 @@ public class NextUnreadServlet extends HttpServlet {
 		final Optional<User> user = new UnitOfWork<Optional<User>>(sessionFactory) {
 			@Override
 			protected Optional<User> runInSession() throws Exception {
-				return userService.login(req.getSession(false));
+				SessionHelper sessionHelper = new SessionHelper(req);
+				Optional<User> loggedInUser = sessionHelper.getLoggedInUser();
+				if (loggedInUser.isPresent()) {
+					userService.performPostLoginActivities(loggedInUser.get());
+				}
+				return loggedInUser;
 			}
 		}.run();
 		if (!user.isPresent()) {

src/test/java/com/commafeed/frontend/auth/SecurityCheckInjectableTest.java

@@ -0,0 +1,104 @@
+package com.commafeed.frontend.auth;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpSession;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.commafeed.backend.model.User;
+import com.commafeed.backend.service.UserService;
+import com.commafeed.backend.service.internal.PostLoginActivities;
+import com.commafeed.frontend.auth.SecurityCheckProvider.SecurityCheckInjectable;
+import com.google.common.base.Optional;
+
+public class SecurityCheckInjectableTest {
+	
+	private static String SESSION_KEY_USER = "user";
+	
+	@Test public void
+	cookie_login_does_not_create_a_session_if_not_present() {
+		HttpServletRequest request = mock(HttpServletRequest.class);
+		UserService service = mock(UserService.class);
+		
+		SecurityCheckInjectable injectable = new SecurityCheckInjectable(request, service, null, false);
+		injectable.cookieSessionLogin();
+		
+		verify(request).getSession(false);
+	}
+	
+	@Test public void
+	cookie_login_should_not_return_user_if_there_is_no_preexisting_http_session() {
+		HttpServletRequest request = mock(HttpServletRequest.class);
+		when(request.getSession(false)).thenReturn(null);
+		
+		UserService service = new UserService(null, null, null, null, null, null);
+		
+		SecurityCheckInjectable injectable = new SecurityCheckInjectable(request, service, null, false);
+		Optional<User> user = injectable.cookieSessionLogin();
+		
+		Assert.assertFalse(user.isPresent());
+	}
+	
+	@Test public void
+	cookie_login_should_not_return_user_if_user_not_present_in_http_session() {
+		HttpSession session = mock(HttpSession.class);
+		when(session.getAttribute(SESSION_KEY_USER)).thenReturn(null);
+		
+		HttpServletRequest request = mock(HttpServletRequest.class);
+		when(request.getSession(false)).thenReturn(session);
+		
+		UserService service = new UserService(null, null, null, null, null, null);
+		
+		SecurityCheckInjectable injectable = new SecurityCheckInjectable(request, service, null, false);
+		Optional<User> user = injectable.cookieSessionLogin();
+		
+		Assert.assertFalse(user.isPresent());
+	}
+	
+	@Test public void
+	cookie_login_should_perform_post_login_activities_if_user_present_in_http_session() {
+		User userInSession = new User();
+		
+		HttpSession session = mock(HttpSession.class);
+		when(session.getAttribute(SESSION_KEY_USER)).thenReturn(userInSession);
+		
+		HttpServletRequest request = mock(HttpServletRequest.class);
+		when(request.getSession(false)).thenReturn(session);
+		
+		PostLoginActivities postLoginActivities = mock(PostLoginActivities.class);
+		
+		UserService service = new UserService(null, null, null, null, null, postLoginActivities);
+		
+		SecurityCheckInjectable injectable = new SecurityCheckInjectable(request, service, null, false);
+		Optional<User> user = injectable.cookieSessionLogin();
+		
+		verify(postLoginActivities).executeFor(userInSession);
+	}
+	
+	@Test public void
+	cookie_login_should_return_user_if_user_present_in_http_session() {
+		User userInSession = new User();
+		
+		HttpSession session = mock(HttpSession.class);
+		when(session.getAttribute(SESSION_KEY_USER)).thenReturn(userInSession);
+		
+		HttpServletRequest request = mock(HttpServletRequest.class);
+		when(request.getSession(false)).thenReturn(session);
+		
+		PostLoginActivities postLoginActivities = mock(PostLoginActivities.class);
+		
+		UserService service = new UserService(null, null, null, null, null, postLoginActivities);
+		
+		SecurityCheckInjectable injectable = new SecurityCheckInjectable(request, service, null, false);
+		Optional<User> user = injectable.cookieSessionLogin();
+		
+		Assert.assertTrue(user.isPresent());
+		Assert.assertEquals(userInSession, user.get());
+	}
+
+}

src/test/java/com/commafeed/frontend/resource/UserRestTest.java

@@ -0,0 +1,112 @@
+package com.commafeed.frontend.resource;
+
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+
+import org.junit.Test;
+import org.mockito.InOrder;
+import org.mockito.Matchers;
+
+import com.commafeed.backend.model.User;
+import com.commafeed.backend.model.UserRole.Role;
+import com.commafeed.backend.service.UserService;
+import com.commafeed.frontend.SessionHelper;
+import com.commafeed.frontend.model.request.LoginRequest;
+import com.commafeed.frontend.model.request.RegistrationRequest;
+import com.google.common.base.Optional;
+
+public class UserRestTest {
+	
+	@Test public void
+	login_should_not_populate_http_session_if_unsuccessfull() {
+		// Absent user
+		Optional<User> absentUser = Optional.absent();
+		
+		// Create UserService partial mock
+		UserService service = mock(UserService.class);
+		when(service.login("user", "password")).thenReturn(absentUser);
+		
+		UserREST userREST = new UserREST(null, null, null, service, null, null, null);
+		SessionHelper sessionHelper = mock(SessionHelper.class);
+		
+		LoginRequest req = new LoginRequest();
+		req.setName("user");
+		req.setPassword("password");
+		
+		userREST.login(req, sessionHelper);
+		
+		verify(sessionHelper, never()).setLoggedInUser(any(User.class));
+	}
+	
+	@Test public void
+	login_should_populate_http_session_if_successfull() {
+		// Create a user
+		User user = new User();
+		
+		// Create UserService mock
+		UserService service = mock(UserService.class);
+		when(service.login("user", "password")).thenReturn(Optional.of(user));
+		
+		LoginRequest req = new LoginRequest();
+		req.setName("user");
+		req.setPassword("password");
+		
+		UserREST userREST = new UserREST(null, null, null, service, null, null, null);
+		SessionHelper sessionHelper = mock(SessionHelper.class);
+		
+		userREST.login(req, sessionHelper);
+		
+		verify(sessionHelper).setLoggedInUser(user);
+	}
+	
+	@Test public void
+	register_should_register_and_then_login() {
+		// Create UserService mock
+		UserService service = mock(UserService.class);
+		
+		RegistrationRequest req = new RegistrationRequest();
+		req.setName("user");
+		req.setPassword("password");
+		req.setEmail("test@test.com");
+		
+		InOrder inOrder = inOrder(service);
+		
+		SessionHelper sessionHelper = mock(SessionHelper.class);
+		UserREST userREST = new UserREST(null, null, null, service, null, null, null);
+		
+		userREST.register(req, sessionHelper);
+		
+		inOrder.verify(service).register("user", "password", "test@test.com", Arrays.asList(Role.USER));
+		inOrder.verify(service).login("user", "password");
+	}
+	
+	@Test public void
+	register_should_populate_http_session() {
+		// Create a user
+		User user = new User();
+		
+		// Create UserService mock
+		UserService service = mock(UserService.class);
+		when(service.register(any(String.class), any(String.class), any(String.class), Matchers.anyListOf(Role.class))).thenReturn(user);
+		when(service.login(any(String.class), any(String.class))).thenReturn(Optional.of(user));
+		
+		RegistrationRequest req = new RegistrationRequest();
+		req.setName("user");
+		req.setPassword("password");
+		req.setEmail("test@test.com");
+		
+		SessionHelper sessionHelper = mock(SessionHelper.class);
+		UserREST userREST = new UserREST(null, null, null, service, null, null, null);
+		
+		userREST.register(req, sessionHelper);
+		
+		verify(sessionHelper).setLoggedInUser(user);
+	}
+
+}