You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by en...@apache.org on 2010/02/24 05:44:43 UTC

svn commit: r915670 - in /sling/trunk: bundles/jcr/jackrabbit-accessmanager/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ launchpad/testing/src/test/java/org/apache/sling/launchpad/webapp/integrationtest/accessManager/

Author: enorman
Date: Wed Feb 24 04:44:43 2010
New Revision: 915670

URL: http://svn.apache.org/viewvc?rev=915670&view=rev
Log:
SLING-997 handle merges involving aggregate privileges properly

Modified:
    sling/trunk/bundles/jcr/jackrabbit-accessmanager/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
    sling/trunk/launchpad/testing/src/test/java/org/apache/sling/launchpad/webapp/integrationtest/accessManager/ModifyAceTest.java

Modified: sling/trunk/bundles/jcr/jackrabbit-accessmanager/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/jcr/jackrabbit-accessmanager/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java?rev=915670&r1=915669&r2=915670&view=diff
==============================================================================
--- sling/trunk/bundles/jcr/jackrabbit-accessmanager/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java (original)
+++ sling/trunk/bundles/jcr/jackrabbit-accessmanager/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java Wed Feb 24 04:44:43 2010
@@ -100,7 +100,6 @@
 	/* (non-Javadoc)
 	 * @see org.apache.sling.jackrabbit.accessmanager.post.AbstractAccessPostServlet#handleOperation(org.apache.sling.api.SlingHttpServletRequest, org.apache.sling.api.servlets.HtmlResponse, java.util.List)
 	 */
-	@SuppressWarnings("unchecked")
 	@Override
 	protected void handleOperation(SlingHttpServletRequest request,
 			HtmlResponse htmlResponse, List<Modification> changes)
@@ -133,12 +132,70 @@
     		}
     	}
 
+		try {
+			AccessControlManager accessControlManager = AccessControlUtil.getAccessControlManager(session);
 
-		List<String> grantedPrivilegeNames = new ArrayList<String>();
-		List<String> deniedPrivilegeNames = new ArrayList<String>();
-		//SLING-997: keep track of the privilege names that were posted, so the others can be preserved
-		Set<String> postedPrivilegeNames = new HashSet<String>();
-		Enumeration parameterNames = request.getParameterNames();
+			//SLING-997: keep track of the privilege names that were posted, so the others can be preserved
+			PrivilegesState privilegesInfo = new PrivilegesState(log.isDebugEnabled());
+			
+			//calculate which privileges were POSTed.
+			collectPostedPrivilegeNames(request, 
+					accessControlManager,
+					privilegesInfo);	
+
+			//find the existing ACL
+			AccessControlList updatedAcl = getAccessControlList(accessControlManager, resourcePath, true);
+
+			//keep track of the existing ACEs for the target principal
+			List<AccessControlEntry> oldAces = processExistingAccessControlEntries(resourcePath, 
+					authorizable, 
+					accessControlManager, 
+					updatedAcl, 
+					privilegesInfo); 
+			
+			//remove the old ACEs.  Re-created below.
+			if (!oldAces.isEmpty()) {
+				for (AccessControlEntry ace : oldAces) {
+					updatedAcl.removeAccessControlEntry(ace);
+				}
+			}
+			
+			//re-aggregate the granted/denied privileges into the best matched aggregate privilege
+			reaggregatePrivileges(resourcePath,
+					accessControlManager,
+					privilegesInfo);
+			
+			//add fresh ACEs with the granted privileges
+			buildFreshAccessControlEntries(authorizable, 
+					accessControlManager, 
+					updatedAcl, 
+					privilegesInfo);
+			
+			//store the updated ACL
+			accessControlManager.setPolicy(resourcePath, updatedAcl);
+			if (session.hasPendingChanges()) {
+				session.save();
+			}
+
+			if (log.isDebugEnabled()) {
+				log.debug("Updated ACE for principalId {0} for resource {1) from {2} to {3}", new Object [] {
+						authorizable.getID(), resourcePath, privilegesInfo.oldPrivileges.toString(), privilegesInfo.newPrivileges.toString()
+				});
+			}
+		} catch (RepositoryException re) {
+			throw new RepositoryException("Failed to create ace.", re);
+		}
+	}
+
+
+	/**
+	 * Collect the privileges to assign from the http request.
+	 */
+	private void collectPostedPrivilegeNames(SlingHttpServletRequest request,
+			AccessControlManager accessControlManager,
+			PrivilegesState privilegesInfo) throws RepositoryException {
+
+		Enumeration<?> parameterNames = request.getParameterNames();
 		while (parameterNames.hasMoreElements()) {
 			Object nextElement = parameterNames.nextElement();
 			if (nextElement instanceof String) {
@@ -146,144 +203,236 @@
 				if (paramName.startsWith("privilege@")) {
 					String privilegeName = paramName.substring(10);
 					//keep track of which privileges should be changed
-					postedPrivilegeNames.add(privilegeName); 
+					privilegesInfo.postedPrivilegeNames.add(privilegeName); 
 					
 					String parameterValue = request.getParameter(paramName);
 					if (parameterValue != null && parameterValue.length() > 0) {
 						if ("granted".equals(parameterValue)) {
-							grantedPrivilegeNames.add(privilegeName);
+							Privilege privilege = accessControlManager.privilegeFromName(privilegeName);
+							if (privilege.isAggregate()) {
+								Privilege[] aggregatePrivileges = privilege.getAggregatePrivileges();
+								for (Privilege privilege2 : aggregatePrivileges) {
+									privilegesInfo.grantedPrivilegeNames.add(privilege2.getName());
+								}
+							} else {
+								privilegesInfo.grantedPrivilegeNames.add(privilegeName);
+							}
 						} else if ("denied".equals(parameterValue)) {
-							deniedPrivilegeNames.add(privilegeName);
+							Privilege privilege = accessControlManager.privilegeFromName(privilegeName);
+							if (privilege.isAggregate()) {
+								Privilege[] aggregatePrivileges = privilege.getAggregatePrivileges();
+								for (Privilege privilege2 : aggregatePrivileges) {
+									privilegesInfo.deniedPrivilegeNames.add(privilege2.getName());
+								}
+							} else {
+								privilegesInfo.deniedPrivilegeNames.add(privilegeName);
+							}
 						}
 					}
 				}
 			}
 		}
+	}
+	
+	/**
+	 * Process the existing ACL to determine which privileges to preserve.
+	 */
+	private List<AccessControlEntry> processExistingAccessControlEntries(
+			String resourcePath,
+			Authorizable authorizable,
+			AccessControlManager accessControlManager,
+			AccessControlList updatedAcl,
+			PrivilegesState privilegesInfo) throws RepositoryException {
+		
+		String principalId = authorizable.getPrincipal().getName();
+		AccessControlEntry[] accessControlEntries = updatedAcl.getAccessControlEntries();
+		List<AccessControlEntry> oldAces = new ArrayList<AccessControlEntry>();
+		for (AccessControlEntry ace : accessControlEntries) {
+			if (principalId.equals(ace.getPrincipal().getName())) {
+				if (log.isDebugEnabled()) {
+					log.debug("Found Existing ACE for principal {0} on resource: ", new Object[] {principalId, resourcePath});
+				}
+				oldAces.add(ace);
 
-		try {
-			AccessControlManager accessControlManager = AccessControlUtil.getAccessControlManager(session);
-			AccessControlList updatedAcl = getAccessControlList(accessControlManager, resourcePath, true);
-
-			StringBuilder oldPrivileges = null;
-			StringBuilder newPrivileges = null;
-			if (log.isDebugEnabled()) {
-				oldPrivileges = new StringBuilder();
-				newPrivileges = new StringBuilder();
-			}
+				boolean isAllow = AccessControlUtil.isAllow(ace);
+				Privilege[] privileges = ace.getPrivileges();
 
-			List<Privilege> preserveGrantedPrivileges = new ArrayList<Privilege>();
-			List<Privilege> preserveDeniedPrivileges = new ArrayList<Privilege>();
-			
-			//keep track of the existing Aces for the target principal
-			AccessControlEntry[] accessControlEntries = updatedAcl.getAccessControlEntries();
-			List<AccessControlEntry> oldAces = new ArrayList<AccessControlEntry>();
-			for (AccessControlEntry ace : accessControlEntries) {
-				if (principalId.equals(ace.getPrincipal().getName())) {
-					if (log.isDebugEnabled()) {
-						log.debug("Found Existing ACE for principal {0} on resource: ", new Object[] {principalId, resourcePath});
+				//build a list of (merged and expanded) privileges 
+				Set<Privilege> mergedExistingPrivileges = new HashSet<Privilege>();
+				for (Privilege privilege : privileges) {
+					if (privilege.isAggregate()) {
+						Privilege[] aggregatePrivileges = privilege.getAggregatePrivileges();
+						for (Privilege privilege2 : aggregatePrivileges) {
+							mergedExistingPrivileges.add(privilege2);
+						}
+					} else {
+						mergedExistingPrivileges.add(privilege);
 					}
-					oldAces.add(ace);
-
-					boolean isAllow = AccessControlUtil.isAllow(ace);
-					Privilege[] privileges = ace.getPrivileges();
-					for (Privilege privilege : privileges) {
-						String privilegeName = privilege.getName();
-						if (!postedPrivilegeNames.contains(privilegeName)) {
-							//this privilege was not posted, so record the existing state to be 
-							// preserved when the ACE is re-created below
-							if (isAllow) {
-								preserveGrantedPrivileges.add(privilege);
-							} else {
-								preserveDeniedPrivileges.add(privilege);
-							}
+				}
+				
+				//now process the merged privileges set
+				for (Privilege privilege : mergedExistingPrivileges) {
+					String privilegeName = privilege.getName();
+					if (!privilegesInfo.postedPrivilegeNames.contains(privilegeName)) {
+						//this privilege was not posted, so record the existing state to be 
+						// preserved when the ACE is re-created below
+						if (isAllow) {
+							privilegesInfo.grantedPrivilegeNames.add(privilegeName);
+						} else {
+							privilegesInfo.deniedPrivilegeNames.add(privilegeName);
 						}
+					}
 
-						if (log.isDebugEnabled()) {
-							//collect the information for debug logging
-							if (oldPrivileges.length() > 0) {
-								oldPrivileges.append(", "); //separate entries by commas
-							}
+					if (log.isDebugEnabled()) {
+						//collect the information for debug logging
+						if (privilegesInfo.oldPrivileges.length() > 0) {
+							privilegesInfo.oldPrivileges.append(", "); //separate entries by commas
+						}
+						if (isAllow) {
+							privilegesInfo.oldPrivileges.append("granted=");
+						} else {
+							privilegesInfo.oldPrivileges.append("denied=");
 						}
+						privilegesInfo.oldPrivileges.append(privilege.getName());
 					}
 				}
 			}
+		}
+		
+		return oldAces;
+	}
+	
+	/**
+	 * Given the set of granted/denied privileges, try to combine them
+	 * into the best aggregate Privilege that contains them all.
+	 */
+	private void reaggregatePrivileges(
+			String resourcePath,
+			AccessControlManager accessControlManager,
+			PrivilegesState privilegesInfo) throws RepositoryException {
+		Privilege[] supportedPrivileges = accessControlManager.getSupportedPrivileges(resourcePath);
+		for (Privilege privilege : supportedPrivileges) {
+			if (privilege.isAggregate()) {
+				boolean grantedAggregatePrivilege = true;
+				boolean deniedAggregatePrivilege = true;
+				Privilege[] aggregatePrivileges = privilege.getAggregatePrivileges();
+				for (Privilege privilege2 : aggregatePrivileges) {
+					String name = privilege2.getName();
+					if (!privilegesInfo.grantedPrivilegeNames.contains(name)) {
+						grantedAggregatePrivilege = false;
+					}
+					if (!privilegesInfo.deniedPrivilegeNames.contains(name)) {
+						deniedAggregatePrivilege = false;
+					}
+				}
+				
+				if (grantedAggregatePrivilege) {
+					//add the aggregate privilege and remove the containing parts
+					privilegesInfo.grantedPrivilegeNames.add(privilege.getName());
+					for (Privilege privilege2 : aggregatePrivileges) {
+						String name = privilege2.getName();
+						privilegesInfo.grantedPrivilegeNames.remove(name);
+					}						
+				}
 
-			//remove the old aces
-			if (!oldAces.isEmpty()) {
-				for (AccessControlEntry ace : oldAces) {
-					updatedAcl.removeAccessControlEntry(ace);
+				if (deniedAggregatePrivilege) {
+					//add the aggregate privilege and remove the containing parts
+					privilegesInfo.deniedPrivilegeNames.add(privilege.getName());
+					for (Privilege privilege2 : aggregatePrivileges) {
+						String name = privilege2.getName();
+						privilegesInfo.deniedPrivilegeNames.remove(name);
+					}						
 				}
 			}
+		}
+	}
+	
+	/**
+	 * Create new ACE for the granted and denied privileges.
+	 */
+	private void buildFreshAccessControlEntries(
+			Authorizable authorizable,
+			AccessControlManager accessControlManager,			
+			AccessControlList updatedAcl,
+			PrivilegesState privilegesInfo) throws RepositoryException {
+		List<Privilege> grantedPrivilegeList = new ArrayList<Privilege>();
+		for (String name : privilegesInfo.grantedPrivilegeNames) {
+			if (name.length() == 0) {
+				continue; //empty, skip it.
+			}
+			Privilege privilege = accessControlManager.privilegeFromName(name);
+			grantedPrivilegeList.add(privilege);
+		}
+		
+		if (log.isDebugEnabled()) {
+			for (Privilege privilege : grantedPrivilegeList) {
+				if (privilegesInfo.newPrivileges.length() > 0) {
+					privilegesInfo.newPrivileges.append(", "); //separate entries by commas
+				}
+				privilegesInfo.newPrivileges.append("granted=");
+				privilegesInfo.newPrivileges.append(privilege.getName());
+			}				
+		}
+
+		if (grantedPrivilegeList.size() > 0) {
+			Principal principal = authorizable.getPrincipal();
+			updatedAcl.addAccessControlEntry(principal, grantedPrivilegeList.toArray(new Privilege[grantedPrivilegeList.size()]));
+		}
 
-			//add a fresh ACE with the granted privileges
-			List<Privilege> grantedPrivilegeList = new ArrayList<Privilege>();
-			for (String name : grantedPrivilegeNames) {
+		//if the authorizable is a user (not a group) process any denied privileges
+		if (!authorizable.isGroup()) {
+			//add a fresh ACE with the denied privileges
+			List<Privilege> deniedPrivilegeList = new ArrayList<Privilege>();
+			for (String name : privilegesInfo.deniedPrivilegeNames) {
 				if (name.length() == 0) {
 					continue; //empty, skip it.
 				}
 				Privilege privilege = accessControlManager.privilegeFromName(name);
-				grantedPrivilegeList.add(privilege);
+				deniedPrivilegeList.add(privilege);
 			}
-			//add the privileges that should be preserved
-			grantedPrivilegeList.addAll(preserveGrantedPrivileges);
-
+			
 			if (log.isDebugEnabled()) {
-				for (Privilege privilege : grantedPrivilegeList) {
-					if (newPrivileges.length() > 0) {
-						newPrivileges.append(", "); //separate entries by commas
-					}
-					newPrivileges.append("granted=");
-					newPrivileges.append(privilege.getName());
-				}				
-			}
-
-			if (grantedPrivilegeList.size() > 0) {
-				Principal principal = authorizable.getPrincipal();
-				updatedAcl.addAccessControlEntry(principal, grantedPrivilegeList.toArray(new Privilege[grantedPrivilegeList.size()]));
-			}
-
-			//if the authorizable is a user (not a group) process any denied privileges
-			if (!authorizable.isGroup()) {
-				//add a fresh ACE with the denied privileges
-				List<Privilege> deniedPrivilegeList = new ArrayList<Privilege>();
-				for (String name : deniedPrivilegeNames) {
-					if (name.length() == 0) {
-						continue; //empty, skip it.
-					}
-					Privilege privilege = accessControlManager.privilegeFromName(name);
-					deniedPrivilegeList.add(privilege);
-				}
-				//add the privileges that should be preserved
-				deniedPrivilegeList.addAll(preserveDeniedPrivileges);
-				
-				if (log.isDebugEnabled()) {
-					for (Privilege privilege : deniedPrivilegeList) {
-						if (newPrivileges.length() > 0) {
-							newPrivileges.append(", "); //separate entries by commas
-						}
-						newPrivileges.append("denied=");
-						newPrivileges.append(privilege.getName());
+				for (Privilege privilege : deniedPrivilegeList) {
+					if (privilegesInfo.newPrivileges.length() > 0) {
+						privilegesInfo.newPrivileges.append(", "); //separate entries by commas
 					}
-				}
-				
-				if (deniedPrivilegeList.size() > 0) {
-					Principal principal = authorizable.getPrincipal();
-					AccessControlUtil.addEntry(updatedAcl, principal, deniedPrivilegeList.toArray(new Privilege[deniedPrivilegeList.size()]), false);
+					privilegesInfo.newPrivileges.append("denied=");
+					privilegesInfo.newPrivileges.append(privilege.getName());
 				}
 			}
-
-			accessControlManager.setPolicy(resourcePath, updatedAcl);
-			if (session.hasPendingChanges()) {
-				session.save();
+			
+			if (deniedPrivilegeList.size() > 0) {
+				Principal principal = authorizable.getPrincipal();
+				AccessControlUtil.addEntry(updatedAcl, principal, deniedPrivilegeList.toArray(new Privilege[deniedPrivilegeList.size()]), false);
 			}
+		}
+	}
+	
+	/**
+	 * Contains the information about the privilege state as it 
+	 * progresses through the update process. 
+	 */
+	private static class PrivilegesState {
+		//stores the names of the privileges that were POSTed
+		Set<String> postedPrivilegeNames = new HashSet<String>();
+		
+		//stores the names of the privileges to be granted 
+		Set<String> grantedPrivilegeNames = new HashSet<String>();
+
+		//stores the names of the privileges to be denied 
+		Set<String> deniedPrivilegeNames = new HashSet<String>();
+
+		//collects debug information about the previous privileges
+		StringBuilder oldPrivileges = null;
+		
+		//collects debug information about the new privileges
+		StringBuilder newPrivileges = null;
 
-			if (log.isDebugEnabled()) {
-				log.debug("Updated ACE for principalId {0} for resource {1) from {2} to {3}", new Object [] {
-						authorizable.getID(), resourcePath, oldPrivileges.toString(), newPrivileges.toString()
-				});
+		public PrivilegesState(boolean isDebugEnabled) {
+			if (isDebugEnabled) {
+				oldPrivileges = new StringBuilder();
+				newPrivileges = new StringBuilder();
 			}
-		} catch (RepositoryException re) {
-			throw new RepositoryException("Failed to create ace.", re);
 		}
 	}
 }

Modified: sling/trunk/launchpad/testing/src/test/java/org/apache/sling/launchpad/webapp/integrationtest/accessManager/ModifyAceTest.java
URL: http://svn.apache.org/viewvc/sling/trunk/launchpad/testing/src/test/java/org/apache/sling/launchpad/webapp/integrationtest/accessManager/ModifyAceTest.java?rev=915670&r1=915669&r2=915670&view=diff
==============================================================================
--- sling/trunk/launchpad/testing/src/test/java/org/apache/sling/launchpad/webapp/integrationtest/accessManager/ModifyAceTest.java (original)
+++ sling/trunk/launchpad/testing/src/test/java/org/apache/sling/launchpad/webapp/integrationtest/accessManager/ModifyAceTest.java Wed Feb 24 04:44:43 2010
@@ -243,5 +243,197 @@
 		assertTrue(deniedPrivilegeNames2.contains("jcr:modifyAccessControl"));
 		assertTrue(deniedPrivilegeNames2.contains("jcr:removeNode"));
 	}
+
+	
+	/**
+	 * Test for SLING-997, preserve privileges that were not posted with the modifyAce 
+	 * request.
+	 */
+	public void testMergeAceForUserSplitAggregatePrincipal() throws IOException, JSONException {
+		testUserId = createTestUser();
+		testFolderUrl = createTestFolder();
+		
+        String postUrl = testFolderUrl + ".modifyAce.html";
+
+        //1. create an initial set of privileges
+		List<NameValuePair> postParams = new ArrayList<NameValuePair>();
+		postParams.add(new NameValuePair("principalId", testUserId));
+		postParams.add(new NameValuePair("privilege@jcr:read", "granted"));
+		postParams.add(new NameValuePair("privilege@jcr:write", "denied"));
+		
+		Credentials creds = new UsernamePasswordCredentials("admin", "admin");
+		assertAuthenticatedPostStatus(creds, postUrl, HttpServletResponse.SC_OK, postParams, null);
+		
+		//fetch the JSON for the acl to verify the settings.
+		String getUrl = testFolderUrl + ".acl.json";
+
+		String json = getAuthenticatedContent(creds, getUrl, CONTENT_TYPE_JSON, null, HttpServletResponse.SC_OK);
+		assertNotNull(json);
+		JSONObject jsonObj = new JSONObject(json);
+		String aceString = jsonObj.getString(testUserId);
+		assertNotNull(aceString);
+		
+		JSONObject aceObject = new JSONObject(aceString); 
+		assertNotNull(aceObject);
+		
+		JSONArray grantedArray = aceObject.getJSONArray("granted");
+		assertNotNull(grantedArray);
+		assertEquals(1, grantedArray.length());
+		Set<String> grantedPrivilegeNames = new HashSet<String>();
+		for (int i=0; i < grantedArray.length(); i++) {
+			grantedPrivilegeNames.add(grantedArray.getString(i));
+		}
+		assertTrue(grantedPrivilegeNames.contains("jcr:read"));
+
+		JSONArray deniedArray = aceObject.getJSONArray("denied");
+		assertNotNull(deniedArray);
+		assertEquals(1, deniedArray.length());
+		Set<String> deniedPrivilegeNames = new HashSet<String>();
+		for (int i=0; i < deniedArray.length(); i++) {
+			deniedPrivilegeNames.add(deniedArray.getString(i));
+		}
+		assertTrue(deniedPrivilegeNames.contains("jcr:write"));
+		
+		
+		
+        //2. post a new set of privileges to merge with the existing privileges
+		List<NameValuePair> postParams2 = new ArrayList<NameValuePair>();
+		postParams2.add(new NameValuePair("principalId", testUserId));
+		//jcr:read is not posted, so it should remain in the granted ACE
+		postParams2.add(new NameValuePair("privilege@jcr:modifyProperties", "granted")); //add a new privilege
+		//jcr:write is not posted, but one of the aggregate privileges is now granted, so the aggregate priviledge should be disagreaged into
+		//  the remaining denied privileges in the denied ACE
+		
+		assertAuthenticatedPostStatus(creds, postUrl, HttpServletResponse.SC_OK, postParams2, null);
+		
+		
+		//fetch the JSON for the acl to verify the settings.
+		String json2 = getAuthenticatedContent(creds, getUrl, CONTENT_TYPE_JSON, null, HttpServletResponse.SC_OK);
+		
+		assertNotNull(json2);
+		JSONObject jsonObj2 = new JSONObject(json2);
+		String aceString2 = jsonObj2.getString(testUserId);
+		assertNotNull(aceString2);
+		
+		JSONObject aceObject2 = new JSONObject(aceString2); 
+		assertNotNull(aceObject2);
+		
+		JSONArray grantedArray2 = aceObject2.getJSONArray("granted");
+		assertNotNull(grantedArray2);
+		assertEquals(2, grantedArray2.length());
+		Set<String> grantedPrivilegeNames2 = new HashSet<String>();
+		for (int i=0; i < grantedArray2.length(); i++) {
+			grantedPrivilegeNames2.add(grantedArray2.getString(i));
+		}
+		assertTrue(grantedPrivilegeNames2.contains("jcr:read"));
+		assertTrue(grantedPrivilegeNames2.contains("jcr:modifyProperties"));
+
+		JSONArray deniedArray2 = aceObject2.getJSONArray("denied");
+		assertNotNull(deniedArray2);
+		assertEquals(3, deniedArray2.length());
+		Set<String> deniedPrivilegeNames2 = new HashSet<String>();
+		for (int i=0; i < deniedArray2.length(); i++) {
+			deniedPrivilegeNames2.add(deniedArray2.getString(i));
+		}
+		assertFalse(deniedPrivilegeNames2.contains("jcr:write"));
+		//only the remaining privileges from the disaggregated jcr:write collection should remain.
+		assertTrue(deniedPrivilegeNames2.contains("jcr:addChildNodes"));
+		assertTrue(deniedPrivilegeNames2.contains("jcr:removeNode"));
+		assertTrue(deniedPrivilegeNames2.contains("jcr:removeChildNodes"));
+	}
+
+	/**
+	 * Test for SLING-997, preserve privileges that were not posted with the modifyAce 
+	 * request.
+	 */
+	public void testMergeAceForUserCombineAggregatePrincipal() throws IOException, JSONException {
+		testUserId = createTestUser();
+		testFolderUrl = createTestFolder();
+		
+        String postUrl = testFolderUrl + ".modifyAce.html";
+
+        //1. create an initial set of privileges
+		List<NameValuePair> postParams = new ArrayList<NameValuePair>();
+		postParams.add(new NameValuePair("principalId", testUserId));
+		postParams.add(new NameValuePair("privilege@jcr:read", "granted"));
+		postParams.add(new NameValuePair("privilege@jcr:removeNode", "denied"));
+		
+		Credentials creds = new UsernamePasswordCredentials("admin", "admin");
+		assertAuthenticatedPostStatus(creds, postUrl, HttpServletResponse.SC_OK, postParams, null);
+		
+		//fetch the JSON for the acl to verify the settings.
+		String getUrl = testFolderUrl + ".acl.json";
+
+		String json = getAuthenticatedContent(creds, getUrl, CONTENT_TYPE_JSON, null, HttpServletResponse.SC_OK);
+		assertNotNull(json);
+		JSONObject jsonObj = new JSONObject(json);
+		String aceString = jsonObj.getString(testUserId);
+		assertNotNull(aceString);
+		
+		JSONObject aceObject = new JSONObject(aceString); 
+		assertNotNull(aceObject);
+		
+		JSONArray grantedArray = aceObject.getJSONArray("granted");
+		assertNotNull(grantedArray);
+		assertEquals(1, grantedArray.length());
+		Set<String> grantedPrivilegeNames = new HashSet<String>();
+		for (int i=0; i < grantedArray.length(); i++) {
+			grantedPrivilegeNames.add(grantedArray.getString(i));
+		}
+		assertTrue(grantedPrivilegeNames.contains("jcr:read"));
+
+		JSONArray deniedArray = aceObject.getJSONArray("denied");
+		assertNotNull(deniedArray);
+		assertEquals(1, deniedArray.length());
+		Set<String> deniedPrivilegeNames = new HashSet<String>();
+		for (int i=0; i < deniedArray.length(); i++) {
+			deniedPrivilegeNames.add(deniedArray.getString(i));
+		}
+		assertTrue(deniedPrivilegeNames.contains("jcr:removeNode"));
+		
+		
+		
+        //2. post a new set of privileges to merge with the existing privileges
+		List<NameValuePair> postParams2 = new ArrayList<NameValuePair>();
+		postParams2.add(new NameValuePair("principalId", testUserId));
+		//jcr:read is not posted, so it should remain in the granted ACE
+		
+		//post the remaining privileges that when combined, correspond to the jcr:write aggregate privilege
+		postParams2.add(new NameValuePair("privilege@jcr:addChildNodes", "denied")); //add a new privilege
+		postParams2.add(new NameValuePair("privilege@jcr:removeChildNodes", "denied")); //add a new privilege
+		postParams2.add(new NameValuePair("privilege@jcr:modifyProperties", "denied")); //add a new privilege
+		
+		assertAuthenticatedPostStatus(creds, postUrl, HttpServletResponse.SC_OK, postParams2, null);
+		
+		
+		//fetch the JSON for the acl to verify the settings.
+		String json2 = getAuthenticatedContent(creds, getUrl, CONTENT_TYPE_JSON, null, HttpServletResponse.SC_OK);
+		
+		assertNotNull(json2);
+		JSONObject jsonObj2 = new JSONObject(json2);
+		String aceString2 = jsonObj2.getString(testUserId);
+		assertNotNull(aceString2);
+		
+		JSONObject aceObject2 = new JSONObject(aceString2); 
+		assertNotNull(aceObject2);
+		
+		JSONArray grantedArray2 = aceObject2.getJSONArray("granted");
+		assertNotNull(grantedArray2);
+		assertEquals(1, grantedArray2.length());
+		Set<String> grantedPrivilegeNames2 = new HashSet<String>();
+		for (int i=0; i < grantedArray2.length(); i++) {
+			grantedPrivilegeNames2.add(grantedArray2.getString(i));
+		}
+		assertTrue(grantedPrivilegeNames2.contains("jcr:read"));
+
+		JSONArray deniedArray2 = aceObject2.getJSONArray("denied");
+		assertNotNull(deniedArray2);
+		assertEquals(1, deniedArray2.length());
+		Set<String> deniedPrivilegeNames2 = new HashSet<String>();
+		for (int i=0; i < deniedArray2.length(); i++) {
+			deniedPrivilegeNames2.add(deniedArray2.getString(i));
+		}
+		assertTrue(deniedPrivilegeNames2.contains("jcr:write"));
+	}
 	
 }