【Fix issue #13600】 auth error when non-admin users clone config#13674
【Fix issue #13600】 auth error when non-admin users clone config#13674KomachiSion merged 4 commits intoalibaba:developfrom
Conversation
|
|
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
修复非管理员用户克隆配置时的认证错误变更概述
变更文件
时序图sequenceDiagram
participant CC as ConfigControllerV3
participant ARP as AbstractResourceParser
participant CHP as ConfigHttpResourceParser
participant AWAF as AbstractWebAuthFilter
CC->>CC: cloneConfig接口添加@Secured注解
CC->>ARP: parse(request, secured)
ARP->>CHP: getNamespaceId(request, secured)
CHP->>CHP: 从secured.tags中提取namespaceId参数名
CHP->>CHP: 从request中获取对应参数值
CHP-->>ARP: 返回namespaceId
ARP-->>CC: 返回解析后的资源信息
CC->>AWAF: doFilter(resource)
AWAF->>AWAF: 使用正确namespaceId进行权限验证
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 1 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 3 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 4 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ auth/src/main/java/com/alibaba/nacos/auth/parser/AbstractResourceParser.java (1 💬)
☕ auth/src/main/java/com/alibaba/nacos/auth/parser/http/ConfigHttpResourceParser.java (1 💬)
☕ auth/src/test/java/com/alibaba/nacos/auth/parser/http/ConfigHttpResourceParserTest.java (1 💬)
- 增加多个测试用例以验证`getNamespaceId`方法在不同标签配置下的行为。 (L144-L203)
☕ plugin/auth/src/main/java/com/alibaba/nacos/plugin/auth/constant/Constants.java (1 💬)
- 定义一个新的安全标签，用于标识需要特殊处理的命名空间ID。 (L70-L71)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 权限解析逻辑与业务逻辑耦合度较高
当前在ConfigHttpResourceParser中，通过Secured注解的标签来解析命名空间ID的逻辑与HTTP请求处理紧密耦合。这种设计可能导致未来在扩展其他资源类型时重复类似的解析逻辑，增加维护成本。建议将标签解析逻辑抽象为独立的策略类或工具类，以提高代码复用性和可维护性。
📌 关键代码
@Override
protected String getNamespaceId(HttpServletRequest request, Secured secured) {
return Arrays.stream(secured.tags())
.filter(tag -> tag.startsWith(Constants.NAMESPACE_ID))
.map(tag -> tag.split(com.alibaba.nacos.plugin.auth.constant.Constants.Resource.SPLITTER))
.filter(splitTags -> splitTags.length >= 2)
.map(splitTags -> request.getParameter(splitTags[1]))
.filter(StringUtils::isNotBlank)
.findFirst()
.orElseGet(() -> getNamespaceId(request));
}随着系统功能扩展，类似的解析逻辑可能在多个地方重复出现，导致代码冗余和维护困难。
🔍2. 安全标签定义缺乏统一管理
在Constants.java和plugin/auth/src/main/java/com/alibaba/nacos/plugin/auth/constant/Constants.java中分别定义了安全相关的常量，存在分散管理的问题。建议建立统一的安全标签管理机制，避免因标签命名不一致或重复定义引发的安全漏洞或解析错误。
📌 关键代码
public static final String TARGET_NAMESPACE_ID = "targetNamespaceId";public static final String SECURED_SPECIAL_TAGS = com.alibaba.nacos.api.common.Constants.NAMESPACE_ID
+ Resource.SPLITTER +com.alibaba.nacos.api.common.Constants.TARGET_NAMESPACE_ID;标签定义分散可能导致命名冲突、使用不一致，进而影响权限控制的准确性。
🔍3. 测试覆盖未充分验证边界条件
虽然新增了多个测试用例，但部分边界情况如标签格式异常、参数缺失等场景的测试仍显不足。例如，对于splitTag:extra这类多分隔符的情况，测试仅验证了正常路径，未覆盖所有可能的异常路径。建议补充更多边界条件测试，确保解析逻辑的健壮性。
📌 关键代码
@Test
@Secured(tags = {"namespaceId:splitTag:extra"})
void testParseWithInvalidSplitTag() throws NoSuchMethodException {
Secured secured = getMethodSecure();
when(request.getParameter("splitTag")).thenReturn("splitNs");
String actualNamespaceId = resourceParser.getNamespaceId(request, secured);
assertEquals("splitNs", actualNamespaceId);
}未充分测试边界条件可能导致在生产环境中出现意外行为，影响系统的稳定性和安全性。
🔍4. 权限控制粒度可能过于宽泛
在ConfigControllerV3.java中，克隆配置操作使用了SECURED_SPECIAL_TAGS作为权限标签，该标签包含了源和目标命名空间ID。然而，实际业务场景中可能需要更细粒度的权限控制，例如区分只读和读写权限。建议评估是否需要引入更精细的权限控制机制，以满足复杂业务需求。
📌 关键代码
config/src/main/java/com/alibaba/nacos/config/server/controller/v3/ConfigControllerV3.java(596-597)
@Secured(action = ActionTypes.WRITE, signType = SignType.CONFIG, apiType = ApiType.CONSOLE_API,
tags = com.alibaba.nacos.plugin.auth.constant.Constants.Tag.SECURED_SPECIAL_TAGS)权限控制粒度过粗可能导致非预期的操作被执行，带来潜在的安全风险。
审查详情
📒 文件清单 （6 个文件）
📝 变更: 6 个文件
📝 变更文件:
api/src/main/java/com/alibaba/nacos/api/common/Constants.javaauth/src/main/java/com/alibaba/nacos/auth/parser/AbstractResourceParser.javaauth/src/main/java/com/alibaba/nacos/auth/parser/http/ConfigHttpResourceParser.javaauth/src/test/java/com/alibaba/nacos/auth/parser/http/ConfigHttpResourceParserTest.javaconfig/src/main/java/com/alibaba/nacos/config/server/controller/v3/ConfigControllerV3.javaplugin/auth/src/main/java/com/alibaba/nacos/plugin/auth/constant/Constants.java
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
| protected String getNamespaceId(R request, Secured secured){ | ||
| return getNamespaceId(request); | ||
| } |
There was a problem hiding this comment.
为getNamespaceId方法添加一个带Secured参数的重载版本，以支持更灵活的命名空间ID解析。
🟢 Minor | 🧹 Code Smells
📋 问题详情
原有的getNamespaceId方法签名不包含Secured对象，导致子类无法直接访问与请求相关的安全注解信息。通过提供一个新的重载方法并传入Secured对象，可以允许子类根据安全注解动态解析命名空间ID，提升权限控制的灵活性和准确性。
💡 解决方案
当前实现是合理的默认行为，它调用了原始的getNamespaceId方法。如果子类需要特殊处理，可以覆盖此方法。建议保持现状，因为这为未来的扩展提供了良好的基础。
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
auth/src/main/java/com/alibaba/nacos/auth/parser/http/ConfigHttpResourceParser.java
Show resolved
Hide resolved
auth/src/test/java/com/alibaba/nacos/auth/parser/http/ConfigHttpResourceParserTest.java
Show resolved
Hide resolved
| public static final String SECURED_SPECIAL_TAGS = com.alibaba.nacos.api.common.Constants.NAMESPACE_ID | ||
| + Resource.SPLITTER +com.alibaba.nacos.api.common.Constants.TARGET_NAMESPACE_ID; |
There was a problem hiding this comment.
|
|
5 participants
fix: #13600
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
This change is to address the problem in ISSUE #13600 where regular users with read and write permissions fail to clone configurations.
Brief changelog
Fixed the cloning exception caused by the mismatch between the
namespaceIdthat requires authentication in the ordinary user's configuration cloning interface and the currently used authenticationnamespaceId. Actually, thetargetNamespaceIdshould be obtained for authentication. However, in line 105 of the doFilter method inAbstractWebAuthFilter.java, theresourcefails to obtain thetargetNamespaceId, and the defaultpublicnamespace is used, resulting in authentication errors and configuration cloning failures. This commit fixes the way the namespace requiring authentication is obtained inside the resource for the cloning interface.Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=trueto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.