<fix>[identity]: ValidateSession clear SSOToken#4159
Open
zstack-robot-2 wants to merge 1 commit into
Open
Conversation
Resolves: ZSTAC-77432 Change-Id: I65796b67786879696f76626f6e62767674727272 (cherry picked from commit 588f211)
总体说明此变更为Session的登出流程添加了扩展点机制，在会话过期时执行注册的登出扩展回调，增加了10行代码。 变更详情会话过期时的登出扩展机制
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@identity/src/main/java/org/zstack/identity/Session.java`:
- Line 278: 静态字段 logoutExtensionPoints 仅在实例方法 start() 中赋值但被静态方法
checkSessionExpired 遍历，存在在组件未启动前调用导致的 NPE 风险；请移除该静态缓存字段并在 checkSessionExpired
中按需获取扩展点（使用
getComponentLoader().getComponent(PluginRegistry.class).getExtensionList(LogoutExtensionPoint.class)），与现有
login() 和 deleteSession() 的模式保持一致，或者若保留缓存则至少在 checkSessionExpired 遍历前做空值保护（null
检查）以避免 NPE；定位符：logoutExtensionPoints、start(), checkSessionExpired(), login(),
deleteSession() 及
getComponentLoader().getComponent(PluginRegistry.class).getExtensionList(...)。
- Around line 227-231: 在 Session 的过期清理分支中，将对 LogoutExtensionPoint 的调用顺序调整为先遍历并调用
ext.beforeLogout(s) 再调用 Session.logout(s.getUuid())，以与 AccountManagerImpl 的
APILogOutMsg 流程保持一致并恢复语义；同时修复 logoutExtensionPoints 可能为 null 的竞态：在 start() 中于
startCleanUpStaleSessionTask() 之前完成 logoutExtensionPoints
的初始化（或将其在声明时初始化），或在清理任务里对 logoutExtensionPoints 做空值检查并安全跳过，以避免 NPE。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e8d0d4b4-df32-4c15-a2d2-72b591dd49bd
📒 Files selected for processing (1)
identity/src/main/java/org/zstack/identity/Session.java
Comment on lines
227
to
+231
| logout(s.getUuid()); | ||
|
|
||
| for (LogoutExtensionPoint ext : logoutExtensionPoints) { | ||
| ext.beforeLogout(s); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查找 LogoutExtensionPoint 接口定义及方法签名
fd -e java LogoutExtensionPoint --exec cat {}
# 查找其它实现/调用点以核对 before/after 约定
rg -nP --type=java '\bbeforeLogout\s*\(' -C2Repository: MatheMatrix/zstack
Length of output: 1799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) 取 Session.java 相关片段：logoutExtensionPoints 声明、赋值/start、以及调用点/被调用的上下文（checkSessionExpired/logout）
sed -n '200,320p' identity/src/main/java/org/zstack/identity/Session.java | nl -ba | sed -n '1,140p'
echo "-----"
rg -n "logoutExtensionPoints|checkSessionExpired|start\\(|logout\\(" identity/src/main/java/org/zstack/identity/Session.java
echo "-----"
# 2) 取 AccountManagerImpl.java 中 beforeLogout 的上下文：它是否在真正注销/删除之前调用
sed -n '430,540p' identity/src/main/java/org/zstack/identity/AccountManagerImpl.java | nl -ba | sed -n '1,140p'
echo "-----"
rg -n "beforeLogout\\(session\\)|LogoutExtensionPoint|logout|deleteSession|sessionLogout" identity/src/main/java/org/zstack/identity/AccountManagerImpl.javaRepository: MatheMatrix/zstack
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Session.java (logoutExtensionPoints lifecycle + call order) ==="
rg -n "logoutExtensionPoints|checkSessionExpired\\(|start\\(|logout\\(s\\.getUuid\\(\\)\\)|logout\\(" identity/src/main/java/org/zstack/identity/Session.java
echo "----- context around declaration"
sed -n '1,120p' identity/src/main/java/org/zstack/identity/Session.java | cat -n | sed -n '1,120p'
echo "----- context around start()/initialization of logoutExtensionPoints"
sed -n '240,320p' identity/src/main/java/org/zstack/identity/Session.java | cat -n
echo "----- context around the beforeLogout call site"
sed -n '200,260p' identity/src/main/java/org/zstack/identity/Session.java | cat -n
echo "=== AccountManagerImpl.java (beforeLogout semantics & order) ==="
rg -n "interface LogoutExtensionPoint|beforeLogout\\(session\\)|deleteSession\\(|logout\\(|sessionLogout\\(" identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
echo "----- context around beforeLogout loop"
sed -n '440,520p' identity/src/main/java/org/zstack/identity/AccountManagerImpl.java | cat -nRepository: MatheMatrix/zstack
Length of output: 16874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Session.logout(String) implementation ==="
sed -n '140,220p' identity/src/main/java/org/zstack/identity/Session.java | cat -n
echo "=== AccountManagerImpl.logOutSession(...) implementation ==="
rg -n "void logOutSession\\(|logOutSession\\(" identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
# show around the call site and definition
sed -n '1120,1180p' identity/src/main/java/org/zstack/identity/AccountManagerImpl.java | cat -n
# if there is another overload further down, also show later chunk around "logOutSession"
rg -n "logOutSession\\(" identity/src/main/java/org/zstack/identity/AccountManagerImpl.java | headRepository: MatheMatrix/zstack
Length of output: 6641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 找出 checkSessionExpired / errorOnTimeout 的调用点 ==="
rg -n "Session\\.checkSessionExpired\\(|Session\\.errorOnTimeout\\(" identity/src/main/java | head -n 200
echo "----- 全局找 LogoutExtensionPoint.beforeLogout 调用顺序是否一致 -----"
rg -n "beforeLogout\\(.*\\)" identity/src/main/java | head -n 200Repository: MatheMatrix/zstack
Length of output: 817
修正 LogoutExtensionPoint 的调用时序并避免 logoutExtensionPoints 初始化竞态
Session的过期清理分支里先执行Session.logout(s.getUuid())（会从缓存与 DB 中删除会话），随后才遍历logoutExtensionPoints调用ext.beforeLogout(s)；与AccountManagerImpl的APILogOutMsg流程（先beforeLogout，再logOutSession->Session.logout）不一致，beforeLogout语义与实际顺序相反。logoutExtensionPoints为static，start()中在startCleanUpStaleSessionTask()之后才赋值，若 periodic task 在赋值前触发，可能出现null导致 NPE。
🔧 建议调整调用顺序
- logout(s.getUuid());
-
- for (LogoutExtensionPoint ext : logoutExtensionPoints) {
- ext.beforeLogout(s);
- }
+ for (LogoutExtensionPoint ext : logoutExtensionPoints) {
+ ext.beforeLogout(s);
+ }
+
+ logout(s.getUuid());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@identity/src/main/java/org/zstack/identity/Session.java` around lines 227 -
231, 在 Session 的过期清理分支中，将对 LogoutExtensionPoint 的调用顺序调整为先遍历并调用
ext.beforeLogout(s) 再调用 Session.logout(s.getUuid())，以与 AccountManagerImpl 的
APILogOutMsg 流程保持一致并恢复语义；同时修复 logoutExtensionPoints 可能为 null 的竞态：在 start() 中于
startCleanUpStaleSessionTask() 之前完成 logoutExtensionPoints
的初始化（或将其在声明时初始化），或在清理任务里对 logoutExtensionPoints 做空值检查并安全跳过，以避免 NPE。
| setupGlobalConfig(); | ||
| startCleanUpStaleSessionTask(); | ||
| setupCanonicalEvents(); | ||
| logoutExtensionPoints = pluginRgty.getExtensionList(LogoutExtensionPoint.class); |
There was a problem hiding this comment.
静态 logoutExtensionPoints 仅在 start() 中赋值，存在 NPE 与模式不一致风险
logoutExtensionPoints（第 46 行）为静态字段且无初始值，只在实例方法 start() 中赋值；而静态方法 checkSessionExpired（第 229 行）会遍历它。若 checkSessionExpired 在 Session 组件 start() 完成之前被调用（如启动早期或单元测试场景），将抛出 NullPointerException，进而中断会话校验。
此外，本文件中 login()（第 63 行）与 deleteSession()（第 180 行）均采用按需获取扩展的写法 getComponentLoader().getComponent(PluginRegistry.class).getExtensionList(...)，可天然规避初始化时序问题。建议在 checkSessionExpired 中沿用同一模式，移除静态缓存字段与此处赋值。
♻️ 方案一（推荐）：按需获取，移除静态缓存
- private static List<LogoutExtensionPoint> logoutExtensionPoints;- for (LogoutExtensionPoint ext : logoutExtensionPoints) {
- ext.beforeLogout(s);
- }
+ getComponentLoader().getComponent(PluginRegistry.class)
+ .getExtensionList(LogoutExtensionPoint.class)
+ .forEach(ext -> ext.beforeLogout(s));- logoutExtensionPoints = pluginRgty.getExtensionList(LogoutExtensionPoint.class);
return true;方案二（保留缓存）：在遍历前对 logoutExtensionPoints 做空值保护。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@identity/src/main/java/org/zstack/identity/Session.java` at line 278, 静态字段
logoutExtensionPoints 仅在实例方法 start() 中赋值但被静态方法 checkSessionExpired
遍历，存在在组件未启动前调用导致的 NPE 风险；请移除该静态缓存字段并在 checkSessionExpired 中按需获取扩展点（使用
getComponentLoader().getComponent(PluginRegistry.class).getExtensionList(LogoutExtensionPoint.class)），与现有
login() 和 deleteSession() 的模式保持一致，或者若保留缓存则至少在 checkSessionExpired 遍历前做空值保护（null
检查）以避免 NPE；定位符：logoutExtensionPoints、start(), checkSessionExpired(), login(),
deleteSession() 及
getComponentLoader().getComponent(PluginRegistry.class).getExtensionList(...)。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: ZSTAC-85582
Cherry-pick from http://dev.zstack.io:9080/zstackio/zstack/-/merge_requests/8579
sync from gitlab !10064