Fixing email update

This is a little post on how i solved APIWordView issue #14

The problem Link to heading

The password salt is generated by mixing email + password, exclusively updating the email breaks the login system, when the user that altered his email attempts to log-in again, the login hash will not match the stored one.

@PutMapping("/me")
public ResponseEntity<?> update(@RequestBody UserUpdateRequest request, HttpServletRequest req) {
	return response(() -> {
		User user = service.getMe(req);
		User userAlter = request.toEntity();

		// merge overlaps userAlter on top of user, creating merged
		User merged = ClassMerger.merge(user, userAlter);

		// insert saves the changes performed
		service.insert(merged);

		return ok();
	});
}

Solving Link to heading

I started by creating a new request type and it’s test. While creating it i noticed that all requests tests are very similar and they could use some refactoring, so i opened a new issue to work on that later on.

@Getter
@Setter
public class UserEmailUpdateRequest {
    private String oldEmail;
    private String newEmail;
    private String password;
// ...
}

To keep things simple i decided to isolate the email update from the other information updates, keeping everything in the same endpoint would just add unnecessary complexity.

The logic is simple: I create two hashes (one with the old and other with the new email), check if the old matches the stored one, and if so update the email and password with the new information.

@PutMapping("/me/email")
public ResponseEntity<?> updateEmail(@RequestBody UserEmailUpdateRequest request, HttpServletRequest req) {					
	return response(() -> {
		request.validate();

		User user = service.getMe(req);

		String oldHash = new HashedPassword(request.getOldEmail(), request.getPassword()).getValue();
		String newHash = new HashedPassword(request.getNewEmail(), request.getPassword()).getValue();

		if (user.getPassword().equals(oldHash)) {
			user.setEmail(request.getNewEmail());
			user.setPassword(newHash);
		} else {
			throw new IncorrectCredentialsException("Email or password did not match");
		}

		service.insert(user);

		return ok();
	});
}

The test is also simple: I log in to the test account, to ensure that the login is working correctly, change the account email, and try to log in again using the new email.

class UserControllerEmailUpdateTest extends ControllerTest {
    @Test
    @Order(1)
    void login() throws Exception {
        MockUser user = new MockUser("mock.user@gmail.com", "S_enha64");

        req.post("/user/login", user.toJson()).andExpect(status().isOk());
    }

    @Test
    @Order(2)
    void updateEmail() throws Exception {
        String jwt = MockValues.getUserJwt(mockMvc);
        MockEmailUpdateRequest request = new MockEmailUpdateRequest("mock.user@gmail.com", "new.email@gmail.com", "S_enha64");

        req.put("/user/me/email", request.toJson(), jwt).andExpect(status().isOk());
    }

    @Test
    @Order(3)
    void loginNewEmail() throws Exception {
        MockUser user = new MockUser("new.email@gmail.com", "S_enha64");

        req.post("/user/login", user.toJson()).andExpect(status().isOk());
    }
}

While creating the test i noticed that the code was missing something: check if the newEmail is already being used by another account.

With this additional logic, it was going to be too ’expensive’ to keep the logic in the Controller, so i moved it to UserService

@PutMapping("/me/email")
public ResponseEntity<?> updateEmail(@RequestBody UserEmailUpdateRequest request, HttpServletRequest req) {
	return response(() -> {
		request.validate();

		service.insertWithNewEmail(req, request.getNewEmail(), request.getOldEmail(), request.getPassword());

		return ok();
	});
}

@Override
public User insertWithNewEmail(HttpServletRequest request, String newEmail, String oldEmail, String password) throws ValueTakenException, InvalidKeySpecException, NoSuchEntryException, IncorrectCredentialsException {
    Optional<User> existingUserWithNewEmail = repository.findByEmail(newEmail);

    if (existingUserWithNewEmail.isPresent()) {
        throw new ValueTakenException("newEmail is already taken by another account.");
    }

    User user = getMe(request);

    String oldHash = new HashedPassword(oldEmail, password).getValue();
    String newHash = new HashedPassword(newEmail, password).getValue();

    if (user.getPassword().equals(oldHash)) {
        user.setEmail(newEmail);
        user.setPassword(newHash);
    } else {
        throw new IncorrectCredentialsException("Email or password did not match");
    }

    return insert(user);
}

With the new variable, the test order is as follows: Log in, try to change the email with an email already in use, change to a new email, log in with the new email.

@Test
@Order(2)
void updateExistingEmail() throws Exception {
    String jwt = MockValues.getUserJwt(mockMvc);
    MockEmailUpdateRequest request = new MockEmailUpdateRequest("mock.user@gmail.com", "mock.admin@gmail.com", "S_enha64");

    req.put("/user/me/email", request.toJson(), jwt).andExpect(status().isForbidden());
}

I run all the tests to make sure everything is working, and it’s done!