----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3427/#review4267 -----------------------------------------------------------
A couple of notes: 1) I think there is room for consolidation of similar functionality into cohesive units (IE UserService) 2) Your services all use checked exceptions, the rest of Rave uses RuntimeExceptions 3) I would review the model for resetting a password via e-mail. There are a couple of attacks that could be made if you were able to get the forgotPasswordHash vi Man in the middle or other means. Though, these are common in a lot of e-mail reset mechanisms https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/ChangePasswordService.java <https://reviews.apache.org/r/3427/#comment9631> IMO, this should live in the UserService, as it is an operation on a user https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/NewPasswordService.java <https://reviews.apache.org/r/3427/#comment9632> Same comment as ChangePasswordService https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultChangePasswordService.java <https://reviews.apache.org/r/3427/#comment9633> This logic is duplicative of that in NewUserService. Consider collapsing services into a consolidated UserService or implement a common utility. https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultNewPasswordService.java <https://reviews.apache.org/r/3427/#comment9635> Only problem with saving this to the database is that there is no time constraint on how long this reset hash is good for, unless you define a cleanup job. https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/test/resources/portal.properties <https://reviews.apache.org/r/3427/#comment9634> small nit: did you mean replyto? https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/ReminderController.java <https://reviews.apache.org/r/3427/#comment9636> Rather than creating an abstract class that has to disambiguate between the operations and perform an action, why not make one controller and split out the common pieces into helper methods? - mfranklin On 2012-01-08 00:50:10, marijan milicevic wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3427/ > ----------------------------------------------------------- > > (Updated 2012-01-08 00:50:10) > > > Review request for rave. > > > Summary > ------- > > - configure mail settings to be able to test, see comments in: > /rave/rave-components/rave-core/src/main/resources/org/apache/rave/core-applicationContext.xml > > > Diffs > ----- > > https://svn.apache.org/repos/asf/incubator/rave/trunk/pom.xml 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/pom.xml > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/NewUser.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/User.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/UserRepository.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/repository/impl/JpaUserRepository.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/ChangePasswordService.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/NewPasswordService.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultChangePasswordService.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/service/impl/DefaultNewPasswordService.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/resources/org/apache/rave/core-applicationContext.xml > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/test/resources/portal.properties > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/ChangePasswordController.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/NewAccountController.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/PasswordReminderController.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/ReminderController.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/UsernameReminderController.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/util/ModelKeys.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/util/ViewNames.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/validator/ChangePasswordValidator.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/validator/NewAccountValidator.java > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/validator/NewPasswordValidator.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/ChangePasswordControllerTest.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/PasswordReminderControllerTest.java > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/resources/messages.properties > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/resources/portal.properties > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/applicationContext-security.xml > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/dispatcher-servlet.xml > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/changepassword.jsp > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/newpassword.jsp > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/retrieveusername.jsp > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/mailtemplates/password_reminder.ftl > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/mailtemplates/username_reminder.ftl > PRE-CREATION > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/login.jsp > 1228743 > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal/pom.xml > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal/src/main/dist/NOTICE > 1228743 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal/src/test/resources/portal.properties > 1228743 > > Diff: https://reviews.apache.org/r/3427/diff > > > Testing > ------- > > > Thanks, > > marijan > >
