Allow regenerating the secret of an application token - #1340
Open
ikhoon wants to merge 13 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds application token secret regeneration across metadata, HTTP API, and web UI. Regeneration requires deactivation, replaces the registry secret, and preserves activation state. Tests cover lifecycle, authorization, concurrency, and UI behavior. Transformation conflict messages omit previous document content, and repository fetching accepts an explicit revision. ChangesToken secret regeneration
Transformation error safety
Revision-aware repository fetching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppIdentityPage
participant RegenerateAppIdentitySecret
participant apiSlice
participant AppIdentityRegistryService
participant AppIdentityService
participant DisplaySecretModal
AppIdentityPage->>RegenerateAppIdentitySecret: confirm regeneration
RegenerateAppIdentitySecret->>apiSlice: submit regeneration mutation
apiSlice->>AppIdentityRegistryService: POST /api/v1/appIdentities/{appId}/secret
AppIdentityRegistryService->>AppIdentityService: regenerateTokenSecret(author, appId, token)
AppIdentityService-->>AppIdentityRegistryService: regenerated Token
AppIdentityRegistryService-->>apiSlice: Token response
apiSlice-->>RegenerateAppIdentitySecret: mutation response
RegenerateAppIdentitySecret->>DisplaySecretModal: display new secret
DisplaySecretModal-->>RegenerateAppIdentitySecret: close modal
RegenerateAppIdentitySecret->>apiSlice: invalidate AppIdentity tag
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Motivation:
When the secret of an application token is leaked, the only remedy has been to
deactivate or delete the token and create a new one. This loses the roles and
permissions granted to the application ID and forces every project to register
the new token again. There should be a way to revoke the leaked secret and
issue a new one in place.
Modifications:
- Add `POST /api/v1/appIdentities/{appId}/secret` which revokes the current
secret and returns the token with a newly-generated secret in a single
commit. Only the token creator or a system administrator is allowed to call
it, the same as deletion.
- Preserve the deactivation state when regenerating; the new secret of a
deactivated token does not authenticate until the token is activated.
- Add a 'Regenerate secret' action with a confirmation dialog to the
application identities settings page. The new secret is displayed once, with
a warning if the token is inactive.
- Stop including the whole file content, which may contain secrets, in the
exception message raised when a content transformer fails.
Result:
- Users can rotate a leaked token secret in place; the old secret stops
working immediately and the application ID keeps its roles and permissions.
ikhoon
force-pushed
the
regenerate-token-secret
branch
from
July 24, 2026 09:59
75fa63c to
8cd8286
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
webapp/src/dogma/features/api/apiSlice.ts (1)
370-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUntyped mutation response flows into loosely-typed component state.
regenerateAppIdentitySecrethas no<ResultType, QueryArg>generics, so the response consumed byRegenerateAppIdentitySecret.tsx(useState(null)) isn't type-checked againstAppIdentityDto/Token. See the consolidated note for the specific fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/features/api/apiSlice.ts` around lines 370 - 377, The regenerateAppIdentitySecret mutation is missing response and argument typing, allowing its result to flow into untyped state in RegenerateAppIdentitySecret.tsx. Add the appropriate ResultType and QueryArg generics to regenerateAppIdentitySecret, using the existing AppIdentityDto/Token response contract and appId argument shape so the modal state is type-checked.webapp/src/dogma/features/app-identity/RegenerateAppIdentitySecret.tsx (1)
30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
appIdentityDetailstate and its setter are untyped.
useState(null)(no generic) combined with the untypedregenerateAppIdentitySecretmutation response meanssetAppIdentityDetail(response)and the laterresponse={appIdentityDetail}prop passed toDisplaySecretModal(which expectsAppIdentityDto) aren't type-checked end-to-end. See consolidated note for a concrete fix spanning this file andapiSlice.ts.Also applies to: 45-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/features/app-identity/RegenerateAppIdentitySecret.tsx` around lines 30 - 31, Type the regenerateAppIdentitySecret mutation response as AppIdentityDto in apiSlice.ts, then update appIdentityDetail in RegenerateAppIdentitySecret to use AppIdentityDto or null. Ensure the mutation result passed to setAppIdentityDetail and the response prop supplied to DisplaySecretModal are checked against AppIdentityDto.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java`:
- Around line 251-269: Update the Javadoc for
AppIdentityRegistryService.regenerateTokenSecret to remove or revise the
inaccurate “old secret is revoked immediately” claim, reflecting that authorizer
cache updates asynchronously. Keep the documented behavior that a newly
generated secret is returned and preserve the method implementation.
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java`:
- Around line 1168-1174: Update the Javadoc for
MetadataService.regenerateTokenSecret to avoid claiming the old secret is
revoked immediately; state that the change is committed immediately while
propagation to the authorization cache may take a short time.
In `@site/src/sphinx/auth.rst`:
- Around line 369-374: Update the token-regeneration documentation around the
``POST /api/v1/appIdentities/{appId}/secret`` endpoint to avoid claiming that
the old secret is revoked immediately; describe revocation using wording that
reflects the authorizer registry’s asynchronous update while preserving that the
new secret is issued to the same application ID and existing roles and
permissions remain unchanged.
---
Nitpick comments:
In `@webapp/src/dogma/features/api/apiSlice.ts`:
- Around line 370-377: The regenerateAppIdentitySecret mutation is missing
response and argument typing, allowing its result to flow into untyped state in
RegenerateAppIdentitySecret.tsx. Add the appropriate ResultType and QueryArg
generics to regenerateAppIdentitySecret, using the existing AppIdentityDto/Token
response contract and appId argument shape so the modal state is type-checked.
In `@webapp/src/dogma/features/app-identity/RegenerateAppIdentitySecret.tsx`:
- Around line 30-31: Type the regenerateAppIdentitySecret mutation response as
AppIdentityDto in apiSlice.ts, then update appIdentityDetail in
RegenerateAppIdentitySecret to use AppIdentityDto or null. Ensure the mutation
result passed to setAppIdentityDetail and the response prop supplied to
DisplaySecretModal are checked against AppIdentityDto.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ec5b764c-9c0e-445c-90d6-229a052c6465
📒 Files selected for processing (13)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/TransformingChangesApplier.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceViaHttpTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.javasite/src/sphinx/auth.rstwebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/app-identity/DisplaySecretModal.tsxwebapp/src/dogma/features/app-identity/RegenerateAppIdentitySecret.tsxwebapp/src/pages/app/settings/app-identities/index.tsxwebapp/tests/dogma/features/app-identity/RegenerateAppIdentitySecret.test.tsxwebapp/tests/pages/app/settings/app-identities/index.test.tsx
Contributor
Author
|
…ation Motivation: The documentation of token secret regeneration claims the old secret is revoked immediately, but the revocation may take a short time to be propagated to the authorization cache. The new webapp mutation and the secret modal state were also untyped. Modifications: - Reword the Javadoc and the authentication documentation to note that the propagation of the revocation to the authorization cache may take a short time. - Type the regenerate mutation response and the secret modal state with `AppIdentityDto`. Result: - The documentation matches the actual revocation timing and the regenerate flow is type-checked.
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.
Motivation: The regenerated token was passed out of the content transformer through an AtomicReference, which is an awkward side channel. Modifications: - Read the app identity registry back at the revision the push produced and return the token from it, instead of capturing the token in the transformer. Result: - No behavior change; the caller still always receives the secret its own commit produced.
…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.
ikhoon
force-pushed
the
regenerate-token-secret
branch
2 times, most recently
from
July 24, 2026 12:36
6e8d772 to
ad97369
Compare
Motivation: It was asked during the review why the permission check of the secret regeneration is not performed inside the content transformer. Modifications: - Add a comment that the metadata layer is caller-agnostic and the permission is checked at the HTTP layer like the other endpoints. Result: - No behavior change.
ikhoon
force-pushed
the
regenerate-token-secret
branch
from
July 24, 2026 12:38
ad97369 to
37438e7
Compare
Motivation: An IDE reformat that was not part of the change set was committed by mistake as 'Clean up the code style of AppIdentityService'. Modifications: - Revert the formatting-only changes. Result: - No behavior change; the formatting noise is removed from the change set.
Motivation: The 'Regenerate secret' button was shown disabled with a tooltip for active tokens, unlike the Delete button which is hidden until the token is deactivated. Modifications: - Hide the button for active tokens instead of disabling it, the same as the Delete button. Result: - The action appears only when it is actually usable.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1340 +/- ##
============================================
- Coverage 69.46% 68.84% -0.63%
+ Complexity 5709 5708 -1
============================================
Files 540 542 +2
Lines 24207 24304 +97
Branches 2771 2808 +37
============================================
- Hits 16816 16732 -84
- Misses 5880 6032 +152
- Partials 1511 1540 +29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Motivation: After a secret is regenerated, pressing the button again revokes the secret that is being distributed to the clients. The remaining steps of the rotation are distributing the new secret and activating the token, so offering another regeneration is misleading. Modifications: - Hide the 'Regenerate secret' button once the secret is regenerated, until the token is deactivated again. Result: - The action list guides the rotation procedure and an accidental second regeneration is prevented.
Motivation: The rotation test suite covered the implementation's state machine but not the promises of the feature: no test proved that a rotated token keeps its roles and permissions, and half of the permission matrix was untested. Modifications: - Prove over REST that a rotated token reads the same repository with the new secret without being registered again, while the old secret stops authenticating. - Cover a system administrator regenerating another user's token, consecutive regenerations keeping only the last secret, a purged token, unauthenticated requests and session requests without a CSRF token. - Assert that everything except the secret is preserved in the regeneration response. Result: - The rotation contract is pinned by end-to-end tests.
Motivation: The rotation tests built request bodies from escaped JSON strings, which are hard to read and silently break when the request format changes. Modifications: - Use `IdAndProjectRole` and `contentJson` for the app identity registration and the status updates instead of raw JSON strings. Result: - The request payloads are type-checked and easier to read.
ikhoon
marked this pull request as ready for review
July 28, 2026 08:52
jrhee17
approved these changes
Jul 29, 2026
jrhee17
left a comment
jrhee17
left a comment
Contributor
There was a problem hiding this comment.
Note) I understood that this API is not intended for zero-downtime rotation.
e.g. Normally users will want to 1) generate a new token 2) migrate tokens in their applications 3) delete the old leaked token
The proposed API assumes at the time of deactivation users have not migrated to a different token
Comment on lines
+208
to
+212
| // 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 = | ||
| removeFromMap(registry.secrets(), oldSecret); |
Contributor
There was a problem hiding this comment.
Question) I didn't understand under what situation this is possible. From my understanding the reverse mapping should be removed from deactivateToken. Otherwise, deactivated tokens can still be used for auth.
Contributor
Author
There was a problem hiding this comment.
Good point. It is no longer valid, so it should be removed. Initially, I tried to regenerate an active token but ended up limiting the feature only to deactivated tokens to avoid subtle race conditions.
Comment on lines
+255
to
+257
| * 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. |
Contributor
There was a problem hiding this comment.
Note) I understand that a user deactivates the token to call this API first. i.e. it is possible that an unlucky case happens where the token is purged before regenerate can be called.
Contributor
Author
There was a problem hiding this comment.
A token has three states - Active -> Inactive(deactivated) -> Deleted
The purge scheduler only purges deleted tokens so deactivated tokens are not purged.
Contributor
Author
Right. I will handle it in #1341 |
Contributor
Author
|
This regeneration flow may be used in an urgent situation where a token has been leaked. |
minwoox
reviewed
Jul 30, 2026
| token.deactivation(), null); | ||
| final Map<String, AppIdentity> newAppIds = | ||
| updateMap(registry.appIds(), appId, newToken); | ||
| return new AppIdentityRegistry(newAppIds, registry.secrets(), registry.certificateIds()); |
Contributor
There was a problem hiding this comment.
We need to create new registry.secrets().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
When the secret of an application token is leaked, the only remedy has been to
deactivate or delete the token and create a new one. This loses the roles and
permissions granted to the application ID and forces every project to register
the new token again. There should be a way to revoke the leaked secret and
issue a new one in place.
Modifications:
POST /api/v1/appIdentities/{appId}/secretwhich issues a newly-generatedsecret to a deactivated token. Only the token creator or a system
administrator is allowed to call it, the same as deletion.
until the token is activated, so the rotation procedure is: deactivate,
regenerate, distribute the new secret and activate.
after the caller was authorized, so that nobody distributes a secret that
will never work.
application identities settings page. The new secret is displayed once; the
button is enabled only for deactivated tokens.
exception message raised when a content transformer fails.
Result:
deactivate → regenerate → distribute → activate procedure; the application ID
keeps its roles and permissions.