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.

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

implimented requestpasswordreset endpoint to receive either email o… #334

Merged
merged 7 commits into from Aug 10, 2018

Conversation

Projects
None yet
2 participants
@fanyui
Copy link
Contributor

fanyui commented Aug 1, 2018

implimented requestpasswordreset endpoint to receive either email or username and trigger the creation of an activation key for the user which will be sent via email to the user provided he / she owns a valid email.

implimented request password reset endpoint to receive either email o…
…r username and trigger the creation of an activation key for the user
@fanyui

This comment has been minimized.

Copy link
Contributor Author

fanyui commented Aug 1, 2018

@wluyima here is a pull request for the request for password reset endpoint.

import org.springframework.web.bind.annotation.ResponseStatus;

@Controller
@RequestMapping(value = "/rest/" + RestConstants.VERSION_1)

This comment has been minimized.

@wluyima

wluyima Aug 2, 2018

Member

You are missing the resource name passwordreset

String userName = body.get("username");
user = userService.getUserByUsername(userName);
if ((user = userService.getUserByEmail(usernameOrEmail)) == null) {
user = userService.getUserByUsername(usernameOrEmail);

This comment has been minimized.

@wluyima

wluyima Aug 3, 2018

Member

I think in openmrs-core, you will need to rename this method to getUserByUsernameOrEmail just for clarity of what it does

catch (InvalidActivationKeyException ex) {
ex.printStackTrace();
}
String activationKey = body.get("token");

This comment has been minimized.

@wluyima

wluyima Aug 3, 2018

Member

should be activationKey

@wluyima

This comment has been minimized.

Copy link
Member

wluyima commented Aug 3, 2018

Where are the unit tests? Please get into the habit of always including unit tests for all your changes

if ((user = userService.getUserByEmail(usernameOrEmail)) == null) {
user = userService.getUserByUsername(usernameOrEmail);
}
//user = userService.getUserByEmail(usernameOrEmail);

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Please don't leave behind commented out code, just remove it


@RequestMapping(value = "/{activationkey}", method = RequestMethod.POST)
@ResponseBody
public ResponseEntity<String> resetPassword(@PathVariable("activationkey") String activationkey,

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

We don't return ResponseEntity, we ONLY return SimpleObject

@RequestBody Map<String, String> body) {
String newPassword = body.get("newPassword");
if (userService.getUserByActivationKey(activationkey) == null) {
return new ResponseEntity<String>("Invalid Activation Key", HttpStatus.BAD_REQUEST);

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

You should instead throw an exception

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Possibly a NPE

public ResponseEntity<String> resetPassword(@PathVariable("activationkey") String activationkey,
@RequestBody Map<String, String> body) {
String newPassword = body.get("newPassword");
if (userService.getUserByActivationKey(activationkey) == null) {

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

This check is already done userService.changePasswordUsingActivationKey(activationkey) so don't do it here again, it's the reason why you added the new exception class, remember?


@RequestMapping(value = "/{activationkey}", method = RequestMethod.GET)
@ResponseBody
public ResponseEntity<String> verifyActivationKey(@PathVariable("activationkey") String activationkey) {

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Why did you add this method?

This comment has been minimized.

@fanyui

fanyui Aug 7, 2018

Author Contributor

@wluyima The sequence diagram Burke presented shows wee need a method to verify activationKey So this method is for that

}

@Test
public void requestPasswordReset_shouldFailIfUsernameOrEmailIsBlanck() throws Exception {

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Why do you need this test in the weblayer? The logic is in the service layer, you don't need to duplicate the test here

}

@Test
public void requestPasswordReset_shouldFailIfUsernameOrEmailIsEmptyl() throws Exception {

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

This test isn't necessary in the web layer too

handle(newPostRequest(RESET_PASSWORD_URI, "{\"usernameOrEmail\":\"" + usernameOrEmail + "\"}"));

}

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Where is the test for a valid password reset request?

This comment has been minimized.

@fanyui

fanyui Aug 7, 2018

Author Contributor

@wluyima activation key expires so am having trouble writing a test for successfully verifying and reseting the password. please for some insight on how to do it

assertEquals(400, response.getStatus());

}
}

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Where is the valid test for a valid change password request?

}

@Test
public void resetPassword_shouldFailWith400BadRequestIfActivationKeyIsInvalid() throws Exception {

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

This behavior is in the service layer, you don't need to duplicate the test here

@wluyima

This comment has been minimized.

Copy link
Member

wluyima commented Aug 7, 2018

I believe you only need 2 tests, one for the request password reset and the other for the change password request, the ones you added don't make sense since they are testing logic that's actually in the service layer that you already added to core anyways.

Removed ResponseEntity and added SimpleObject for returning in the re…
…sponse body Removed duplicate test that was done in the webservice
public void requestPasswordReset(@RequestBody Map<String, String> body) {
User user = null;
String usernameOrEmail = body.get("usernameOrEmail");
if ((user = userService.getUserByEmail(usernameOrEmail)) == null) {

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

This code looks vague, you assign user in the if clause, I think you don't have to do the assignment again inside the if clause


@RequestMapping(value = "/{activationkey}", method = RequestMethod.GET)
@ResponseBody
public SimpleObject verifyActivationKey(@PathVariable("activationkey") String activationkey, HttpServletRequest request,

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Why is this method needed? You should not expose a rest endpoint to verify the activationKey


@RequestMapping(value = "/{activationkey}", method = RequestMethod.POST)
@ResponseBody
public SimpleObject resetPassword(@PathVariable("activationkey") String activationkey,

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Why do you need a return value anyways? I think this method should not return a value.

}

@Test
public void verifyActivationKey_shouldFailWith400BadRequestIfActivationKeyIsInvalid() throws Exception {

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Where are the tests that test the setting a user activation key and changing the password?

This test won't be needed, because you don't need an endpoint to verify the activation key


private static final String RESET_PASSWORD_URI = "passwordreset";

private MessageSourceService messages;

This comment has been minimized.

@wluyima

wluyima Aug 7, 2018

Member

Why is this needed?

@fanyui

This comment has been minimized.

Copy link
Contributor Author

fanyui commented Aug 8, 2018

Hi @wluyima am having trouble writing the test for testing reseting of password as activation key is generated at runtime and usually expires and i wouldn't have access to a correct one to test with.

@wluyima

This comment has been minimized.

Copy link
Member

wluyima commented Aug 8, 2018

Write them the same way you wrote the service layer tests in core

@wluyima

This comment has been minimized.

Copy link
Member

wluyima commented Aug 8, 2018

I'm a little confused, the activation key takes 10min to expire while a test takes milliseconds to run, there is no way the token can expire before the test completes.

Renamed getUserByEmail to getUserbyUsernameOrEmail and added tests fo…
…r method resetpassword and method to trigger paswordreset

@RequestMapping(value = "/{activationkey}", method = RequestMethod.POST)
public void resetPassword(@PathVariable("activationkey") String activationkey,
@RequestBody Map<String, String> body, HttpServletRequest request,

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

If you never use the request parameter, remove it

response.setStatus(HttpServletResponse.SC_OK);
}
catch (InvalidActivationKeyException exception) {
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

You should instead rethrow a ValidationException and let the framework take care of it


try {
userService.changePasswordUsingActivationKey(activationkey, newPassword);
response.setStatus(HttpServletResponse.SC_OK);

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

Status should be defined on the method

This comment has been minimized.

@fanyui

fanyui Aug 8, 2018

Author Contributor

@wluyima Does that mean we should also always respond with thesame response on a request to this method. ie something like http.OK. because from the talk discurssion it was said to respond with ok when the resetting succeeded and bad request when it doesn't succeed

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

I think you might have missed one of my other comments, I did say re throw a ValidationException when InvalidActivationKeyException is caught, that should take care of it


@Test
public void requestPasswordReset_shouldCreateUserActivationKeyGivenUsername() throws Exception {
User u = new User();

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?

This comment has been minimized.

@fanyui

fanyui Aug 8, 2018

Author Contributor

@wluyima I have been checking and the last dataset that contained an existing user was defined since 1.8 so i am thinking declaring a new user here is appropriate.

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

I'm not sure where you are looking, your test class is extending RestControllerTestUtils which has other superclasses form core that load this standardTestDataset


@Test
public void requestPasswordReset_shouldCreateUserActivationKeyGivenEmail() throws Exception {
User u = new User();

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?


@Test
public void resetPassword_shouldResetUserPasswordIfActivationKeyIsCorrect() throws Exception {
User u = new User();

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?

String newPassword = "newPasswordString123";
MockHttpServletResponse response = handle(newPostRequest(RESET_PASSWORD_URI + "/" + key, "{\"newPassword\":\""
+ newPassword + "\"}"));
assertEquals(200, response.getStatus());

This comment has been minimized.

@wluyima

wluyima Aug 8, 2018

Member

You should have an assertion that ensures that you can login with the new password

private static final String RESET_PASSWORD_URI = "passwordreset";

@Rule
public ExpectedException expectedException = ExpectedException.none();

This comment has been minimized.

@wluyima

wluyima Aug 9, 2018

Member

Where are you using this?


@Before
public void setup() {
userService = Context.getUserService();

This comment has been minimized.

@wluyima

wluyima Aug 9, 2018

Member

This is redundant and a duplication because you are already autowiring it

@fanyui fanyui force-pushed the fanyui:passwordresetrequest branch from 9097865 to 38c6878 Aug 10, 2018

@fanyui

This comment has been minimized.

Copy link
Contributor Author

fanyui commented Aug 10, 2018

@wluyima Please is there still anything left with this pull request?

@wluyima wluyima merged commit d5e6d56 into openmrs:passwordreset Aug 10, 2018

9 of 12 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
security/snyk - omod/pom.xml (openmrs) Failed to detect issues
Details
security/snyk - omod-1.12/pom.xml (openmrs) 8272 new vulnerable dependency paths
Details
security/snyk - integration-tests/pom.xml (openmrs) No new issues
Details
security/snyk - omod-1.10/pom.xml (openmrs) No new issues
Details
security/snyk - omod-1.11/pom.xml (openmrs) No new issues
Details
security/snyk - omod-1.8/pom.xml (openmrs) No new issues
Details
security/snyk - omod-1.9/pom.xml (openmrs) No new issues
Details
security/snyk - omod-2.0/pom.xml (openmrs) No new issues
Details
security/snyk - omod-2.1/pom.xml (openmrs) No new issues
Details
security/snyk - omod-common/pom.xml (openmrs) No new issues
Details
security/snyk - pom.xml (openmrs) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.