diff --git a/src/main/java/com/commafeed/CommaFeedApplication.java b/src/main/java/com/commafeed/CommaFeedApplication.java index 68a8cdd7..db904e89 100644 --- a/src/main/java/com/commafeed/CommaFeedApplication.java +++ b/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 { 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/*"); diff --git a/src/main/java/com/commafeed/backend/model/User.java b/src/main/java/com/commafeed/backend/model/User.java index e1fe1394..ddd3fab5 100644 --- a/src/main/java/com/commafeed/backend/model/User.java +++ b/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)); + } } diff --git a/src/main/java/com/commafeed/backend/service/UserService.java b/src/main/java/com/commafeed/backend/service/UserService.java index 93b79eff..747de4fb 100644 --- a/src/main/java/com/commafeed/backend/service/UserService.java +++ b/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); - return Optional.fromNullable(user); - } - } - return Optional.absent(); - } - - /** - * try to log in with given credentials and create a session for the user - */ - public Optional login(String nameOrEmail, String password, HttpSession sessionToFill) { - Optional 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 login(HttpSession session) { - if (session != null) { - User user = (User) session.getAttribute(SESSION_KEY_USER); - if (user != null) { - afterLogin(user); + performPostLoginActivities(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); - return Optional.fromNullable(user); + performPostLoginActivities(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); } diff --git a/src/main/java/com/commafeed/backend/service/internal/PostLoginActivities.java b/src/main/java/com/commafeed/backend/service/internal/PostLoginActivities.java index 92f0d3e7..0466cb99 100644 --- a/src/main/java/com/commafeed/backend/service/internal/PostLoginActivities.java +++ b/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() - && (user.getLastFullRefresh() == null || user.getLastFullRefresh().before(DateUtils.addMinutes(now, -30)))) { + if (config.getApplicationSettings().isHeavyLoad() && user.shouldRefreshFeedsAt(now)) { + feedSubscriptionService.refreshAll(user); user.setLastFullRefresh(now); saveUser = true; - feedSubscriptionService.refreshAll(user); } if (saveUser) { userDAO.merge(user); diff --git a/src/main/java/com/commafeed/frontend/SessionHelper.java b/src/main/java/com/commafeed/frontend/SessionHelper.java new file mode 100644 index 00000000..021d0e44 --- /dev/null +++ b/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 getLoggedInUser() { + Optional 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 session = getSession(true); + session.get().setAttribute(SESSION_KEY_USER, user); + } + + private Optional getSession(boolean force) { + HttpSession session = request.getSession(force); + return Optional.fromNullable(session); + } + +} diff --git a/src/main/java/com/commafeed/frontend/SessionHelperProvider.java b/src/main/java/com/commafeed/frontend/SessionHelperProvider.java new file mode 100644 index 00000000..b77a6d5d --- /dev/null +++ b/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 { + + private final ThreadLocal request; + + public SessionHelperProvider(@Context ThreadLocal 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() { + @Override + public SessionHelper getValue() { + final HttpServletRequest req = request.get(); + if (req != null) { + return new SessionHelper(req); + } + return null; + } + }; + } + return null; + } +} \ No newline at end of file diff --git a/src/main/java/com/commafeed/frontend/auth/SecurityCheckProvider.java b/src/main/java/com/commafeed/frontend/auth/SecurityCheckProvider.java index 3c9c055e..ab1ba238 100644 --- a/src/main/java/com/commafeed/frontend/auth/SecurityCheckProvider.java +++ b/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 extends AbstractHttpContextInjectable { + static class SecurityCheckInjectable extends AbstractHttpContextInjectable { private static final String PREFIX = "Basic"; private final HttpServletRequest request; @@ -66,8 +67,13 @@ public class SecurityCheckProvider implements InjectableProvider cookieSessionLogin() { - return userService.login(request.getSession(false)); + Optional cookieSessionLogin() { + SessionHelper sessionHelper = new SessionHelper(request); + Optional loggedInUser = sessionHelper.getLoggedInUser(); + if (loggedInUser.isPresent()) { + userService.performPostLoginActivities(loggedInUser.get()); + } + return loggedInUser; } private Optional basicAuthenticationLogin(HttpContext c) { diff --git a/src/main/java/com/commafeed/frontend/resource/UserREST.java b/src/main/java/com/commafeed/frontend/resource/UserREST.java index 2dc0c807..4865e7de 100644 --- a/src/main/java/com/commafeed/frontend/resource/UserREST.java +++ b/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)); - userService.login(req.getName(), req.getPassword(), session); + User registeredUser = userService.register(req.getName(), req.getPassword(), req.getEmail(), Arrays.asList(Role.USER)); + 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.> 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) { - Optional user = userService.login(req.getName(), req.getPassword(), session); + public Response login(@ApiParam(required = true) LoginRequest req, @Context SessionHelper sessionHelper) { + Optional 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(); diff --git a/src/main/java/com/commafeed/frontend/servlet/CustomCssServlet.java b/src/main/java/com/commafeed/frontend/servlet/CustomCssServlet.java index f65e17cc..2f5f4b44 100644 --- a/src/main/java/com/commafeed/frontend/servlet/CustomCssServlet.java +++ b/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 = new UnitOfWork>(sessionFactory) { @Override protected Optional runInSession() throws Exception { - return userService.login(req.getSession(false)); + SessionHelper sessionHelper = new SessionHelper(req); + Optional loggedInUser = sessionHelper.getLoggedInUser(); + if (loggedInUser.isPresent()) { + userService.performPostLoginActivities(loggedInUser.get()); + } + return loggedInUser; } }.run(); if (!user.isPresent()) { diff --git a/src/main/java/com/commafeed/frontend/servlet/NextUnreadServlet.java b/src/main/java/com/commafeed/frontend/servlet/NextUnreadServlet.java index affebcce..0e4fed98 100644 --- a/src/main/java/com/commafeed/frontend/servlet/NextUnreadServlet.java +++ b/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 = new UnitOfWork>(sessionFactory) { @Override protected Optional runInSession() throws Exception { - return userService.login(req.getSession(false)); + SessionHelper sessionHelper = new SessionHelper(req); + Optional loggedInUser = sessionHelper.getLoggedInUser(); + if (loggedInUser.isPresent()) { + userService.performPostLoginActivities(loggedInUser.get()); + } + return loggedInUser; } }.run(); if (!user.isPresent()) { diff --git a/src/test/java/com/commafeed/frontend/auth/SecurityCheckInjectableTest.java b/src/test/java/com/commafeed/frontend/auth/SecurityCheckInjectableTest.java new file mode 100644 index 00000000..417d7183 --- /dev/null +++ b/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 = 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 = 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 = 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 = injectable.cookieSessionLogin(); + + Assert.assertTrue(user.isPresent()); + Assert.assertEquals(userInSession, user.get()); + } + +} diff --git a/src/test/java/com/commafeed/frontend/resource/UserRestTest.java b/src/test/java/com/commafeed/frontend/resource/UserRestTest.java new file mode 100644 index 00000000..3b6a59ba --- /dev/null +++ b/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 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); + } + +}