Validate the token state only in the transformer when regenerating a secret

Motivation:

The regeneration endpoint passed a snapshot of the authorized token into the
content transformer and compared it against the current state. The snapshot is
taken outside the commit lock, so the comparisons were a non-atomic double
check of what the transformer already validates atomically.

Modifications:

- Remove the expected-token parameter and its comparisons; the token state is
  validated only in the content transformer.

Result:

- Validation happens once at the atomic point. Concurrent regenerations follow
  last-writer-wins semantics.

Author: ikhoon

server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java

@@ -270,9 +270,7 @@ public CompletableFuture<Token> regenerateTokenSecret(ServiceRequestContext ctx,
                         throw new IllegalArgumentException(
                                 "You can't regenerate the secret of an active token. Deactivate it first.");
                     }
-                    // Pass the authorized token so that the regeneration fails if the token is
-                    // recreated or regenerated concurrently in the meantime.
-                    return mds.regenerateTokenSecret(author, appId, token);
+                    return mds.regenerateTokenSecret(author, appId);
                 });
     }
 

server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.java

@@ -30,12 +30,9 @@
 import static java.util.Objects.requireNonNull;
 
 import java.util.Map;
-import java.util.Objects;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
 
-import org.jspecify.annotations.Nullable;
-
 import com.fasterxml.jackson.core.JsonPointer;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.google.common.collect.ImmutableMap;
@@ -172,11 +169,6 @@ Revision purgeAppIdentity(Author author, String appId) {
     }
 
     CompletableFuture<Token> regenerateTokenSecret(Author author, String appId) {
-        return regenerateTokenSecret(author, appId, null);
-    }
-
-    CompletableFuture<Token> regenerateTokenSecret(Author author, String appId,
-                                                   @Nullable Token expectedToken) {
         requireNonNull(author, "author");
         requireNonNull(appId, "appId");
 
@@ -193,19 +185,6 @@ CompletableFuture<Token> regenerateTokenSecret(Author author, String appId,
                                 "The app identity is already destroyed: " + appId);
                     }
                     throwIfInvalidType(appId, appIdentity, AppIdentityType.TOKEN);
-                    if (expectedToken != null &&
-                        !expectedToken.creation().equals(appIdentity.creation())) {
-                        // The token the caller was authorized for has been recreated in the meantime.
-                        throw new ChangeConflictException(
-                                "The app identity has been recreated concurrently: " + appId);
-                    }
-                    if (expectedToken != null &&
-                        !Objects.equals(expectedToken.secret(), ((Token) appIdentity).secret())) {
-                        // Another regeneration has been committed in the meantime; failing loudly
-                        // prevents the caller from distributing a secret that will never work.
-                        throw new ChangeConflictException(
-                                "The secret has been regenerated concurrently: " + appId);
-                    }
                     if (appIdentity.deactivation() == null) {
                         // Regenerating the secret of an active token would break its clients with no
                         // way to prepare, so the token must be deactivated first.

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

@@ -1175,20 +1175,6 @@ public CompletableFuture<Token> regenerateTokenSecret(Author author, String appI
         return appIdentityService.regenerateTokenSecret(author, appId);
     }
 
-    /**
-     * Regenerates the secret of the deactivated {@link Token} of the specified {@code appId} and
-     * returns the {@link Token} with the newly-generated secret. The token must be deactivated first
-     * and the new secret does not authenticate until the token is activated. The regeneration fails
-     * with a {@link ChangeConflictException} if the token is still active, or if the token does not
-     * match {@code expectedToken} anymore because it was recreated or regenerated after the caller
-     * was authorized.
-     */
-    public CompletableFuture<Token> regenerateTokenSecret(Author author, String appId,
-                                                          Token expectedToken) {
-        requireNonNull(expectedToken, "expectedToken");
-        return appIdentityService.regenerateTokenSecret(author, appId, expectedToken);
-    }
-
     /**
      * Returns an {@link AppIdentity} which has the specified {@code appId}.
      */

server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.java

@@ -617,56 +617,6 @@ void cannotRegenerateSecretOfDestroyedToken() {
                 .hasStackTraceContaining("already destroyed");
     }
 
-    @Test
-    void cannotRegenerateSecretOfRecreatedToken() {
-        final MetadataService mds = newMetadataService(manager);
-
-        mds.createToken(author, app1).join();
-        await().untilAsserted(() -> assertThat(mds.getAppIdentityRegistry().getOrDefault(app1, null))
-                .isNotNull());
-        final Token oldToken = (Token) mds.getAppIdentityRegistry().get(app1);
-
-        // Destroy, purge and recreate a token with the same application ID.
-        mds.destroyToken(author, app1).join();
-        mds.purgeAppIdentity(author, app1);
-        mds.createToken(author, app1).join();
-        await().untilAsserted(() -> assertThat(mds.getAppIdentityRegistry().get(app1).creation())
-                .isNotEqualTo(oldToken.creation()));
-
-        // A regeneration authorized against the old token must not rotate the recreated one,
-        // even before the recreated token is deactivated.
-        assertThatThrownBy(() -> mds.regenerateTokenSecret(author, app1, oldToken).join())
-                .hasCauseInstanceOf(ChangeConflictException.class)
-                .hasStackTraceContaining("recreated concurrently");
-
-        // A regeneration with the matching token succeeds once it is deactivated.
-        mds.deactivateToken(author, app1).join();
-        await().untilAsserted(() -> assertThat(mds.getAppIdentityRegistry().get(app1).isActive())
-                .isFalse());
-        final Token newToken = (Token) mds.getAppIdentityRegistry().get(app1);
-        final Token returned = mds.regenerateTokenSecret(author, app1, newToken).join();
-        assertThat(returned.secret()).startsWith("appToken-");
-    }
-
-    @Test
-    void cannotRegenerateSecretConcurrently() {
-        final MetadataService mds = newMetadataService(manager);
-
-        mds.createToken(author, app1).join();
-        mds.deactivateToken(author, app1).join();
-        await().untilAsserted(() -> assertThat(mds.getAppIdentityRegistry().get(app1).isActive())
-                .isFalse());
-        final Token snapshot = (Token) mds.getAppIdentityRegistry().get(app1);
-
-        // Another regeneration is committed after the caller was authorized against the snapshot.
-        mds.regenerateTokenSecret(author, app1).join();
-
-        // The stale caller fails loudly instead of receiving a secret that will never work.
-        assertThatThrownBy(() -> mds.regenerateTokenSecret(author, app1, snapshot).join())
-                .hasCauseInstanceOf(ChangeConflictException.class)
-                .hasStackTraceContaining("regenerated concurrently");
-    }
-
     @Test
     void cannotRegenerateSecretOfCertificate() {
         final MetadataService mds = newMetadataService(manager);