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
Copy path View file
@@ -11,7 +11,7 @@
<description>OpenMRS module project for Rest Web Services</description>

<properties>
<openmrs.version.2.2.0-SNAPSHOT>2.2.0-SNAPSHOT</openmrs.version.2.2.0-SNAPSHOT>
<openmrs.version.2.2.0-SNAPSHOT>2.2.0-passwordreset</openmrs.version.2.2.0-SNAPSHOT>
</properties>

<dependencies>
@@ -0,0 +1,66 @@
/**
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.webservices.rest.web.v1_0.controller.openmrs2_2;

import java.util.Map;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.openmrs.User;
import org.openmrs.api.InvalidActivationKeyException;
import org.openmrs.api.UserService;
import org.openmrs.module.webservices.rest.web.RestConstants;
import org.openmrs.module.webservices.rest.web.v1_0.controller.BaseRestController;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.ResponseStatus;

@Controller
@RequestMapping(value = "/rest/" + RestConstants.VERSION_1 + "/passwordreset")
public class PasswordResetController2_2 extends BaseRestController {

@Qualifier("userService")
@Autowired
private UserService userService;

@RequestMapping(method = RequestMethod.POST)
@ResponseStatus(HttpStatus.OK)
public void requestPasswordReset(@RequestBody Map<String, String> body) {
String usernameOrEmail = body.get("usernameOrEmail");
User user = userService.getUserByUsernameOrEmail(usernameOrEmail);
if (user != null) {
userService.setUserActivationKey(user);
}
}

@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.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

If you never use the request parameter, remove it

HttpServletResponse response) {
String newPassword = body.get("newPassword");

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

Status should be defined on the method

This comment has been minimized.

Copy link
@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.

Copy link
@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

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

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

}

}

}
@@ -0,0 +1,101 @@
/**
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.webservices.rest.web.v1_0.controller.openmrs2_2;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.openmrs.Person;
import org.openmrs.PersonName;
import org.openmrs.User;
import org.openmrs.api.UserService;
import org.openmrs.api.context.Context;
import org.openmrs.api.db.LoginCredential;
import org.openmrs.api.db.UserDAO;
import org.openmrs.module.webservices.rest.web.v1_0.controller.RestControllerTestUtils;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.mock.web.MockHttpServletResponse;

public class PasswordResetController2_2Test extends RestControllerTestUtils {

private static final String RESET_PASSWORD_URI = "passwordreset";

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 9, 2018

Member

Where are you using this?


@Autowired
@Qualifier("userService")
private UserService userService;

@Autowired
private UserDAO dao;

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 9, 2018

Member

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

}

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?

This comment has been minimized.

Copy link
@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.

Copy link
@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

u.setPerson(new Person());
u.addName(new PersonName("Benjamin", "A", "Wolfe"));
u.setUsername("bwolfe");
u.getPerson().setGender("M");
User createdUser = userService.createUser(u, "Openmr5xy");
assertNull(dao.getLoginCredential(createdUser).getActivationKey());
assertEquals(createdUser, userService.setUserActivationKey(createdUser));
handle(newPostRequest(RESET_PASSWORD_URI, "{\"usernameOrEmail\":\"" + "bwolfe" + "\"}"));
assertNotNull(dao.getLoginCredential(createdUser).getActivationKey());
}

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?

u.setPerson(new Person());
u.addName(new PersonName("Benjamin", "A", "Wolfe"));
u.setUsername("bwolfe");
u.setEmail("bwolf@gmail.com");
u.getPerson().setGender("M");
User createdUser = userService.createUser(u, "Openmr5xy");
assertNull(dao.getLoginCredential(createdUser).getActivationKey());
assertEquals(createdUser, userService.setUserActivationKey(createdUser));
handle(newPostRequest(RESET_PASSWORD_URI, "{\"usernameOrEmail\":\"" + "bwolfe@gmail.com" + "\"}"));
assertNotNull(dao.getLoginCredential(createdUser).getActivationKey());
}

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?

u.setPerson(new Person());
u.addName(new PersonName("Benjamin", "A", "Wolfe"));
u.setUsername("bwolfe");
u.getPerson().setGender("M");
User createdUser = userService.createUser(u, "Openmr5xy");
String key = "h4ph0fpNzQCIPSw8plJI";
int validTime = 10 * 60 * 1000; //equivalent to 10 minutes for token to be valid
Long tokenTime = System.currentTimeMillis() + validTime;
LoginCredential credentials = dao.getLoginCredential(createdUser);
credentials
.setActivationKey("b071c88d6d877922e35af2e6a90dd57d37ac61143a03bb986c5f353566f3972a86ce9b2604c31a22dfa467922dcfd54fa7d18b0a7c7648d94ca3d97a88ea2fd0:"
+ tokenTime);
dao.updateLoginCredential(credentials);
String newPassword = "newPasswordString123";
MockHttpServletResponse response = handle(newPostRequest(RESET_PASSWORD_URI + "/" + key, "{\"newPassword\":\""
+ newPassword + "\"}"));
assertEquals(200, response.getStatus());

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

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

}
}

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

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

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.