Merged 5.2.0 (5.2.0) to HEAD (5.2)

133849 rmunteanu: REPO-1746: Merge fixes for 5.2 GA issues to 5.2.0 branch
      Merged 5.2.N (5.2.1) to 5.2.0 (5.2.0)
         133351 jvonka: REPO-1646: V1 REST API - cannot unset optional fields (eg. when updating person / site details ...) - minor fixes with tests (update person)
         - ensure enabled & emailNotificationsEnabled cannot be null
         - null/empty company object should unset all fields (fix for empty case)


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@134188 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

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

@@ -520,12 +520,22 @@ public class PeopleImpl implements People
 		{
 			throw new InvalidArgumentException("Field '"+fieldName+"' is null, but is required.");
 		}
+
+        // belts-and-braces - note: should not see empty string (since converted to null via custom json deserializer)
+        if ((fieldValue instanceof String) && ((String)fieldValue).isEmpty())
+        {
+            throw new InvalidArgumentException("Field '"+fieldName+"' is empty, but is required.");
+        }
 	}
 
     public Person update(String personId, final Person person)
     {
         MutableAuthenticationService mutableAuthenticationService = (MutableAuthenticationService) authenticationService;
 
+        validateUpdatePersonData(person);
+
+        boolean isAdmin = isAdminAuthority();
+
         String currentUserId = AuthenticationUtil.getFullyAuthenticatedUser();
         if (!isAdminAuthority() && !currentUserId.equalsIgnoreCase(personId))
         {
@@ -575,6 +585,7 @@ public class PeopleImpl implements People
                 throw new PermissionDeniedException("Admin authority cannot be disabled.");
             }
 
+            // note: if current user is not an admin then permission denied exception is thrown
             mutableAuthenticationService.setAuthenticationEnabled(personIdToUpdate, person.isEnabled());
         }
 
@@ -612,6 +623,78 @@ public class PeopleImpl implements People
         return getPerson(personId);
     }
 
+    private void validateUpdatePersonData(Person person)
+    {
+        if (person.wasSet(ContentModel.PROP_FIRSTNAME))
+        {
+            checkRequiredField("firstName", person.getFirstName());
+        }
+
+        if (person.wasSet(ContentModel.PROP_EMAIL))
+        {
+            checkRequiredField("email", person.getEmail());
+        }
+
+        if (person.wasSet(ContentModel.PROP_ENABLED) && (person.isEnabled() == null))
+        {
+            throw new IllegalArgumentException("'enabled' field cannot be empty.");
+        }
+
+        if (person.wasSet(ContentModel.PROP_EMAIL_FEED_DISABLED) && (person.isEmailNotificationsEnabled() == null))
+        {
+            throw new IllegalArgumentException("'emailNotificationsEnabled' field cannot be empty.");
+        }
+    }
+
+    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);
+            }
+        }
+    }
+
     private boolean isAdminAuthority()
     {
         return authorityService.hasAdminAuthority();

source/java/org/alfresco/rest/api/model/Person.java

@@ -493,50 +493,62 @@ public class Person
 		if (wasSet(PROP_PERSON_COMPANY))
 		{
 			Company company = getCompany();
+
+			int setCount = 0;
 			if (company != null)
 			{
 				if (company.wasSet(ContentModel.PROP_ORGANIZATION))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_ORGANIZATION, company.getOrganization());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYADDRESS1))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYADDRESS1, company.getAddress1());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYADDRESS2))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYADDRESS2, company.getAddress2());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYADDRESS3))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYADDRESS3, company.getAddress3());
 				}
 
 				if (company.wasSet(ContentModel.PROP_COMPANYPOSTCODE))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYPOSTCODE, company.getPostcode());
 				}
 
 				if (company.wasSet(ContentModel.PROP_COMPANYTELEPHONE))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYTELEPHONE, company.getTelephone());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYFAX))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYFAX, company.getFax());
 				}
 
 				if (company.wasSet(ContentModel.PROP_COMPANYEMAIL))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYEMAIL, company.getEmail());
 				}
 			}
-			else
+
+			if (setCount == 0)
 			{
+				// company was null or {} (no individual properties set)
 				properties.put(ContentModel.PROP_ORGANIZATION, null);
 				properties.put(ContentModel.PROP_COMPANYADDRESS1, null);
 				properties.put(ContentModel.PROP_COMPANYADDRESS2, null);

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

@@ -833,6 +833,11 @@ public class TestPeople extends EnterpriseTestApi
         // TODO: temp fix, set back to orig firstName
         publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin"));
         people.update(personId, qjson("{ `firstName`:`Bill` }"), 200);
+
+        // -ve test: check that required/mandatory/non-null fields cannot be unset (or empty string)
+        people.update("people", personId, null, null, qjson("{ `firstName`:`` }"), null, "Expected 400 response when updating " + personId, 400);
+        people.update("people", personId, null, null, qjson("{ `email`:`` }"), null, "Expected 400 response when updating " + personId, 400);
+        people.update("people", personId, null, null, qjson("{ `emailNotificationsEnabled`:`` }"), null, "Expected 400 response when updating " + personId, 400);
     }
 
     @Test
@@ -977,7 +982,6 @@ public class TestPeople extends EnterpriseTestApi
                         + "  \"location\":null,\n"
 
                         + "  \"company\": {\n"
-                        + "    \"organization\":null,\n" 
                         + "    \"address1\":null,\n"
                         + "    \"address2\":null,\n"
                         + "    \"address3\":null,\n"
@@ -1005,7 +1009,8 @@ public class TestPeople extends EnterpriseTestApi
         assertNull(updatedPerson.getLocation());
 
         assertNotNull(updatedPerson.getCompany());
-        assertNull(updatedPerson.getCompany().getOrganization());
+        assertNotNull(updatedPerson.getCompany().getOrganization());
+
         assertNull(updatedPerson.getCompany().getAddress1());
         assertNull(updatedPerson.getCompany().getAddress2());
         assertNull(updatedPerson.getCompany().getAddress3());
@@ -1018,6 +1023,19 @@ public class TestPeople extends EnterpriseTestApi
         assertNull(updatedPerson.getTelephone());
         assertNull(updatedPerson.getUserStatus());
 
+        // test ability to unset company fields as a whole
+        response = people.update("people", personId, null, null,
+                "{\n"
+                        + "  \"company\": {} \n"
+                        + "}", params,
+                "Expected 200 response when updating " + personId, 200);
+
+        updatedPerson = Person.parsePerson((JSONObject) response.getJsonResponse().get("entry"));
+
+        // note: empty company object is returned for backwards compatibility (with pre-existing getPerson API <= 5.1)
+        assertNotNull(updatedPerson.getCompany());
+        assertNull(updatedPerson.getCompany().getOrganization());
+
         // set at least one company field
         String updatedOrgName = "another org";
 
@@ -1045,7 +1063,6 @@ public class TestPeople extends EnterpriseTestApi
 
         // note: empty company object is returned for backwards compatibility (with pre-existing getPerson API <= 5.1)
         assertNotNull(updatedPerson.getCompany());
-
         assertNull(updatedPerson.getCompany().getOrganization());
     }
 
@@ -1085,6 +1102,10 @@ public class TestPeople extends EnterpriseTestApi
             assertEquals(enabled, updatedPerson.isEnabled());
         }
 
+        // -ve test: enabled flag cannot be null/empty
+        people.update("people", personId, null, null, qjson("{ `enabled`: null }"), null, "Expected 400 response when updating " + personId, 400);
+        people.update("people", personId, null, null, qjson("{ `enabled`: `` }"), null, "Expected 400 response when updating " + personId, 400);
+
         // Use non-admin user's own credentials
         publicApiClient.setRequestContext(new RequestContext(account3.getId(), personId, "password"));