Require a token to be deactivated before regenerating its secret

Motivation:

Regenerating the secret of an active token breaks its clients immediately with
no way to prepare, while the token still looks active in the UI. Deactivation
already serves as the immediate kill switch, so there is no reason to rotate an
active token in place.

Modifications:

- Reject regenerating the secret of an active token; the rotation procedure is
  now deactivate, regenerate, distribute the new secret and activate.
- Fail with a conflict when the token was recreated or regenerated concurrently
  after the caller was authorized, so that nobody distributes a secret that
  will never work.
- Disable the 'Regenerate secret' button for active tokens with a tooltip that
  guides to deactivate first, and hide it for tokens scheduled for deletion.
- Document the rotation procedure.

Result:

- A token secret can be rotated only while the token is deactivated, so the new
  secret can be distributed to the clients before it takes effect.

Author: ikhoon

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

@@ -251,9 +251,10 @@ public CompletableFuture<AppIdentity> updateAppIdentity(ServiceRequestContext ct
     /**
      * POST /appIdentities/{appId}/secret
      *
-     * <p>Regenerates the secret of the token of the specified {@code appId} and returns the token with
-     * a newly-generated secret. The old secret is revoked in the same commit, but it may take a short
-     * time for the revocation to be propagated to the authorization cache.
+     * <p>Regenerates the secret of the deactivated token of the specified {@code appId} and returns the
+     * token with a newly-generated secret. The token must be deactivated first and the new secret does
+     * not authenticate until the token is activated, so that the new secret can be distributed to the
+     * clients before it takes effect.
      */
     @Post("/appIdentities/{appId}/secret")
     public CompletableFuture<Token> regenerateTokenSecret(ServiceRequestContext ctx,
@@ -265,9 +266,13 @@ public CompletableFuture<Token> regenerateTokenSecret(ServiceRequestContext ctx,
                         throw new IllegalArgumentException(
                                 "You can't regenerate the secret of the token scheduled for deletion.");
                     }
-                    // Pass the creation metadata of the authorized token so that a token recreated
-                    // with the same application ID in the meantime is not rotated.
-                    return mds.regenerateTokenSecret(author, appId, token.creation());
+                    if (token.isActive()) {
+                        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);
                 });
     }
 

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

@@ -30,6 +30,7 @@
 import static java.util.Objects.requireNonNull;
 
 import java.util.Map;
+import java.util.Objects;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.atomic.AtomicReference;
@@ -176,7 +177,7 @@ CompletableFuture<Token> regenerateTokenSecret(Author author, String appId) {
     }
 
     CompletableFuture<Token> regenerateTokenSecret(Author author, String appId,
-                                                   @Nullable UserAndTimestamp expectedCreation) {
+                                                   @Nullable Token expectedToken) {
         requireNonNull(author, "author");
         requireNonNull(appId, "appId");
 
@@ -196,11 +197,25 @@ CompletableFuture<Token> regenerateTokenSecret(Author author, String appId,
                                 "The app identity is already destroyed: " + appId);
                     }
                     throwIfInvalidType(appId, appIdentity, AppIdentityType.TOKEN);
-                    if (expectedCreation != null && !expectedCreation.equals(appIdentity.creation())) {
+                    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.
+                        throw new ChangeConflictException(
+                                "The token must be deactivated before regenerating its secret: " + appId);
+                    }
 
                     final Token token = (Token) appIdentity;
                     final String oldSecret = token.secret();
@@ -216,12 +231,11 @@ CompletableFuture<Token> regenerateTokenSecret(Author author, String appId,
                     newTokenRef.set(newToken);
                     final Map<String, AppIdentity> newAppIds =
                             updateMap(registry.appIds(), appId, newToken);
-                    final Map<String, String> secretsWithoutOld =
-                            removeFromMap(registry.secrets(), oldSecret);
-                    // A deactivated token has no entry in the secret map.
+                    // A deactivated token has no entry in the secret map, so the new secret is not
+                    // added; it is registered when the token is activated. The old secret is removed
+                    // defensively in case a stale entry is left over.
                     final Map<String, String> newSecrets =
-                            token.isActive() ? addToMap(secretsWithoutOld, newSecret, appId)
-                                             : secretsWithoutOld;
+                            removeFromMap(registry.secrets(), oldSecret);
                     return new AppIdentityRegistry(newAppIds, newSecrets, registry.certificateIds());
                 });
         return appIdentityRegistryRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author,

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

@@ -1166,26 +1166,27 @@ public CompletableFuture<Revision> deactivateToken(Author author, String appId)
     }
 
     /**
-     * Regenerates the secret of the {@link Token} of the specified {@code appId} and returns the
-     * {@link Token} with the newly-generated secret. The old secret is revoked in the same commit,
-     * but it may take a short time for the revocation to be propagated to the authorization cache.
+     * 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.
      */
     public CompletableFuture<Token> regenerateTokenSecret(Author author, String appId) {
         return appIdentityService.regenerateTokenSecret(author, appId);
     }
 
     /**
-     * Regenerates the secret of the {@link Token} of the specified {@code appId} and returns the
-     * {@link Token} with the newly-generated secret. The old secret is revoked in the same commit,
-     * but it may take a short time for the revocation to be propagated to the authorization cache.
-     * The regeneration fails with a {@link ChangeConflictException} if the token's creation metadata
-     * does not match {@code expectedCreation}, which prevents rotating a token that was recreated
-     * with the same application ID after the caller was authorized.
+     * 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,
-                                                          UserAndTimestamp expectedCreation) {
-        requireNonNull(expectedCreation, "expectedCreation");
-        return appIdentityService.regenerateTokenSecret(author, appId, expectedCreation);
+                                                          Token expectedToken) {
+        requireNonNull(expectedToken, "expectedToken");
+        return appIdentityService.regenerateTokenSecret(author, appId, expectedToken);
     }
 
     /**

server/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceViaHttpTest.java

@@ -88,6 +88,18 @@ void regenerateTokenSecret() throws JsonProcessingException {
         assertThat(newTokenClient(oldSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
                                             .status()).isEqualTo(HttpStatus.OK);
 
+        // Deactivate the token to revoke the old secret.
+        final RequestHeaders patchHeaders =
+                RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "appIdentities/forRegenerate",
+                                  HttpHeaderNames.CONTENT_TYPE, MediaType.JSON);
+        assertThat(systemAdminClient.execute(patchHeaders, "{\"status\":\"inactive\"}").aggregate().join()
+                                    .status()).isEqualTo(HttpStatus.OK);
+        // The registry used by the authorizer is updated asynchronously so await the changes.
+        await().untilAsserted(() -> {
+            assertThat(newTokenClient(oldSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
+                                                .status()).isEqualTo(HttpStatus.UNAUTHORIZED);
+        });
+
         final AggregatedHttpResponse regenerateResponse =
                 systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/forRegenerate/secret",
                                        HttpData.empty())
@@ -99,20 +111,48 @@ void regenerateTokenSecret() throws JsonProcessingException {
         final String newSecret = regenerated.get("secret").asText();
         assertThat(newSecret).startsWith("appToken-")
                              .isNotEqualTo(oldSecret);
-
-        // The old secret is revoked and the new secret authenticates requests.
-        // The registry used by the authorizer is updated asynchronously so await the changes.
+        // The token remains deactivated so neither secret authenticates yet.
+        assertThat(regenerated.get("deactivation")).isNotNull();
         await().untilAsserted(() -> {
+            assertThat(newTokenClient(newSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
+                                                .status()).isEqualTo(HttpStatus.UNAUTHORIZED);
             assertThat(newTokenClient(oldSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
                                                 .status()).isEqualTo(HttpStatus.UNAUTHORIZED);
+        });
+
+        // Activating the token makes the new secret usable while the old one stays revoked.
+        assertThat(systemAdminClient.execute(patchHeaders, "{\"status\":\"active\"}").aggregate().join()
+                                    .status()).isEqualTo(HttpStatus.OK);
+        await().untilAsserted(() -> {
             assertThat(newTokenClient(newSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
                                                 .status()).isEqualTo(HttpStatus.OK);
+            assertThat(newTokenClient(oldSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
+                                                .status()).isEqualTo(HttpStatus.UNAUTHORIZED);
         });
     }
 
+    @Test
+    void cannotRegenerateSecretOfActiveToken() {
+        assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities",
+                                          QueryParams.of("appId", "forActive", "type", "TOKEN",
+                                                         "isSystemAdmin", false),
+                                          HttpData.empty())
+                                    .aggregate()
+                                    .join()
+                                    .status()).isEqualTo(HttpStatus.CREATED);
+
+        final AggregatedHttpResponse response =
+                systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/forActive/secret",
+                                       HttpData.empty())
+                                 .aggregate()
+                                 .join();
+        assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST);
+        assertThat(response.contentUtf8()).contains("Deactivate it first");
+    }
+
     @Test
     void ownerCanRegenerateOwnTokenSecret() throws JsonProcessingException {
-        // A non-admin user creates its own token and regenerates its secret.
+        // A non-admin user creates its own token, deactivates it and regenerates its secret.
         final WebClient userClient =
                 WebClient.builder(dogma.httpClient().uri())
                          .auth(AuthToken.ofOAuth2(getAccessToken(dogma.httpClient(),
@@ -128,6 +168,11 @@ void ownerCanRegenerateOwnTokenSecret() throws JsonProcessingException {
                              .aggregate()
                              .join()
                              .status()).isEqualTo(HttpStatus.CREATED);
+        final RequestHeaders patchHeaders =
+                RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "appIdentities/ownedByUser2",
+                                  HttpHeaderNames.CONTENT_TYPE, MediaType.JSON);
+        assertThat(userClient.execute(patchHeaders, "{\"status\":\"inactive\"}").aggregate().join()
+                             .status()).isEqualTo(HttpStatus.OK);
 
         final AggregatedHttpResponse response =
                 userClient.post(API_V1_PATH_PREFIX + "appIdentities/ownedByUser2/secret", HttpData.empty())
@@ -136,38 +181,6 @@ void ownerCanRegenerateOwnTokenSecret() throws JsonProcessingException {
         assertThat(response.status()).isEqualTo(HttpStatus.OK);
     }
 
-    @Test
-    void regenerateSecretOfDeactivatedTokenViaHttp() throws JsonProcessingException {
-        assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities",
-                                          QueryParams.of("appId", "forInactive", "type", "TOKEN",
-                                                         "isSystemAdmin", false),
-                                          HttpData.empty())
-                                    .aggregate()
-                                    .join()
-                                    .status()).isEqualTo(HttpStatus.CREATED);
-        final RequestHeaders headers = RequestHeaders.of(HttpMethod.PATCH,
-                                                         API_V1_PATH_PREFIX + "appIdentities/forInactive",
-                                                         HttpHeaderNames.CONTENT_TYPE, MediaType.JSON);
-        assertThat(systemAdminClient.execute(headers, "{\"status\":\"inactive\"}").aggregate().join()
-                                    .status()).isEqualTo(HttpStatus.OK);
-
-        final AggregatedHttpResponse response =
-                systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/forInactive/secret",
-                                       HttpData.empty())
-                                 .aggregate()
-                                 .join();
-        assertThat(response.status()).isEqualTo(HttpStatus.OK);
-        final JsonNode regenerated = Jackson.readTree(response.contentUtf8());
-        final String newSecret = regenerated.get("secret").asText();
-        assertThat(newSecret).startsWith("appToken-");
-        // The token remains deactivated so the new secret does not authenticate until activation.
-        assertThat(regenerated.get("deactivation")).isNotNull();
-        await().untilAsserted(() -> {
-            assertThat(newTokenClient(newSecret).get(API_V1_PATH_PREFIX + "appIdentities").aggregate().join()
-                                                .status()).isEqualTo(HttpStatus.UNAUTHORIZED);
-        });
-    }
-
     @Test
     void cannotRegenerateSecretOfMissingToken() {
         assertThat(systemAdminClient.post(API_V1_PATH_PREFIX + "appIdentities/nonexistent/secret",