Feature/first auth v1 #175
Conversation
# Conflicts: # cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java # cim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.java # cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java
- 新增直连模式配置项，允许客户端直接连接到指定服务器 - 修改客户端连接逻辑，支持直连和路由两种方式 - 更新服务端注册逻辑，支持不注册模式 - 优化鉴权流程，使用 JWT令牌进行身份验证
- 在根 pom.xml 中添加 spring-boot-starter-logging 依赖管理，版本为 3.3.0 - 在 cim-client 子模块中添加 spring-boot-starter-logging 依赖 - 删除无效日志 - 删除 ChannelInboundDebugHandler 中的冗余注释
WalkthroughThis update introduces JWT-based authentication to the client-server architecture, including token issuance on login, token verification on server connection, and propagation of authentication state via Netty channel attributes. It adds configuration options for registry types and meta store implementations, with conditional logic for direct or Zookeeper-based registration. Debugging handlers and new configuration properties are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouteAPI
participant Server
Client->>RouteAPI: Login (userId, userName)
RouteAPI->>RouteAPI: Authenticate user
RouteAPI->>RouteAPI: Generate JWT token
RouteAPI-->>Client: Return server info + JWT token
Client->>Server: Connect (with JWT token)
Server->>Server: Verify JWT token (ClientAuthHandler)
alt Token valid
Server->>Server: Set channel attributes (userId, userName, AUTH_RES)
Server-->>Client: Authentication success
Client->>Server: Proceed with messaging
else Token invalid
Server-->>Client: Authentication failure, close connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (28)
cim-client/pom.xml (1)
23-26:spring-boot-starter-loggingis already pulled transitively – consider removing the explicit dependencyBecause the module inherits
spring-boot-starter-parent, Logback + Log4j 2 APIs are already supplied by the default BOM. Keeping a second, direct starter reference is harmless but bloats the class-path and can cause version skew if the parent is upgraded but this stanza is forgotten.- <dependency> - <groupId>org.springframework.boot</groupId> - <artifactId>spring-boot-starter-logging</artifactId> - </dependency>Unless you intend to pin a different logging stack (e.g. Logback-classic exclusion, Log4j2 only, etc.), drop the block for leaner builds.
pom.xml (2)
48-57: Duplicate logging starter definition in dependency-managementThe same argument as above applies here. Managing an explicit version for
spring-boot-starter-loggingis unnecessary because the Spring Boot BOM already does it. More importantly, the managed version (3.3.0) must stay in lock-step with the parent version; forgetting to update one side silently produces two different versions on the tree.If you really need to override, document the reason in a comment, otherwise delete the entry to avoid drift.
54-57: Consider extractingjava-jwtversion into a<jwt.version>propertyGood call centralising the JWT library here. To ease future upgrades and keep the POM tidy, expose the version as a property just like
netty.version, then reference it:<properties> ... <jwt.version>3.19.0</jwt.version> </properties> <dependency> <groupId>com.auth0</groupId> <artifactId>java-jwt</artifactId> <version>${jwt.version}</version> </dependency>This keeps the
<dependencyManagement>section free of hard-coded literals.cim-common/pom.xml (1)
17-20: Rely on dependency-management; omit explicit versionBecause the root POM now manages
com.auth0:java-jwt, you can (and should) drop the un-versioned declaration and inherit the managed version automatically:- <dependency> - <groupId>com.auth0</groupId> - <artifactId>java-jwt</artifactId> - </dependency> + <dependency> + <groupId>com.auth0</groupId> + <artifactId>java-jwt</artifactId> + </dependency> <!-- version comes from parent BOM -->This avoids accidental divergence if the managed version is bumped later.
cim-common/src/main/java/com/crossoverjie/cim/common/enums/ChannelAttributeKeys.java (1)
9-34: Channel attribute constants look good – minor naming/package tweakImplementation is clean. Two nitpicks you may consider:
- The package
…enumsis misleading since this is an interface, not an enum. A neutral package like…constantsreduces confusion.- If you anticipate more attributes later, a final utility-class with a private constructor avoids the “constant interface” antipattern while keeping callers unchanged:
public final class ChannelAttributeKeys { private ChannelAttributeKeys() {} public static final AttributeKey<String> AUTH_TOKEN = AttributeKey.newInstance("auth_token"); ... }No functional impact, purely stylistic.
cim-common/src/main/java/com/crossoverjie/cim/common/enums/AuthTypeEnum.java (1)
7-9: Empty enum gives no compile-time value – delete or populate it
AuthTypeEnumis declared but contains no constants or methods, so every reference to it elsewhere will be useless or compile-time unreachable.
Either remove the file until concrete auth types are known, or add the expected constants (e.g.JWT,BASIC,OAUTH) now.cim-common/src/main/java/com/crossoverjie/cim/common/enums/RegistryType.java (1)
7-12: Interface-constants pattern is an anti-pattern; prefer an enumUsing an interface only to expose
Stringconstants leaks them into every implementing class and provides no type-safety.
A tiny enum (or afinalutility class withpublic static finalfields) is clearer and prevents accidental typo values.-public interface RegistryType { - - String NO = "no"; - String ZOOKEEPER = "zk"; -} +public enum RegistryType { + NO("no"), + ZOOKEEPER("zk"); + + private final String code; + RegistryType(String code) { this.code = code; } + public String code() { return code; } +}This preserves the string representation while enforcing valid values at compile time.
cim-client/src/main/java/com/crossoverjie/cim/client/CIMClientApplication.java (1)
17-27: Replace raw thread with managed executorStarting a thread manually inside a Spring Boot application makes graceful shutdown and error handling harder.
UseTaskExecutor/@Asyncor anExecutorServicebean so Spring can manage lifecycle and propagate uncaught exceptions.-Scan scan = new Scan(); -Thread thread = new Thread(scan); -thread.setName("scan-thread"); -thread.start(); +applicationContext + .getBeanProvider(java.util.concurrent.ExecutorService.class) + .ifAvailable(executor -> executor.submit(new Scan()));At minimum, mark the thread as daemon or add proper interruption handling.
cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java (1)
21-38: Serializable DTO is missingserialVersionUIDBecause
PayloadVOimplementsSerializable, add an explicitserialVersionUIDto avoid unexpectedInvalidClassExceptionafter future field changes.You could also reduce boilerplate with Lombok:
-public class PayloadVO implements Serializable { - /** - * 用户id - */ - private Long userId; - /** - * 用户名称 - */ - private String userName; +@Data +@Accessors(chain = true) +public class PayloadVO implements Serializable { + private static final long serialVersionUID = 1L; + + /** 用户id */ + private Long userId; + /** 用户名称 */ + private String userName;This keeps the fluent setters while auto-generating
equals,hashCode, andtoString.cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java (1)
59-59: Boolean flags in constructor hurt readability & future maintenance
new CIMServerInitializer(Boolean.TRUE, Boolean.TRUE)gives no clue what the arguments mean (auth? debug?).
Introduce an explicit builder or enums, e.g.CIMServerInitializer.enableAuth().enableDebug()to make the intent self-documenting.cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/Client.java (1)
32-33: Remove stray semicolons for cleaner interface bodyThe standalone
;after each default method adds no value and clutters the interface.Also applies to: 40-41
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientConfigurationData.java (1)
26-31: Missing validation for host / port trio
host,serverPort, andhttpPortcan be null or inconsistent (e.g., negative).
Add basic sanity checks when the configuration is finalised to fail fast on mis-configuration.cim-rout-api/src/main/java/com/crossoverjie/cim/route/api/vo/res/CIMServerResVO.java (1)
19-25: Serializable class lacksserialVersionUIDAdd an explicit
private static final long serialVersionUIDto avoid deserialization issues when the class evolves.cim-client/src/main/java/com/crossoverjie/cim/client/config/AppConfiguration.java (1)
18-22: Default credentials in code may leak to productionHard-coded defaults (
userId=1,userName=test_cim) are convenient for dev but risk accidental deployment to prod.
Move them toapplication-dev.ymland keep prod values explicit.cim-server/src/main/java/com/crossoverjie/cim/server/CIMServerApplication.java (1)
47-50: Thread should be daemon or managed by ExecutorService.Creating a raw thread that never terminates prevents clean JVM shutdown and makes monitoring harder.
Consider submitting the task to a managed executor or at least:thread.setDaemon(true);cim-common/src/main/java/com/crossoverjie/cim/common/metastore/NoMetaStoreImpl.java (1)
18-20: Return an immutable empty set to avoid accidental mutation.- return Sets.newHashSet(); + return Collections.emptySet();Mutating callers would currently modify a shared empty cache with no effect; returning an immutable view makes the intent explicit.
cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java (1)
87-96: Consider early‐return for PING to reduce nesting.if (msg.getCmd() == BaseCommand.PING) { // handle ping return; }Keeps the handler flatter and avoids executing further logic accidentally when new commands are added below.
cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelOutboundDebugHandler.java (1)
21-30: Unconditional logging may flood production logs.You already gate the handler by adding it only when
debugflag is on, but double-check thatINSTANCEisn’t added unconditionally elsewhere.
Additionally, wrap the log call:if (log.isDebugEnabled()) { log.debug("hex dump …"); }to avoid string building cost when debug is off.
cim-server/src/main/java/com/crossoverjie/cim/server/init/CIMServerInitializer.java (1)
67-73: InitializeAUTH_RESexplicitly when auth is required
ClientAuthHandlerreadsAUTH_RESand treatsnullas “not authenticated”.
Explicitly setting it avoids ambiguity and is safer for future Boolean logic.- if (BooleanUtils.isTrue(authInChannelActive)) { - pipeline.addLast(new ClientAuthHandler()); - } else { - ch.attr(ChannelAttributeKeys.AUTH_RES).set(Boolean.TRUE); - } + if (BooleanUtils.isTrue(authInChannelActive)) { + ch.attr(ChannelAttributeKeys.AUTH_RES).set(Boolean.FALSE); + pipeline.addLast(new ClientAuthHandler()); + } else { + ch.attr(ChannelAttributeKeys.AUTH_RES).set(Boolean.TRUE); + }cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelInboundDebugHandler.java (1)
21-25: PropagatechannelInactivedownstream
channelInactiveoverrides the default implementation but never callssuper.channelInactive, preventing the event from reaching subsequent handlers.public void channelInactive(ChannelHandlerContext ctx) throws Exception { final Long userId = ctx.channel().attr(ChannelAttributeKeys.USER_ID).get(); log.info("user id:{}, channel is inactive", userId); + super.channelInactive(ctx); }cim-client/src/main/java/com/crossoverjie/cim/client/config/BeanConfig.java (1)
57-63: Prefer builder for immutable configuration
ClientConfigurationDatais mutated via setters; switching to a builder (similar to other objects in the codebase) would make the config immutable and thread-safe.No functional bug, just maintainability.
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandle.java (1)
15-16: Use commons-langStringUtilsinstead of Zookeeper’sImporting
org.apache.zookeeper.common.StringUtilspulls an unnecessary dependency and lacksisBlank. Replace withorg.apache.commons.lang3.StringUtilswhich is already on the classpath elsewhere.cim-forward-route/src/main/java/com/crossoverjie/cim/route/config/BeanConfig.java (1)
55-63: Avoid magic strings forregisterType; leverage the existingRegistryTypeenumUsing raw
"no"/"zk"strings is error-prone and bypasses compile-time safety.-switch (registerType) { - case "no": +switch (RegistryType.NO) { ... - case "zk": + case RegistryType.ZK:Also consider validating the property value early with
@ConfigurationPropertiesto fail fast at startup.cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandleInitializer.java (1)
36-46: Null-safe debug flag check
debugis a boxedBoolean;if (debug)throws NPE when the constructor receivesnull. UseBoolean.TRUE.equals(debug)(andFALSElikewise) for null-safety.-if (debug) { +if (Boolean.TRUE.equals(debug)) { pip.addLast(ChannelInboundDebugHandler.INSTANCE); }Apply to both inbound and outbound sections.
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java (1)
42-50: Success path never flushes the response
ctx.write("auto success ,welcome !")queues bytes but doesn’t flush; the client may never receive the acknowledgement before further reads or even before the connection is closed elsewhere.-ctx.write("auto success ,welcome !"); +ctx.writeAndFlush("auth success, welcome!");cim-server/src/main/java/com/crossoverjie/cim/server/config/AppConfiguration.java (2)
89-96: Fluent setters look good – keep them consistent.The fluent setter for
registerTypereturnsthis, which is great for a builder-like style. Verify other setters in this class follow the same pattern for consistency (some still returnvoid).
98-101: Minor: duplicate chaining logic.
setZkConnectTimeoutnow chains; earlier setters such assetZkRoot,setZkAddrstill returnvoid. Either switch them to fluent or keep all non-fluent to avoid mixed API styles.cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java (1)
3-3: Avoid wildcard imports.
import com.crossoverjie.cim.client.sdk.*;hampers readability and can introduce unexpected name collisions. Explicit imports (or IDE-configured collapses) keep the surface clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/Client.java(1 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientConfigurationData.java(2 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java(7 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandle.java(3 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandleInitializer.java(1 hunks)cim-client/pom.xml(1 hunks)cim-client/src/main/java/com/crossoverjie/cim/client/CIMClientApplication.java(1 hunks)cim-client/src/main/java/com/crossoverjie/cim/client/config/AppConfiguration.java(1 hunks)cim-client/src/main/java/com/crossoverjie/cim/client/config/BeanConfig.java(3 hunks)cim-common/pom.xml(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/enums/AuthTypeEnum.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/enums/ChannelAttributeKeys.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/enums/RegistryType.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelInboundDebugHandler.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelOutboundDebugHandler.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/metastore/NoMetaStoreImpl.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/pojo/RouteInfo.java(1 hunks)cim-forward-route/src/main/java/com/crossoverjie/cim/route/config/BeanConfig.java(4 hunks)cim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.java(3 hunks)cim-rout-api/src/main/java/com/crossoverjie/cim/route/api/vo/res/CIMServerResVO.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/CIMServerApplication.java(2 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/config/AppConfiguration.java(3 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/config/BeanConfig.java(2 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfiguration.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java(4 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/init/CIMServerInitializer.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/kit/RegistryMetaStore.java(2 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java(3 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java (2)
cim-server/src/main/java/com/crossoverjie/cim/server/util/SessionSocketHolder.java (1)
SessionSocketHolder(16-70)cim-common/src/main/java/com/crossoverjie/cim/common/util/NettyAttrUtil.java (1)
NettyAttrUtil(14-37)
cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelOutboundDebugHandler.java (3)
cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelInboundDebugHandler.java (1)
Slf4j(15-37)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandleInitializer.java (1)
Slf4j(17-51)cim-server/src/main/java/com/crossoverjie/cim/server/init/CIMServerInitializer.java (1)
Slf4j(27-76)
cim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.java (2)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java (1)
PayloadVO(9-38)cim-common/src/main/java/com/crossoverjie/cim/common/util/RouteInfoParseUtil.java (1)
RouteInfoParseUtil(15-32)
cim-server/src/main/java/com/crossoverjie/cim/server/init/CIMServerInitializer.java (3)
cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelInboundDebugHandler.java (1)
Slf4j(15-37)cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelOutboundDebugHandler.java (1)
Slf4j(15-31)cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java (1)
Slf4j(22-56)
cim-forward-route/src/main/java/com/crossoverjie/cim/route/config/BeanConfig.java (2)
cim-common/src/main/java/com/crossoverjie/cim/common/metastore/NoMetaStoreImpl.java (1)
NoMetaStoreImpl(11-36)cim-common/src/main/java/com/crossoverjie/cim/common/metastore/ZkConfiguration.java (1)
ZkConfiguration(8-12)
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java (4)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java (1)
PayloadVO(9-38)cim-common/src/main/java/com/crossoverjie/cim/common/util/StringUtil.java (1)
StringUtil(10-29)cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java (1)
Slf4j(23-71)cim-server/src/main/java/com/crossoverjie/cim/server/init/CIMServerInitializer.java (1)
Slf4j(27-76)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java (3)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandleInitializer.java (1)
Slf4j(17-51)cim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.java (1)
Slf4j(47-275)cim-client-sdk/src/test/java/com/crossoverjie/cim/client/sdk/ClientTest.java (1)
Slf4j(27-460)
🪛 ast-grep (0.38.1)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java
[warning] 25-25: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: "cim#crossoverjie"
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(java-jwt-hardcoded-secret-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / build_and_test
🔇 Additional comments (7)
cim-common/src/main/java/com/crossoverjie/cim/common/pojo/RouteInfo.java (1)
22-24: Manual no-arg constructor on afinalLombok class – verify Lombok interactionAdding the zero-arg ctor solves Jackson/Protobuf reflection needs; however note:
@Builder+@AllArgsConstructoralready generate an all-args constructor; the manual ctor is fine but remember Lombok will not add@NoArgsConstructorautomatically.- Because the class is
final, frameworks that use subclass proxies (rare for pure DTOs) cannot extend it. Confirm this is intentional.- If further Lombok annotations are added later, avoid constructor clashes by explicitly adding
@NoArgsConstructorinstead of a hand-written one.No action required if you’re confident, just keep the implications in mind.
cim-server/src/main/java/com/crossoverjie/cim/server/kit/RegistryMetaStore.java (1)
45-49: Null-safety and future registry types
StringUtils.equals(appConfiguration.getRegisterType(), RegistryType.NO)is null-safe, but every new registry style will require string comparison scattered across classes.
Migrating to theRegistryTypeenum (see previous comment) will let youswitchon the enum and remove string literals entirely.If sticking with strings, consider validating
registerTypeonce at startup and throwing if the value is unsupported to fail fast.cim-rout-api/src/main/java/com/crossoverjie/cim/route/api/vo/res/CIMServerResVO.java (1)
26-30: Duplicate constructor clashes with Lombok-generated one
@AllArgsConstructoralready creates the 4-arg constructor (includingauthToken).
Introducing another constructor with three args can lead to ambiguity and maintenance overhead.
Replace with a builder pattern or mark the additional ctor with@Tolerate.cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java (1)
75-83: Attribute key generic types should match stored values.Make sure
ChannelAttributeKeys.USER_IDis declared asAttributeKey<Long>(same forUSER_NAME→String).
A raw / mismatched type causes unchecked casts later (ctx.channel().attr(...).get()).If they are not, update enum definitions accordingly.
cim-server/src/main/java/com/crossoverjie/cim/server/config/BeanConfig.java (1)
45-52: Conditional bean wiring looks correctMaking
MetaStoreconditional onmeta.store.type=zkremoves the need for manual profiles and prevents an unused ZK client from being started when direct-connect mode is active. Well-spotted.cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelInboundDebugHandler.java (1)
28-36: Large hex dumps may flood logsWhen debug is enabled and packets are large,
hexDumpwill produce very long strings. Consider truncating or sampling to avoid log bloat, or guard with a max length.cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java (1)
134-146: Good: early auth-token validation.Fail-fast behaviour on blank
authTokenprevents useless TCP dials and keeps error semantics tight.
cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java
Outdated
Show resolved
Hide resolved
cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java
Outdated
Show resolved
Hide resolved
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/Client.java
Show resolved
Hide resolved
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientConfigurationData.java
Show resolved
Hide resolved
| @Value("${register.type:no}") | ||
| private Boolean registerType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
registerType should be String, not Boolean.
The rest of the codebase (e.g. AppConfiguration#getRegisterType(), CIMServerApplication) compares the value against string literals such as "no" and constants in RegistryType.
Declaring it as Boolean will coerce "no"/zk to false, breaking the logic and silently disabling registry startup.
- @Value("${register.type:no}")
- private Boolean registerType;
+ /**
+ * Registry type: "no" | "zk" | …
+ */
+ @Value("${register.type:no}")
+ private String registerType;Also, combining @Value with @ConfigurationProperties is redundant – pick one binder to avoid confusion.
📝 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.
| @Value("${register.type:no}") | |
| private Boolean registerType; | |
| /** | |
| * Registry type: "no" | "zk" | … | |
| */ | |
| @Value("${register.type:no}") | |
| private String registerType; |
🤖 Prompt for AI Agents
In
cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfiguration.java
at lines 23-24, change the type of the field registerType from Boolean to String
to match the expected string values like "no" or "zk" used elsewhere in the
codebase. Also, remove either the @Value annotation or the
@ConfigurationProperties annotation to avoid redundant binding and potential
confusion, choosing one consistent approach for property injection.
| // check server available | ||
| RouteInfo routeInfo = new RouteInfo(); | ||
| Set<String> availableServerList = metaStore.getAvailableServerList(); | ||
| String key = String.valueOf(loginReqVO.getUserId()); | ||
| String server = | ||
| routeHandle.routeServer(List.copyOf(availableServerList), key); | ||
| log.info("userInfo=[{}] route server info=[{}]", loginReqVO, server); | ||
| if (!CollectionUtils.isEmpty(availableServerList)) { | ||
| String key = String.valueOf(loginReqVO.getUserId()); | ||
| String server = | ||
| routeHandle.routeServer(List.copyOf(availableServerList), key); | ||
| log.info("userInfo=[{}] route server info=[{}]", loginReqVO, server); | ||
|
|
||
| RouteInfo routeInfo = RouteInfoParseUtil.parse(server); | ||
| routeInfo = commonBizService.checkServerAvailable(routeInfo, key); | ||
| routeInfo = RouteInfoParseUtil.parse(server); | ||
| routeInfo = commonBizService.checkServerAvailable(routeInfo, key); | ||
|
|
||
| //保存路由信息 | ||
| accountService.saveRouteInfo(loginReqVO, server); | ||
| } | ||
|
|
||
| //保存路由信息 | ||
| accountService.saveRouteInfo(loginReqVO, server); | ||
| // 颁发认证 Token | ||
| PayloadVO pv = new PayloadVO(); | ||
| pv.setUserId(loginReqVO.getUserId()); | ||
| pv.setUserName(loginReqVO.getUserName()); | ||
| final String token = JwtUtils.generateToken(loginReqVO.getUserId(),pv); | ||
|
|
||
| CIMServerResVO vo = | ||
| new CIMServerResVO(routeInfo.getIp(), routeInfo.getCimServerPort(), routeInfo.getHttpPort()); | ||
| new CIMServerResVO(routeInfo.getIp(), routeInfo.getCimServerPort(), routeInfo.getHttpPort(), token); | ||
| res.setDataBody(vo); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
routeInfo may be empty – will propagate null IP/port to client
When no CIM server is available, availableServerList is empty ⇒ routeInfo is the default-initialised object whose fields are null/0. Constructing CIMServerResVO with these values yields an unusable address and the client side will fail later with a NPE / connect error. Return a meaningful error instead.
-if (!CollectionUtils.isEmpty(availableServerList)) {
+if (CollectionUtils.isEmpty(availableServerList)) {
+ res.setCode(StatusEnum.SERVER_NOT_AVAILABLE.getCode());
+ res.setMessage("No CIM server available");
+ return res;
+}
+
+String key = String.valueOf(loginReqVO.getUserId());
+String server = routeHandle.routeServer(List.copyOf(availableServerList), key);
...📝 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.
| // check server available | |
| RouteInfo routeInfo = new RouteInfo(); | |
| Set<String> availableServerList = metaStore.getAvailableServerList(); | |
| String key = String.valueOf(loginReqVO.getUserId()); | |
| String server = | |
| routeHandle.routeServer(List.copyOf(availableServerList), key); | |
| log.info("userInfo=[{}] route server info=[{}]", loginReqVO, server); | |
| if (!CollectionUtils.isEmpty(availableServerList)) { | |
| String key = String.valueOf(loginReqVO.getUserId()); | |
| String server = | |
| routeHandle.routeServer(List.copyOf(availableServerList), key); | |
| log.info("userInfo=[{}] route server info=[{}]", loginReqVO, server); | |
| RouteInfo routeInfo = RouteInfoParseUtil.parse(server); | |
| routeInfo = commonBizService.checkServerAvailable(routeInfo, key); | |
| routeInfo = RouteInfoParseUtil.parse(server); | |
| routeInfo = commonBizService.checkServerAvailable(routeInfo, key); | |
| //保存路由信息 | |
| accountService.saveRouteInfo(loginReqVO, server); | |
| } | |
| //保存路由信息 | |
| accountService.saveRouteInfo(loginReqVO, server); | |
| // 颁发认证 Token | |
| PayloadVO pv = new PayloadVO(); | |
| pv.setUserId(loginReqVO.getUserId()); | |
| pv.setUserName(loginReqVO.getUserName()); | |
| final String token = JwtUtils.generateToken(loginReqVO.getUserId(),pv); | |
| CIMServerResVO vo = | |
| new CIMServerResVO(routeInfo.getIp(), routeInfo.getCimServerPort(), routeInfo.getHttpPort()); | |
| new CIMServerResVO(routeInfo.getIp(), routeInfo.getCimServerPort(), routeInfo.getHttpPort(), token); | |
| res.setDataBody(vo); | |
| // check server available | |
| RouteInfo routeInfo = new RouteInfo(); | |
| Set<String> availableServerList = metaStore.getAvailableServerList(); | |
| if (CollectionUtils.isEmpty(availableServerList)) { | |
| res.setCode(StatusEnum.SERVER_NOT_AVAILABLE.getCode()); | |
| res.setMessage("No CIM server available"); | |
| return res; | |
| } | |
| String key = String.valueOf(loginReqVO.getUserId()); | |
| String server = routeHandle.routeServer(List.copyOf(availableServerList), key); | |
| log.info("userInfo=[{}] route server info=[{}]", loginReqVO, server); | |
| routeInfo = RouteInfoParseUtil.parse(server); | |
| routeInfo = commonBizService.checkServerAvailable(routeInfo, key); | |
| // 保存路由信息 | |
| accountService.saveRouteInfo(loginReqVO, server); | |
| // 颁发认证 Token | |
| PayloadVO pv = new PayloadVO(); | |
| pv.setUserId(loginReqVO.getUserId()); | |
| pv.setUserName(loginReqVO.getUserName()); | |
| final String token = JwtUtils.generateToken(loginReqVO.getUserId(), pv); | |
| CIMServerResVO vo = | |
| new CIMServerResVO( | |
| routeInfo.getIp(), | |
| routeInfo.getCimServerPort(), | |
| routeInfo.getHttpPort(), | |
| token); | |
| res.setDataBody(vo); |
🤖 Prompt for AI Agents
In
cim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.java
around lines 181 to 205, the code uses a default-initialized routeInfo object
with null or zero fields when no CIM server is available, causing the client to
receive invalid IP/port and potentially fail. To fix this, add a check after
retrieving availableServerList to detect if it is empty; if so, return a
meaningful error response or throw an exception instead of proceeding to
construct CIMServerResVO with invalid data. This prevents propagating null or
zero values to the client and avoids downstream errors.
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java
Outdated
Show resolved
Hide resolved
cim-server/src/main/java/com/crossoverjie/cim/server/config/AppConfiguration.java
Show resolved
Hide resolved
| @Override | ||
| public String getHost() { | ||
| // 优先使用直连的方式 | ||
| return StringUtils.isNoneBlank(conf.getHost()) ? conf.getHost() : serverInfo.getIp(); | ||
| } | ||
|
|
||
| @Override | ||
| public Integer getPort() { | ||
| return Objects.nonNull(conf.getServerPort()) ? conf.getServerPort() : serverInfo.getCimServerPort(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement the null-safe accessors suggested above.
Accessors still assume serverInfo is non-null. Integrate the diff shown in the previous comment to eliminate potential crashes during first-time connection or retry loops.
🤖 Prompt for AI Agents
In
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
around lines 253 to 262, the getHost and getPort methods assume serverInfo is
non-null, risking NullPointerExceptions. Modify these methods to safely check if
serverInfo is null before accessing its fields, returning a default or null
value if serverInfo is null. This will prevent crashes during initial
connections or retries by ensuring null-safe access to serverInfo properties.
| private void doConnectServer(CompletableFuture<Boolean> future) { | ||
| Bootstrap bootstrap = new Bootstrap(); | ||
| bootstrap.group(group) | ||
| .channel(NioSocketChannel.class) | ||
| .handler(new CIMClientHandleInitializer()); | ||
| .handler(new CIMClientHandleInitializer(Boolean.TRUE, getAuth())); | ||
| ChannelFuture sync; | ||
| try { | ||
| sync = bootstrap.connect(cimServer.getIp(), cimServer.getCimServerPort()).sync(); | ||
| final String host = getHost(); | ||
| final Integer port = getPort(); | ||
| if (StringUtils.isBlank(host) || Objects.isNull(port)) { | ||
| this.conf.getEvent().error("cim server host or port is null"); | ||
| future.complete(false); | ||
| return; | ||
| } | ||
| sync = bootstrap.connect(host, port).sync(); | ||
| if (sync.isSuccess()) { |
There was a problem hiding this comment.
Possible NPE before null-check when resolving host/port.
getHost() / getPort() may dereference serverInfo when it is still null (e.g., startup failure, concurrent reconnect). That produces an immediate NullPointerException before your explicit blank/null guard below.
final String host = getHost(); // ← may NPE
final Integer port = getPort(); // ← may NPEGuard inside the accessor methods:
public String getHost() {
- return StringUtils.isNoneBlank(conf.getHost()) ? conf.getHost() : serverInfo.getIp();
+ if (StringUtils.isNotBlank(conf.getHost())) {
+ return conf.getHost();
+ }
+ return serverInfo != null ? serverInfo.getIp() : null;
}
public Integer getPort() {
- return Objects.nonNull(conf.getServerPort()) ? conf.getServerPort() : serverInfo.getCimServerPort();
+ if (conf.getServerPort() != null) {
+ return conf.getServerPort();
+ }
+ return serverInfo != null ? serverInfo.getCimServerPort() : null;
}📝 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.
| private void doConnectServer(CompletableFuture<Boolean> future) { | |
| Bootstrap bootstrap = new Bootstrap(); | |
| bootstrap.group(group) | |
| .channel(NioSocketChannel.class) | |
| .handler(new CIMClientHandleInitializer()); | |
| .handler(new CIMClientHandleInitializer(Boolean.TRUE, getAuth())); | |
| ChannelFuture sync; | |
| try { | |
| sync = bootstrap.connect(cimServer.getIp(), cimServer.getCimServerPort()).sync(); | |
| final String host = getHost(); | |
| final Integer port = getPort(); | |
| if (StringUtils.isBlank(host) || Objects.isNull(port)) { | |
| this.conf.getEvent().error("cim server host or port is null"); | |
| future.complete(false); | |
| return; | |
| } | |
| sync = bootstrap.connect(host, port).sync(); | |
| if (sync.isSuccess()) { | |
| public String getHost() { | |
| if (StringUtils.isNotBlank(conf.getHost())) { | |
| return conf.getHost(); | |
| } | |
| return serverInfo != null ? serverInfo.getIp() : null; | |
| } | |
| public Integer getPort() { | |
| if (conf.getServerPort() != null) { | |
| return conf.getServerPort(); | |
| } | |
| return serverInfo != null ? serverInfo.getCimServerPort() : null; | |
| } |
🤖 Prompt for AI Agents
In
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
around lines 175 to 190, the methods getHost() and getPort() may cause a
NullPointerException if serverInfo is null before the explicit null/blank checks
in doConnectServer. To fix this, add null checks inside the getHost() and
getPort() accessor methods to safely handle the case when serverInfo is null,
returning null or a default value instead of dereferencing it directly.
- 增加参数校验和异常处理 - 优化连接逻辑，增加对服务器地址和端口的检查 - 改进配置类，为 Auth 类添加 @JsonIgnore 注解 -调整 Client 接口方法名称，增加 check 前缀 - 修改 ServerConfiguration 中注册类型为枚举类型 - 优化 CIMServer 启动逻辑，使用配置端口
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientBuilderImpl.java (1)
35-41: 💡 Verification agent🧩 Analysis chain
auth()should null-check the argument and validate the new token
authcan be passed asnull, which would raise an NPE onauth.getUserId().
With the new authentication layer, an empty/blankauthTokenshould also be rejected.+ Objects.requireNonNull(auth, "auth must not be null"); - if (auth.getUserId() <= 0 || StringUtil.isEmpty(auth.getUserName())) { + if (auth.getUserId() <= 0 + || StringUtil.isEmpty(auth.getUserName()) + || StringUtil.isEmpty(auth.getAuthToken())) { throw new IllegalArgumentException("userId, userName and authToken must be set"); }
🏁 Script executed:
#!/bin/bash echo "Locating Auth class definition:" rg -n "class Auth" -n . echo "Checking for authToken field or method in Auth:" rg -n "authToken" -n . echo "Verifying presence of getAuthToken():" rg -n "getAuthToken" -n .Length of output: 1309
Ensure auth() handles null Auth and validates authToken
The builder’s auth(ClientConfigurationData.Auth) method should:
- Reject a null auth argument before accessing its properties
- Verify that authToken is neither empty nor blank
At
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientBuilderImpl.javaupdate the method as follows:public ClientBuilder auth(ClientConfigurationData.Auth auth) { + Objects.requireNonNull(auth, "auth must not be null"); - if (auth.getUserId() <= 0 || StringUtil.isEmpty(auth.getUserName())) { + if (auth.getUserId() <= 0 + || StringUtil.isEmpty(auth.getUserName()) + || StringUtil.isEmpty(auth.getAuthToken())) { throw new IllegalArgumentException("userId, userName and authToken must be set"); } this.conf.setAuth(auth); return this; }
- Import
java.util.Objects- Ensure
Authhas the correspondinggetAuthToken()getter (it does inClientConfigurationData.java)
🧹 Nitpick comments (1)
cim-common/src/main/java/com/crossoverjie/cim/common/enums/RegistryType.java (1)
24-30: Consider utility lookup & validation helpersTypical enum patterns expose a
fromCode(String)orboolean isValid(String)for safer external usage instead of scatteringequalschecks across the codebase.public static RegistryType fromCode(String code) { for (RegistryType t : values()) { if (t.code.equals(code)) { return t; } } throw new IllegalArgumentException("Unknown registry type: " + code); }Optional but boosts readability and centralises validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/Client.java(1 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientBuilderImpl.java(2 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientConfigurationData.java(2 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java(7 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/enums/RegistryType.java(1 hunks)cim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.java(3 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/CIMServerApplication.java(2 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/config/AppConfiguration.java(3 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfiguration.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/kit/RegistryMetaStore.java(2 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- cim-server/src/main/java/com/crossoverjie/cim/server/kit/RegistryMetaStore.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/Client.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientConfigurationData.java
- cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java
- cim-server/src/main/java/com/crossoverjie/cim/server/config/AppConfiguration.java
- cim-server/src/main/java/com/crossoverjie/cim/server/CIMServerApplication.java
- cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfiguration.java
- cim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.java
- cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientBuilderImpl.java (1)
cim-common/src/main/java/com/crossoverjie/cim/common/util/StringUtil.java (1)
StringUtil(10-29)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / build_and_test
🔇 Additional comments (1)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientBuilderImpl.java (1)
24-28: Nice defensive null-check on constructor parameter
Objects.requireNonNull(conf, …)prevents an easily-missed NPE and gives a clear message – well done.
cim-common/src/main/java/com/crossoverjie/cim/common/enums/RegistryType.java
Show resolved
Hide resolved
- 修改 RegistryType 构造函数的参数顺序
- 修改注册中心类型枚举，将 ZOOKEEPER 改为 ZK - 更新配置文件中的注册中心类型键值对 - 调整代码中对注册中心类型的引用
…tive 方法 - 在 channelInactive 方法中添加 fireChannelInactive 以确保通道正确关闭
1. 删除默认连接端口
1. 删除默认连接端口
1. fix NPE（虽然我也不知道错在哪里
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cim-integration-test/src/main/java/com/crossoverjie/cim/client/sdk/route/AbstractRouteBaseTest.java (2)
22-22: Field namerunis too generic – prefer a more descriptive identifier.Using a verb for a member variable can be confusing and clashes with
Thread#runsemantics.
Suggestion: rename to something likerouteContextto convey its purpose.- protected ConfigurableApplicationContext run; + protected ConfigurableApplicationContext routeContext;
37-42: Nice null-safety, but container shutdown is duplicated & could be more robust.
@Containeralready manages the Redis container lifecycle; manually invokingredis.close()risks double-shutdown or masking JUnit-managed behaviour.- It’s safer to guard Redis shutdown the same way you guarded the Spring context, e.g. check
isRunning().- Consider
try { … } finally { … }or putting both resources insideAutoCloseables to guarantee order & idempotence.- super.close(); - redis.close(); - if (Objects.nonNull(run)) { - run.close(); + super.close(); + if (redis != null && redis.isRunning()) { + redis.stop(); + } + if (Objects.nonNull(routeContext)) { + routeContext.close(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cim-integration-test/src/main/java/com/crossoverjie/cim/client/sdk/route/AbstractRouteBaseTest.java(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cim-integration-test/src/main/java/com/crossoverjie/cim/client/sdk/route/AbstractRouteBaseTest.java (1)
cim-common/src/main/java/com/crossoverjie/cim/common/res/BaseResponse.java (1)
BaseResponse(10-101)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / build_and_test
🔇 Additional comments (2)
cim-integration-test/src/main/java/com/crossoverjie/cim/client/sdk/route/AbstractRouteBaseTest.java (2)
4-4: Import addition looks correct.
BaseResponseis now referenced unqualified below, so the new import is needed and accurate.
15-15: Import is fine but currently only used once.
java.util.Objectsis introduced solely for the null-check. No action needed, just noting to keep the import list lean if usages change.
1. 删除默认直连地址
- 修改认证失败后的处理逻辑，关闭连接并记录日志- 认证成功后记录用户信息，并设置通道属性 -优化日志输出，增加用户信息 - 修复一些潜在的 NPE 问题
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cim-client-sdk/src/test/java/com/crossoverjie/cim/client/sdk/OfflineMsgTest.java (1)
126-129: Consider dropping thefinalClientindirection
finalClientis only introduced to satisfy the “effectively-final” rule inside the lambda.
You can avoid the extra variable (and a potential source of confusion) by capturing a final copy with a clearer name or by moving the assertion intoAwaitility’s supplier form:-Client finalClient = client3; -Awaitility.await().atMost(10, TimeUnit.SECONDS) - .untilAsserted(() -> Assertions.assertEquals(ClientState.State.Ready, finalClient.getState())); +final Client client3Copy = client3; // explicit intent +Awaitility.await().atMost(10, TimeUnit.SECONDS) + .untilAsserted(() -> Assertions.assertEquals(ClientState.State.Ready, client3Copy.getState()));Purely cosmetic, but it tightens readability and avoids a name that suggests immutability of the underlying client.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/Client.java(1 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java(7 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandle.java(3 hunks)cim-client-sdk/src/test/java/com/crossoverjie/cim/client/sdk/OfflineMsgTest.java(2 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelInboundDebugHandler.java(1 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelOutboundDebugHandler.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/Client.java
- cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelOutboundDebugHandler.java
- cim-common/src/main/java/com/crossoverjie/cim/common/handler/ChannelInboundDebugHandler.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandle.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
🔇 Additional comments (1)
cim-client-sdk/src/test/java/com/crossoverjie/cim/client/sdk/OfflineMsgTest.java (1)
12-13: Explicit JUnit imports – nice clean-up
Replacing the wildcard import with the two concrete classes improves compile-time safety and keeps the imported namespace minimal.
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
Outdated
Show resolved
Hide resolved
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
Outdated
Show resolved
Hide resolved
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
Outdated
Show resolved
Hide resolved
1. 修改认证拦截
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java (1)
61-64: Fix duplicate write issue in failure handling.This is a duplicate of a previous review comment. The current code still has the issue where failure handling doesn't return early after the catch block, causing
writeAndCloseto be called twice.} catch (Exception e) { log.error("client auto failure,e:", e); + writeAndClose("Authentication failed", ctx); + return; } -writeAndClose("auto failure!", ctx);
🧹 Nitpick comments (1)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java (1)
31-65: Consider adding equals, hashCode, and toString methods.As a DTO class, having proper
equals(),hashCode(), andtoString()methods would improve debugging and enable proper comparison operations.You can add these methods at the end of the class:
public PayloadVO setPort(Integer port) { this.port = port; return this; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + PayloadVO payloadVO = (PayloadVO) o; + return Objects.equals(userId, payloadVO.userId) && + Objects.equals(userName, payloadVO.userName) && + Objects.equals(host, payloadVO.host) && + Objects.equals(port, payloadVO.port); + } + + @Override + public int hashCode() { + return Objects.hash(userId, userName, host, port); + } + + @Override + public String toString() { + return "PayloadVO{" + + "userId=" + userId + + ", userName='" + userName + '\'' + + ", host='" + host + '\'' + + ", port=" + port + + '}'; + } }Remember to add
import java.util.Objects;at the top.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientConfigurationData.java(1 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java(7 hunks)cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandleInitializer.java(1 hunks)cim-client-sdk/src/test/java/com/crossoverjie/cim/client/sdk/ClientTest.java(7 hunks)cim-client/src/test/java/com/crossoverjie/cim/server/test/CommonTest.java(7 hunks)cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java(1 hunks)cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- cim-client/src/test/java/com/crossoverjie/cim/server/test/CommonTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java
- cim-client-sdk/src/test/java/com/crossoverjie/cim/client/sdk/ClientTest.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientConfigurationData.java
- cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java
🔇 Additional comments (2)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/io/CIMClientHandleInitializer.java (2)
24-28: LGTM!The constructor properly accepts authentication configuration and debug flag, enabling conditional pipeline setup.
32-49: Well-structured pipeline initialization.The pipeline setup is correctly ordered with proper separation of concerns:
- IdleStateHandler for connection management
- Conditional debug handlers for troubleshooting
- Protobuf decoders and encoders
- Business handler with authentication context
The explicit separation of decoder and encoder sections improves readability.
cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java
Show resolved
Hide resolved
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java
Show resolved
Hide resolved
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java
Show resolved
Hide resolved
| } | ||
|
|
||
| // 处理 Token | ||
| final String autoToken = ((Request) msg).getReqMsg(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety check before casting to Request.
The code directly casts msg to Request without type checking, which could cause ClassCastException if unexpected message types are received.
+if (!(msg instanceof Request)) {
+ log.warn("Received non-Request message during authentication: {}", msg.getClass());
+ writeAndClose("Invalid message type", ctx);
+ return;
+}
+
final String autoToken = ((Request) msg).getReqMsg();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java
at line 45, the code casts msg to Request without verifying its type, risking a
ClassCastException. Add an instanceof check to confirm msg is an instance of
Request before casting. If the check fails, handle the situation appropriately,
such as logging an error or ignoring the message, to ensure type safety.
| private void writeAndClose(String msg, ChannelHandlerContext ctx) { | ||
| // 当前的 Handler 还在就说明链接还未认证 | ||
| ctx.writeAndFlush("auto failure!") | ||
| .addListener((ChannelFutureListener) future -> { | ||
| if (future.isDone()) { | ||
| log.info("client auth failure,close the channel."); | ||
| ctx.close(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix writeAndClose method to use the provided message parameter.
The method accepts a msg parameter but ignores it, hardcoding "auto failure!" instead. This reduces flexibility and could confuse callers.
private void writeAndClose(String msg, ChannelHandlerContext ctx) {
// 当前的 Handler 还在就说明链接还未认证
- ctx.writeAndFlush("auto failure!")
+ ctx.writeAndFlush(msg)
.addListener((ChannelFutureListener) future -> {
if (future.isDone()) {
log.info("client auth failure,close the channel.");
ctx.close();
}
});
}📝 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.
| private void writeAndClose(String msg, ChannelHandlerContext ctx) { | |
| // 当前的 Handler 还在就说明链接还未认证 | |
| ctx.writeAndFlush("auto failure!") | |
| .addListener((ChannelFutureListener) future -> { | |
| if (future.isDone()) { | |
| log.info("client auth failure,close the channel."); | |
| ctx.close(); | |
| } | |
| }); | |
| } | |
| private void writeAndClose(String msg, ChannelHandlerContext ctx) { | |
| // 当前的 Handler 还在就说明链接还未认证 | |
| ctx.writeAndFlush(msg) | |
| .addListener((ChannelFutureListener) future -> { | |
| if (future.isDone()) { | |
| log.info("client auth failure,close the channel."); | |
| ctx.close(); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java
around lines 74 to 83, the writeAndClose method ignores the provided msg
parameter and instead sends a hardcoded "auto failure!" message. To fix this,
replace the hardcoded string with the msg parameter in the ctx.writeAndFlush
call so that the method uses the passed-in message, improving flexibility and
clarity.
1. 三方接口返回变更导致测试失败
1. 删除 MockBean
1. 删除 MockBean
…ure/first_auth_v1 # Conflicts: # cim-client-sdk/src/test/java/com/crossoverjie/cim/client/sdk/ClientTest.java
1. 新增测试用例 未鉴权读消息报错
1. add JwtUtils test case
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java (2)
26-26: Hard-coded secret poses security risk.The SECRET key is embedded in source code, which is a credentials leak risk (CWE-798). This should be externalized to configuration.
Move the secret to external configuration:
-private static final String SECRET = "cim#crossoverjie"; +@Value("${cim.jwt.secret}") +private static String SECRET;Or use environment variables:
-private static final String SECRET = "cim#crossoverjie"; +private static final String SECRET = System.getenv("CIM_JWT_SECRET");
52-54: Missing issuer validation in token verification.The token verification does not validate the issuer, allowing tokens from any source with the same secret to be accepted. This is a security vulnerability.
final DecodedJWT decodedObj = JWT.require(ALGORITHM) + .withIssuer(issuer) .build() .verify(token);
🧹 Nitpick comments (2)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java (2)
34-45: Consider security implications of FastJSON serialization.The method uses FastJSON to serialize the payload, which has had security vulnerabilities in the past. Consider using Jackson or other more secure serialization libraries.
-import com.alibaba.fastjson.JSONObject; +import com.fasterxml.jackson.databind.ObjectMapper; -final JSONObject payload = JSONObject.parseObject(JSONObject.toJSONString(p)); +ObjectMapper mapper = new ObjectMapper(); +final Map<String, Object> payload = mapper.convertValue(p, Map.class);
66-70: Consider removing redundant header creation.The JWT library typically handles headers automatically. If custom headers are needed, derive the algorithm name from the ALGORITHM constant rather than hardcoding it.
private static Map<String, Object> createHeader() { Map<String, Object> header = new HashMap<>(); - header.put("alg", "HS256"); + header.put("alg", ALGORITHM.getName()); return header; }Or consider removing this method entirely if no custom headers are needed:
return JWT.create() .withIssuer(issuer) .withSubject(String.valueOf(userId)) .withIssuedAt(new Date()) - .withHeader(createHeader()) .withPayload(payload) .sign(ALGORITHM);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java(1 hunks)cim-common/src/test/java/com/crossoverjie/cim/common/auth/JwtAuthTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cim-common/src/test/java/com/crossoverjie/cim/common/auth/JwtAuthTest.java (1)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.java (1)
PayloadVO(9-66)
🪛 ast-grep (0.38.6)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java
[warning] 25-25: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: "cim#crossoverjie"
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(java-jwt-hardcoded-secret-java)
⏰ 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). (1)
- GitHub Check: build / build_and_test
| final PayloadVO vo = new PayloadVO(); | ||
| vo.setUserName(decodedObj.getClaim("userName").as(String.class)); | ||
| vo.setUserId(decodedObj.getClaim("userId").as(Long.class)); | ||
| return vo; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete payload reconstruction.
The verification only reconstructs userId and userName from the token but ignores host and port fields that were included in the payload during token generation. This could cause issues if the full payload is needed.
final PayloadVO vo = new PayloadVO();
vo.setUserName(decodedObj.getClaim("userName").as(String.class));
vo.setUserId(decodedObj.getClaim("userId").as(Long.class));
+vo.setHost(decodedObj.getClaim("host").as(String.class));
+vo.setPort(decodedObj.getClaim("port").as(Integer.class));
return vo;📝 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.
| final PayloadVO vo = new PayloadVO(); | |
| vo.setUserName(decodedObj.getClaim("userName").as(String.class)); | |
| vo.setUserId(decodedObj.getClaim("userId").as(Long.class)); | |
| return vo; | |
| final PayloadVO vo = new PayloadVO(); | |
| vo.setUserName(decodedObj.getClaim("userName").as(String.class)); | |
| vo.setUserId(decodedObj.getClaim("userId").as(Long.class)); | |
| vo.setHost(decodedObj.getClaim("host").as(String.class)); | |
| vo.setPort(decodedObj.getClaim("port").as(Integer.class)); | |
| return vo; |
🤖 Prompt for AI Agents
In cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java
around lines 56 to 59, the code reconstructs the PayloadVO from the token but
only sets userId and userName, omitting the host and port fields included during
token generation. To fix this, extract the host and port claims from the decoded
token and set them on the PayloadVO object before returning it, ensuring the
full payload is properly reconstructed.
| @Test | ||
| public void testJwtToken() { | ||
| Long userId = 111L; | ||
| String userName = "chenqwwq"; | ||
| String host = "127.0.0.1"; | ||
| Integer port = 8888; | ||
| PayloadVO vo = new PayloadVO(); | ||
| vo.setUserId(userId); | ||
| vo.setUserName(userName); | ||
| vo.setHost(host); | ||
| vo.setPort(port); | ||
|
|
||
| // generate a jwt token | ||
| final String token = JwtUtils.generateToken(userId, vo); | ||
| // verify | ||
| final PayloadVO payloadVO = JwtUtils.verifyToken(token); | ||
| Assert.assertEquals(userId, payloadVO.getUserId()); | ||
| Assert.assertEquals(userName, payloadVO.getUserName()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for comprehensive JWT validation.
The current test only covers the happy path scenario. Consider adding tests for:
- All PayloadVO fields: Currently only
userIdanduserNameare verified, buthostandportare not tested. - Negative test cases: Invalid tokens, null inputs, malformed tokens.
- Edge cases: Empty strings, special characters in username.
@Test
public void testJwtToken() {
Long userId = 111L;
String userName = "chenqwwq";
String host = "127.0.0.1";
Integer port = 8888;
PayloadVO vo = new PayloadVO();
vo.setUserId(userId);
vo.setUserName(userName);
vo.setHost(host);
vo.setPort(port);
// generate a jwt token
final String token = JwtUtils.generateToken(userId, vo);
// verify
final PayloadVO payloadVO = JwtUtils.verifyToken(token);
Assert.assertEquals(userId, payloadVO.getUserId());
Assert.assertEquals(userName, payloadVO.getUserName());
+ Assert.assertEquals(host, payloadVO.getHost());
+ Assert.assertEquals(port, payloadVO.getPort());
}
+
+@Test(expected = IllegalArgumentException.class)
+public void testVerifyToken_WithNullToken() {
+ JwtUtils.verifyToken(null);
+}
+
+@Test(expected = IllegalArgumentException.class)
+public void testVerifyToken_WithEmptyToken() {
+ JwtUtils.verifyToken("");
+}
+
+@Test(expected = JWTDecodeException.class)
+public void testVerifyToken_WithInvalidToken() {
+ JwtUtils.verifyToken("invalid.token.here");
+}📝 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.
| @Test | |
| public void testJwtToken() { | |
| Long userId = 111L; | |
| String userName = "chenqwwq"; | |
| String host = "127.0.0.1"; | |
| Integer port = 8888; | |
| PayloadVO vo = new PayloadVO(); | |
| vo.setUserId(userId); | |
| vo.setUserName(userName); | |
| vo.setHost(host); | |
| vo.setPort(port); | |
| // generate a jwt token | |
| final String token = JwtUtils.generateToken(userId, vo); | |
| // verify | |
| final PayloadVO payloadVO = JwtUtils.verifyToken(token); | |
| Assert.assertEquals(userId, payloadVO.getUserId()); | |
| Assert.assertEquals(userName, payloadVO.getUserName()); | |
| } | |
| @Test | |
| public void testJwtToken() { | |
| Long userId = 111L; | |
| String userName = "chenqwwq"; | |
| String host = "127.0.0.1"; | |
| Integer port = 8888; | |
| PayloadVO vo = new PayloadVO(); | |
| vo.setUserId(userId); | |
| vo.setUserName(userName); | |
| vo.setHost(host); | |
| vo.setPort(port); | |
| // generate a jwt token | |
| final String token = JwtUtils.generateToken(userId, vo); | |
| // verify | |
| final PayloadVO payloadVO = JwtUtils.verifyToken(token); | |
| Assert.assertEquals(userId, payloadVO.getUserId()); | |
| Assert.assertEquals(userName, payloadVO.getUserName()); | |
| Assert.assertEquals(host, payloadVO.getHost()); | |
| Assert.assertEquals(port, payloadVO.getPort()); | |
| } | |
| @Test(expected = IllegalArgumentException.class) | |
| public void testVerifyToken_WithNullToken() { | |
| JwtUtils.verifyToken(null); | |
| } | |
| @Test(expected = IllegalArgumentException.class) | |
| public void testVerifyToken_WithEmptyToken() { | |
| JwtUtils.verifyToken(""); | |
| } | |
| @Test(expected = JWTDecodeException.class) | |
| public void testVerifyToken_WithInvalidToken() { | |
| JwtUtils.verifyToken("invalid.token.here"); | |
| } |
🤖 Prompt for AI Agents
In cim-common/src/test/java/com/crossoverjie/cim/common/auth/JwtAuthTest.java
around lines 13 to 31, expand the testJwtToken method to verify all fields of
PayloadVO including host and port, add negative test cases for invalid, null,
and malformed tokens, and include edge cases such as empty strings and special
characters in the username to ensure comprehensive JWT validation coverage.
1. fix null and not instanceOf
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores/Dependencies