V1 REST API - update person (password)

- REPO-1627 - Update password for a person with empty string
- REPO-1643 - Missing 'oldPassword' field accompanies 403 status code, but should be 400

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/BRANCHES/DEV/5.2.N/root@133307 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/rest/api/impl/PeopleImpl.java

@@ -518,59 +518,20 @@ public class PeopleImpl implements People
 
     public Person update(String personId, final Person person)
     {
-        MutableAuthenticationService mutableAuthenticationService = (MutableAuthenticationService) authenticationService;
+        boolean isAdmin = isAdminAuthority();
 
         String currentUserId = AuthenticationUtil.getFullyAuthenticatedUser();
-        if (!isAdminAuthority() && !currentUserId.equalsIgnoreCase(personId))
+        if (!isAdmin && !currentUserId.equalsIgnoreCase(personId))
         {
             // The user is not an admin user and is not attempting to update *their own* details.
             throw new PermissionDeniedException();
         }
-        if (!isAdminAuthority() && person.getOldPassword() != null && person.getPassword() == null)
-        {
-            throw new IllegalArgumentException("To change password, both 'oldPassword' and 'password' fields are required.");
-        }
 
         final String personIdToUpdate = validatePerson(personId);
         final Map<QName, Serializable> properties = person.toProperties();
 
-        if (person.getPassword() != null && !person.getPassword().isEmpty())
+        // if requested, update password
-        {
+        updatePassword(isAdmin, personIdToUpdate, person);
-            // An admin user can update without knowing the original pass - but must know their own!
-            char[] newPassword = person.getPassword().toCharArray();
-            if (isAdminAuthority())
-            {
-                mutableAuthenticationService.setAuthentication(personIdToUpdate, newPassword);
-            }
-            else
-            {
-                // Non-admin users can update their own password, but must provide their current password.
-                if (person.getOldPassword() == null)
-                {
-                    throw new PermissionDeniedException("Existing password is required, but missing (field 'oldPassword').");
-                }
-                char[] oldPassword = person.getOldPassword().toCharArray();
-                try
-                {
-                    mutableAuthenticationService.updateAuthentication(personIdToUpdate, oldPassword, newPassword);
-                }
-                catch (AuthenticationException e)
-                {
-                    // TODO: add to exception mappings/handler
-                    throw new PermissionDeniedException("Incorrect existing password.");
-                }
-            }
-        }
-
-        if (person.isEnabled() != null)
-        {
-            if (isAdminAuthority(personIdToUpdate))
-            {
-                throw new PermissionDeniedException("Admin authority cannot be disabled.");
-            }
-
-            mutableAuthenticationService.setAuthenticationEnabled(personIdToUpdate, person.isEnabled());
-        }
 
 		NodeRef personNodeRef = personService.getPerson(personIdToUpdate, false);
 		if (person.wasSet(Person.PROP_PERSON_DESCRIPTION))
@@ -606,6 +567,66 @@ public class PeopleImpl implements People
         return getPerson(personId);
     }
 
+    private void updatePassword(boolean isAdmin, String personIdToUpdate, Person person)
+    {
+        MutableAuthenticationService mutableAuthenticationService = (MutableAuthenticationService) authenticationService;
+
+        boolean isOldPassword = person.wasSet(Person.PROP_PERSON_OLDPASSWORD);
+        boolean isPassword = person.wasSet(Person.PROP_PERSON_PASSWORD);
+
+        if (isPassword || isOldPassword)
+        {
+            if (isOldPassword && ((person.getOldPassword() == null) || (person.getOldPassword().isEmpty())))
+            {
+                throw new IllegalArgumentException("'oldPassword' field cannot be empty.");
+            }
+
+            if (!isPassword || (person.getPassword() == null) || (person.getPassword().isEmpty()))
+            {
+                throw new IllegalArgumentException("password' field cannot be empty.");
+            }
+
+            char[] newPassword = person.getPassword().toCharArray();
+
+            if (!isAdmin)
+            {
+                // Non-admin users can update their own password, but must provide their current password.
+                if (!isOldPassword)
+                {
+                    throw new IllegalArgumentException("To change password, both 'oldPassword' and 'password' fields are required.");
+                }
+
+                char[] oldPassword = person.getOldPassword().toCharArray();
+                try
+                {
+                    mutableAuthenticationService.updateAuthentication(personIdToUpdate, oldPassword, newPassword);
+                }
+                catch (AuthenticationException e)
+                {
+                    throw new PermissionDeniedException("Incorrect password.");
+                }
+            }
+            else
+            {
+                // An admin user can update without knowing the original pass - but must know their own!
+                // note: is it reasonable to silently ignore oldPassword if supplied ?
+
+                mutableAuthenticationService.setAuthentication(personIdToUpdate, newPassword);
+            }
+        }
+
+        if (person.isEnabled() != null)
+        {
+            if (isAdminAuthority(personIdToUpdate))
+            {
+                throw new PermissionDeniedException("Admin authority cannot be disabled.");
+            }
+
+            mutableAuthenticationService.setAuthenticationEnabled(personIdToUpdate, person.isEnabled());
+        }
+
+    }
+
     private boolean isAdminAuthority()
     {
         return authorityService.hasAdminAuthority();

source/test-java/org/alfresco/rest/api/tests/TestPeople.java

@@ -1134,11 +1134,14 @@ public class TestPeople extends EnterpriseTestApi
         people.update(me.getId(), qjson("{ `oldPassword`:`password123`, `password`:`newpassword456` }"), 403);
 
         // update with no oldPassword
-        people.update(me.getId(), qjson("{ `password`:`newpassword456` }"), 403);
+        people.update(me.getId(), qjson("{ `password`:`newpassword456` }"), 400);
+        people.update(me.getId(), qjson("{ `oldPassword`:``, `password`:`newpassword456` }"), 400);
+        people.update(me.getId(), qjson("{ `oldPassword`:null, `password`:`newpassword456` }"), 400);
 
-        // update with no password
+        // update with no new password
-        people.update(me.getId(), qjson("{ `oldPassword`:`newpassword456`, `password`:`` }"), 400);
         people.update(me.getId(), qjson("{ `oldPassword`:`newpassword456` }"), 400);
+        people.update(me.getId(), qjson("{ `oldPassword`:`newpassword456`, `password`:`` }"), 400);
+        people.update(me.getId(), qjson("{ `oldPassword`:`newpassword456`, `password`:null }"), 400);
     }
 
     @Test
@@ -1168,6 +1171,30 @@ public class TestPeople extends EnterpriseTestApi
 
         publicApiClient.setRequestContext(new RequestContext(networkId, personId, updatedPassword));
         this.people.getPerson(personId);
+
+        publicApiClient.setRequestContext(new RequestContext(networkId, account3Admin, "admin"));
+
+        // update with another new password but note that oldPassword is ignored (even if sent by admin)
+        String updatedPassword2 = "newPassword2";
+        people.update(personId, qjson("{ `password`:`" + updatedPassword2 + "`, `oldPassword`:`rubbish` }"), 200);
+
+        publicApiClient.setRequestContext(new RequestContext(networkId, personId, updatedPassword));
+        try
+        {
+            this.people.getPerson(personId);
+            fail("");
+        }
+        catch (PublicApiException e)
+        {
+            assertEquals(HttpStatus.SC_UNAUTHORIZED, e.getHttpResponse().getStatusCode());
+        }
+
+        publicApiClient.setRequestContext(new RequestContext(networkId, personId, updatedPassword2));
+        this.people.getPerson(personId);
+
+        // -ve: update with no new password
+        people.update(personId, qjson("{ `password`:`` }"), 400);
+        people.update(personId, qjson("{ `password`:null }"), 400);
     }
 
     @Test