Add support for mTLS-based application authentication#1255
Merged
Add support for mTLS-based application authentication#1255
Conversation
Motivation: This change introduces mutual TLS (mTLS) as a first-class authentication method for client applications. mTLS provides a stronger security posture by authenticating the client machine itself through a cryptographic certificate. By supporting mTLS, we can offer a more secure, automatable, and manageable authentication mechanism for server-to-server communication. Modifications: - Introduced a new `AppIdentity` interface to abstract the concept of a client machine that connects to Central Dogma. - Created two implementations of this interface: `Token` for legacy token-based auth, and `CertificateAppIdentity` for the new mTLS-based auth. - An `appId` is now used as the primary identifier, which maps to a certificate's identity (extracted from the SPIFFE URI by default) or a token ID. Permissions are granted to this `appId`. - Renamed `Tokens` to `AppIdentityRegistry` to reflect that it now manages both tokens and certificates. - Renamed the corresponding service from `TokenService` to `AppIdentityRegistryService`. - Refactored the codebase by extracting token-related logic from `MetadataService` into the new `AppIdentityService` for better encapsulation and clarity. - Renamed `ProjectMetadata.tokens()` and `Roles.tokens()` to `ProjectMetadata.appIds()` and `Roles.appIds()` respectively. - Deprecated the existing token-based REST APIs in favor of new, more generic `/appIdentities` APIs that handle both types. Result: - Client applications can now authenticate with the Central Dogma server using mTLS client certificates in addition to API tokens. To-do: - Prepare the backward and forward compatiblity logic because tokens are renamed to applications in various places.
Motivation: This commit is the follow-up to line#1228, which introduced the backend support for mTLS and the generic `AppIdentity` model. Modifications: - Replaced all frontend API calls from the deprecated `/tokens` endpoints to the new `/appIdentities` endpoints. - The UI now fetches the server configuration from the `/configs` endpoint upon loading. - Based on the `mtlsEnabled` flag in the server configuration, the UI will conditionally display an option to create a certificate-based App Identity. - Performed a comprehensive renaming throughout the frontend codebase, replacing the term "token" with "App Identity". Result: - You can now create, view, and manage both token-based and certificate-based App Identities through the web UI.
|
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:
📝 WalkthroughWalkthroughRenames token concepts to "app identity" across backend and frontend, adds mTLS support and certificate-based authentication, replaces TokenService/Tokens with AppIdentityRegistry/AppIdentityService, updates metadata and APIs (/appIdentities), and adjusts web UI and tests to the new model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TLS as TLS/mTLS Layer
participant CertAuth as ApplicationCertificateAuthorizer
participant AppRegistry as AppIdentityRegistry
participant Metadata as MetadataService
Client->>TLS: HTTPS request with client certificate
TLS->>CertAuth: authorize(ctx, request)
CertAuth->>CertAuth: extract peer cert(s) from SSLSession
CertAuth->>CertAuth: extract certificateId via ID_EXTRACTOR
CertAuth->>AppRegistry: findByCertificateId(certificateId)
AppRegistry->>Metadata: lookup identity data
Metadata-->>AppRegistry: CertificateAppIdentity
AppRegistry-->>CertAuth: AppIdentity (or NOT_FOUND)
CertAuth->>CertAuth: bind authenticated UserWithAppIdentity to context
CertAuth-->>TLS: authorization result (success/failure)
TLS-->>Client: HTTP response
sequenceDiagram
participant Web as Web App
participant Api as RTK Query (apiSlice)
participant Backend as Server API
participant Metadata as MetadataService
Web->>Api: useGetAppIdentitiesQuery()
Api->>Backend: GET /api/v1/appIdentities
Backend->>Metadata: getAppIdentityRegistry()
Metadata-->>Backend: AppIdentityRegistry (masked/selections)
Backend-->>Api: AppIdentityDto[]
Api-->>Web: query result
Web->>Web: render rows via isToken / isCertificate guards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/src/test/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataDeserializerTest.java (1)
30-46:⚠️ Potential issue | 🟡 MinorKeep the legacy-field coverage in
deserializeWithTokens().Line 43 now uses
appIds, so the test no longer verifies backward compatibility for the legacytokensfield as its name/comment indicate. Either restoretokenshere or rename the test and update the comment.💡 Suggested fix (restore legacy field)
- " \"appIds\": {" + + " \"tokens\": {" +webapp/src/dogma/features/repo/settings/ConfirmAddUserOrAppIdentityRepositoryRole.tsx (1)
35-60:⚠️ Potential issue | 🟡 MinorHumanize
entityTypein user-facing copy.Interpolating
appIdentitydirectly yields awkward UI text (“appIdentity”). Map to a display label before use.✅ Suggested change
const dispatch = useAppDispatch(); const data = { id: loginId, role: repositoryRole as RepositoryRole, }; + const entityLabel = entityType === 'appIdentity' ? 'app identity' : 'user'; const handleUpdate = async () => { try { const response = await addUserRepositoryRole({ projectName, repoName, data }).unwrap(); @@ dispatch( newNotification( - `Repository ${entityType} role is added`, + `Repository ${entityLabel} role is added`, `Successfully updated ${repoName}`, 'success', ), ); @@ <ModalBody> - Add a {entityType} {loginId}? + Add a {entityLabel} {loginId}? </ModalBody>server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityRegistration.java (1)
43-45:⚠️ Potential issue | 🟡 MinorFix stale Javadoc reference to Token.
The class was renamed, but the role doc still references
Token, which is now misleading.✏️ Suggested fix
- /** - * A role of the {`@link` Token} in a project. - */ + /** + * A role of the {`@link` AppIdentity} in a project. + */
🤖 Fix all issues with AI agents
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java`:
- Around line 562-566: The duplicate-item error message is using outdated
"Token" wording; update the ChangeConflictException thrown inside
ProjectMetadataTransformer (the lambda checking
projectMetadata.appIds().containsKey(registration.id())) to use the new App
Identity wording—for example throw new ChangeConflictException("App identity
already exists: " + registration.id())—so the message references App Identity
and still includes registration.id() for context.
In
`@server/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.java`:
- Around line 307-310: The test is asserting exceptions inconsistently for
asynchronous calls: when you invoke
appIdentityRegistryService.createToken(...).join() (an async CompletableFuture),
assertThatThrownBy should expect a CompletionException or assert the join's
cause; update the assertions that call .join() (e.g., the createToken(...) usage
in AppIdentityRegistryServiceTest) to either expect CompletionException.class
via .isInstanceOf(CompletionException.class) or use
.hasCauseInstanceOf(IllegalArgumentException.class) to check the underlying
cause, and conversely if you remove .join() and assert on the CompletableFuture
itself, assert the IllegalArgumentException on the future’s completion; review
all assertThatThrownBy usages in AppIdentityRegistryServiceTest and make them
consistent (match .join() usages to CompletionException or hasCauseInstanceOf
for the underlying IllegalArgumentException).
- Around line 156-161: The test currently asserts IllegalArgumentException
directly when calling appIdentityRegistryService.createAppIdentity(...).join();
update the assertion to account for the async future by expecting a
CompletionException that wraps an IllegalArgumentException: replace the direct
isInstanceOf(IllegalArgumentException.class) check with an assertion that the
thrown exception is a CompletionException and that its cause is an
IllegalArgumentException (use appIdentityRegistryService.createAppIdentity(...)
and .join() as the action and assert
.isInstanceOf(CompletionException.class).hasCauseInstanceOf(IllegalArgumentException.class)
or equivalent).
- Around line 116-119: The test incorrectly asserts IllegalArgumentException
directly while createToken(...).join() will throw a CompletionException wrapping
the real cause; update the assertion to expect CompletionException and assert
its cause is IllegalArgumentException — e.g., use assertThatThrownBy(() ->
appIdentityRegistryService.createToken("forAdmin2", true, null, guestAuthor,
guest).join()).isInstanceOf(CompletionException.class).hasCauseInstanceOf(IllegalArgumentException.class)
so the test checks the wrapper and the underlying exception from the
CompletableFuture.
In
`@server/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceViaHttpTest.java`:
- Around line 73-94: The test method createTokenAndUpdateLevel should assert the
PATCH response status before parsing the body: after calling
systemAdminClient.execute(headers, body) and assigning AggregatedHttpResponse
response, add an assertion that response.status() (or response.status().code())
equals the expected success status (e.g. HttpStatus.OK) to fail fast on errors,
then proceed to parse response.contentUtf8() into JSON; update the assertion
placement in createTokenAndUpdateLevel so the status check happens immediately
after the response is received.
In
`@server/src/test/java/com/linecorp/centraldogma/server/metadata/TokenGuestPermissionTest.java`:
- Around line 152-155: In testNormalToken(), the request uses a lowercase
"token" value for the type parameter which fails since AppIdentityDeserializer
calls AppIdentityType.valueOf() (case-sensitive); update the test's POST call in
testNormalToken() to use the uppercase enum constant "TOKEN" (i.e., set the
query param key "type" to value "TOKEN" to match AppIdentityType and the usage
in TestAuthMessageUtil).
In `@webapp/src/dogma/features/server-config/ServerConfigLoader.tsx`:
- Around line 14-22: The fetch block handling '/configs' should validate
response.ok and ensure mtlsEnabled is a boolean before dispatching
setServerConfig; update the promise chain in ServerConfigLoader.tsx to check
response.ok (and handle non-OK by throwing or defaulting), parse JSON only on
OK, and defensively coerce or default data.mtlsEnabled to false when missing or
not boolean before calling dispatch(setServerConfig({ mtlsEnabled })). Ensure
the .catch still logs the error and dispatches a safe boolean fallback.
In `@webapp/src/pages/app/settings/app-identities/index.tsx`:
- Around line 70-76: The action visibility logic in the columnHelper.accessor
for AppIdentityDto is using strict === undefined checks against info.getValue(),
which mismatches the status truthiness logic and mishandles null; update the
hidden conditions for ActivateAppIdentity, DeactivateAppIdentity, and
DeleteAppIdentity to use truthiness like info.getValue() (or !info.getValue())
so that a null deactivation is treated the same as undefined/false—i.e., hide
Activate and Delete when deactivation is truthy, and hide Deactivate when
deactivation is falsy—so the actions align with the status display.
🧹 Nitpick comments (18)
server/src/test/java/com/linecorp/centraldogma/server/metadata/CertificateAppIdentityTest.java (2)
86-125: Consider usingisEqualTo(original)for the round-trip assertion.If
CertificateAppIdentityimplementsequals()andhashCode()(common for value objects), the entire field-by-field comparison could be simplified toassertThat(deserialized).isEqualTo(original). The current approach is also valid and provides more explicit failure messages, so this is optional.
26-145: Consider adding test coverage for thedeletionfield.The
serializeAndDeserializetest validatesdeactivationwhen non-null but always passesnullfordeletion. Adding a test case with a non-nulldeletionvalue would ensure complete coverage of the object's lifecycle states.webapp/src/dogma/features/app-identity/CertificateWrapper.tsx (1)
18-22: Consider accessibility: nested interactive elements in popover trigger.The
HStackcontaining anIconButtoninsidePopoverTriggercreates nested interactive elements. ThePopoverTriggermakes its child clickable, but theIconButtonis also an interactive element. This can cause confusing behavior for keyboard users and screen readers.Consider using a single
Buttoncomponent as the trigger:♻️ Suggested refactor
<PopoverTrigger> - <HStack> - <Text fontWeight="bold">{appId}</Text> - <IconButton aria-label="View certificate" icon={<MdArrowDropDown />} variant="ghost" /> - </HStack> + <Button + variant="ghost" + rightIcon={<MdArrowDropDown />} + aria-label="View certificate details" + > + <Text fontWeight="bold">{appId}</Text> + </Button> </PopoverTrigger>webapp/src/dogma/features/repo/settings/RepositorySettingsView.tsx (1)
131-136: Consider simplifying tab label rendering.The capitalization logic at lines 133-134 is now redundant since
TabNamevalues like'App Identities'and'Danger Zone'are already properly cased. The current code works correctly but could be simplified to just rendertab.namedirectly.♻️ Optional simplification
return ( <Tab as={Link} key={tab.name} href={link} isDisabled={!allowed}> - <Heading size="sm"> - {tab.name.charAt(0).toUpperCase()} - {tab.name.slice(1)} - </Heading> + <Heading size="sm">{tab.name}</Heading> </Tab> );server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.java (1)
92-95: Resolve the TODO: distinguish user vs. app-identity types.Leaving this TODO in permission logic risks long-term ambiguity; please track it with an issue or implement before release.
webapp/src/pages/api/v1/appIdentities.ts (1)
6-14: Consider randomizingallowGuestAccessfor more realistic mock data.The
systemAdminfield is randomized viafaker.datatype.boolean(), butallowGuestAccessis hardcoded totrue. For more representative test data, consider randomizing this field as well:💡 Suggested improvement
const token: Token = { appId: `${faker.animal.snake().replaceAll(' ', '-').toLowerCase()}-${id}`, type: 'TOKEN', secret: faker.datatype.uuid(), systemAdmin: faker.datatype.boolean(), - allowGuestAccess: true, + allowGuestAccess: faker.datatype.boolean(), creation: { user: faker.internet.email(), timestamp: faker.datatype.datetime().toISOString() }, };server/src/main/java/com/linecorp/centraldogma/server/auth/ApplicationCertificateIdExtractor.java (1)
28-32: Consider enhancing JavaDoc with expected ID format details.The interface is part of the public API and allows custom implementations. Documenting the expected format of the returned certificate ID (e.g., "a SPIFFE ID path like
trust-domain/workload-id") would help implementers understand what downstream components expect.server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/SpiffeIdExtractor.java (1)
52-55: Consider handling edge case of empty SPIFFE ID.If a certificate contains
spiffe://with nothing after it,substring(9)returns an empty string. Depending on how the ID is used downstream, an empty string might cause unexpected behavior versus returningnull.💡 Optional defensive check
if (uri.startsWith("spiffe://")) { - return uri.substring(9); // Remove "spiffe://" + final String spiffeId = uri.substring(9); // Remove "spiffe://" + return spiffeId.isEmpty() ? null : spiffeId; }webapp/src/dogma/features/server-config/serverConfigSlice.ts (1)
3-6: Consider adding error state for failed config fetches.If the server config fetch fails, the UI currently has no way to distinguish between "still loading" and "failed to load". Adding an
errorfield could improve UX by allowing the app to display appropriate feedback.💡 Optional enhancement
export interface ServerConfigState { mtlsEnabled: boolean; isLoaded: boolean; + error: string | null; } const initialState: ServerConfigState = { mtlsEnabled: false, isLoaded: false, + error: null, };webapp/javaTest/java/com/linecorp/centraldogma/webapp/ShiroCentralDogmaMtlsTestServer.java (2)
64-64: Privileged port 443 may fail on typical developer machines.Port 443 requires elevated privileges (root/admin) on most systems. Consider using a non-privileged port (e.g., 8443) for easier local testing, or document that elevated privileges are required.
Suggested change
- .port(443, SessionProtocol.HTTPS) + .port(8443, SessionProtocol.HTTPS)
51-51: Temporary directory is not cleaned up on shutdown.The shutdown hook only closes the server but does not delete the temporary directory created at line 51. This could lead to accumulation of orphaned temp directories over multiple test runs.
Add cleanup in shutdown hook
- Runtime.getRuntime().addShutdownHook(new Thread(server::close)); + Runtime.getRuntime().addShutdownHook(new Thread(() -> { + server.close(); + // Optionally clean up rootDir + }));Also applies to: 70-71
webapp/src/dogma/features/repo/RepositoriesMetadataDto.ts (1)
26-28: Interface name is descriptive but verbose.
UserOrAppIdentityRepositoryRoleDtoclearly conveys its purpose. If brevity is preferred, considerRepositoryRoleMapDtoor similar, though the current name is acceptable.server/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.java (1)
42-44: Variable naming is misleading.The variable is named
tokenbut its type isAppIdentityRegistration. Consider renaming toappIdentityorappIdentityRegistrationfor consistency with the new terminology.Suggested rename
- final AppIdentityRegistration token = metadata.appIds().get("app-token-1"); - assertThat(token.id()).isEqualTo("app-token-1"); - assertThat(token.role()).isEqualTo(ProjectRole.MEMBER); + final AppIdentityRegistration appIdentity = metadata.appIds().get("app-token-1"); + assertThat(appIdentity.id()).isEqualTo("app-token-1"); + assertThat(appIdentity.role()).isEqualTo(ProjectRole.MEMBER);And similarly for line 58:
- final AppIdentityRegistration token = metadata.appIds().get("app-id-1"); - assertThat(token.id()).isEqualTo("app-id-1"); - assertThat(token.role()).isEqualTo(ProjectRole.OWNER); + final AppIdentityRegistration appIdentity = metadata.appIds().get("app-id-1"); + assertThat(appIdentity.id()).isEqualTo("app-id-1"); + assertThat(appIdentity.role()).isEqualTo(ProjectRole.OWNER);Also applies to: 58-60
webapp/src/dogma/features/app-identity/DisplaySecretModal.tsx (1)
32-36: Make theresponsecontract consistent with the guard.
responseis required but the component returnsundefinedwhen it’s falsy. Either remove the guard or makeresponseoptional and returnnull.✅ Suggested change
}: { isOpen: boolean; onClose: () => void; - response: AppIdentityDto; + response?: AppIdentityDto; }) => { const dispatch = useAppDispatch(); - if (!response) return; + if (!response) return null; const { appId, systemAdmin, creation } = response;server/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.java (1)
95-108: TearDown may fail silently if purge operations throw exceptions.The tearDown method iterates over all app identities and calls destroy/purge without handling potential exceptions. If one identity fails to purge, subsequent identities won't be cleaned up, potentially causing test pollution.
Consider wrapping each cleanup operation in a try-catch:
♻️ Suggested improvement for robust cleanup
`@AfterEach` public void tearDown() { final AppIdentityRegistry registry = metadataService.fetchAppIdentityRegistry().join(); registry.appIds().forEach((appId, appIdentity) -> { - if (!appIdentity.isDeleted()) { - if (appIdentity.type() == AppIdentityType.TOKEN) { - metadataService.destroyToken(systemAdminAuthor, appId); - } else { - metadataService.destroyCertificate(systemAdminAuthor, appId); + try { + if (!appIdentity.isDeleted()) { + if (appIdentity.type() == AppIdentityType.TOKEN) { + metadataService.destroyToken(systemAdminAuthor, appId).join(); + } else { + metadataService.destroyCertificate(systemAdminAuthor, appId).join(); + } } + metadataService.purgeAppIdentity(systemAdminAuthor, appId).join(); + } catch (Exception e) { + // Log and continue cleanup } - metadataService.purgeAppIdentity(systemAdminAuthor, appId); }); }webapp/src/dogma/features/project/settings/ProjectSettingsView.tsx (1)
134-138: Minor: Tab label capitalization logic may produce unexpected results for multi-word tab names.The current logic at lines 136-137 capitalizes only the first character:
{tab.name.charAt(0).toUpperCase()} {tab.name.slice(1)}For
'App Identities', this works correctly since it's already capitalized. However, this pattern is fragile - if any tab name were lowercase (e.g.,'app identities'), it would display as "App identities" rather than "App Identities".Since tab names now include pre-capitalized multi-word names, consider whether this transformation is still needed or could be simplified.
server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java (1)
62-63: TODO: SPI-based certificate ID extractor configuration.The hardcoded
SpiffeIdExtractor.INSTANCElimits flexibility. The TODO acknowledges this should be configurable via SPI. Consider tracking this as a follow-up item if SPIFFE is not the only expected certificate ID format.Would you like me to open an issue to track making the
ApplicationCertificateIdExtractorconfigurable via SPI?server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java (1)
882-924: TODO for API exposure is still open.
If you want, I can draft the MetadataApiService endpoint for this update path.
Comment on lines
562
to
566
| final ProjectMetadataTransformer transformer = | ||
| new ProjectMetadataTransformer((headRevision, projectMetadata) -> { | ||
| if (projectMetadata.appIds().containsKey(registration.id())) { | ||
| throw new ChangeConflictException("Token already exists: " + registration.id()); | ||
| } |
There was a problem hiding this comment.
Use App Identity wording in duplicate error message.
The exception still says “Token already exists,” which is misleading after the rename.
🛠️ Suggested fix
- throw new ChangeConflictException("Token already exists: " + registration.id());
+ throw new ChangeConflictException("App identity already exists: " +
+ registration.id());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final ProjectMetadataTransformer transformer = | |
| new ProjectMetadataTransformer((headRevision, projectMetadata) -> { | |
| if (projectMetadata.appIds().containsKey(registration.id())) { | |
| throw new ChangeConflictException("Token already exists: " + registration.id()); | |
| } | |
| final ProjectMetadataTransformer transformer = | |
| new ProjectMetadataTransformer((headRevision, projectMetadata) -> { | |
| if (projectMetadata.appIds().containsKey(registration.id())) { | |
| throw new ChangeConflictException("App identity already exists: " + | |
| registration.id()); | |
| } |
🤖 Prompt for AI Agents
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java`
around lines 562 - 566, The duplicate-item error message is using outdated
"Token" wording; update the ChangeConflictException thrown inside
ProjectMetadataTransformer (the lambda checking
projectMetadata.appIds().containsKey(registration.id())) to use the new App
Identity wording—for example throw new ChangeConflictException("App identity
already exists: " + registration.id())—so the message references App Identity
and still includes registration.id() for context.
.../test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.java
Show resolved
Hide resolved
.../test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.java
Show resolved
Hide resolved
.../test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.java
Show resolved
Hide resolved
...ava/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceViaHttpTest.java
Show resolved
Hide resolved
server/src/test/java/com/linecorp/centraldogma/server/metadata/TokenGuestPermissionTest.java
Show resolved
Hide resolved
Comment on lines
+70
to
+76
| columnHelper.accessor((row: AppIdentityDto) => row.deactivation, { | ||
| cell: (info) => ( | ||
| <Wrap> | ||
| <ActivateAppIdentity appId={info.row.original.appId} hidden={info.getValue() === undefined} /> | ||
| <DeactivateAppIdentity appId={info.row.original.appId} hidden={info.getValue() !== undefined} /> | ||
| <DeleteAppIdentity appId={info.row.original.appId} hidden={info.getValue() === undefined} /> | ||
| </Wrap> |
There was a problem hiding this comment.
Align action visibility with status logic.
Status uses truthiness, but actions use === undefined. If deactivation is null, status shows Active while actions show Activate/Delete.
✅ Suggested change
columnHelper.accessor((row: AppIdentityDto) => row.deactivation, {
cell: (info) => (
+ (() => {
+ const isInactive = Boolean(info.getValue());
+ return (
<Wrap>
- <ActivateAppIdentity appId={info.row.original.appId} hidden={info.getValue() === undefined} />
- <DeactivateAppIdentity appId={info.row.original.appId} hidden={info.getValue() !== undefined} />
- <DeleteAppIdentity appId={info.row.original.appId} hidden={info.getValue() === undefined} />
+ <ActivateAppIdentity appId={info.row.original.appId} hidden={!isInactive} />
+ <DeactivateAppIdentity appId={info.row.original.appId} hidden={isInactive} />
+ <DeleteAppIdentity appId={info.row.original.appId} hidden={!isInactive} />
</Wrap>
+ );
+ })()
),🤖 Prompt for AI Agents
In `@webapp/src/pages/app/settings/app-identities/index.tsx` around lines 70 - 76,
The action visibility logic in the columnHelper.accessor for AppIdentityDto is
using strict === undefined checks against info.getValue(), which mismatches the
status truthiness logic and mishandles null; update the hidden conditions for
ActivateAppIdentity, DeactivateAppIdentity, and DeleteAppIdentity to use
truthiness like info.getValue() (or !info.getValue()) so that a null
deactivation is treated the same as undefined/false—i.e., hide Activate and
Delete when deactivation is truthy, and hide Deactivate when deactivation is
falsy—so the actions align with the status display.
jrhee17
approved these changes
Feb 5, 2026
Contributor
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Approving as already reviewed 👍 👍
Contributor
|
There are lint violations in the webapp. |
Contributor
Author
Fixed. 😉 |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java (1)
564-566:⚠️ Potential issue | 🟡 MinorUse App Identity wording in duplicate conflict message.
Line 565 still says
"Token already exists"although this path is AppIdentity-based. Please rename it to AppIdentity terminology for consistency.Suggested fix
- throw new ChangeConflictException("Token already exists: " + registration.id()); + throw new ChangeConflictException("App identity already exists: " + + registration.id());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java` around lines 564 - 566, The duplicate-conflict message uses "Token" but this branch is AppIdentity-based; in MetadataService where you check projectMetadata.appIds().containsKey(registration.id()) and throw new ChangeConflictException(...), change the message to use AppIdentity wording (e.g. "AppIdentity already exists: " + registration.id() or "App identity already exists: " + registration.id()) so the exception text matches the AppIdentity code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/main/java/com/linecorp/centraldogma/server/auth/MtlsConfig.java`:
- Around line 98-105: caCertInputStream currently treats the converted value as
certificate data bytes instead of a filesystem path, causing
CertificateUtil.toX509Certificates to fail; change caCertInputStream to
interpret the converted string (from convertValue) as a file path and return an
InputStream for that file (e.g., open the file with
Files.newInputStream(Paths.get(converted)) or similar), ensuring you propagate
IOExceptions appropriately and keep the method name caCertInputStream and the
convertValue call intact so callers (and CertificateUtil.toX509Certificates)
receive the actual certificate file contents.
In `@server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java`:
- Around line 755-766: The code silently disables mtls by setting mtlsEnabled =
false and logging a warning when TLS config is missing; instead fail fast during
startup to avoid unintentionally starting without certificate auth. In
CentralDogma (the block handling mtlsEnabled), replace the mtlsEnabled=false +
logger.warn branch with an explicit startup error (e.g., throw new
IllegalStateException or use Exceptions.throwUnsafely) that includes a clear
message like "mTLS is enabled but TLS is not configured; aborting startup", so
the process fails rather than continuing; update any related test expectations
to expect a startup failure.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java`:
- Around line 108-126: The loop in ApplicationCertificateAuthorizer currently
extracts certificate IDs from any X509Certificate in peerCertificates; re-enable
the CA/intermediate skip by checking x509Certificate.getBasicConstraints() and
continue when it != -1 (i.e., it's a CA), logging that a CA cert was skipped
(use ctx.clientAddress() and x509Certificate as in existing logs) before calling
ID_EXTRACTOR.extractCertificateId; ensure extraction only runs for end-entity
certs so certificateId comes from the leaf client certificate.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java`:
- Line 208: Update the stale inline comment in DefaultProject to reflect app
identities instead of "token": change the comment that reads "author.name() is
the appId of the token." to something like "author.name() is the appId of the
app identity." so it accurately describes the current semantics where
author.name() represents an app identity/appId.
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityRegistry.java`:
- Around line 198-203: The withoutSecret() method currently removes secrets but
also clears certificate ID mappings by passing ImmutableMap.of() for
certificateIds; update withoutSecret() in AppIdentityRegistry so it
preserves/rebuilds the certificateIds mapping instead of erasing it—either pass
the existing certificateIds map through to the new AppIdentityRegistry or
reconstruct certificateIds from the transformed appIds (use each AppIdentity's
certificateId to map to its id) before calling the AppIdentityRegistry
constructor; keep the secret removal logic for Token.withoutSecret() intact.
---
Duplicate comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java`:
- Around line 564-566: The duplicate-conflict message uses "Token" but this
branch is AppIdentity-based; in MetadataService where you check
projectMetadata.appIds().containsKey(registration.id()) and throw new
ChangeConflictException(...), change the message to use AppIdentity wording
(e.g. "AppIdentity already exists: " + registration.id() or "App identity
already exists: " + registration.id()) so the exception text matches the
AppIdentity code path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 382328ab-7f6b-44e6-ab93-8c766ab933aa
📒 Files selected for processing (18)
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.javaserver/src/main/java/com/linecorp/centraldogma/server/CentralDogmaBuilder.javaserver/src/main/java/com/linecorp/centraldogma/server/auth/ApplicationCertificateIdExtractor.javaserver/src/main/java/com/linecorp/centraldogma/server/auth/AuthConfig.javaserver/src/main/java/com/linecorp/centraldogma/server/auth/MtlsConfig.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/HttpApiExceptionHandler.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/SpiffeIdExtractor.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/PurgeSchedulingService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityRegistry.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/Roles.java
🚧 Files skipped from review as they are similar to previous changes (5)
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.java
- server/src/main/java/com/linecorp/centraldogma/server/metadata/Roles.java
- server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.java
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java
server/src/main/java/com/linecorp/centraldogma/server/auth/MtlsConfig.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java
Outdated
Show resolved
Hide resolved
Comment on lines
+108
to
+126
| for (Certificate peerCert : peerCertificates) { | ||
| logger.trace("Peer certificate: addr={}, cert={}", ctx.clientAddress(), peerCert); | ||
| if (!(peerCert instanceof X509Certificate)) { | ||
| continue; | ||
| } | ||
| final X509Certificate x509Certificate = (X509Certificate) peerCert; | ||
| /* // Uncomment the following lines after fixing Singned Certificate Extension to | ||
| generate end-entity certs. | ||
| if (x509Certificate.getBasicConstraints() != -1) { | ||
| logger.trace("Skipping CA certificate: addr={}, cert={}", ctx.clientAddress(), x509Certificate); | ||
| continue; | ||
| } | ||
| */ | ||
|
|
||
| certificateId = ID_EXTRACTOR.extractCertificateId(x509Certificate); | ||
| if (certificateId != null) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do not authorize using certificate IDs from CA/intermediate certificates.
With the CA-skip block disabled, Line 122 can accept a SPIFFE ID from any certificate in the chain, not strictly the end-entity client certificate. That weakens identity binding and can lead to incorrect authorization.
🔒 Proposed fix
- /* // Uncomment the following lines after fixing Singned Certificate Extension to
- generate end-entity certs.
if (x509Certificate.getBasicConstraints() != -1) {
logger.trace("Skipping CA certificate: addr={}, cert={}", ctx.clientAddress(), x509Certificate);
continue;
}
- */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java`
around lines 108 - 126, The loop in ApplicationCertificateAuthorizer currently
extracts certificate IDs from any X509Certificate in peerCertificates; re-enable
the CA/intermediate skip by checking x509Certificate.getBasicConstraints() and
continue when it != -1 (i.e., it's a CA), logging that a CA cert was skipped
(use ctx.clientAddress() and x509Certificate as in existing logs) before calling
ID_EXTRACTOR.extractCertificateId; ensure extraction only runs for end-entity
certs so certificateId comes from the leaf client certificate.
.../src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityRegistry.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
webapp/tests/dogma/feature/project/ProjectMetadata.test.tsx (1)
27-29: Rename leftoverAppToken*helper naming toAppId*for consistency.Line 27’s comment and Line 28’s alias still use token terminology, which is now outdated in this PR and makes the test harder to scan.
♻️ Suggested rename
-// Helper type for testing - AppTokenDto is Map<string, AppTokenDetailDto> but server returns object -type AppTokenObject = { [key: string]: AppIdDetailDto }; +// Helper type for testing - AppIdDto is Map<string, AppIdDetailDto> but server returns object +type AppIdObject = { [key: string]: AppIdDetailDto };-const appIds = mockMetadata.appIds as unknown as AppTokenObject; +const appIds = mockMetadata.appIds as unknown as AppIdObject;(Apply the usage rename similarly at Line 91, Line 96, Line 106, and Line 114.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/tests/dogma/feature/project/ProjectMetadata.test.tsx` around lines 27 - 29, The comment and helper type names still use "AppToken" terminology; rename the helper symbols and comment to use "AppId" instead — change the comment referencing AppTokenDto to AppIdDto (or the correct current type name) and rename the alias AppTokenObject to AppIdObject, then update all usages of that alias in this file (notably where referenced around the previous Line 91, 96, 106, and 114) so tests and comments consistently use AppId* names.webapp/e2e/project-tokens.spec.ts (1)
43-56: Consider finishing the terminology cleanup in test names and local variables.Assertions/routes are updated, but names like
tokensTaband test titles/comments still say “token(s)”, which makes the suite harder to scan and grep.♻️ Optional naming cleanup
- test('should display tokens tab in project settings', async ({ page }) => { + test('should display App Identities tab in project settings', async ({ page }) => { ... - const tokensTab = page.getByRole('tab', { name: 'App Identities' }); - await expect(tokensTab).toBeVisible(); + const appIdentitiesTab = page.getByRole('tab', { name: 'App Identities' }); + await expect(appIdentitiesTab).toBeVisible(); - await tokensTab.click(); + await appIdentitiesTab.click();- test('should display Add Token button', async ({ page }) => { + test('should display Add App Identity button', async ({ page }) => {- test('should display tokens tab in repository settings', async ({ page }) => { + test('should display App Identities tab in repository settings', async ({ page }) => { ... - const tokensTab = page.getByRole('tab', { name: 'App Identities' }); - await expect(tokensTab).toBeVisible(); + const appIdentitiesTab = page.getByRole('tab', { name: 'App Identities' }); + await expect(appIdentitiesTab).toBeVisible(); - await tokensTab.click(); + await appIdentitiesTab.click();Also applies to: 58-63, 119-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/e2e/project-tokens.spec.ts` around lines 43 - 56, Rename remaining "token(s)" terminology in the test to match the updated route/assertions: change the test title string "should display tokens tab in project settings" to "should display App Identities tab in project settings", rename the local variable tokensTab to appIdentitiesTab (and update its usage in the click/assert), and update any comments that mention "tokens" accordingly; apply the same rename/cleanup for the other occurrences noted (around the blocks referenced at 58-63 and 119-132) so test names, variable names, and comments consistently use "App Identities" / appIdentities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@webapp/e2e/project-tokens.spec.ts`:
- Around line 43-56: Rename remaining "token(s)" terminology in the test to
match the updated route/assertions: change the test title string "should display
tokens tab in project settings" to "should display App Identities tab in project
settings", rename the local variable tokensTab to appIdentitiesTab (and update
its usage in the click/assert), and update any comments that mention "tokens"
accordingly; apply the same rename/cleanup for the other occurrences noted
(around the blocks referenced at 58-63 and 119-132) so test names, variable
names, and comments consistently use "App Identities" / appIdentities.
In `@webapp/tests/dogma/feature/project/ProjectMetadata.test.tsx`:
- Around line 27-29: The comment and helper type names still use "AppToken"
terminology; rename the helper symbols and comment to use "AppId" instead —
change the comment referencing AppTokenDto to AppIdDto (or the correct current
type name) and rename the alias AppTokenObject to AppIdObject, then update all
usages of that alias in this file (notably where referenced around the previous
Line 91, 96, 106, and 114) so tests and comments consistently use AppId* names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ef1c543-304d-4117-acca-89aff02f65d8
📒 Files selected for processing (2)
webapp/e2e/project-tokens.spec.tswebapp/tests/dogma/feature/project/ProjectMetadata.test.tsx
This was referenced Mar 19, 2026
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.
This commit merges the #1228 and #1230 to the main branch.