You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2020/09/07 08:03:31 UTC
[GitHub] [incubator-iotdb] haimeiguo opened a new pull request #1698: 850 - check list user privilege
haimeiguo opened a new pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698
user can only LIST USER if he is granted list_user privilege, check if he is granted in plan executor
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] jt2594838 commented on a change in pull request #1698: [IOTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r489142360
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/PhysicalPlan.java
##########
@@ -48,6 +48,7 @@
private static final String SERIALIZATION_UNIMPLEMENTED = "serialization unimplemented";
+ protected String userName;
Review comment:
I have a different opinion. Every physical plan is issued by a user and keeping this information will help us tracking user behaviors in the future.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] haimeiguo commented on a change in pull request #1698: [IoTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
haimeiguo commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r487023608
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] haimeiguo commented on a change in pull request #1698: [IoTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
haimeiguo commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r486717423
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/IoTDBAuthorizationIT.java
##########
@@ -793,15 +793,19 @@ public void testListRolePrivileges() throws ClassNotFoundException, SQLException
try {
adminStmt.execute("CREATE ROLE role1");
- adminStmt.execute(
+ ResultSet resultSet = adminStmt.executeQuery("LIST ROLE PRIVILEGES role1");
+ String ans = "";
+ try {
+ //not granted list role privilege, should return empty
+ validateResultSet(resultSet, ans);
+
+ adminStmt.execute(
"GRANT ROLE role1 PRIVILEGES 'READ_TIMESERIES','INSERT_TIMESERIES','DELETE_TIMESERIES' ON root.a.b.c");
- adminStmt.execute(
+ adminStmt.execute(
Review comment:
done. Thank you :)
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] qiaojialin merged pull request #1698: [IOTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
qiaojialin merged pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] haimeiguo commented on a change in pull request #1698: [IoTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
haimeiguo commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r487023608
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
done. thank you!
index is for rowRocord, it was there before my edit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] mychaow commented on a change in pull request #1698: [IoTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
mychaow commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r486133953
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/IoTDBAuthorizationIT.java
##########
@@ -793,15 +793,19 @@ public void testListRolePrivileges() throws ClassNotFoundException, SQLException
try {
adminStmt.execute("CREATE ROLE role1");
- adminStmt.execute(
+ ResultSet resultSet = adminStmt.executeQuery("LIST ROLE PRIVILEGES role1");
+ String ans = "";
+ try {
+ //not granted list role privilege, should return empty
+ validateResultSet(resultSet, ans);
+
+ adminStmt.execute(
"GRANT ROLE role1 PRIVILEGES 'READ_TIMESERIES','INSERT_TIMESERIES','DELETE_TIMESERIES' ON root.a.b.c");
- adminStmt.execute(
+ adminStmt.execute(
Review comment:
delete the blank space.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1059,7 +1060,7 @@ protected boolean deleteStorageGroups(DeleteStorageGroupPlan deleteStorageGroupP
}
return true;
}
-
+
Review comment:
delete the blank space.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] jt2594838 commented on a change in pull request #1698: [IoTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r486717381
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1116,14 +1125,26 @@ private ListDataSet executeListRole() {
return dataSet;
}
- private ListDataSet executeListUser() {
- List<String> userList = authorizer.listAllUsers();
+ private ListDataSet executeListUser(AuthorPlan plan) throws AuthException {
+ List<PartialPath> paths = new ArrayList<>();
+ paths.add(plan.getNodeName());
+ // check if current user is granted list_user privilege
Review comment:
Better replaced with `Collections.singletonList()`
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1116,14 +1125,26 @@ private ListDataSet executeListRole() {
return dataSet;
}
- private ListDataSet executeListUser() {
- List<String> userList = authorizer.listAllUsers();
+ private ListDataSet executeListUser(AuthorPlan plan) throws AuthException {
+ List<PartialPath> paths = new ArrayList<>();
+ paths.add(plan.getNodeName());
+ // check if current user is granted list_user privilege
+ boolean hasListUserPrivilege = AuthorityChecker
+ .check(plan.getLoginUserName(), paths, plan.getOperatorType(), plan.getLoginUserName());
+
List<PartialPath> headerList = new ArrayList<>();
List<TSDataType> typeList = new ArrayList<>();
headerList.add(new PartialPath(COLUMN_USER, false));
typeList.add(TSDataType.TEXT);
- int index = 0;
ListDataSet dataSet = new ListDataSet(headerList, typeList);
+
+ if (!hasListUserPrivilege) {
+ return dataSet;
+ }
+
+ List<String> userList = authorizer.listAllUsers();
+ int index = 0;
Review comment:
What is this index for?
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
I think maybe we can push loginUserName down to PhysicalPlan. It is often meaningful to know who issued this plan.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1098,13 +1099,21 @@ protected QueryDataSet processAuthorQuery(AuthorPlan plan)
return dataSet;
}
- private ListDataSet executeListRole() {
+ private ListDataSet executeListRole(AuthorPlan plan) throws AuthException {
int index = 0;
List<PartialPath> headerList = new ArrayList<>();
List<TSDataType> typeList = new ArrayList<>();
headerList.add(new PartialPath(COLUMN_ROLE, false));
typeList.add(TSDataType.TEXT);
ListDataSet dataSet = new ListDataSet(headerList, typeList);
+
+ boolean hasListRolePrivilege = AuthorityChecker
+ .check(plan.getLoginUserName(), new ArrayList<>(), plan.getOperatorType(), plan.getLoginUserName());
Review comment:
Better replaced with `Collections.emptyList()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] haimeiguo commented on a change in pull request #1698: [IoTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
haimeiguo commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r486717423
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/IoTDBAuthorizationIT.java
##########
@@ -793,15 +793,19 @@ public void testListRolePrivileges() throws ClassNotFoundException, SQLException
try {
adminStmt.execute("CREATE ROLE role1");
- adminStmt.execute(
+ ResultSet resultSet = adminStmt.executeQuery("LIST ROLE PRIVILEGES role1");
+ String ans = "";
+ try {
+ //not granted list role privilege, should return empty
+ validateResultSet(resultSet, ans);
+
+ adminStmt.execute(
"GRANT ROLE role1 PRIVILEGES 'READ_TIMESERIES','INSERT_TIMESERIES','DELETE_TIMESERIES' ON root.a.b.c");
- adminStmt.execute(
+ adminStmt.execute(
Review comment:
done. Thank you :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] jt2594838 commented on a change in pull request #1698: [IoTDB-850] check list user/role privilege
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r486717381
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1116,14 +1125,26 @@ private ListDataSet executeListRole() {
return dataSet;
}
- private ListDataSet executeListUser() {
- List<String> userList = authorizer.listAllUsers();
+ private ListDataSet executeListUser(AuthorPlan plan) throws AuthException {
+ List<PartialPath> paths = new ArrayList<>();
+ paths.add(plan.getNodeName());
+ // check if current user is granted list_user privilege
Review comment:
Better replaced with `Collections.singletonList()`
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1116,14 +1125,26 @@ private ListDataSet executeListRole() {
return dataSet;
}
- private ListDataSet executeListUser() {
- List<String> userList = authorizer.listAllUsers();
+ private ListDataSet executeListUser(AuthorPlan plan) throws AuthException {
+ List<PartialPath> paths = new ArrayList<>();
+ paths.add(plan.getNodeName());
+ // check if current user is granted list_user privilege
+ boolean hasListUserPrivilege = AuthorityChecker
+ .check(plan.getLoginUserName(), paths, plan.getOperatorType(), plan.getLoginUserName());
+
List<PartialPath> headerList = new ArrayList<>();
List<TSDataType> typeList = new ArrayList<>();
headerList.add(new PartialPath(COLUMN_USER, false));
typeList.add(TSDataType.TEXT);
- int index = 0;
ListDataSet dataSet = new ListDataSet(headerList, typeList);
+
+ if (!hasListUserPrivilege) {
+ return dataSet;
+ }
+
+ List<String> userList = authorizer.listAllUsers();
+ int index = 0;
Review comment:
What is this index for?
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/sys/AuthorPlan.java
##########
@@ -39,12 +39,13 @@
public class AuthorPlan extends PhysicalPlan {
private AuthorOperator.AuthorType authorType;
- private String userName;
private String roleName;
private String password;
private String newPassword;
private Set<Integer> permissions;
private PartialPath nodeName;
+ private String userName;
+ private String loginUserName;
Review comment:
I think maybe we can push loginUserName down to PhysicalPlan. It is often meaningful to know who issued this plan.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -1098,13 +1099,21 @@ protected QueryDataSet processAuthorQuery(AuthorPlan plan)
return dataSet;
}
- private ListDataSet executeListRole() {
+ private ListDataSet executeListRole(AuthorPlan plan) throws AuthException {
int index = 0;
List<PartialPath> headerList = new ArrayList<>();
List<TSDataType> typeList = new ArrayList<>();
headerList.add(new PartialPath(COLUMN_ROLE, false));
typeList.add(TSDataType.TEXT);
ListDataSet dataSet = new ListDataSet(headerList, typeList);
+
+ boolean hasListRolePrivilege = AuthorityChecker
+ .check(plan.getLoginUserName(), new ArrayList<>(), plan.getOperatorType(), plan.getLoginUserName());
Review comment:
Better replaced with `Collections.emptyList()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] haimeiguo commented on a change in pull request #1698: 850 - check list user privilege
Posted by GitBox <gi...@apache.org>.
haimeiguo commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r485327011
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/PhysicalPlan.java
##########
@@ -48,6 +48,7 @@
private static final String SERIALIZATION_UNIMPLEMENTED = "serialization unimplemented";
+ protected String userName;
Review comment:
Yes, I will change it to AuthorPlan only. Thank you for your comment!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] neuyilan commented on a change in pull request #1698: 850 - check list user privilege
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #1698:
URL: https://github.com/apache/incubator-iotdb/pull/1698#discussion_r485299335
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/PhysicalPlan.java
##########
@@ -48,6 +48,7 @@
private static final String SERIALIZATION_UNIMPLEMENTED = "serialization unimplemented";
+ protected String userName;
Review comment:
Does every physical plan have to have a userName?
for OOP, I do not think its a good design.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org