Skip to content

Conversation

@ZStack-Robot
Copy link
Collaborator

Resolves: ZSTAC-76390

Change-Id: I62646e64637a67777a667768686373626f696d72

sync from gitlab !8454

@coderabbitai
Copy link

coderabbitai bot commented Sep 22, 2025

Warning

Rate limit exceeded

@MatheMatrix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 23 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 18e7ca0 and c7dbbdb.

⛔ Files ignored due to path filters (1)
  • conf/springConfigXml/AccountManager.xml is excluded by !**/*.xml
📒 Files selected for processing (2)
  • identity/src/main/java/org/zstack/identity/AccountManagerImpl.java (3 hunks)
  • identity/src/main/java/org/zstack/identity/BeforeLoginInAccountPoint.java (1 hunks)

Walkthrough

在登录流程中引入“登录前”扩展点:新增接口 BeforeLoginInAccountPoint,并在 AccountManagerImpl 中实现与调用该扩展点。处理 APILogInByAccountMsg 时在登录前触发扩展回调,并新增 beforeLogin(APISessionMessage) 用于检查第三方账户的本地登录限制。

Changes

Cohort / File(s) Change Summary
扩展点接口新增
identity/src/main/java/org/zstack/identity/BeforeLoginInAccountPoint.java
新增公共接口 BeforeLoginInAccountPoint,定义 beforeLogin(APISessionMessage) 回调,作为账户登录前的钩子。
AccountManager 集成与控制流调整
identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
类签名新增实现 BeforeLoginInAccountPoint;新增并实现 beforeLogin(APISessionMessage);在处理 APILogInByAccountMsg 时迭代调用所有 BeforeLoginInAccountPoint 扩展;在 beforeLogin 中对第三方账户的本地登录进行校验并抛出异常。

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant C as Client
    participant API as API Gateway
    participant AM as AccountManagerImpl
    participant EP as BeforeLoginInAccountPoint (extensions)

    C->>API: APILogInByAccountMsg
    API->>AM: handle(msg)
    rect rgba(230,245,255,0.6)
      note over AM,EP: 登录前扩展点调用(新增)
      AM->>EP: forEach.ext.beforeLogin(msg)
      EP-->>AM: 校验结果 / 异常
    end
    alt 校验通过
      AM->>AM: 执行后续认证流程
      AM-->>API: 登录成功/失败结果
      API-->>C: 响应
    else 拦截/异常
      AM-->>API: 抛出异常(禁止第三方本地登录)
      API-->>C: 错误响应
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

小兔敲键啪嗒嗒,
登入之前先盘查。
钩子一声把关卡,
第三方呀别乱闯啦。
代码稳稳不喧哗,
月下胡萝卜也笑呀 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed 标题 "[feature][identity]: not allowed thirdParty, OAuth2 local login" 明确指出主要改动是禁止 thirdParty/OAuth2 类型账户进行本地登录,这与 changeset 中在 AccountManagerImpl 中新增 beforeLogin 检查并引入 BeforeLoginInAccountPoint 接口的修改直接对应,措辞简洁且聚焦主旨,便于团队快速理解主要变更。
Description Check ✅ Passed PR 描述非常简短,仅包含问题跟踪号、Change-Id 和“sync from gitlab”说明,但这些信息与变更集相关且并非完全离题,因此按宽松检查规则该描述通过。

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
identity/src/main/java/org/zstack/identity/AccountManagerImpl.java (2)

531-533: 建议使用传统 for 循环替代 forEach

虽然 Lambda 表达式简洁,但在此处使用传统的 for 循环或增强 for 循环会更符合代码库的整体风格,且在异常处理时更加清晰。

建议修改为:

-        pluginRgty.getExtensionList(BeforeLoginInAccountPoint.class).forEach(point->{
-            point.beforeLogin(msg);
-        });
+        for (BeforeLoginInAccountPoint point : pluginRgty.getExtensionList(BeforeLoginInAccountPoint.class)) {
+            point.beforeLogin(msg);
+        }

1802-1803: AccountType 可以直接使用,无需创建局部变量

AccountType.ThirdParty 是一个枚举常量,可以直接在比较时使用,无需创建局部变量。

建议简化代码:

-        String name = sessionMessage.getUsername();
-        AccountType thirdPartyType = AccountType.ThirdParty;
-
-        AccountVO account = Q.New(AccountVO.class).eq(AccountVO_.name, name).find();
+        String name = sessionMessage.getUsername();
+        AccountVO account = Q.New(AccountVO.class).eq(AccountVO_.name, name).find();

         if (account == null) {
             return;
         }

-        if (account.getType() == thirdPartyType) {
-            throw new CloudRuntimeException(String.format("Account [name=%s] [type=%s] cannot local login", name, thirdPartyType));
+        if (account.getType() == AccountType.ThirdParty) {
+            throw new CloudRuntimeException(String.format("Account [name=%s] [type=%s] cannot local login", name, AccountType.ThirdParty));
         }
📜 Review details

Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1790d8 and 18e7ca0.

📒 Files selected for processing (2)
  • identity/src/main/java/org/zstack/identity/AccountManagerImpl.java (3 hunks)
  • identity/src/main/java/org/zstack/identity/BeforeLoginInAccountPoint.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.*

⚙️ CodeRabbit configuration file

**/*.*: - 代码里不应当有有中文,包括报错、注释等都应当使用正确的、无拼写错误的英文来写

Files:

  • identity/src/main/java/org/zstack/identity/BeforeLoginInAccountPoint.java
  • identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
**/*.java

⚙️ CodeRabbit configuration file

**/*.java: ## 1. API 设计要求

  • API 命名:
    • API 名称必须唯一,不能重复。
    • API 消息类需要继承 APIMessage;其返回类必须继承 APIReplyAPIEvent,并在注释中用 @RestResponse 进行标注。
    • API 消息上必须添加注解 @RestRequest,并满足如下规范:
      • path:
        • 针对资源使用复数形式。
        • 当 path 中引用消息类变量时,使用 {variableName} 格式。
      • HTTP 方法对应:
        • 查询操作 → HttpMethod.GET
        • 更新操作 → HttpMethod.PUT
        • 创建操作 → HttpMethod.POST
        • 删除操作 → HttpMethod.DELETE
    • API 类需要实现 __example__ 方法以便生成 API 文档,并确保生成对应的 Groovy API Template 与 API Markdown 文件。

2. 命名与格式规范

  • 类名:

    • 使用 UpperCamelCase 风格。
    • 特殊情况:
      • VO/AO/EO 类型类除外。
      • 抽象类采用 AbstractBase 前缀/后缀。
      • 异常类应以 Exception 结尾。
      • 测试类需要以 TestCase 结尾。
  • 方法名、参数名、成员变量和局部变量:

    • 使用 lowerCamelCase 风格。
  • 常量命名:

    • 全部大写,使用下划线分隔单词。
    • 要求表达清楚,避免使用含糊或不准确的名称。
  • 包名:

    • 统一使用小写,使用点分隔符,每个部分应是一个具有自然语义的英文单词(参考 Spring 框架的结构)。
  • 命名细节:

    • 避免在父子类或同一代码块中出现相同名字的成员或局部变量,防止混淆。
    • 命名缩写:
      • 不允许使用不必要的缩写,如:AbsSchedulerJobcondiFu 等。应使用完整单词提升可读性。

3. 编写自解释代码

  • 意图表达:

    • 避免使用布尔型参数造成含义不明确。例如:
      • 对于 stopAgent(boolean ignoreError),建议拆分为不同函数(如 stopAgentIgnoreError()),或使用枚举表达操作类型。
    • 命名应尽量用完整的单词组合表达意图,并在名称中体现数据类型或用途(例如在常量与变量名称中,将类型词放在末尾)。
    • 避免使用魔法值(Magic Value):
      • 直接使用未经定义的数值或字符串(如 if (status == 5))应替换为枚举或常量。

      • 示例:

      • // 错误示例:魔法值

      • if (user.getStatus() == 5) { ... }

      • // 正确示例:常量或枚举

      • public static final int STATUS_ACTIVE = 5;

      • if (user.getStatus() == STATUS_ACTIVE) { ... }

      • // 或使用枚举

      • enum UserStatus { ACTIVE, INACTIVE }

  • 注释:

    • 代码应尽量做到自解释,对少于两行的说明可以直接写在代码中。
    • 对于较长的注释,需要仔细校对并随代码更新,确保内容正确。
    • 接口方法不应有多余的修饰符(例如 public),且必须配有有效的 Javadoc 注释。

4. 流程控制和结构优化

  • if...else 的使用:

    • 应尽量减少 if...else 结构的使用,建议:
      • 限制嵌套层级最多为两层,且内层不应再出现 else 分支。
      • 尽早返回(Early Return),将条件判断中的处理逻辑提前结束或抽成独立方法。
      • 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
  • 条件判断:

    • if 条件表达不宜过长或过于复杂,必要时可以将条件抽成 boolean 变量描述。
  • 代码块长度:
    ...

Files:

  • identity/src/main/java/org/zstack/identity/BeforeLoginInAccountPoint.java
  • identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: zstack-robot-1
PR: MatheMatrix/zstack#2360
File: network/src/main/java/org/zstack/network/l3/L3BasicNetwork.java:449-490
Timestamp: 2025-08-04T04:48:19.103Z
Learning: ZStack项目在cherry-pick操作中,即使发现了性能优化机会(如IP地址批量保存的内存优化),也严格遵循不做额外修改的政策,优先保证cherry-pick的完整性和一致性。
Learnt from: ZStack-Robot
PR: MatheMatrix/zstack#2489
File: storage/src/main/java/org/zstack/storage/snapshot/VolumeTree.java:471-545
Timestamp: 2025-08-22T05:36:40.467Z
Learning: In ZStack项目中,对于核心逻辑相关的代码,团队倾向于暂时不进行重构,即使代码复杂度较高,优先保证核心功能的稳定性。
Learnt from: ZStack-Robot
PR: MatheMatrix/zstack#2325
File: compute/src/main/java/org/zstack/compute/vm/VmMigrateCallExtensionFlow.java:29-29
Timestamp: 2025-07-24T05:53:10.246Z
Learning: 在ZStack项目的重构过程中,团队采用务实的优先级策略:优先完成影响运行时功能的代码重构,对于注释中的旧引用等非功能性问题可以延后处理,因为这些不会影响系统的实际运行。
🔇 Additional comments (3)
identity/src/main/java/org/zstack/identity/BeforeLoginInAccountPoint.java (1)

1-7: 接口设计合理

新增的扩展点接口定义清晰,方法签名设计合理,符合 ZStack 的扩展点机制规范。

identity/src/main/java/org/zstack/identity/AccountManagerImpl.java (2)

1800-1813: 错误信息应使用英文

根据编码规范,代码中的报错信息应当使用正确的、无拼写错误的英文。当前的实现在第 1811 行的错误信息格式正确,符合规范。


1800-1813: 考虑添加对 OAuth2 账户类型的限制

PR 标题提到需要限制 thirdPartyOAuth2 本地登录,但当前实现只检查了 AccountType.ThirdParty。请确认是否需要同时限制 OAuth2 账户类型的本地登录。

如果存在 AccountType.OAuth2 或类似的账户类型,建议扩展检查逻辑:

if (account.getType() == AccountType.ThirdParty || account.getType() == AccountType.OAuth2) {
    throw new CloudRuntimeException(String.format("Account [name=%s] [type=%s] cannot local login", name, account.getType()));
}

Resolves: ZSTAC-76390

Change-Id: I62646e64637a67777a667768686373626f696d72
@MatheMatrix MatheMatrix force-pushed the sync/lin.ma/ZSTAC-76390@@2 branch from 18e7ca0 to c7dbbdb Compare September 22, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants