{"action":"created","issue":{"url":"https://api.github.com/repos/spring-projects/spring-security/issues/851","repository_url":"https://api.github.com/repos/spring-projects/spring-security","labels_url":"https://api.github.com/repos/spring-projects/spring-security/issues/851/labels{/name}","comments_url":"https://api.github.com/repos/spring-projects/spring-security/issues/851/comments","events_url":"https://api.github.com/repos/spring-projects/spring-security/issues/851/events","html_url":"https://github.com/spring-projects/spring-security/issues/851","id":131767013,"number":851,"title":"SEC-590: BasicLookupStrategy#readAclsById(ObjectIdentity[], Sid[]) has a loop that sometimes terminates prematurely","user":{"login":"spring-issuemaster","id":16028288,"avatar_url":"https://avatars.githubusercontent.com/u/16028288?v=3","gravatar_id":"","url":"https://api.github.com/users/spring-issuemaster","html_url":"https://github.com/spring-issuemaster","followers_url":"https://api.github.com/users/spring-issuemaster/followers","following_url":"https://api.github.com/users/spring-issuemaster/following{/other_user}","gists_url":"https://api.github.com/users/spring-issuemaster/gists{/gist_id}","starred_url":"https://api.github.com/users/spring-issuemaster/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/spring-issuemaster/subscriptions","organizations_url":"https://api.github.com/users/spring-issuemaster/orgs","repos_url":"https://api.github.com/users/spring-issuemaster/repos","events_url":"https://api.github.com/users/spring-issuemaster/events{/privacy}","received_events_url":"https://api.github.com/users/spring-issuemaster/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/spring-projects/spring-security/labels/ACLs","name":"ACLs","color":"000000"},{"url":"https://api.github.com/repos/spring-projects/spring-security/labels/Bug","name":"Bug","color":"eeeeee"},{"url":"https://api.github.com/repos/spring-projects/spring-security/labels/Closed","name":"Closed","color":"ededed"},{"url":"https://api.github.com/repos/spring-projects/spring-security/labels/Fixed","name":"Fixed","color":"ededed"},{"url":"https://api.github.com/repos/spring-projects/spring-security/labels/Jira","name":"Jira","color":"ededed"}],"state":"closed","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/spring-projects/spring-security/milestones/17","html_url":"https://github.com/spring-projects/spring-security/milestones/2.0.0","labels_url":"https://api.github.com/repos/spring-projects/spring-security/milestones/17/labels","id":1567181,"number":17,"title":"2.0.0","description":null,"creator":{"login":"spring-issuemaster","id":16028288,"avatar_url":"https://avatars.githubusercontent.com/u/16028288?v=3","gravatar_id":"","url":"https://api.github.com/users/spring-issuemaster","html_url":"https://github.com/spring-issuemaster","followers_url":"https://api.github.com/users/spring-issuemaster/followers","following_url":"https://api.github.com/users/spring-issuemaster/following{/other_user}","gists_url":"https://api.github.com/users/spring-issuemaster/gists{/gist_id}","starred_url":"https://api.github.com/users/spring-issuemaster/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/spring-issuemaster/subscriptions","organizations_url":"https://api.github.com/users/spring-issuemaster/orgs","repos_url":"https://api.github.com/users/spring-issuemaster/repos","events_url":"https://api.github.com/users/spring-issuemaster/events{/privacy}","received_events_url":"https://api.github.com/users/spring-issuemaster/received_events","type":"User","site_admin":false},"open_issues":0,"closed_issues":29,"state":"closed","created_at":"2016-02-05T21:45:43Z","updated_at":"2016-02-05T22:17:17Z","due_on":"2008-04-13T00:00:00Z","closed_at":"2016-02-05T21:45:43Z"},"comments":5,"created_at":"2007-11-05T14:17:19Z","updated_at":"2016-02-06T05:18:48Z","closed_at":"2009-10-09T06:57:32Z","body":"[\"Simon van der Sluis\":https://jira.spring.io/secure/ViewProfile.jspa?name=svanders] (Migrated from [\"SEC-590\":https://jira.spring.io/browse/SEC-590]) said:\n\nThe main for loop in \norg.acegisecurity.acls.jdbc.BasicLookupStrategy#readAclsById(ObjectIdentity[], Sid[])  \nuses 'continue;' when an Acl is either found to be already in the results, or is found in the cache.  But the section of code that looks up batches Acls from the database is in the loop, so if the last ObjectIdentity in the in the objects array is found in the results or cache, the loop will terminate without looking up the last batch of Acls from the database.\n\nI've written a unit test to prove this, it should be added to org.acegisecurity.acls.jdbc.LookupStrategyTest\n\n/**\n   * A test specifically for a bug, where if the LAST {@link ObjectIdentity} in \n   * the array of of Oids we are getting ACL's for is in the cache, the \n   * collection of {@link ObjectIdentity} being collected for a bulk database\n   * lookup will not be processed due to some inapropriate <code>Continue</code>\n   * statements in the loop. \n   * \n   * @throws Exception\n   */\n  public void testReadLastIsCachedBugFix() throws Exception\n  {\n    ObjectIdentity grandParentOid = new ObjectIdentityImpl(\"org.acegisecurity.TargetObject\", new Long(104));\n    ObjectIdentity parent1Oid = new ObjectIdentityImpl(\"org.acegisecurity.TargetObject\", new Long(105));\n    ObjectIdentity parent2Oid = new ObjectIdentityImpl(\"org.acegisecurity.TargetObject\", new Long(106));\n    ObjectIdentity childOid = new ObjectIdentityImpl(\"org.acegisecurity.TargetObject\", new Long(107));\n        \n    MutableAcl grandParentAcl = jdbcMutableAclService.createAcl(grandParentOid);\n    MutableAcl parent1Acl = jdbcMutableAclService.createAcl(parent1Oid);\n    MutableAcl parent2Acl = jdbcMutableAclService.createAcl(parent2Oid);\n    MutableAcl childAcl = jdbcMutableAclService.createAcl(childOid);\n\n    // Specify the inheritence hierarchy\n    parent1Acl.setParent(grandParentAcl);\n    parent2Acl.setParent(grandParentAcl);\n    childAcl.setParent(parent1Acl);\n\n    // Now let's add a couple of permissions\n    grandParentAcl.insertAce(null, BasePermission.READ, new PrincipalSid(auth), true);\n    \n    // Explictly save the changed ACL\n    jdbcMutableAclService.updateAcl(grandParentAcl);\n    jdbcMutableAclService.updateAcl(parent1Acl);\n    jdbcMutableAclService.updateAcl(parent2Acl);\n    jdbcMutableAclService.updateAcl(childAcl);\n    \n    // fluch cache to make sure we they are retieved from DB\n    cache.removeAll();\n    \n    // first lookup only child\n    Permission[] checkPermision = new Permission[] {BasePermission.READ};\n    Sid[] sids = new Sid[] { new PrincipalSid(auth)};\n    ObjectIdentity[] childOids = new ObjectIdentity[] {childOid};\n    Map foundAcls = lookupStrategy.readAclsById(childOids, sids);\n\n    Acl foundChildAcl = (Acl) foundAcls.get(childOid);\n    assertNotNull(foundChildAcl);\n    assertTrue(foundChildAcl.isGranted(checkPermision, sids, false));\n    \n    // we now expect that the acls for child, parent1 and grandparent are cached\n    // if the bug is present and we now search for all for acls, and make sure\n    // that batchsize is greater than 4, and that parent2 is not the last in the \n    // oid array (it's the only one not cached), then having an aleady cached\n    // ACL as the last in the oid array to lookup, will cause the BUG we are \n    // for testing to skip the DB lookup when the last entry to check is \n    // retireved from the cache.\n    ObjectIdentity[] allOids = new ObjectIdentity[] {grandParentOid, parent1Oid, parent2Oid, childOid};\n    foundAcls = lookupStrategy.readAclsById(allOids, sids);\n\n    Acl foundParent2Acl = (Acl) foundAcls.get(parent2Oid);\n    assertNotNull(foundParent2Acl);\n    assertTrue(foundParent2Acl.isGranted(checkPermision, sids, false));\n }\n\n\nAnd rewritten org.acegisecurity.acls.jdbc.BasicLookupStrategy#readAclsById(ObjectIdentity[], Sid[])  to fix it\n\n/**\n     * The main method.<p>WARNING: This implementation completely disregards the \"sids\" parameter! Every item\n     * in the cache is expected to contain all SIDs. If you have serious performance needs (eg a very large number of\n     * SIDs per object identity), you'll probably want to develop a custom {@link LookupStrategy} implementation\n     * instead.</p>\n     *  <p>The implementation works in batch sizes specfied by {@link #batchSize}.</p>\n     *\n     * @param objects DOCUMENT ME!\n     * @param sids DOCUMENT ME!\n     *\n     * @return DOCUMENT ME!\n     *\n     * @throws NotFoundException DOCUMENT ME!\n     * @throws IllegalStateException DOCUMENT ME!\n     */\n    public Map readAclsById(ObjectIdentity[] objects, Sid[] sids)\n        throws NotFoundException {\n        Assert.isTrue(batchSize >= 1, \"BatchSize must be >= 1\");\n        Assert.notEmpty(objects, \"Objects to lookup required\");\n\n        // Map<ObjectIdentity,Acl>\n        Map result = new HashMap(); // contains FULLY loaded Acl objects\n\n        Set currentBatchToLoad = new HashSet(); // contains ObjectIdentitys\n\n        for (int i = 0; i < objects.length; i++) {\n            \n            // flag to record if we have found the acl for this iteration\n            boolean aclFound = false;\n          \n            // 1.) Check we don't already have this ACL in the results\n            if (result.containsKey(objects[i])) {\n                aclFound = true; // flag as found\n            }\n\n            // 2.) Check cache for the present ACL entry\n            if (!aclFound) {\n              Acl acl = aclCache.getFromCache(objects[i]);\n  \n              // Ensure any cached element supports all the requested SIDs\n              // (they should always, as our base impl doesn't filter on SID)\n              if (acl != null) {\n                  if (acl.isSidLoaded(sids)) {\n                      result.put(acl.getObjectIdentity(), acl);\n                      aclFound = true; // flag as found\n                  } else {\n                      throw new IllegalStateException(\n                          \"Error: SID-filtered element detected when implementation does not perform SID filtering - have you added something to the cache manually?\");\n                  }\n              }\n            }\n\n            // 3. Load the Acl from the database\n            if (!aclFound) {\n              currentBatchToLoad.add(objects[i]);\n            }\n\n            // Is it time to load from JDBC the currentBatchToLoad?\n            if ((currentBatchToLoad.size() == this.batchSize) || ((i + 1) == objects.length)) {\n              \n                if (currentBatchToLoad.size() > 0) {\n                  \n                    Map loadedBatch = lookupObjectIdentities((ObjectIdentity[]) currentBatchToLoad.toArray(\n                                new ObjectIdentity[] {}), sids);\n    \n                    // Add loaded batch (all elements 100% initialized) to results\n                    result.putAll(loadedBatch);\n    \n                    // Add the loaded batch to the cache\n                    Iterator loadedAclIterator = loadedBatch.values().iterator();\n    \n                    while (loadedAclIterator.hasNext()) {\n                        aclCache.putInCache((AclImpl) loadedAclIterator.next());\n                    }\n                }\n                currentBatchToLoad.clear();\n            }\n        }\n\n        // Now we're done, check every requested object identity was found \n        // and log it.\n        if (LOG.isWarnEnabled()) {\n          for (int i = 0; i < objects.length; i++) {\n              if (!result.containsKey(objects[i])) {\n                  LOG.warn(\"Unable to find ACL information for object identity '\"\n                      + objects[i].toString() + \"'\");\n              }\n          }\n        }\n        \n        return result;\n    }\n\n\nNote: This should probably be added to \nhttp://opensource.atlassian.com/projects/spring/browse/SEC-532\n\nNOTE: 2\nThis should be fixed after \nhttp://opensource.atlassian.com/projects/spring/browse/SEC-547\n(included code contains fix for 547 and 589 also)\n\nNOTE: 3 This bug was very difficult to replicate in our application, because it is dependent on the last ObjectIdentity being found in the cache AND there being a batch or part batch of Acls to be looked up.\n"},"comment":{"url":"https://api.github.com/repos/spring-projects/spring-security/issues/comments/180687288","html_url":"https://github.com/spring-projects/spring-security/issues/851#issuecomment-180687288","issue_url":"https://api.github.com/repos/spring-projects/spring-security/issues/851","id":180687288,"user":{"login":"spring-issuemaster","id":16028288,"avatar_url":"https://avatars.githubusercontent.com/u/16028288?v=3","gravatar_id":"","url":"https://api.github.com/users/spring-issuemaster","html_url":"https://github.com/spring-issuemaster","followers_url":"https://api.github.com/users/spring-issuemaster/followers","following_url":"https://api.github.com/users/spring-issuemaster/following{/other_user}","gists_url":"https://api.github.com/users/spring-issuemaster/gists{/gist_id}","starred_url":"https://api.github.com/users/spring-issuemaster/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/spring-issuemaster/subscriptions","organizations_url":"https://api.github.com/users/spring-issuemaster/orgs","repos_url":"https://api.github.com/users/spring-issuemaster/repos","events_url":"https://api.github.com/users/spring-issuemaster/events{/privacy}","received_events_url":"https://api.github.com/users/spring-issuemaster/received_events","type":"User","site_admin":false},"created_at":"2016-02-06T05:18:48Z","updated_at":"2016-02-06T05:18:48Z","body":"\n\nThis issue depends on #807\nThis issue depends on #850"}}