Introduce AppIdentity and use transformer in MetadataService #1232
Conversation
Motivation: For compatiblity with line#1228 Modifications: - Introduced a new `AppIdentity` interface to serve as a common abstraction for application identities. - Created two concrete implementations: `Token` and `CertificateAppIdentity`, representing the different types of app identities. - Replaced the `JsonPatch` mechanism in `MetadataService` with a transformer. - This transformer is designed to handle different versions of the metadata schema, allowing for seamless updates regardless of whether the stored data is in the old or new format. Result: - The system's app identity model is now extensible and can easily support new types of application identities in the future.
📝 WalkthroughWalkthroughUnifies application identities by replacing legacy Token-only models with an AppIdentity abstraction (Token and CertificateAppIdentity), migrates metadata from tokens → appIds, removes LegacyToken, and converts MetadataService mutations to transformer-based updates; updates callers and tests to the new types and JSON shapes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/TokenService.java (1)
91-110: LGTM!The filtering logic correctly handles the new
AppIdentityabstraction by:
- Filtering for
AppIdentityType.TOKENto exclude certificate-based identities- Safely casting to
Tokenafter the type filter- Properly masking secrets for non-admin users
Consider extracting the common filtering logic to reduce duplication:
🔎 Optional refactor to reduce duplication
+ private Stream<Token> tokenStream() { + return mds.getTokens() + .appIds() + .values() + .stream() + .filter(appIdentity -> appIdentity.type() == AppIdentityType.TOKEN) + .map(appIdentity -> (Token) appIdentity); + } + @Get("/tokens") public Collection<Token> listTokens(User loginUser) { if (loginUser.isSystemAdmin()) { - return mds.getTokens() - .appIds() - .values() - .stream() - .filter(appIdentity -> appIdentity.type() == AppIdentityType.TOKEN) - .map(appIdentity -> (Token) appIdentity) - .collect(toImmutableList()); + return tokenStream().collect(toImmutableList()); } else { - return mds.getTokens() - .appIds() - .values() - .stream() - .filter(appIdentity -> appIdentity.type() == AppIdentityType.TOKEN) - .map(appIdentity -> (Token) appIdentity) - .map(Token::withoutSecret) - .collect(toImmutableList()); + return tokenStream() + .map(Token::withoutSecret) + .collect(toImmutableList()); } }server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializer.java (1)
44-56: Consider handling unknown type values more gracefully.The
AppIdentityType.valueOf(typeValue)on line 47 will throwIllegalArgumentExceptionif the type value is not a valid enum constant. Thedefaultcase on lines 54-55 is unreachable sincevalueOfthrows before reaching it.For better error handling and forward compatibility (e.g., if new identity types are added in the future but not yet supported in this deserializer), consider catching the exception:
🔎 Suggested improvement
if (typeNode != null) { // Type field exists, deserialize based on type final String typeValue = typeNode.asText(); - final AppIdentityType type = AppIdentityType.valueOf(typeValue); - + final AppIdentityType type; + try { + type = AppIdentityType.valueOf(typeValue); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Unknown AppIdentityType: " + typeValue, e); + } switch (type) { case TOKEN: return deserializeToken(node); case CERTIFICATE: return deserializeCertificate(node); - default: - throw new IllegalArgumentException("Unknown AppIdentityType: " + type); } + // Unreachable, but required by compiler + throw new AssertionError("Unhandled AppIdentityType: " + type);server/src/test/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializerTest.java (1)
105-120: Consider adding a test for unknown type values.The current tests cover missing type and valid types, but don't test behavior when an unknown type value (e.g.,
"type": "UNKNOWN") is provided. This would exercise the error handling path for future-proofing.🔎 Example additional test
@Test void deserializeUnknownTypeShouldFail() { final String unknownTypeJson = '{' + " \"type\": \"UNKNOWN\"," + " \"appId\": \"app\"," + " \"creation\": {" + " \"user\": \"[email protected]\"," + " \"timestamp\": \"2025-01-01T00:00:00Z\"" + " }" + '}'; assertThatThrownBy(() -> Jackson.readValue(unknownTypeJson, AppIdentity.class)) .hasCauseInstanceOf(IllegalArgumentException.class); }server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentity.java (1)
43-44: Consider tracking the TODO for future cleanup.The TODO indicates a planned refactor to use
JsonTypeInfoinstead of the explicit@JsonProperty("type")annotation. This would be a cleaner polymorphic deserialization approach.Would you like me to open an issue to track this technical debt?
server/src/main/java/com/linecorp/centraldogma/server/metadata/Token.java (1)
104-113: Consider usingrequireNonNullinstead ofassertfor secret validation.The
assertat line 110 can be disabled at runtime with-da. If this invariant is critical, consider usingrequireNonNull(secret, "secret")or a similar explicit check that throws an exception.🔎 Proposed fix
@Override public Token withSystemAdmin(boolean isSystemAdmin) { if (isSystemAdmin == isSystemAdmin()) { return this; } final String secret = secret(); - assert secret != null; + if (secret == null) { + throw new IllegalStateException("withSystemAdmin() must be called on a Token with a secret"); + } return new Token(id(), secret, isSystemAdmin, allowGuestAccess(), creation(), deactivation(), deletion()); }server/src/main/java/com/linecorp/centraldogma/server/metadata/Tokens.java (1)
179-188: Consider includingcertificateIdsin weight calculation.The
weight()method accounts forsecretsbut notcertificateIds. IfcertificateIdscontributes to memory/storage overhead, it should be included for consistency.🔎 Proposed fix
@Override public int weight() { int weight = 0; weight += secrets.size(); for (Entry<String, String> entry : secrets.entrySet()) { weight += entry.getKey().length(); weight += entry.getValue().length(); } + weight += certificateIds.size(); + for (Entry<String, String> entry : certificateIds.entrySet()) { + weight += entry.getKey().length(); + weight += entry.getValue().length(); + } return weight; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/LegacyToken.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/sysadmin/TokenService.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/metadata/AbstractAppIdentity.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentity.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializer.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityType.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/CertificateAppIdentity.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/RepositoryMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/Roles.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/Token.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/Tokens.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/TokenServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializerTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MissingRepositoryMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataDeserializerTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/RolesTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/TokenTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/TokensTest.java
💤 Files with no reviewable changes (1)
- server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/LegacyToken.java
🧰 Additional context used
🧬 Code graph analysis (6)
server/src/test/java/com/linecorp/centraldogma/server/metadata/RolesTest.java (3)
webapp/src/dogma/features/auth/RepositoryRole.tsx (1)
RepositoryRole(8-8)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(68-390)server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectRoles.java (1)
ProjectRoles(29-106)
server/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.java (2)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(68-390)server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectRoles.java (1)
ProjectRoles(29-106)
server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java (2)
server/src/main/java/com/linecorp/centraldogma/server/metadata/UserAndTimestamp.java (1)
UserAndTimestamp(35-123)common/src/main/java/com/linecorp/centraldogma/common/RepositoryExistsException.java (1)
RepositoryExistsException(25-55)
server/src/test/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataDeserializerTest.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(68-390)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.java (1)
webapp/src/dogma/features/auth/RepositoryRole.tsx (1)
RepositoryRole(8-8)
server/src/main/java/com/linecorp/centraldogma/server/metadata/AbstractAppIdentity.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
Util(38-513)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: lint
- GitHub Check: flaky-tests
- GitHub Check: docker-build
🔇 Additional comments (51)
server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityType.java (1)
22-32: LGTM!Clean and well-documented enum for distinguishing application identity types. The Javadoc appropriately describes both constants.
server/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.java (2)
70-85: LGTM!The test updates correctly reflect the API changes:
- Token constructor signature updated to the new form
- ProjectMetadata constructor includes the additional
nullparameter for the deprecated tokens field- Token registrations are now keyed under
appIdsin the expected JSON outputThe tests properly validate the new serialization contract.
192-202: Deserialization assertions properly updated.The assertions now correctly verify
obj.appIds()instead of the former tokens accessor, maintaining round-trip serialization test coverage.server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java (5)
185-199: LGTM!The transformer-based approach for
removeProjectis clean. The precondition check for already-removed projects is now encapsulated within the transformer callback, and the newProjectMetadataconstruction correctly propagatesappIds()while setting the removal timestamp.
251-272: LGTM!The
addMembertransformer correctly:
- Validates that the member doesn't already exist
- Uses
builderWithExpectedSizefor efficient map construction- Preserves all existing metadata fields while adding the new member
313-316: Roles constructor usage is consistent.The
Rolesconstructor is uniformly called withnullfor the deprecated tokens parameter (3rd position) and usesappIdsfor the active role mappings (4th position). This pattern is applied consistently across all repository role mutations.
1121-1147: LGTM!The
createTokentransformer properly:
- Validates uniqueness of both the appId and secret before creation
- Efficiently constructs new immutable maps with pre-sized builders
- Preserves
certificateIdsfrom the existing tokens structure
1203-1213: LGTM!The
purgeTokenmethod correctly removes the token from bothappIdsandsecretsmaps while preservingcertificateIds. The blocking join is documented in the Javadoc.server/src/main/java/com/linecorp/centraldogma/server/internal/storage/PurgeSchedulingService.java (1)
152-164: LGTM!The change correctly uses the
AppIdentityinterface methods (isDeleted()andappId()) instead ofToken-specific methods. This aligns with the new abstraction wheretokens.appIds()returns a collection ofAppIdentityinstances (which may include both tokens and certificates in the future).server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java (1)
220-232: LGTM!Test assertions correctly updated to use
roles().appIds()instead of the deprecatedroles().tokens(), maintaining proper test coverage for token-based repository role management.server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java (1)
112-118: LGTM!The token lookup correctly uses
metadata.appIds()to retrieve theTokenRegistrationfor the authenticated user's token, consistent with the broadertokens→appIdsmigration.server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java (1)
220-234: LGTM!The
ProjectMetadataconstructor call is correctly updated to include the additionalnullparameter for the deprecated tokens field. Thetokensvariable (containing the owner's token registration) is passed as theappIdsparameter in the new schema.server/src/test/java/com/linecorp/centraldogma/server/internal/api/TokenServiceTest.java (1)
148-149: LGTM! Test assertions correctly updated to reflect appIds API.The test assertions have been properly migrated from
tokens()toappIds(), maintaining the same validation logic while aligning with the new public API surface.Also applies to: 167-167
server/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadata.java (1)
105-106: LGTM! Roles constructor updated to 4-argument form.The constructor call correctly uses the new 4-argument signature:
Roles(projectRoles, users, tokens, appIds), passingnullfor the tokens parameter and an emptyappIdsmap, which aligns with the broader migration to appIds-based identity management.server/src/test/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataDeserializerTest.java (3)
30-53: LGTM! Backward compatibility test properly validates tokens→appIds migration.The renamed test method now explicitly validates that the legacy
tokensfield is correctly mapped toappIdsduring deserialization, ensuring backward compatibility with existing metadata.
55-79: LGTM! New format test validates appIds deserialization.The new test method validates that the
appIdsfield deserializes correctly, ensuring the new metadata format works as expected. Both legacy and new formats converge to the same internal representation.
90-91: LGTM! Assertion updated to use appIds API.The validation helper now correctly asserts on
appIds()instead oftokens(), which is used by both backward-compatible and new format tests.server/src/test/java/com/linecorp/centraldogma/server/metadata/MissingRepositoryMetadataTest.java (1)
80-80: LGTM! Assertion correctly updated to appIds API.The test now validates that auto-generated repository metadata contains an empty
appIdsmap, consistent with the API migration.server/src/test/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTest.java (1)
66-67: LGTM! Validates backward compatibility of tokens→appIds migration.The test deserializes JSON containing the legacy
tokensfield (line 47) and correctly validates it via theappIds()accessor, confirming backward compatibility of the migration.server/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.java (4)
31-45: LGTM! Backward compatibility test validates tokens→appIds migration.This test confirms that ProjectMetadata with the legacy
tokensfield deserializes correctly and maps to theappIds()accessor, ensuring existing metadata files continue to work.
47-61: LGTM! New format test validates appIds deserialization.This test validates that ProjectMetadata with the new
appIdsfield deserializes correctly, confirming the forward path of the migration.
63-93: LGTM! Serialization test ensures appIds field is written.This test constructs ProjectMetadata programmatically and validates that serialization produces JSON with the
appIdsfield, and that round-trip deserialization preserves equality.
95-109: LGTM! Round-trip test validates serialization stability.This test ensures that deserializing legacy
tokensformat, serializing to new format, and deserializing again maintains object equality, confirming the migration path is stable.server/src/test/java/com/linecorp/centraldogma/server/metadata/TokenTest.java (1)
83-83: LGTM! Legacy token format removed in favor of standardized format.The test now uses a single non-legacy token format with the
systemAdminfield, removing the conditional legacyadminfield handling. This simplification aligns with the PR's goal of removingLegacyTokenclass.Also applies to: 92-93, 98-106
server/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.java (1)
156-166: LGTM! Roles constructor updated and test validates correct identity segregation.The test correctly uses the 4-argument Roles constructor and validates that:
- User-created repositories populate the
usersmap with the creator- Token-created repositories populate the
appIdsmap with the token IDThis confirms proper segregation of user and application identities in repository metadata.
server/src/test/java/com/linecorp/centraldogma/server/metadata/TokensTest.java (5)
28-45: LGTM! Comprehensive null handling test.The test correctly validates that when
certificateIdsis null, theTokensmodel returns an empty (not null) map viatokens.certificateIds(), which is important for null-safety guarantees.
47-63: LGTM! Good coverage for mixed identity types.This test validates that
Tokenscorrectly handles a mix ofTokenandCertificateAppIdentityentries in theappIdsmap, ensuring both identity types can coexist.
65-92: LGTM! Validates backward compatibility with legacy format.This test ensures that JSON without the
certificateIdsfield (legacy format) deserializes correctly, withcertificateIdsdefaulting to an empty map.
94-136: LGTM! Validates new format deserialization with both identity types.This test correctly validates deserialization of the new JSON format containing both
TOKENandCERTIFICATEtype entries with explicittypediscriminators.
157-177: LGTM! Round-trip serialization ensures data integrity.Good practice to verify that serialization followed by deserialization preserves all fields, including the new
certificateIdsmapping.server/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.java (1)
51-57: LGTM! Correct migration to appIds-based metadata construction.The
ProjectMetadataconstruction now correctly passesnullfor the deprecatedtokensparameter and usesprojectMetadata.appIds()for the newappIdsparameter, aligning with the PR's migration from tokens to appIds.server/src/test/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1Test.java (1)
117-129: LGTM! Test correctly updated for the tokens() → appIds() API migration.The assertions now use
appIds()instead oftokens(), and correctly verify that:
- User-created projects have empty
appIdsand populatedmembers- Token-created projects have populated
appIds(withTokenRegistration) and emptymembersserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializer.java (2)
70-93: LGTM! Clean extraction of deserialization logic.The helper methods
deserializeTokenanddeserializeCertificatecorrectly extract and validate all required fields, with appropriate handling for optional fields likedeactivationanddeletion.
118-150: LGTM! Robust field extraction with clear error messages.The helper methods provide strong validation with informative error messages, distinguishing between missing fields and type mismatches.
server/src/test/java/com/linecorp/centraldogma/server/metadata/RolesTest.java (2)
29-56: LGTM! Validates backward compatibility for legacy tokens field.This test correctly verifies that JSON containing the legacy
tokensfield is properly deserialized and accessible via the newappIds()accessor, ensuring smooth migration for existing data.
85-103: LGTM! Validates one-way migration to appIds.The test confirms that:
- Serialization outputs
appIds(nottokens)- Round-trip preserves data integrity
- The new format is forward-compatible
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.java (1)
46-57: LGTM! Correct migration to appIds-based Roles construction.The repository creation logic now correctly:
- Populates
appIds(instead oftokens) for token-based authors- Uses the 4-argument
Rolesconstructor withnullfor the deprecatedtokensparameter- Maintains the same access control semantics (ADMIN role for the creator)
server/src/main/java/com/linecorp/centraldogma/server/metadata/AbstractAppIdentity.java (3)
44-54: LGTM! Well-designed constructor with appropriate validation.The constructor:
- Validates
appIdusingUtil.validateFileNamefor consistency with file-based storage- Requires non-null
typeandcreationfields- Properly accepts nullable
deactivationanddeletionfor lifecycle management
98-121: LGTM! Correct equality implementation for abstract base class.Using
getClass() != o.getClass()is the right choice for an abstract class, ensuring thatToken.equals(CertificateAppIdentity)returns false even if they share the same base fields.
123-137: LGTM! Extensible toString() pattern.The
finalmodifier ontoString()with the abstractaddProperties(ToStringHelper)hook is a good template method pattern, allowing subclasses to add type-specific fields without duplicating the base field formatting.server/src/test/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializerTest.java (1)
27-53: LGTM! Excellent test for backward compatibility.This test validates the critical migration path where legacy tokens without a
typefield are correctly deserialized asTokeninstances. The additional assertion that re-serialization includes"type":"TOKEN"ensures data will be upgraded on next write.server/src/main/java/com/linecorp/centraldogma/server/metadata/Roles.java (1)
39-40: LGTM! Backward-compatible migration from tokens to appIds.The constructor pattern correctly handles both old JSON (with
tokenskey) and new JSON (withappIdskey), prioritizingappIdswhen present. The validation at lines 58-60 ensures data integrity.Also applies to: 51-67
server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentity.java (1)
26-96: Well-designed interface for the AppIdentity abstraction.The interface provides a clean contract for both token and certificate-based identities. The default methods
isActive()andisDeleted()encapsulate the state logic nicely, and the JSON annotations ensure proper serialization behavior.server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.java (2)
91-113: LGTM! Constructor correctly handles backward-compatible migration.The constructor properly validates that at least one of
tokensorappIdsis non-null, and prioritizesappIdswhen present. This pattern is consistent withRoles.java.
147-150: Verify JSON property name for serialization.The accessor uses
@JsonPropertywithout an explicit name, which will serialize as"appIds". Old clients expecting"tokens"may need to be updated. Confirm this breaking change in the serialization output is acceptable.server/src/main/java/com/linecorp/centraldogma/server/metadata/CertificateAppIdentity.java (1)
37-104: LGTM! Clean implementation of certificate-based identity.The class properly extends
AbstractAppIdentitywith thecertificateIdfield. Notable design choices:
allowGuestAccessdefaults tofalsefor certificates (vstruefor tokens) - appropriate for stricter certificate-based access.withSystemAdmin()optimizes by returningthiswhen unchanged.- Proper
equals()/hashCode()implementation combining superclass state.server/src/main/java/com/linecorp/centraldogma/server/metadata/Token.java (2)
34-46: LGTM! Token now properly extends the identity abstraction.The refactoring to extend
AbstractAppIdentityis clean. TheallowGuestAccessdefault oftrue(line 61) maintains backward compatibility for existing tokens.Also applies to: 59-66
120-128: Theequals()method is correct and safe. TheAbstractAppIdentity.equals()implementation at line 104 enforces strict class equality viagetClass() != o.getClass(), which ensures thatsuper.equals(o)only returnstrueifois of the exact same class. Therefore, the cast toTokenat line 126 is guaranteed to succeed and will not throw aClassCastException.server/src/main/java/com/linecorp/centraldogma/server/metadata/Tokens.java (3)
126-133: LGTM! Proper type checking before casting.The
getOrDefault()method correctly checksappIdentity.type() != AppIdentityType.TOKENbefore casting toToken, preventingClassCastExceptionwhen the identity is a certificate-based one.
166-177: LGTM!withoutSecret()correctly handles mixed identity types.The stream-based transformation properly preserves non-token identities while stripping secrets from tokens.
190-197: Good security practice: loggingsecrets.sizeinstead of actual secrets.This prevents accidental exposure of sensitive data in logs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializer.java (1)
54-62: Replacethrow new Error()with a default case orIllegalStateException.Line 61 uses
throw new Error()without a message, which is inappropriate for application logic. While currently unreachable (assumingAppIdentityTypeonly containsTOKENandCERTIFICATE), if a new enum value is added in the future and this switch isn't updated, the rawErrorwill be thrown without context.🔎 Suggested fix
Replace the switch statement with one that includes a default case:
switch (type) { case TOKEN: return deserializeToken(node); case CERTIFICATE: return deserializeCertificate(node); + default: + throw new IllegalStateException("Unhandled AppIdentityType: " + type); } -// Should never reach here -throw new Error();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/TokenService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializer.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/Tokens.java
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/main/java/com/linecorp/centraldogma/server/metadata/Tokens.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/TokenService.java (1)
server/src/main/java/com/linecorp/centraldogma/server/metadata/Token.java (1)
Token(34-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: lint
- GitHub Check: docker-build
- GitHub Check: flaky-tests
🔇 Additional comments (5)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/TokenService.java (2)
92-98: LGTM!The refactoring correctly maintains the admin vs non-admin distinction (secrets visible only to admins) while adapting to the new
AppIdentityabstraction. The use oftokenStream()cleanly encapsulates the filtering logic.
101-108: LGTM!The filter-then-cast pattern is appropriate here—filtering by
AppIdentityType.TOKENensures the subsequent cast toTokenis type-safe. The method cleanly extracts the token-specific logic for reuse.server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityDeserializer.java (3)
75-98: LGTM! Clean and consistent deserialization helpers.Both
deserializeTokenanddeserializeCertificatefollow a consistent pattern, properly extracting required and optional fields with appropriate validation. The type-specific differences (secret vs. certificateId) are handled correctly.
100-113: LGTM! Proper handling of required and optional nested objects.The
UserAndTimestampdeserialization methods correctly distinguish between required and optional fields, with appropriate null-safety checks for both missing fields and explicit JSON null values.
115-155: LGTM! Robust validation utilities with clear error messages.The helper methods provide thorough validation for required and optional fields with appropriate type checking. Error messages are informative, specifying both the field name and the nature of the validation failure.
minwoox
changed the title
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Understood this PR is basically refactoring/schema change to support AppIdentity more natively.
2 participants
Motivation:
For compatiblity with #1228
Modifications:
AppIdentityinterface to serve as a common abstraction for application identities.TokenandCertificateAppIdentity, representing the different types of app identities.JsonPatchmechanism inMetadataServicewith a transformer.Result: