Deleting projects makes getNextAvailableProjectId reuse ids that still have ACL rows, which breaks all authoring for non-admin users

[Isaries]

Summary

ProjectServiceImpl.getNextAvailableProjectId() can return an id that already has
an acl_object_identity row. When it does, setupNewProject attaches the new
project to a pre-existing ACL owned by someone else, and the new owner fails the
owner check inside AclImpl.insertAce. Every non-admin author action then fails
with:

org.springframework.security.acls.model.NotFoundException:
Unable to locate a matching ACE for passed permissions and SIDs
  at org.springframework.security.acls.domain.DefaultPermissionGrantingStrategy.isGranted
  at org.springframework.security.acls.domain.AclImpl.isGranted
  at org.springframework.security.acls.domain.AclAuthorizationStrategyImpl.securityCheck
  at org.springframework.security.acls.domain.AclImpl.insertAce
  at org.wise.portal.service.acl.impl.AclServiceImpl.addPermission
  at org.wise.portal.service.project.impl.ProjectServiceImpl.setupNewProject
  at org.wise.portal.service.project.impl.ProjectServiceImpl.createProject

Found on a TWISE deployment. Confirmed present in develop at 1958196c2.

The fix below is written and compiling against develop. Happy to open a pull
request for it if that is useful, or to adjust the approach first if you would
rather it looked different.

How this came up, because the trigger is not hypothetical

This deployment ran open self-service teacher registration. Over about six
months, a group of accounts used it to publish SEO spam disguised as curriculum;
by the time it was measured, roughly half of every project on the site belonged
to them, and the entire upper range of project ids was theirs.

The response was the obvious one: disable the accounts, delete their projects and
the matching curriculum/ directories, and close self-service registration
behind administrator approval. 457 projects went, and that deletion pulled
MAX(projects.id) back below the highest deleted id.

Nine days later the first teacher to create a project could not, and neither
could anyone else. The site had been silently unable to author anything since the
cleanup.

The point of telling it this way is that a deployment removing a large block of
projects is a normal thing to have to do, and it is the exact input that turns
this defect from dormant into a total outage. Anything that deletes projects
outside the application, which is the only way it is done today, leaves the ACL
rows behind and arms this.

It was also slow to notice, for two reasons that are both properties of the code
rather than of the operators. Administrators are exempt from the failing check,
so the site looks healthy from the account most likely to be testing it. And the
error names a permission problem on a teacher's account, which reads as a
misconfigured user rather than as an id allocation fault.

Why the id gets reused

public long getNextAvailableProjectId() {
  File curriculumBaseDirFile = new File(curriculumBaseDir);
  long nextId = Math.max(projectDao.getMaxProjectId(), runDao.getMaxRunId()) + 1;
  while (true) {
    File nextFolder = new File(curriculumBaseDirFile, String.valueOf(nextId));
    if (nextFolder.exists()) { nextId++; } else { break; }
  }
  return nextId;
}

Two things stop an id from being handed out twice: a row in projects or runs,
and a directory under curriculum/. Deleting a project removes both. It does not
remove acl_object_identity, and it cannot fail because of it either: Spring ACL
deliberately keeps no foreign key to the domain tables, so the delete passes its
referential checks and leaves the ACL behind.

The result only appears after a bulk delete, because MAX(id) normally only
moves upward. Once the watermark drops back below the highest deleted id, every
subsequent allocation walks through ids that the ACL table still remembers.

Why the failure is total, and invisible to administrators

AclServiceImpl.addPermission adopts an existing ACL rather than creating one:

try {
  acl = (MutableAcl) mutableAclService.readAclById(objectIdentity);
} catch (NotFoundException nfe) {
  acl = mutableAclService.createAcl(objectIdentity);
}
acl.insertAce(acl.getEntries().size(), permission, new PrincipalSid(getAuthentication()), true);

insertAce then calls AclAuthorizationStrategyImpl.securityCheck, whose three
exits are all closed: the current user is not the stale ACL's owner, does not
hold the authority configured in ACLContext for ACL administration, and is
granted nothing by an ACL that predates them.

getNextAvailableProjectId() is used by new project creation
(AuthorAPIController), copy (ProjectServiceImpl.copyProject), import
(ImportProjectController), and run creation (RunServiceImpl.createRun, which
copies the project first), so all four fail together.

An account holding the ACL administration authority returns from securityCheck
before the ACE lookup and succeeds. That has two consequences worth stating
explicitly: the fault is invisible when tested from an administrator account, and
when an administrator does create a project on a reused id, the project silently
inherits the previous owner's ACL.

The second one is not hypothetical. On the deployment where this was found, one
project created during the outage turned out to have the deleted project's ACL:
its acl_object_identity.owner_sid was the previous, disabled owner, and that
account's original ADMINISTRATION entry was still ace_order 0, sitting in the
same entry list as the creator's entry and the two shares added afterwards. No
error was raised at any point, and the project behaves normally, so the only way
to find it is to compare acl_object_identity.owner_sid against
projects.owner. Anyone cleaning up after this should run that comparison and
not only look for ACL rows whose project is gone: those are two different
symptoms of the same cause, and the one an administrator produces is the quiet
one.

There is also a filesystem side effect. copyProject runs FileManager.copy
before the transactional database work, so each failed attempt leaves a
curriculum/<id> directory behind. That directory then pushes the allocator one
id forward, so retrying walks the id space one orphan at a time and accumulates
empty directories.

Reproduction

1. Note the current MAX(projects.id).
2. As a teacher, create a project, so that a new id N and its
   acl_object_identity row both exist.
3. Delete the projects row for N and the curriculum/N directory, leaving the
   ACL rows in place. This is what any external cleanup of unwanted content does,
   since nothing in the application deletes ACL rows.
4. As a non-admin teacher, create or copy a project. It is allocated id N again
   and fails with the exception above. Repeating the attempt allocates N+1 and
   fails again for as long as consecutive deleted ids have ACL rows.

Suggested fix

Treat acl_object_identity as a third watermark:

long nextId = Math.max(
    Math.max(projectDao.getMaxProjectId(), runDao.getMaxRunId()),
    aclTargetObjectIdentityDao.getMaxObjectIdForClasses(ProjectImpl.class.getName(),
        RunImpl.class.getName())) + 1;

with a new getMaxObjectIdForClasses on AclTargetObjectIdentityDao,
implemented in HibernateAclTargetObjectIdentityDao using the same
criteria-query shape as the existing retrieveByObjectIdentity:

public long getMaxObjectIdForClasses(String... classnames) {
  CriteriaBuilder cb = getCriteriaBuilder();
  CriteriaQuery<Long> cq = cb.createQuery(Long.class);
  Root<PersistentAclTargetObjectIdentity> root = cq.from(PersistentAclTargetObjectIdentity.class);
  cq.select(cb.coalesce(cb.max(root.<Long> get("aclTargetObjectId")), 0L))
      .where(root.get("aclTargetObject").get("classname").in((Object[]) classnames));
  TypedQuery<Long> query = entityManager.createQuery(cq);
  return query.getSingleResult();
}

RunImpl is in the list because RunServiceImpl.createRun does
run.setId((Long) project.getId()), so runs and projects share one id space and
acl_object_identity carries RunImpl rows at those same ids. Checking only
ProjectImpl would let a reused id through and move the identical owner-check
failure from project creation to run creation.

The result is coalesced rather than catching the unboxing NullPointerException
that a MAX over no rows raises. That catch cannot distinguish an empty table
from a query that failed, and for a watermark both answers are zero, which means
silently reverting to the bound this change exists to raise.

An id that any surviving record remembers is not free, and this makes the
allocator say so. It costs one aggregate query per project creation. This is
written, compiling against develop, and running in production on the
deployment where the fault was found; say the word and it becomes a pull
request.

One thing to know before testing it. Once the leftover ACL rows have been
cleaned up, the highest ProjectImpl ACL id equals MAX(projects.id) again, so
the patched allocator and the unpatched one return the same next id and
creating a project proves nothing either way. What distinguishes them is an
acl_object_identity row planted above both watermarks with no project behind
it: the patched code skips it and allocates marker + 1, and the unpatched code
lands on it and fails as above. Do that from a non-admin account, or the
administrator exemption will adopt the marker instead of failing on it.

Deleting ACL rows alongside projects would be a weaker alternative on its own: it
only helps deletions that go through code that knows to do it, and the deletions
that cause this are exactly the ones that do not.

A second, smaller change worth making with it

The watermark stops the collision. It does not stop a collision being silent if
one arrives by another route, and silence is what produced the corrupted project
above. So AclServiceImpl.addPermission(T, Permission) can also say no:

PrincipalSid currentUser = new PrincipalSid(getAuthentication());
try {
  acl = (MutableAcl) mutableAclService.readAclById(objectIdentity);
  if (!currentUser.equals(acl.getOwner())) {
    throw new AlreadyExistsException(objectIdentity + " already has an ACL owned by "
        + acl.getOwner() + ", so it cannot be given to " + currentUser
        + ". Its id is in use by a deleted object's ACL.");
  }
} catch (NotFoundException nfe) {
  acl = mutableAclService.createAcl(objectIdentity);
}

Refusing every pre-existing ACL would be wrong, because TagServiceImpl calls
this three times in a row on one new tag and the second and third calls must find
the ACL the first created. Requiring that the signed-in user owns it keeps that
case and rejects the other one, and it rejects it for administrators too, which
is the whole point: it turns the case nobody notices into an error, and leaves the
case a teacher already sees with a message that names what is actually wrong
rather than "Unable to locate a matching ACE".

Two more that belong with it, both in the same allocator's inputs.
getMaxProjectId and getMaxRunId end with

try { return query.getSingleResult(); } catch (NullPointerException e) { return 0; }

which cannot tell an empty table from a query that failed, and answers zero to
both. A watermark that returns zero contributes nothing, so the allocator would
quietly fall back to a lower bound with nothing in the log. Coalescing in SQL
gives the empty table its zero and lets a broken query throw.

And copyProject calls FileManager.copy before it writes anything to the
database, which is the opposite order from AuthorAPIController.createProject,
where the row is written and the directory made afterwards. Two consequences.

A rollback leaves the new curriculum/<id> directory behind, and that directory
then makes the allocator skip the id permanently, so a repeated failure walks the
id space leaving an abandoned folder per attempt. That is how nine folders
accumulated during the outage described above.

The other one is worse and is not about this defect at all. getNextAvailableProjectId
holds no lock and the row does not exist yet, so two authors copying at the same
moment are handed the same id. Where they collide is the ACL row, which carries a
unique constraint on class and object id and is inserted through JDBC rather than
deferred to flush. Copying first means both have already written into the same
folder by the time that happens: the loser rolls back and removes its rows, and
the winner is left holding a project whose files are somebody else's copy, with
nothing anywhere to say so. Copying after the insert makes the loser fail having
touched nothing on disk. Nothing between the two points reads the copied files,
so it is a move rather than a rewrite.

One thing has to move with it, or the reorder trades one bad state for a worse
one. copyProject carries a bare @Transactional, FileManager.copy throws a
checked IOException, and Spring rolls back on RuntimeException and Error
only. Once the row is written first, a missing source folder or a full disk
commits the project row and its ACL for a project that has no content: the author
gets a 500 and a permanently broken entry in their list, where the old order
would simply have failed. rollbackFor = Exception.class on copyProject, and
on the createRun overload that begins by calling it, since that one would
otherwise commit a run around the same empty project.

While there, FileManager.copy closes its two streams only on the success path.
A failed write leaves both open, and a caller trying to delete the half-written
file cannot do it on a platform that refuses to unlink an open one, which is
exactly when the cleanup is needed.

What this deployment ended up changing, in case any of it belongs in WISE itself

Four of these are findings rather than opinions, in the sense that each one was
something that looked done and was not. Take whichever are useful; happy to turn
any of them into pull requests.

1. Deleting a project has to delete the directory, not only the row.
nginx/conf.d/wise.conf in WISE-Docker-Server serves curriculum/ with root
and try_files, so that path never reaches the API and the enabled check in
InformationController does not apply to it. Disabling an account makes the
preview page fail, because that goes through the backend, while
curriculum/<id>/project.json keeps returning 200. Deleting the projects rows
alone therefore invalidates no external link at all; only removing the
directories does. Worth verifying with a two-sided control, that the removed ids
return 404 and that legitimate projects still return 200, because checking
only the first half would not notice a cleanup that took other people's content
with it.

2. Deleting a project has to delete its ACL rows. The subject of this issue.
Nothing in the application does it and no foreign key forces it, so every
external cleanup arms the id-reuse fault.

3. If the ACL cache is Redis-backed, the rows are not the whole state.
Spring Boot autoconfigures a RedisCacheManager whenever
spring-boot-starter-data-redis is on the classpath and no other cache provider
is, and SpringCacheBasedAclCache then stores ACLs in Redis with no TTL.
Cleaning acl_object_identity and restarting the application fixes nothing on
its own, because the stale ACL is still served from the cache. This is the part
that cost the most time here: the database was correct, the service had been
restarted, the health check was green, and authoring was still broken. The
aclCache* keys have to be cleared too, by prefix rather than by restarting
Redis, since sessions may live in the same instance.

4. Select the deletion list from data, never from a written-down list of
names. An inventory of offending accounts starts going stale on the day it is
written. Selecting by a property, here "projects whose owner is disabled",
produced a list that a hardcoded one would have under-counted.

5. Self-service teacher registration now creates the account disabled, pending
administrator approval. This closed the door that was being used, and the
operators who came back after the ban hit the new gate and could not get in,
which is about as direct a measurement of effectiveness as one gets. If you would
take it as a configurable option rather than something each deployment patches in
for itself, it is straightforward to contribute.

One caveat that came with it and is worth passing on: a queue of pending
approvals is state, not an event. The first version reported new accounts
once, on the day they registered, which means somebody registering on a Friday
evening is invisible by Monday morning, and from inside the system that failure
is silent. The account exists, no error is raised anywhere, and the person simply
cannot log in. It has to be reported for as long as it is waiting.