diff --git a/source/java/org/alfresco/rest/api/Nodes.java b/source/java/org/alfresco/rest/api/Nodes.java index 4f750cf8e6..2c07dd0c13 100644 --- a/source/java/org/alfresco/rest/api/Nodes.java +++ b/source/java/org/alfresco/rest/api/Nodes.java @@ -269,13 +269,15 @@ public interface Nodes * Convert from node properties (map of QName to Serializable) retrieved from * the respository to a map of String to Object that can be formatted/expressed * as required by the API JSON response for get nodes, get person etc. + *

+ * Returns null if there are no properties to return, rather than an empty map. * * @param nodeProps * @param selectParam * @param mapUserInfo * @param excludedNS * @param excludedProps - * @return + * @return The map of properties, or null if none to return. */ Map mapFromNodeProperties(Map nodeProps, List selectParam, Map mapUserInfo, List excludedNS, List excludedProps); @@ -288,15 +290,26 @@ public interface Nodes */ Map mapToNodeProperties(Map props); + /** + * Map from a String representation of aspect names to a set + * of QName objects, as used by the repository. + * + * @param aspectNames + * @return + */ + Set mapToNodeAspects(List aspectNames); + /** * Map from aspects (Set of QName) retrieved from the repository to a * map List of String required that can be formatted/expressed as required * by the API JSON response for get nodes, get person etc. + *

+ * Returns null if there are no aspect names to return, rather than an empty list. * * @param nodeAspects * @param excludedNS * @param excludedAspects - * @return + * @return The list of aspect names, or null if none to return. */ List mapFromNodeAspects(Set nodeAspects, List excludedNS, List excludedAspects); diff --git a/source/java/org/alfresco/rest/api/impl/NodesImpl.java b/source/java/org/alfresco/rest/api/impl/NodesImpl.java index c865d46905..3cf4f41f3a 100644 --- a/source/java/org/alfresco/rest/api/impl/NodesImpl.java +++ b/source/java/org/alfresco/rest/api/impl/NodesImpl.java @@ -1099,7 +1099,7 @@ public class NodesImpl implements Nodes return new PathInfo(pathStr, isComplete, pathElements); } - protected Set mapToNodeAspects(List aspectNames) + public Set mapToNodeAspects(List aspectNames) { Set nodeAspects = new HashSet<>(aspectNames.size()); diff --git a/source/java/org/alfresco/rest/api/impl/PeopleImpl.java b/source/java/org/alfresco/rest/api/impl/PeopleImpl.java index be0dba52fe..acdc5e2482 100644 --- a/source/java/org/alfresco/rest/api/impl/PeopleImpl.java +++ b/source/java/org/alfresco/rest/api/impl/PeopleImpl.java @@ -82,8 +82,10 @@ public class PeopleImpl implements People { private static final List EXCLUDED_NS = Arrays.asList( NamespaceService.SYSTEM_MODEL_1_0_URI, - "http://www.alfresco.org/model/user/1.0"); + "http://www.alfresco.org/model/user/1.0", + NamespaceService.CONTENT_MODEL_1_0_URI); private static final List EXCLUDED_ASPECTS = Arrays.asList(); + // TODO: no longer needed? (can be empty) private static final List EXCLUDED_PROPS = Arrays.asList( ContentModel.PROP_USERNAME, ContentModel.PROP_FIRSTNAME, @@ -411,8 +413,8 @@ public class PeopleImpl implements People // Expose properties if (include.contains(PARAM_INCLUDE_PROPERTIES)) { - Map custProps = new HashMap<>(); - custProps.putAll(nodes.mapFromNodeProperties(nodeProps, new ArrayList<>(), new HashMap<>(), EXCLUDED_NS, EXCLUDED_PROPS)); + // Note that custProps may be null. + Map custProps = nodes.mapFromNodeProperties(nodeProps, new ArrayList<>(), new HashMap<>(), EXCLUDED_NS, EXCLUDED_PROPS); person.setProperties(custProps); } if (include.contains(PARAM_INCLUDE_ASPECTNAMES)) @@ -529,13 +531,41 @@ public class PeopleImpl implements People private void validateCreatePersonData(Person person) { + validateNamespaces(person.getAspectNames(), person.getProperties()); checkRequiredField("id", person.getUserName()); checkRequiredField("firstName", person.getFirstName()); checkRequiredField("email", person.getEmail()); checkRequiredField("password", person.getPassword()); } - - private void checkRequiredField(String fieldName, Object fieldValue) + + private void validateNamespaces(List aspectNames, Map properties) + { + if (aspectNames != null) + { + Set aspects = nodes.mapToNodeAspects(aspectNames); + aspects.forEach(aspect -> + { + if (EXCLUDED_NS.contains(aspect.getNamespaceURI())) + { + throw new IllegalArgumentException("Namespace cannot be used by People API: "+aspect.toPrefixString()); + } + }); + } + + if (properties != null) + { + Map nodeProps = nodes.mapToNodeProperties(properties); + nodeProps.keySet().forEach(qname -> + { + if (EXCLUDED_NS.contains(qname.getNamespaceURI())) + { + throw new IllegalArgumentException("Namespace cannot be used by People API: "+qname.toPrefixString()); + } + }); + } + } + + private void checkRequiredField(String fieldName, Object fieldValue) { if (fieldValue == null) { @@ -616,6 +646,8 @@ public class PeopleImpl implements People private void validateUpdatePersonData(Person person) { + validateNamespaces(person.getAspectNames(), person.getProperties()); + if (person.wasSet(ContentModel.PROP_FIRSTNAME)) { checkRequiredField("firstName", person.getFirstName()); diff --git a/source/test-java/org/alfresco/rest/api/tests/TestPeople.java b/source/test-java/org/alfresco/rest/api/tests/TestPeople.java index cc6eeddcfe..fd8f9c6682 100644 --- a/source/test-java/org/alfresco/rest/api/tests/TestPeople.java +++ b/source/test-java/org/alfresco/rest/api/tests/TestPeople.java @@ -73,6 +73,10 @@ import static org.junit.Assert.fail; public class TestPeople extends EnterpriseTestApi { + private static final QName ASPECT_COMMS = QName.createQName("test.people.api", "comms"); + private static final QName PROP_TELEHASH = QName.createQName("test.people.api", "telehash"); + private static final QName ASPECT_LUNCHABLE = QName.createQName("test.people.api", "lunchable"); + private static final QName PROP_LUNCH = QName.createQName("test.people.api", "lunch"); private People people; private Iterator accountsIt; private TestNetwork account1; @@ -90,6 +94,8 @@ public class TestPeople extends EnterpriseTestApi private Person personAlice; private Person personAliceD; private Person personBen; + private NodeService nodeService; + private PersonService personService; @Before public void setUp() throws Exception @@ -110,6 +116,9 @@ public class TestPeople extends EnterpriseTestApi account3.createUser(); account3PersonIt = account3.getPersonIds().iterator(); + nodeService = applicationContext.getBean("NodeService", NodeService.class); + personService = applicationContext.getBean("PersonService", PersonService.class); + // Capture authentication pre-test, so we can restore it again afterwards. AuthenticationUtil.pushAuthentication(); } @@ -488,6 +497,27 @@ public class TestPeople extends EnterpriseTestApi // Attempt to create the person a second time - as non-admin expect 403 people.create(person, 403); } + + // -ve: cannot set built-in/non-custom props + { + publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin")); + Person person = new Person(); + String personId = UUID.randomUUID().toString()+"@"+account1.getId(); + person.setUserName(personId); + person.setFirstName("Joe"); + person.setEmail(personId); + person.setEnabled(true); + person.setPassword("password123"); + + person.setProperties(Collections.singletonMap("usr:enabled", false)); + people.create(person, 400); + + person.setProperties(Collections.singletonMap("cm:title", "hello-world")); + people.create(person, 400); + + person.setProperties(Collections.singletonMap("sys:locale", "en_GB")); + people.create(person, 400); + } } @Test @@ -495,12 +525,11 @@ public class TestPeople extends EnterpriseTestApi { // Create the person directly using the Java services - we don't want to test // the REST API's "create person" function here, so we're isolating this test from it. - PersonService personService = applicationContext.getBean("PersonService", PersonService.class); MutableAuthenticationService authService = applicationContext.getBean("AuthenticationService", MutableAuthenticationService.class); PreferenceService prefService = applicationContext.getBean("PreferenceService", PreferenceService.class); Map nodeProps = new HashMap<>(); - // The cm:titled aspect should be auto-added for the cm:title property - nodeProps.put(ContentModel.PROP_TITLE, "A title"); + // The papi:lunchable aspect should be auto-added for the papi:lunch property + nodeProps.put(PROP_LUNCH, "Falafel wrap"); // These properties should not be present when a person is retrieved // since they are present as top-level fields. @@ -551,43 +580,13 @@ public class TestPeople extends EnterpriseTestApi // Did we get the correct aspects/properties? assertEquals(userName, person.getId()); assertEquals("Doc", person.getFirstName()); - assertEquals("A title", person.getProperties().get("cm:title")); - assertTrue(person.getAspectNames().contains("cm:titled")); - - // Properties that are already represented as specific fields in the API response (e.g. firstName, lastName...) - // must be filtered from the generic properties datastructure. - assertFalse(person.getProperties().containsKey("cm:userName")); - assertFalse(person.getProperties().containsKey("cm:firstName")); - assertFalse(person.getProperties().containsKey("cm:lastName")); - assertFalse(person.getProperties().containsKey("cm:jobtitle")); - assertFalse(person.getProperties().containsKey("cm:location")); - assertFalse(person.getProperties().containsKey("cm:telephone")); - assertFalse(person.getProperties().containsKey("cm:mobile")); - assertFalse(person.getProperties().containsKey("cm:email")); - assertFalse(person.getProperties().containsKey("cm:organization")); - assertFalse(person.getProperties().containsKey("cm:companyaddress1")); - assertFalse(person.getProperties().containsKey("cm:companyaddress2")); - assertFalse(person.getProperties().containsKey("cm:companyaddress3")); - assertFalse(person.getProperties().containsKey("cm:companypostcode")); - assertFalse(person.getProperties().containsKey("cm:companytelephone")); - assertFalse(person.getProperties().containsKey("cm:companyfax")); - assertFalse(person.getProperties().containsKey("cm:companyemail")); - assertFalse(person.getProperties().containsKey("cm:skype")); - assertFalse(person.getProperties().containsKey("cm:instantmsg")); - assertFalse(person.getProperties().containsKey("cm:userStatus")); - assertFalse(person.getProperties().containsKey("cm:userStatusTime")); - assertFalse(person.getProperties().containsKey("cm:googleusername")); - assertFalse(person.getProperties().containsKey("cm:sizeQuota")); - assertFalse(person.getProperties().containsKey("cm:sizeCurrent")); - assertFalse(person.getProperties().containsKey("cm:emailFeedDisabled")); - assertFalse(person.getProperties().containsKey("cm:persondescription")); - // We also don't want cm:preferenceValues (see REPO-1636) - assertFalse(person.getProperties().containsKey("cm:preferenceValues")); + assertEquals("Falafel wrap", person.getProperties().get("papi:lunch")); + assertTrue(person.getAspectNames().contains("papi:lunchable")); // Check that no properties are present that should have been filtered by namespace. for (String key : person.getProperties().keySet()) { - if (key.startsWith("sys:") || key.startsWith("usr:")) + if (key.startsWith("cm:") || key.startsWith("sys:") || key.startsWith("usr:")) { Object value = person.getProperties().get(key); String keyValueStr = String.format("(key=%s, value=%s)", key, value); @@ -609,34 +608,34 @@ public class TestPeople extends EnterpriseTestApi person.setPassword("password123"); Map props = new HashMap<>(); - props.put("cm:title", "This is a title"); + props.put("papi:telehash", "724332b5796a8"); person.setProperties(props); // Explicitly add an aspect List aspectNames = new ArrayList<>(); - aspectNames.add("cm:classifiable"); + aspectNames.add("papi:lunchable"); person.setAspectNames(aspectNames); // REST API call to create person Person retPerson = people.create(person); // Check that the response contains the expected aspects and properties - assertTrue(retPerson.getAspectNames().contains("cm:titled")); - assertTrue(retPerson.getAspectNames().contains("cm:classifiable")); - assertEquals("This is a title", retPerson.getProperties().get("cm:title")); + assertEquals(2, retPerson.getAspectNames().size()); + assertTrue(retPerson.getAspectNames().contains("papi:comms")); + assertEquals(1, retPerson.getProperties().size()); + assertEquals("724332b5796a8", retPerson.getProperties().get("papi:telehash")); // Get the NodeRef AuthenticationUtil.setFullyAuthenticatedUser("admin@"+account1.getId()); - PersonService personService = applicationContext.getBean("PersonService", PersonService.class); NodeRef nodeRef = personService.getPerson("jbloggs@"+account1.getId(), false); // Check the node has the properties and aspects we expect - NodeService nodeService = applicationContext.getBean("NodeService", NodeService.class); - assertTrue(nodeService.hasAspect(nodeRef, ContentModel.ASPECT_TITLED)); - assertTrue(nodeService.hasAspect(nodeRef, ContentModel.ASPECT_CLASSIFIABLE)); + assertTrue(nodeService.hasAspect(nodeRef, ASPECT_COMMS)); + assertTrue(nodeService.hasAspect(nodeRef, ASPECT_LUNCHABLE)); Map retProps = nodeService.getProperties(nodeRef); - assertEquals("This is a title", retProps.get(ContentModel.PROP_TITLE)); + assertEquals("724332b5796a8", retProps.get(PROP_TELEHASH)); + assertEquals(null, retProps.get(PROP_LUNCH)); } // Create a person for use in the testing of updating custom aspects/props @@ -650,21 +649,21 @@ public class TestPeople extends EnterpriseTestApi person.setEnabled(true); person.setPassword("password123"); person.setDescription("This is a very short bio."); - person.setProperties(Collections.singletonMap("cm:title", "Initial title")); - person.setAspectNames(Collections.singletonList("cm:projectsummary")); + person.setProperties(Collections.singletonMap("papi:jabber", "jbloggs@example.com")); + person.setAspectNames(Collections.singletonList("papi:dessertable")); person = people.create(person); AuthenticationUtil.setFullyAuthenticatedUser("admin@"+account1.getId()); - NodeService nodeService = applicationContext.getBean("NodeService", NodeService.class); - PersonService personService = applicationContext.getBean("PersonService", PersonService.class); NodeRef nodeRef = personService.getPerson(person.getId()); + // Add some non-custom aspects, these should be untouched by the people API. nodeService.addAspect(nodeRef, ContentModel.ASPECT_AUDITABLE, null); + nodeService.setProperty(nodeRef, ContentModel.PROP_TITLE, "This is a title"); - assertEquals("Initial title", person.getProperties().get("cm:title")); - assertTrue(person.getAspectNames().contains("cm:titled")); - assertTrue(person.getAspectNames().contains("cm:projectsummary")); - assertTrue(nodeService.hasAspect(nodeRef, ContentModel.ASPECT_AUDITABLE)); + assertEquals("jbloggs@example.com", person.getProperties().get("papi:jabber")); + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); return person; } @@ -676,89 +675,134 @@ public class TestPeople extends EnterpriseTestApi // Add a property { Person person = createTestUpdatePerson(); - assertNull(person.getProperties().get("cm:middleName")); + assertNull(person.getProperties().get("papi:lunch")); + assertFalse(person.getAspectNames().contains("papi:lunchable")); String json = qjson( "{" + " `properties`: {" + - " `cm:middleName`: `Bertrand`" + + " `papi:lunch`: `Tomato soup`" + " }" + "}" ); person = people.update(person.getId(), json, 200); // Property added - assertEquals("Bertrand", person.getProperties().get("cm:middleName")); - assertEquals("Initial title", person.getProperties().get("cm:title")); - // Aspect untouched - assertTrue(person.getAspectNames().contains("cm:titled")); + assertEquals("Tomato soup", person.getProperties().get("papi:lunch")); + assertTrue(person.getAspectNames().contains("papi:lunchable")); + // Aspects untouched + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); } // Simple update of properties { Person person = createTestUpdatePerson(); - person = people.update(person.getId(), qjson("{`properties`: {`cm:title`: `Updated title`}}"), 200); + person = people.update(person.getId(), qjson("{`properties`: {`papi:jabber`: `updated@example.com`}}"), 200); // Property updated - assertEquals("Updated title", person.getProperties().get("cm:title")); - // Aspect untouched - assertTrue(person.getAspectNames().contains("cm:titled")); + assertEquals("updated@example.com", person.getProperties().get("papi:jabber")); + // Aspects untouched + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); } // Update with zero aspects - clear them all, except for protected items. { Person person = createTestUpdatePerson(); + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); + person = people.update(person.getId(), qjson("{`aspectNames`: []}"), 200); - // Aspect should no longer be present. - assertFalse(person.getAspectNames().contains("cm:titled")); - assertFalse(person.getProperties().containsKey("cm:title")); - // Protected aspects should still be present. - List aspectNames = person.getAspectNames(); - assertTrue(aspectNames.contains("cm:auditable")); + // Aspects should no longer be present. + assertNull(person.getAspectNames()); // Check for the protected (but filtered) sys:* properties - NodeService nodeService = applicationContext.getBean("NodeService", NodeService.class); - PersonService personService = applicationContext.getBean("PersonService", PersonService.class); NodeRef nodeRef = personService.getPerson(person.getId()); Set aspects = nodeService.getAspects(nodeRef); assertTrue(aspects.contains(ContentModel.ASPECT_REFERENCEABLE)); assertTrue(aspects.contains(ContentModel.ASPECT_LOCALIZED)); } - // Set aspects - all except protected items will be replaced with those presented. + // Set aspects - all "custom" aspects will be replaced with those presented. { Person person = createTestUpdatePerson(); - String json = qjson( - "{" + - " `aspectNames`: [" + - " `cm:dublincore`," + - " `cm:summarizable`" + - " ]" + - "}" - ); - person = people.update(person.getId(), json, 200); - // Aspect should no longer be present. - assertFalse(person.getAspectNames().contains("cm:titled")); - assertFalse(person.getProperties().containsKey("cm:title")); - // Protected aspects should still be present. - List aspectNames = person.getAspectNames(); - assertTrue(aspectNames.contains("cm:auditable")); + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); - // Newly added aspects - assertTrue(aspectNames.contains("cm:dublincore")); - assertTrue(aspectNames.contains("cm:summarizable")); + String json = qjson("{ `aspectNames`: [`papi:lunchable`] }"); + person = people.update(person.getId(), json, 200); + + // Get the person's NodeRef + AuthenticationUtil.setFullyAuthenticatedUser("admin@"+account1.getId()); + NodeRef nodeRef = personService.getPerson(person.getId(), false); + // Aspects from non-custom models should still be present. + nodeService.hasAspect(nodeRef, ContentModel.ASPECT_AUDITABLE); + nodeService.hasAspect(nodeRef, ContentModel.ASPECT_TITLED); + + // Newly added aspect should be the only one exposed by the people API. + List aspectNames = person.getAspectNames(); + assertEquals(1, aspectNames.size()); + assertTrue(aspectNames.contains("papi:lunchable")); + assertNull(person.getProperties()); } // Remove a property by setting it to null { Person person = createTestUpdatePerson(); - person = people.update(person.getId(), qjson("{`properties`: {`cm:title`: null}}"), 200); - assertFalse(person.getProperties().containsKey("cm:title")); + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); + assertEquals(1, person.getProperties().size()); + assertTrue(person.getProperties().containsKey("papi:jabber")); + + person = people.update(person.getId(), qjson("{`properties`: {`papi:jabber`: null}}"), 200); + + // No properties == null + assertNull(person.getProperties()); // The aspect will still be there, I don't think we can easily remove the aspect automatically // just because the associated properties have all been removed. - assertTrue(person.getAspectNames().contains("cm:titled")); + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); + } + + // Cannot set built-in/non-custom props + { + Person person = createTestUpdatePerson(); + final String personId = person.getId(); + + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); + + String json = qjson("{ `properties`: {`usr:enabled`: false} }"); + people.update(person.getId(), json, 400); + + json = qjson("{ `properties`: {`cm:title`: `hello-world`} }"); + people.update(person.getId(), json, 400); + + json = qjson("{ `properties`: {`sys:locale`: `en_GB`} }"); + people.update(person.getId(), json, 400); + + // Get the person's NodeRef + AuthenticationUtil.setFullyAuthenticatedUser("admin@"+account1.getId()); + NodeRef nodeRef = personService.getPerson(person.getId(), false); + // Aspects from non-custom models should still be present. + nodeService.hasAspect(nodeRef, ContentModel.ASPECT_AUDITABLE); + nodeService.hasAspect(nodeRef, ContentModel.ASPECT_TITLED); + + // Custom aspects should be undisturbed + person = people.getPerson(personId); + assertEquals(2, person.getAspectNames().size()); + assertTrue(person.getAspectNames().contains("papi:comms")); + assertTrue(person.getAspectNames().contains("papi:dessertable")); + assertEquals("jbloggs@example.com", person.getProperties().get("papi:jabber")); } } @@ -1318,9 +1362,9 @@ public class TestPeople extends EnterpriseTestApi filter(p -> p.getUserName().equals("alice@"+account4.getId())) .findFirst().get(); assertNotNull(alice.getAspectNames()); - assertTrue(alice.getAspectNames().contains("cm:titled")); + assertTrue(alice.getAspectNames().contains("papi:lunchable")); assertNotNull(alice.getProperties()); - assertEquals("Alice through the REST API", alice.getProperties().get("cm:title")); + assertEquals("Magical sandwich", alice.getProperties().get("papi:lunch")); } } @@ -1523,7 +1567,7 @@ public class TestPeople extends EnterpriseTestApi personAlice.setEmail("alison.smith@example.com"); personAlice.setPassword("password"); personAlice.setEnabled(true); - personAlice.setProperties(Collections.singletonMap("cm:title", "Alice through the REST API")); + personAlice.setProperties(Collections.singletonMap("papi:lunch", "Magical sandwich")); people.create(personAlice); publicApiClient.setRequestContext(new RequestContext(account4.getId(), account4Admin, "admin")); diff --git a/source/test-resources/models/people-api.xml b/source/test-resources/models/people-api.xml new file mode 100644 index 0000000000..a449e79108 --- /dev/null +++ b/source/test-resources/models/people-api.xml @@ -0,0 +1,67 @@ + + + + + + + + + Custom model for testing the People API + Matt Ward + 1.0 + + + + + + + + + + + + + + + + + Custom Communications Channels + + + d:text + false + + + d:text + false + + + + + + + Favourite lunch + + + d:text + false + + + + + + + Favourite dessert + + + d:text + false + + + + + + \ No newline at end of file diff --git a/source/test-resources/rest-api-test-context.xml b/source/test-resources/rest-api-test-context.xml index 63869a1253..5c8d47f8cc 100644 --- a/source/test-resources/rest-api-test-context.xml +++ b/source/test-resources/rest-api-test-context.xml @@ -19,6 +19,7 @@ models/custom-model.xml models/bpmDelegateeModel.xml + models/people-api.xml