[ISSUE #3406] fix change self's password fail #4536
Conversation
| if (!authConfigs.isAuthEnabled()) { | ||
| return true; | ||
| } | ||
| if (request.getAttribute(RequestUtil.NACOS_USER_KEY) == null) { |
There was a problem hiding this comment.
use com.alibaba.nacos.common.utils.StringUtil#isBlank will be better, I think
There was a problem hiding this comment.
use util is better , but it is an object. I will use com.alibaba.nacos.common.utils.Objects#isNull
There was a problem hiding this comment.
If nacos_user_key is "", whether it return false directly?
| // admin | ||
| if (user.isGlobalAdmin()) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I think this implementation will cause security problem. If you get admin from user input, users can mock or inject this value so that they can change others' password.
There was a problem hiding this comment.
The current version also has this problem, because admin can modify the password of other users.I just kept this feature.
There was a problem hiding this comment.
But current judge admin has auth to change other password is used the role queried by datasource.
Your implementation is used the request input. If I'm not admin, but I call this API with myself input. I will change other's password.
I am not saying that there is a problem with this function, I just describe that your implementation allows other non-admin users to modify the passwords of other users.
There was a problem hiding this comment.
抱歉，我不是很明白您“I call this API with myself input. I will change other's password”这里表达的意思，因为我看代码在com.alibaba.nacos.core.auth.AuthFilter#doFilter这里会调用方法com.alibaba.nacos.console.senicurity.nacos.NacosAuthManager#login,这里它将设置NACOS_USER_KEY
List<RoleInfo> roleInfoList = roleService.getRoles(username);
if (roleInfoList != null) {
for (RoleInfo roleInfo : roleInfoList) {
if (roleInfo.getRole().equals(NacosRoleServiceImpl.GLOBAL_ADMIN_ROLE)) {
user.setGlobalAdmin(true);
break;
}
}
}
req.setAttribute(RequestUtil.NACOS_USER_KEY, user);
在我的理解里，我在后续从request中取出的NACOS_USER_KEY是已经认证过后的用户信息。或许有哪些地方我没有考虑到的，会造成您所说的安全问题。
如是确实存在这个问题，我在这里认证的时候调用com.alibaba.nacos.console.senicurity.nacos.NacosAuthManager#login这个方法获取用户信息，这样应该保险的。
There was a problem hiding this comment.
如果是从AuthFilter中覆盖的应该也可以。
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
#3406
Use a special resource to update the password. any user can access,authentication is implemented inside the method
Brief changelog
XX
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.