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,96 @@
/**
* 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.SimpleObject;
import org.openmrs.module.webservices.rest.web.RestConstants;
import org.openmrs.module.webservices.rest.web.RestUtil;
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.ResponseBody;
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) {
User user = null;
String usernameOrEmail = body.get("usernameOrEmail");
if ((user = userService.getUserByEmail(usernameOrEmail)) == null) {

This comment has been minimized.

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

user = userService.getUserByUsername(usernameOrEmail);

This comment has been minimized.

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

}
if (user != null) {
userService.setUserActivationKey(user);
}
}

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

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

@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) {
SimpleObject resp = new SimpleObject();
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

resp.add("message", "success");
}
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

return RestUtil.wrapErrorResponse(exception, "Password reset failed");
}

return resp;
}

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

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

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

HttpServletResponse response) {
SimpleObject resp = new SimpleObject();
User user = userService.getUserByActivationKey(activationkey);
if (user == null) {
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
resp.add("message", "Invalid Activation Key");
return resp;
}
else {
response.setStatus(HttpServletResponse.SC_OK);
resp.add("message", "success");
return resp;
}

}

}
@@ -0,0 +1,56 @@
/**
* 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 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.APIException;
import org.openmrs.api.UserService;
import org.openmrs.api.context.Context;
import org.openmrs.messagesource.MessageSourceService;
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";

private MessageSourceService messages;

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Why is this needed?


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

@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

messages = Context.getMessageSourceService();
}

@Test
public void verifyActivationKey_shouldFailWith400BadRequestIfActivationKeyIsInvalid() throws Exception {

This comment has been minimized.

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

String activationKey = "wrongActivationKey12";
MockHttpServletResponse response = handle(newGetRequest(RESET_PASSWORD_URI + "/" + activationKey));
assertEquals(400, response.getStatus());
}

}

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.