Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JC-2379 Clear password and passwordConfirm fields when they're different during new user registration #261

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

JC-2379 Clear password and passwordConfirm fields when they're different during new user registration #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions ...e/src/main/java/org/jtalks/jcommune/service/transactional/TransactionalAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public AuthenticationStatus authenticate(LoginUserDto loginUserDto, HttpServletR
user = getByUsername(loginUserDto.getUserName());
result = authenticateDefault(user, loginUserDto.getPassword(), loginUserDto.isRememberMe(), request, response);
} catch (NotFoundException e) {
LOGGER.info("User was not found during login process, username = {}, IP={}",
LOGGER.info("User was not found during login process, username = {}, IP={}",
loginUserDto.getUserName(), loginUserDto.getClientIp());
result = authenticateByPluginAndUpdateUserInfo(loginUserDto, true, request, response);
} catch(DisabledException e) {
Expand Down Expand Up @@ -177,7 +177,7 @@ private AuthenticationStatus authenticateByPluginAndUpdateUserInfo(LoginUserDto
String passwordHash = encryptionService.encryptPassword(loginUserDto.getPassword());
String encodedUsername;
try {
encodedUsername = loginUserDto.getUserName() == null ? null :
encodedUsername = loginUserDto.getUserName() == null ? null :
URLEncoder.encode(loginUserDto.getUserName(), "UTF-8").replace("+", "%20");
} catch (UnsupportedEncodingException e) {
LOGGER.error("Could not encode username '{}'", loginUserDto.getUserName());
Expand Down Expand Up @@ -341,7 +341,12 @@ public BindingResult register(RegisterUserDto registerUserDto)
}

if(result.hasErrors()) {
userDto.setPassword(notEncodedPassword);
if (wrongPasswordConfirmError(result)) {
removePasswords(registerUserDto);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removePasswordsFromForm?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think that it's correct to mix layers. TransactionalAuthenticator wouldn't know about form. Proposed method name just for programmer and can tangle.

result = removeWrongPasswordConfirmError(result);
}else {
userDto.setPassword(notEncodedPassword);
}
}

return result;
Expand Down Expand Up @@ -446,5 +451,36 @@ public JCUser storeRegisteredUser(UserDto userDto) {
LOGGER.info("JCUser registered: {}", user.getUsername());
return user;
}


private void removePasswords(final RegisterUserDto registerUserDto) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add JavaDocs explaining what this does and why it's needed.

Also, we don't have a habit to put final for local vars and parameters :)

registerUserDto.setPasswordConfirm(null);
registerUserDto.getUserDto().setPassword(null);
}

private BindingResult removeWrongPasswordConfirmError(BindingResult srcErrors) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaDocs for this too

BindingResult result = new BeanPropertyBindingResult(srcErrors.getTarget(), srcErrors.getObjectName());
for (FieldError error : srcErrors.getFieldErrors()) {
if (wrongPasswordConfirmError(error)) {
result.addError(new FieldError(error.getObjectName(), error.getField(), error.getDefaultMessage()));
} else {
result.addError(error);
}
}

return result;
}

private boolean wrongPasswordConfirmError(final FieldError error) {
return "passwordConfirm".equals(error.getField());
}

private boolean wrongPasswordConfirmError(final BindingResult errors) {
for (FieldError error : errors.getFieldErrors()) {
if (wrongPasswordConfirmError(error)) {
return true;
}
}

return false;
}
}
74 changes: 68 additions & 6 deletions ...c/test/java/org/jtalks/jcommune/service/transactional/TransactionalAuthenticatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,24 @@
import org.jtalks.common.model.entity.User;
import org.jtalks.common.service.security.SecurityContextHolderFacade;
import org.jtalks.jcommune.model.dao.UserDao;
import org.jtalks.jcommune.model.dto.LoginUserDto;
import org.jtalks.jcommune.model.dto.RegisterUserDto;
import org.jtalks.jcommune.model.dto.UserDto;
import org.jtalks.jcommune.model.entity.JCUser;
import org.jtalks.jcommune.plugin.api.PluginLoader;
import org.jtalks.jcommune.plugin.api.core.AuthenticationPlugin;
import org.jtalks.jcommune.plugin.api.core.Plugin;
import org.jtalks.jcommune.plugin.api.core.RegistrationPlugin;
import org.jtalks.jcommune.plugin.api.exceptions.NoConnectionException;
import org.jtalks.jcommune.plugin.api.exceptions.UnexpectedErrorException;
import org.jtalks.jcommune.plugin.api.filters.TypeFilter;
import org.jtalks.jcommune.service.Authenticator;
import org.jtalks.jcommune.service.PluginService;
import org.jtalks.jcommune.service.util.AuthenticationStatus;
import org.jtalks.jcommune.service.nontransactional.EncryptionService;
import org.jtalks.jcommune.service.nontransactional.ImageService;
import org.jtalks.jcommune.service.nontransactional.MailService;
import org.jtalks.jcommune.plugin.api.PluginLoader;
import org.jtalks.jcommune.plugin.api.filters.TypeFilter;
import org.jtalks.jcommune.service.security.AdministrationGroup;
import org.jtalks.jcommune.service.util.AuthenticationStatus;
import org.mockito.Mock;
import org.springframework.security.authentication.AuthenticationManager;
import org.springframework.security.authentication.BadCredentialsException;
Expand All @@ -60,11 +61,18 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import org.jtalks.jcommune.model.dto.LoginUserDto;

import static org.mockito.Mockito.*;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.refEq;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.MockitoAnnotations.initMocks;
import static org.testng.Assert.*;
import static org.testng.Assert.assertEquals;
import static org.unitils.reflectionassert.ReflectionAssert.assertReflectionEquals;

/**
Expand Down Expand Up @@ -486,6 +494,60 @@ public void userMustHaveInitialFieldValuesWhenPluginRegistrationFailWithRegistra
assertReflectionEquals(expectedRegisterUserDto, registerUserDto);
}

@Test
public void userMustSetToNullPasswordAndPasswordConfirmFieldsWhenTheyAreDifferent() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emptiesPassowordAndConfirmationFields_ifTheyDoNotMatch

Maybe let's add some javadocs here explaining why it behaves like this.


Validator customValidator = new Validator() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't you use the usual validator that's used in prod code?

@Override
public boolean supports(Class<?> clazz) {
return true;
}

@Override
public void validate(Object target, Errors errors) {
errors.rejectValue("passwordConfirm", "", "Password and confirmation password do not match");
}
};
RegisterUserDto registerUserDto = createRegisterUserDto("username", "password", "[email protected]", null);
registerUserDto.setPasswordConfirm("wrongPassword");
EncryptionService realEncryptionService = new EncryptionService(new Md5PasswordEncoder());
ReflectionTestUtils.setField(authenticator, "encryptionService", realEncryptionService);
ReflectionTestUtils.setField(authenticator, "validator", customValidator);
when(pluginService.getRegistrationPlugins()).thenReturn(Collections.EMPTY_MAP);

authenticator.register(registerUserDto);
RegisterUserDto expectedRegisterUserDto = createRegisterUserDto("username", null, "[email protected]", null);

assertReflectionEquals(expectedRegisterUserDto, registerUserDto);
}

@Test
public void userMustRestorePasswordAndPasswordConfirmFieldsWhenTheyAreEquals() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesNotEmptyPasswordAndConfirmationFields_ifTheyMatch

Validator customValidator = new Validator() {
@Override
public boolean supports(Class<?> clazz) {
return true;
}

@Override
public void validate(Object target, Errors errors) {
errors.rejectValue("userDto.email", "", "An email format should be like [email protected]");
}
};
RegisterUserDto registerUserDto = createRegisterUserDto("username", "password", "email", null);
registerUserDto.setPasswordConfirm("password");
EncryptionService realEncryptionService = new EncryptionService(new Md5PasswordEncoder());
ReflectionTestUtils.setField(authenticator, "encryptionService", realEncryptionService);
ReflectionTestUtils.setField(authenticator, "validator", customValidator);
when(pluginService.getRegistrationPlugins()).thenReturn(Collections.EMPTY_MAP);

authenticator.register(registerUserDto);
RegisterUserDto expectedRegisterUserDto = createRegisterUserDto("username", "password", "email", null);
expectedRegisterUserDto.setPasswordConfirm("password");

assertReflectionEquals(expectedRegisterUserDto, registerUserDto);
}

private RegisterUserDto createRegisterUserDto(String username, String password, String email, String honeypotCaptcha) {
RegisterUserDto registerUserDto = new RegisterUserDto();
UserDto userDto = new UserDto();
Expand Down