You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2022/08/02 08:19:49 UTC
[GitHub] [dolphinscheduler] lyleshaw opened a new pull request, #11255: [Feature] Add params check in createUser Function
lyleshaw opened a new pull request, #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255
## Purpose of the pull request
- Add params check in createUser Function
## Brief change log
- Add params check in createUser Function
## Verify this pull request
This pull request is already covered by existing tests, such as *(please describe tests)*.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature] Add params check in createUser Function [dolphinscheduler]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1935134408
This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1203438370
@lyleshaw Please add a issue to target this PR, BTW, can we add some separate function from `create` and `update` function? I have do the same thing in Tenant and Queue https://github.com/apache/dolphinscheduler/blob/78e912257330c8ec488df7d42e025f47e15331c9/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/QueueServiceImpl.java#L70-L110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1237257442
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11255)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list) [70.7% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1202365377
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11255)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list) [59.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r944056233
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -203,23 +198,23 @@ public User createUser(String userName,
String phone,
String queue,
int state) {
- User user = new User();
Date now = new Date();
+ checkUserParams(userName, userPassword, email, phone);
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new ServiceException(Status.NAME_EXIST, userName);
+ }
Review Comment:
Why we should add this check, BTW?
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -203,23 +198,23 @@ public User createUser(String userName,
String phone,
String queue,
int state) {
- User user = new User();
Date now = new Date();
+ checkUserParams(userName, userPassword, email, phone);
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new ServiceException(Status.NAME_EXIST, userName);
+ }
+
+ User user = new User(userName, userPassword, email, phone, tenantId, state);
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
- user.setTenantId(tenantId);
- user.setPhone(phone);
- user.setState(state);
// create general users, administrator users are currently built-in
user.setUserType(UserType.GENERAL_USER);
- user.setCreateTime(now);
- user.setUpdateTime(now);
if (StringUtils.isEmpty(queue)) {
queue = "";
}
user.setQueue(queue);
+ user.setCreateTime(now);
Review Comment:
can we add `createtime` to the class `User` constructor? we can add multiple constructor to different behavior
```java
// for new user
public User(String userName, String userPassword, String email, String phone, Integer tenantId, Integer state) {
Date now = new Date();
this.userName = userName;
this.userPassword = EncryptionUtils.getMd5(userPassword);
this.email = email;
this.phone = phone;
this.tenantId = tenantId;
this.state = state;
this.updateTime = now;
this.createTime = now;
}
// for update user, we keep the create
public User(int userId, String userName, String userPassword, String email, String phone, Integer tenantId, Integer state, Date createTime) {
this.User(userName, userPassword, email, phone, tenantId, state)
this.id = userId;
this.createTime = createTime;
}
```
##########
dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/UsersServiceTest.java:
##########
@@ -622,37 +644,33 @@ public void testRegisterUser() {
String userPassword = "userTest";
String repeatPassword = "userTest";
String email = "123@qq.com";
- try {
- //userName error
- Map<String, Object> result = usersService.registerUser(userName, userPassword, repeatPassword, email);
- Assert.assertEquals(Status.REQUEST_PARAMS_NOT_VALID_ERROR, result.get(Constants.STATUS));
- userName = "userTest0002";
- userPassword = "userTest000111111111111111";
- //password error
- result = usersService.registerUser(userName, userPassword, repeatPassword, email);
- Assert.assertEquals(Status.REQUEST_PARAMS_NOT_VALID_ERROR, result.get(Constants.STATUS));
+ //userName error
+ Map<String, Object> result = usersService.registerUser(userName, userPassword, repeatPassword, email);
+ Assert.assertEquals(Status.REQUEST_PARAMS_NOT_VALID_ERROR, result.get(Constants.STATUS));
Review Comment:
You should also make this tests work, I just change the comemnt yesterday
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,77 +377,36 @@ public Map<String, Object> updateUser(User loginUser, int userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException(Status.FUNCTION_DISABLED.getMsg());
}
if (check(result, !canOperator(loginUser, userId), Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException(Status.USER_NO_OPERATION_PERM.getMsg());
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
+ throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
}
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, userName);
- return result;
- }
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
- }
-
- if (StringUtils.isNotEmpty(userPassword)) {
- if (!CheckUtils.checkPasswordLength(userPassword)) {
- putMsg(result, Status.USER_PASSWORD_LENGTH_ERROR);
- return result;
- }
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- }
-
- if (StringUtils.isNotEmpty(email)) {
- if (!CheckUtils.checkEmail(email)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, email);
- return result;
+ checkUserParams(userName, userPassword, email, phone);
+ if (StringUtils.isNotEmpty(timeZone)) {
+ if (!CheckUtils.checkTimeZone(timeZone)) {
+ throw new ServiceException(MessageFormat.format(Status.TIME_ZONE_ILLEGAL.getMsg(), timeZone));
}
- user.setEmail(email);
- }
-
- if (StringUtils.isNotEmpty(phone) && !CheckUtils.checkPhone(phone)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, phone);
- return result;
+ user.setTimeZone(timeZone);
}
+ user = new User(userName, userPassword, email, phone, tenantId, state);
Review Comment:
You have to pass exists user id to the user object you want to update, otherwise will raise error
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,77 +377,36 @@ public Map<String, Object> updateUser(User loginUser, int userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException(Status.FUNCTION_DISABLED.getMsg());
}
if (check(result, !canOperator(loginUser, userId), Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException(Status.USER_NO_OPERATION_PERM.getMsg());
Review Comment:
I preview reviewed have a mistake, you can directly use `throw new ServiceException(Status.USER_NO_OPERATION_PERM);`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r935539299
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,9 +206,41 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s doesn't valid", userName));
+ }
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s exist", userName));
+ }
+ user.setUserName(userName);
+ }
+
+ if (StringUtils.isNotEmpty(userPassword)) {
Review Comment:
> Please merge if statement
Sorry, I don't get your point...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] EricGao888 commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r936192326
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,9 +206,41 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
+ if (StringUtils.isNotEmpty(userName)) {
Review Comment:
Hello @lyleshaw , thx for this improvement. Since you have added several parameters checks, could you please update related unit tests to get those checks covered? Thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] github-code-scanning[bot] commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r976127221
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,86 +380,37 @@
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException(Status.FUNCTION_DISABLED);
}
if (check(result, !canOperator(loginUser, userId), Status.USER_NO_OPERATION_PERM)) {
- logger.warn("User does not have permission for this feature, userId:{}, userName:{}.", loginUser.getId(), loginUser.getUserName());
- return result;
+ throw new ServiceException(Status.USER_NO_OPERATION_PERM);
}
User user = userMapper.selectById(userId);
if (user == null) {
- logger.error("User does not exist, userId:{}.", userId);
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
- }
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- logger.warn("Parameter userName check failed.");
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, userName);
- return result;
- }
-
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- logger.warn("User name already exists, userName:{}.", tempUser.getUserName());
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
- }
-
- if (StringUtils.isNotEmpty(userPassword)) {
- if (!CheckUtils.checkPasswordLength(userPassword)) {
- logger.warn("Parameter userPassword check failed.");
- putMsg(result, Status.USER_PASSWORD_LENGTH_ERROR);
- return result;
- }
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- }
-
- if (StringUtils.isNotEmpty(email)) {
- if (!CheckUtils.checkEmail(email)) {
- logger.warn("Parameter email check failed.");
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, email);
- return result;
- }
- user.setEmail(email);
- }
-
- if (StringUtils.isNotEmpty(phone) && !CheckUtils.checkPhone(phone)) {
- logger.warn("Parameter phone check failed.");
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, phone);
- return result;
- }
-
- if (state == 0 && user.getState() != state && Objects.equals(loginUser.getId(), user.getId())) {
- logger.warn("Not allow to disable your own account, userId:{}, userName:{}.", user.getId(), user.getUserName());
- putMsg(result, Status.NOT_ALLOW_TO_DISABLE_OWN_ACCOUNT);
- return result;
+ logger.warn("User does not have permission for this feature, userId:{}, userName:{}.", loginUser.getId(), loginUser.getUserName());
+ throw new ServiceException(Status.USER_NOT_EXIST);
}
+ checkUserParams(userPassword, email, phone);
if (StringUtils.isNotEmpty(timeZone)) {
if (!CheckUtils.checkTimeZone(timeZone)) {
- logger.warn("Parameter time zone is illegal.");
- putMsg(result, Status.TIME_ZONE_ILLEGAL, timeZone);
- return result;
+ throw new ServiceException(Status.TIME_ZONE_ILLEGAL, timeZone);
}
user.setTimeZone(timeZone);
}
- user.setPhone(phone);
+ user = new User(userName, userPassword, email, phone, tenantId, state);
+ if (state == 0 && user.getState() != state && loginUser.getId() == user.getId()) {
Review Comment:
## Reference equality test of boxed types
Suspicious reference comparison of boxed numerical values.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/1510)
##########
dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/UsersServiceTest.java:
##########
@@ -741,13 +779,15 @@
String userName = "userTest0001";
String userPassword = "userTest";
String email = "abc@x.com";
- String phone = "123456789";
+ String phone = "17366666666";
String tenantCode = "tenantCode";
+ Integer userId = 1;
Review Comment:
## Unread local variable
Variable 'Integer userId' is never read.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/1511)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1202369892
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11255)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list) [59.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1237123429
> @lyleshaw It seems dwonload jdbc driver error, could you try to rebase on upstream/dev, and force push to restart CI?
I usually use git merge instead of git rebase, hoping things won't go wrong...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r943387613
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -1107,6 +1070,52 @@ private String checkUserParams(String userName, String password, String email, S
return msg;
}
+ /**
+ * @return if check failed throw error, otherwise return null
+ */
+ private void checkUserParams(String userName, String userPassword, String email, String phone, Integer state) {
+
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new ServiceException(String.format("userName %s doesn't valid", userName));
Review Comment:
> ```diff
> - throw new ServiceException(String.format("userName %s doesn't valid", userName));
> + throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR.getMsg(), userName));
> ```
get
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] codecov-commenter commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1202359827
# [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/11255?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#11255](https://codecov.io/gh/apache/dolphinscheduler/pull/11255?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7175ed1) into [dev](https://codecov.io/gh/apache/dolphinscheduler/commit/d9516e22360fca283e011040e1bef7a8b239cc06?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d9516e2) will **decrease** coverage by `0.04%`.
> The diff coverage is `23.80%`.
```diff
@@ Coverage Diff @@
## dev #11255 +/- ##
============================================
- Coverage 40.24% 40.19% -0.05%
+ Complexity 4839 4837 -2
============================================
Files 973 973
Lines 37302 37316 +14
Branches 4141 4145 +4
============================================
- Hits 15012 15000 -12
- Misses 20749 20763 +14
- Partials 1541 1553 +12
```
| [Impacted Files](https://codecov.io/gh/apache/dolphinscheduler/pull/11255?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inscheduler/api/service/impl/UsersServiceImpl.java](https://codecov.io/gh/apache/dolphinscheduler/pull/11255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvYXBpL3NlcnZpY2UvaW1wbC9Vc2Vyc1NlcnZpY2VJbXBsLmphdmE=) | `70.21% <23.80%> (-1.91%)` | :arrow_down: |
| [...erver/master/processor/queue/TaskEventService.java](https://codecov.io/gh/apache/dolphinscheduler/pull/11255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1tYXN0ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9wcm9jZXNzb3IvcXVldWUvVGFza0V2ZW50U2VydmljZS5qYXZh) | `69.64% <0.00%> (-10.72%)` | :arrow_down: |
| [...er/master/dispatch/host/assign/RandomSelector.java](https://codecov.io/gh/apache/dolphinscheduler/pull/11255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1tYXN0ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9kaXNwYXRjaC9ob3N0L2Fzc2lnbi9SYW5kb21TZWxlY3Rvci5qYXZh) | `77.77% <0.00%> (-5.56%)` | :arrow_down: |
| [...e/dolphinscheduler/remote/NettyRemotingClient.java](https://codecov.io/gh/apache/dolphinscheduler/pull/11255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1yZW1vdGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcmVtb3RlL05ldHR5UmVtb3RpbmdDbGllbnQuamF2YQ==) | `50.00% <0.00%> (-2.78%)` | :arrow_down: |
| [...r/plugin/task/sqoop/parameter/SqoopParameters.java](https://codecov.io/gh/apache/dolphinscheduler/pull/11255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci10YXNrLXBsdWdpbi9kb2xwaGluc2NoZWR1bGVyLXRhc2stc3Fvb3Avc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcGx1Z2luL3Rhc2svc3Fvb3AvcGFyYW1ldGVyL1Nxb29wUGFyYW1ldGVycy5qYXZh) | `55.12% <0.00%> (-1.29%)` | :arrow_down: |
| [...pache/dolphinscheduler/common/utils/DateUtils.java](https://codecov.io/gh/apache/dolphinscheduler/pull/11255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvY29tbW9uL3V0aWxzL0RhdGVVdGlscy5qYXZh) | `74.83% <0.00%> (-0.17%)` | :arrow_down: |
| [.../dolphinscheduler/plugin/task/datax/DataxTask.java](https://codecov.io/gh/apache/dolphinscheduler/pull/11255/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci10YXNrLXBsdWdpbi9kb2xwaGluc2NoZWR1bGVyLXRhc2stZGF0YXgvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcGx1Z2luL3Rhc2svZGF0YXgvRGF0YXhUYXNrLmphdmE=) | `0.00% <0.00%> (ø)` | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1203429902
> We need to update corresponding unit tests to get those new checks covered.
Got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r943385967
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -1107,6 +1070,52 @@ private String checkUserParams(String userName, String password, String email, S
return msg;
}
+ /**
+ * @return if check failed throw error, otherwise return null
+ */
+ private void checkUserParams(String userName, String userPassword, String email, String phone, Integer state) {
+
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new ServiceException(String.format("userName %s doesn't valid", userName));
Review Comment:
```diff
- throw new ServiceException(String.format("userName %s doesn't valid", userName));
+ throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR.getMsg(), userName));
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -1107,6 +1070,52 @@ private String checkUserParams(String userName, String password, String email, S
return msg;
}
+ /**
+ * @return if check failed throw error, otherwise return null
+ */
+ private void checkUserParams(String userName, String userPassword, String email, String phone, Integer state) {
+
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new ServiceException(String.format("userName %s doesn't valid", userName));
Review Comment:
```diff
- throw new ServiceException(String.format("userName %s doesn't valid", userName));
+ throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR.getMsg(), userName));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1236729623
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11255)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list) [72.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1203689839
Hi all, I have solved the above problems (including adding tests, abstracting to checkParams functions, etc.)
Please feel free to re-review it.
@zhongjiajie @EricGao888
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature] Add params check in createUser Function [dolphinscheduler]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11255: [Feature] Add params check in createUser Function
URL: https://github.com/apache/dolphinscheduler/pull/11255
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1204719842
> Hi all, I have solved the above problems (including adding tests, abstracting to checkParams functions, etc.)
> Please feel free to re-review it.
> @zhongjiajie @EricGao888
@lyleshaw But I find out we have some failed CI step, could you please make a check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r937320395
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId), Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
- }
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, userName);
- return result;
- }
-
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
+ throw new ServiceException("User Doesn't Exist");
}
- if (StringUtils.isNotEmpty(userPassword)) {
- if (!CheckUtils.checkPasswordLength(userPassword)) {
- putMsg(result, Status.USER_PASSWORD_LENGTH_ERROR);
- return result;
- }
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- }
-
- if (StringUtils.isNotEmpty(email)) {
- if (!CheckUtils.checkEmail(email)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, email);
- return result;
- }
- user.setEmail(email);
- }
-
- if (StringUtils.isNotEmpty(phone) && !CheckUtils.checkPhone(phone)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, phone);
- return result;
- }
-
- if (state == 0 && user.getState() != state && loginUser.getId() == user.getId()) {
- putMsg(result, Status.NOT_ALLOW_TO_DISABLE_OWN_ACCOUNT);
- return result;
- }
+ checkUserParams(userName, userPassword, email, phone, state);
if (StringUtils.isNotEmpty(timeZone)) {
if (!CheckUtils.checkTimeZone(timeZone)) {
- putMsg(result, Status.TIME_ZONE_ILLEGAL, timeZone);
- return result;
+ throw new ServiceException(String.format("timeZone %s doesn't valid", timeZone));
Review Comment:
same as here, reuse status
```suggestion
throw new ServiceException(MessageFormat.format(Status.TIME_ZONE_ILLEGAL.getMsg(), timeZone));
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId), Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
Review Comment:
we can reuse the `status` here
```suggestion
throw new ServiceException(Status.USER_NO_OPERATION_PERM.getMsg());
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId), Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
- }
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, userName);
- return result;
- }
-
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
+ throw new ServiceException("User Doesn't Exist");
}
- if (StringUtils.isNotEmpty(userPassword)) {
- if (!CheckUtils.checkPasswordLength(userPassword)) {
- putMsg(result, Status.USER_PASSWORD_LENGTH_ERROR);
- return result;
- }
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- }
-
- if (StringUtils.isNotEmpty(email)) {
- if (!CheckUtils.checkEmail(email)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, email);
- return result;
- }
- user.setEmail(email);
- }
-
- if (StringUtils.isNotEmpty(phone) && !CheckUtils.checkPhone(phone)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, phone);
- return result;
- }
-
- if (state == 0 && user.getState() != state && loginUser.getId() == user.getId()) {
- putMsg(result, Status.NOT_ALLOW_TO_DISABLE_OWN_ACCOUNT);
- return result;
- }
+ checkUserParams(userName, userPassword, email, phone, state);
if (StringUtils.isNotEmpty(timeZone)) {
if (!CheckUtils.checkTimeZone(timeZone)) {
- putMsg(result, Status.TIME_ZONE_ILLEGAL, timeZone);
- return result;
+ throw new ServiceException(String.format("timeZone %s doesn't valid", timeZone));
}
user.setTimeZone(timeZone);
}
+ user.setUserName(userName);
+ user.setUserPassword(EncryptionUtils.getMd5(userPassword));
+ user.setEmail(email);
user.setPhone(phone);
user.setQueue(queue);
user.setState(state);
Review Comment:
better to add new construct to init new object
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
Review Comment:
```suggestion
throw new ServiceException(Status.FUNCTION_DISABLED.getMsg());
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -1107,6 +1070,52 @@ private String checkUserParams(String userName, String password, String email, S
return msg;
}
+ /**
+ * @return if check failed throw error, otherwise return null
+ */
+ private void checkUserParams(String userName, String userPassword, String email, String phone, Integer state) {
+
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new ServiceException(String.format("userName %s doesn't valid", userName));
Review Comment:
same here, when we already have status, we can reuse the status.getMsg()
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,11 +206,13 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
+ checkUserParams(userName, userPassword, email, phone, state);
+
user.setUserName(userName);
user.setUserPassword(EncryptionUtils.getMd5(userPassword));
user.setEmail(email);
- user.setTenantId(tenantId);
user.setPhone(phone);
+ user.setTenantId(tenantId);
user.setState(state);
// create general users, administrator users are currently built-in
user.setUserType(UserType.GENERAL_USER);
Review Comment:
Make we can add a new constructors to class `User`, just like we add it to https://github.com/apache/dolphinscheduler/blob/a6154220dc9fd78a894f9fc47c78378cba1d03cd/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/entity/Queue.java#L57-L63
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId), Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
- }
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, userName);
- return result;
- }
-
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
+ throw new ServiceException("User Doesn't Exist");
Review Comment:
```suggestion
throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhuxt2015 commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhuxt2015 commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r935546657
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,9 +206,41 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s doesn't valid", userName));
+ }
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s exist", userName));
+ }
+ user.setUserName(userName);
+ }
+
+ if (StringUtils.isNotEmpty(userPassword)) {
Review Comment:
I mean please fix code smell
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r945478853
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -203,23 +198,23 @@ public User createUser(String userName,
String phone,
String queue,
int state) {
- User user = new User();
Date now = new Date();
+ checkUserParams(userName, userPassword, email, phone);
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new ServiceException(Status.NAME_EXIST, userName);
+ }
Review Comment:
> Why we should add this check, BTW?
to avoid userName duplicate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r943663470
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -406,8 +392,7 @@ public Map<String, Object> updateUser(User loginUser, int userId,
throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
}
- checkUserParams(userPassword, email, phone, state);
-
+ checkUserParams(userName, userPassword, email, phone);
Review Comment:
Due to `check params` are used by both `createUser` and `updateUser` (one of which requires that `userName` does not exist and one of which requires that it must exist), `check params` cannot take `userName` as an argument and should be handled by each function.
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -406,8 +392,7 @@ public Map<String, Object> updateUser(User loginUser, int userId,
throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
}
- checkUserParams(userPassword, email, phone, state);
-
+ checkUserParams(userName, userPassword, email, phone);
Review Comment:
Due to `check params` are used by both `createUser` and `updateUser` (one of which requires that `userName` does not exist and one of which requires that it must exist), `check params` cannot take `userName` as an argument and should be handled by each function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1237087559
@lyleshaw It seems dwonload jdbc driver error, could you try to rebase on upstream/dev, and force push to restart CI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r936200654
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,9 +206,41 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s doesn't valid", userName));
+ }
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s exist", userName));
+ }
+ user.setUserName(userName);
+ }
+
+ if (StringUtils.isNotEmpty(userPassword)) {
Review Comment:
Yeah, agree with Calvin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] CalvinKirs commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r936199135
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,9 +206,41 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s doesn't valid", userName));
+ }
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s exist", userName));
+ }
+ user.setUserName(userName);
+ }
+
+ if (StringUtils.isNotEmpty(userPassword)) {
Review Comment:
I think OK, and merging can lead to unclear exception semantics.
sometimes, code smell needs to be judged based on the actual situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature] Add params check in createUser Function [dolphinscheduler]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1947546955
This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1202172293
PTAL @zhongjiajie
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhuxt2015 commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhuxt2015 commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r935524174
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,9 +206,41 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s doesn't valid", userName));
+ }
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new org.apache.dolphinscheduler.service.exceptions.ServiceException(String.format("userName %s exist", userName));
+ }
+ user.setUserName(userName);
+ }
+
+ if (StringUtils.isNotEmpty(userPassword)) {
Review Comment:
Please merge if statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] lyleshaw commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
lyleshaw commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r943664256
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -406,8 +392,7 @@ public Map<String, Object> updateUser(User loginUser, int userId,
throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
}
- checkUserParams(userPassword, email, phone, state);
-
+ checkUserParams(userName, userPassword, email, phone);
Review Comment:
@zhongjiajie
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1236723629
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11255)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list) [72.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#issuecomment-1237255863
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11255)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11255&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list) [70.7% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11255&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #11255: [Feature] Add params check in createUser Function
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r944056000
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -406,8 +392,7 @@ public Map<String, Object> updateUser(User loginUser, int userId,
throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
}
- checkUserParams(userPassword, email, phone, state);
-
+ checkUserParams(userName, userPassword, email, phone);
Review Comment:
> Due to `check params` are used by both `createUser` and `updateUser` (one of which requires that `userName` does not exist and one of which requires that it must exist), `check params` cannot take `userName` as an argument and should be handled by each function.
I look at the code, and your comment above it true. but when I check the income form from the frontend, it will always pass the username when it tries to update the user, so I think we can check `username` in both create or update user, WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org