fix: scope deleteExtension to requesting user's versions

[gnugomez]

No description provided.

[netomi]

  ---
  TOCTOU Analysis
  
  The race in deleteExtension (line 218–245) has two nested gaps, both under the default READ COMMITTED isolation:

  Gap 1 — inside isDeleteAllVersions (two separate queries)

  Query 1: SELECT COUNT(*) all   = 3   ← concurrent publish commits here
  Query 2: SELECT COUNT(*) actual = 3   ← still matches (user published all 3)
  → returns true  (wrong: there are now 4 versions, one owned by someone else)

  Gap 2 — between the check and the actual deletion

  Even if Query 1 and Query 2 happen to agree, a new version can appear after isDeleteAllVersions returns true and before findVersions() runs inside deleteExtension(user, extension) (line
  265). Under READ COMMITTED, findVersions sees the now-committed version and deletes it — an unauthorized deletion of another publisher's work.

  ---
  Why REPEATABLE READ alone is insufficient
  
  It fixes Gap 1 (both queries see the same snapshot), but not Gap 2. Because REPEATABLE READ snapshots only affect reads, a version inserted concurrently after the transaction's snapshot
  point would not be returned by findVersions — so entityManager.remove(extension) would leave it as an orphaned row with a dangling extension_id FK.

  ---
  Recommendation: pessimistic write lock on the Extension row
  
  Publishing always updates the extension row (extension.setLastUpdatedDate(...), line 181 of PublishExtensionVersionHandler). This makes a SELECT … FOR UPDATE on the Extension entity a
  natural serialization point.

  Proposed change — acquire the lock before the version count check:

  @Transactional(rollbackOn = ErrorResultException.class)
  public ResultJson deleteExtension(
          String namespaceName,
          String extensionName,
          List<TargetPlatformVersionJson> targetVersions,
          UserData user
  ) throws ErrorResultException {
      // Lock the extension row first. Publishing always updates extension.last_updated_date,
      // so this serializes concurrent publish+delete on the same extension.
      var extension = repositories.findExtensionForUpdate(extensionName, namespaceName);
      if (extension == null) {
          throw new ErrorResultException("Extension not found: " + NamingUtil.toLogFormat(namespaceName, extensionName),
                  HttpStatus.NOT_FOUND);
      }

      var results = new ArrayList<ResultJson>();
      if (repositories.isDeleteAllVersions(namespaceName, extensionName, targetVersions, user)) {
          results.add(deleteExtension(user, extension));
      } else {
          for (var targetVersion : targetVersions) {
              var extVersion = repositories.findVersion(user, targetVersion.version(), targetVersion.targetPlatform(), extensionName, namespaceName);
              if (extVersion == null) {
                  throw new ErrorResultException(
                          "Extension not found: " + NamingUtil.toLogFormat(namespaceName, extensionName, targetVersion.targetPlatform(), targetVersion.version()),
                          HttpStatus.NOT_FOUND);
              }
              results.add(deleteExtension(user, extVersion));
          }
      }
      // ... aggregate results
  }

  Add to RepositoryService (and the underlying JPA or JOOQ repository):

  // JPA Spring Data style:
  @Lock(LockModeType.PESSIMISTIC_WRITE)
  @Query("SELECT e FROM Extension e JOIN e.namespace n WHERE LOWER(e.name) = LOWER(:name) AND LOWER(n.name) = LOWER(:namespace)")
  Optional<Extension> findExtensionForUpdate(@Param("name") String name, @Param("namespace") String namespace);

  Why this works

  ┌────────────────────────┬─────────────────────────┬─────────────────────────────────────────────────────────┐
  │          Step          │      Without lock       │                     With FOR UPDATE                     │
  ├────────────────────────┼─────────────────────────┼─────────────────────────────────────────────────────────┤
  │ isDeleteAllVersions Q1 │ Sees committed snapshot │ Same, but concurrent publish is now blocked by our lock │
  ├────────────────────────┼─────────────────────────┼─────────────────────────────────────────────────────────┤
  │ isDeleteAllVersions Q2 │ May see newer snapshot  │ Consistent — no new versions can be committed           │
  ├────────────────────────┼─────────────────────────┼─────────────────────────────────────────────────────────┤
  │ findVersions + remove  │ May find extra versions │ Only existing versions; no interleaved inserts possible │
  └────────────────────────┴─────────────────────────┴─────────────────────────────────────────────────────────┘

  Any concurrent publishVersion call for the same extension will block on extension.setLastUpdatedDate(...) (which issues an UPDATE extension SET …), which in PostgreSQL blocks on the
  row-level lock we hold.

  ---
  Alternative: SERIALIZABLE isolation

  If you want the database to enforce correctness without application-level locking:

  @Transactional(rollbackOn = ErrorResultException.class)
  @org.springframework.transaction.annotation.Transactional(
      isolation = Isolation.SERIALIZABLE,
      rollbackFor = ErrorResultException.class
  )
  public ResultJson deleteExtension(...) { ... }

  PostgreSQL implements SERIALIZABLE via SSI (no extra locking overhead), and it would abort one of the conflicting transactions with a PSQLException: ERROR: could not serialize access. The
  downside is that this requires retry logic at the call site (Spring's @Retryable or a manual loop), and the isolation applies to the whole transaction — potentially increasing contention
  if this transaction touches unrelated tables.

  ---
  My recommendation is the pessimistic lock: it's surgical (only locks the one row that matters), avoids retry complexity, and directly exploits the fact that publishing is required to
  update the extension row.

[netomi]

I posted here the analysis of Claude Code for the actual problem which is a potential TOCTOU race condition in the when deleting extension versions.

My understanding of the proposed PR is that it changes the deleteExtension method such that only version of the given user are deleted. That would change the semantics of this method, as the idea is to delete the extension as a whole. I would rather go with the suggestion from Claude to do pessimistic locking to prevent the race condition which would be beneficial as we could use that locking at other places as well where it makes sense.

Long term we will do deletion of extension not based on publisher but based on role, so that method would change in any way.