Permission for Relation model. #13158

ColinLeeo · 2024-08-14T00:18:50Z

In this pr:

Supported permission management under the table model, including user management and permission control.
Optimized some logical structures in the code, adjusted abstractions, making it easier to maintain and expand.
Removed support for upgrading from the old permission system before version 1.3. Upgrading to the 1.4 series requires upgrading to version 1.3 first.

Caideyipi · 2024-09-11T02:17:10Z

...nfignode/src/main/java/org/apache/iotdb/confignode/consensus/request/ConfigPhysicalPlan.java

-          plan = new AuthorPlan(configPhysicalPlanType);
+          plan = new AuthorTreePlan(configPhysicalPlanType);
+          break;
+        case RCreateRole:


Better keep the order consistent to the definitions.

Caideyipi · 2024-09-11T02:30:44Z

...e/src/main/java/org/apache/iotdb/confignode/consensus/request/auth/AuthorRelationalPlan.java

+      final boolean grantOpt,
+      final int permission,
+      final String password) {
+    super(authorType);


May directly call the super() with more parameters.

Caideyipi · 2024-09-11T03:29:05Z

...src/test/java/org/apache/iotdb/confignode/consensus/request/ConfigPhysicalPlanSerDeTest.java

@@ -576,7 +577,7 @@ public void AuthorPlanTest() throws IOException, IllegalPathException {

    // create user
    req0 =
-        new AuthorPlan(
+        new AuthorTreePlan(


Remember to add the ser/de tests for new plans.

Their ser/de functions were tested at authOperationProcedureTest, which ser/de itself and its owned author plan.

Caideyipi · 2024-09-11T03:51:59Z

.../src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/AuthorRStatement.java

+      case REVOKE_USER_ROLE:
+        return QueryType.WRITE;
+      case LIST_ROLE:
+      case LIST_USER:


Has the "List privilege" been designed, or are they shown in the "list role/user"?

This file is outdate, see RelationalAuthorStatement.java

Caideyipi · 2024-09-11T03:52:39Z

.../src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/AuthorRStatement.java

+        + tableName
+        + "user name:"
+        + userName
+        + "role name"


The format can be more normalized..

This file is not exist in this pr, see RelationalAuthorStatement.java.

Caideyipi · 2024-09-11T04:04:35Z

...ode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/parser/AstBuilder.java

+    }
+    return text;
+  }
+  @Override


Bettter add a blank line, and why do we use "''" in the password?

Caideyipi · 2024-09-11T04:21:14Z

...ode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/parser/AstBuilder.java

+    if (ctx.grant_privilege_object().ON() != null) {
+      String privilegeText = ctx.grant_privilege_object().object_privilege().getText();
+      PrivilegeType priv = PrivilegeType.valueOf(privilegeText.toUpperCase());
+      if (ctx.grant_privilege_object().object_type().getText().equalsIgnoreCase("TABLE")) {


Are the "db.table" and "ANY" taken into account here?

Caideyipi · 2024-09-11T04:23:37Z

iotdb-core/datanode/src/main/java/org/apache/iotdb/db/auth/IAuthorityFetcher.java


-  TSStatus checkUserSysPrivileges(String username, int permisssion);
+  TSStatus checkUserSysPrivileges(String username, PrivilegeType permisssion);


Better change it to "permission", not "permisssion"..

Caideyipi · 2024-09-11T06:45:43Z

Be sure to check the license issues.

Caideyipi · 2024-09-11T09:09:16Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/PrivilegeUnin.java

+    this.grantOption = false;
+  }
+
+  public PrivilegeModelType ModelType() {


use camelCase

Caideyipi · 2024-09-11T09:33:44Z

iotdb-protocol/thrift-confignode/src/main/thrift/confignode.thrift

+  7: required bool grantOpt
+}
+
+struct TCheckUserRelationalPrivilegesReq {


Seems useless?

add author rpc.

ColinLeeo · 2025-01-02T00:39:37Z

...lational-grammar/src/main/antlr4/org/apache/iotdb/db/relational/grammar/sql/RelationalSql.g4

+    ;
+
+privilegeObjectScope
+    : systemPrivilege


This pr is to big.. I will create a new pr base this pr to support ALL.

Caideyipi · 2025-01-03T01:55:38Z

...lational-grammar/src/main/antlr4/org/apache/iotdb/db/relational/grammar/sql/RelationalSql.g4

@@ -1191,6 +1301,7 @@ PASSING: 'PASSING';
 PAST: 'PAST';
 PATH: 'PATH';
 PATTERN: 'PATTERN';
+PASSWORD: 'PASSWORD';


Be sure to check the dictionary order...

Caideyipi · 2025-01-03T02:19:34Z

...main/java/org/apache/iotdb/confignode/consensus/request/write/auth/AuthorRelationalPlan.java

+    return tableName;
+  }
+
+  public Set<Integer> getPermissions() {


Seemingly not useful?

It works when we support ALL privileges.

Caideyipi · 2025-01-03T02:22:28Z

...main/java/org/apache/iotdb/confignode/consensus/request/write/auth/AuthorRelationalPlan.java

+    ReadWriteIOUtils.write(getType().getPlanType(), stream);
+    BasicStructureSerDeUtil.write(userName, stream);
+    BasicStructureSerDeUtil.write(roleName, stream);
+    BasicStructureSerDeUtil.write(password, stream);


Do you need to write "newPassword"?

Actually, newPassword is useless. In TreeAuthorPlan, It is used to store the user's new password, which can be replaced by a password field. So in RelationalAuthorPlan, the user's new password or his original password will all be stored in the password field.

Caideyipi · 2025-01-03T02:26:03Z

...main/java/org/apache/iotdb/confignode/consensus/request/write/auth/AuthorRelationalPlan.java

+    BasicStructureSerDeUtil.write(userName, stream);
+    BasicStructureSerDeUtil.write(roleName, stream);
+    BasicStructureSerDeUtil.write(password, stream);
+    if (databaseName == null) {


Can we write this string directly?

Caideyipi · 2025-01-03T02:47:48Z

...pache/iotdb/confignode/manager/pipe/extractor/PipeConfigPhysicalPlanPatternParseVisitor.java

+                plan.getRoleName(),
+                plan.getPassword(),
+                plan.getNewPassword(),
+                plan.getPermissions(),


Better change this to back to "permissions"... This has been trimmed by user's pattern....

Caideyipi · 2025-01-03T03:57:28Z

iotdb-core/node-commons/src/test/java/org/apache/iotdb/commons/utils/AuthUtilsTest.java

-    Assert.assertEquals(privs.get(0).getPrivileges().size(), 2);
-    Assert.assertFalse(privs.get(0).getPrivileges().contains(PrivilegeType.READ_DATA.ordinal()));
+    AuthUtils.removePrivilege(rootPath, PrivilegeType.READ_DATA, privs);
+    Assert.assertEquals(privs.get(0).getPrivilegeIntSet().size(), 2);


May put the "expected value" first...

Caideyipi · 2025-01-03T04:01:32Z

.../node-commons/src/main/java/org/apache/iotdb/commons/schema/column/ColumnHeaderConstant.java

@@ -604,7 +605,7 @@ private ColumnHeaderConstant() {
  public static final List<ColumnHeader> LIST_USER_PRIVILEGES_Column_HEADERS =


Use upper case here, and better name it as "USER_OR_ROLE_PRIVILEGES"

Caideyipi · 2025-01-03T04:07:18Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/BasicUserManager.java

-      LOGGER.warn("Cannot load admin, Creating a new one", e);
-      admin = null;
-    }
+    User admin = getEntry(CommonDescriptor.getInstance().getConfig().getAdminName());


”Entry“ may be too unclear... Maybe "identity" is better?

Caideyipi · 2025-01-03T04:10:35Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/BasicUserManager.java

  }

-  @Override
-  public boolean createUser(
+  public boolean createEntry(


Name it "createUser" if this is not overrided from its super class.

Caideyipi · 2025-01-03T04:12:36Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/BasicUserManager.java

-    lock.readUnlock(username);
-    return user;
+  public User getEntry(String username) {
+    return (User) super.getEntry(username);


Do we need to override it here?

Caideyipi · 2025-01-03T06:34:19Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java

+  public boolean checkTBVisible(String database, String tbName) {
+    return !anyScopePrivilegeSet.isEmpty()
+        || (objectPrivilegeMap.containsKey(database)
+            && objectPrivilegeMap.get(database).getTablePrivilegeMap().containsKey(tbName));


Seemingly this shall be "||" according to the definition? Though you may need to check if any DB privileges exist....

Caideyipi · 2025-01-03T07:17:45Z

iotdb-core/datanode/src/main/java/org/apache/iotdb/db/auth/ClusterAuthorityFetcher.java

@@ -387,6 +466,10 @@ private void checkCacheAvailable() {
    cacheOutDate = false;
  }

+  public void setAcceptCache(boolean acceptCache) {


Is this used?

Caideyipi · 2025-01-03T07:41:43Z

...rc/main/java/org/apache/iotdb/db/queryengine/plan/relational/security/AccessControlImpl.java

@@ -88,4 +94,184 @@ public void checkCanShowOrDescTable(String userName, QualifiedObjectName tableNa
  public void checkUserHasMaintainPrivilege(String userName) {
    authChecker.checkGlobalPrivilege(userName, TableModelPrivilege.MAINTAIN);
  }
+
+  @Override
+  public void checkUserCanRunAuthorStatement(String userName, RelationalAuthorStatement statement) {


Can we reuse the AuthorType and checking logic of the (tree) AuthorStatement? They look the same....

Caideyipi · 2025-01-03T08:01:08Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java


+  protected Set<PrivilegeType> anyScopePrivileGrantOptSet;


Caideyipi · 2025-01-03T08:05:38Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java

+    return objectPrivilegeMap;
+  }
+
+  public DatabasePrivilege getObjectPrivilege(String objectName) {


This method is dangerous... Currently it's only used in "grant" thus it is OK. However, if someone wants to only get the database privilege from outside, there will be an empty database privilege, then the database may be able to be shown.

Caideyipi · 2025-01-03T08:56:28Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/role/BasicRoleManager.java

+              role.revokeTBPrivilegeGrantOption(dbName, tbName, privilegeType);
+              return true;
+            }
+            if (role.hasPrivilegeToRevoke(dbName, tbName, privilegeType)) {


Seems useless here?

Caideyipi · 2025-01-03T08:59:45Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java

-  public boolean checkPathPrivilege(PartialPath path, int privilegeId) {
-    return AuthUtils.checkPathPrivilege(path, privilegeId, pathPrivilegeList);
+  public boolean checkTablePrivilegeGrantOption(String dbname, String table, PrivilegeType priv) {
+    if (checkAnyScopePrivilegeGrantOption(priv)) {


This may be called twice..

Caideyipi · 2025-01-03T09:05:30Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java

@@ -193,33 +535,43 @@ public boolean equals(Object o) {
    return Objects.equals(name, role.name)
        && Objects.equals(pathPrivilegeList, role.pathPrivilegeList)
        && Objects.equals(sysPrivilegeSet, role.sysPrivilegeSet)
-        && Objects.equals(sysPriGrantOpt, role.sysPriGrantOpt);
+        && Objects.equals(sysPriGrantOpt, role.sysPriGrantOpt)
+        && Objects.equals(objectPrivilegeMap, role.objectPrivilegeMap);
  }

  @Override
  public int hashCode() {
    return Objects.hash(name, pathPrivilegeList, sysPrivilegeSet);


Caideyipi · 2025-01-03T09:05:50Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java

@@ -193,33 +535,43 @@ public boolean equals(Object o) {
    return Objects.equals(name, role.name)
        && Objects.equals(pathPrivilegeList, role.pathPrivilegeList)
        && Objects.equals(sysPrivilegeSet, role.sysPrivilegeSet)
-        && Objects.equals(sysPriGrantOpt, role.sysPriGrantOpt);
+        && Objects.equals(sysPriGrantOpt, role.sysPriGrantOpt)
+        && Objects.equals(objectPrivilegeMap, role.objectPrivilegeMap);


Caideyipi · 2025-01-03T09:50:02Z

...ore/node-commons/src/main/java/org/apache/iotdb/commons/auth/role/LocalFileRoleAccessor.java

-        return role;
-      }
+      int version = dataInputStream.readInt();
+      assert version == VERSION;


Should guarantee the compatibility here...

Caideyipi · 2025-01-03T11:25:26Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java

  }

  @Override
  public int hashCode() {
    return Objects.hash(name, pathPrivilegeList, sysPrivilegeSet);
  }

+  private void serializePrivilegeSet(DataOutputStream outputStream, Set<PrivilegeType> set)


May be identical to SerializeUtils.serializePrivilegeTypeSet...

Caideyipi · 2025-01-03T11:26:00Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/utils/SerializeUtils.java

+      for (PrivilegeType type : types) {
+        dataOutputStream.writeInt(type.ordinal());
+      }
+    } catch (IOException e) {


Do we need to catch it?

Caideyipi · 2025-01-03T11:27:53Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/Role.java

@@ -246,6 +598,14 @@ public void deserialize(ByteBuffer buffer) {
      pathPrivilege.deserialize(buffer);
      pathPrivilegeList.add(pathPrivilege);
    }
+


anyScopePrivilegeSet?

Caideyipi · 2025-01-03T11:42:20Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/role/IEntryManager.java

+  void reset() throws AuthException;
+
+  /**
+   * List all roles in the database.


users/roles

Caideyipi · 2025-01-03T11:42:36Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/role/IEntryManager.java

+  /**
+   * List all roles in the database.
+   *
+   * @return A list that contains names of all roles.


users/roles

Caideyipi · 2025-01-03T11:43:17Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/role/IEntryManager.java

+  void grantPrivilegeToEntry(String entryName, PrivilegeUnion privilegeUnion) throws AuthException;
+
+  /**
+   * Revoke a privilege on seriesPath from a entry.


on series path?

Caideyipi · 2025-01-03T11:47:01Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/TablePrivilege.java

+  }
+
+  public TablePrivilege() {
+    //


Leaving "//" may not be a good idea...

Caideyipi · 2025-01-03T12:44:58Z

iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/AuthorInfo.java

@@ -423,6 +465,7 @@ public TAuthizedPatternTreeResp generateAuthizedPTree(String username, int permi
      throws AuthException {
    TAuthizedPatternTreeResp resp = new TAuthizedPatternTreeResp();


Better rename this to "Authorized"

Caideyipi · 2025-01-07T01:47:19Z

...ode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/util/SqlFormatter.java

@@ -1169,6 +1170,67 @@ protected Void visitShowTopics(ShowTopics node, Integer context) {
      return null;
    }

+    @Override
+    protected Void visitRelationalAuthorPlan(RelationalAuthorStatement node, Integer context) {


Well, personally I think all other sqls just write the full SQL...

Caideyipi · 2025-01-07T02:02:06Z

iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/persistence/AuthorInfoTest.java

+
+    assertEquals(
+        authorInfo.checkRoleOfUser("user", "role").getStatus().getCode(),
+        TSStatusCode.SUCCESS_STATUS.getStatusCode());


Better place the expected value at first.

Caideyipi · 2025-01-07T02:23:27Z

iotdb-protocol/thrift-confignode/src/main/thrift/confignode.thrift

+// to check relational privilege, database or table is required
+// to check system privilege, just spec permission
+
+// if grant opt is ture, check grant option of spec permission.


Caideyipi · 2025-01-07T02:32:26Z

...-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/DatabasePrivilege.java

+  }
+
+  public DatabasePrivilege() {
+    //


May not leave "//"?

Caideyipi · 2025-01-07T02:41:45Z

...-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/DatabasePrivilege.java

+    }
+
+    DatabasePrivilege that = (DatabasePrivilege) o;
+    return databaseName.equals(that.databaseName)


Using "Objects.equals()" may be safer because the "databaseName" is nullable(though now nullable privilege won't call "equals()")

Caideyipi · 2025-01-07T03:09:30Z

...ain/java/org/apache/iotdb/db/queryengine/plan/relational/security/ITableAuthCheckerImpl.java

+            tableName.getDatabaseName(),
+            tableName.getObjectName());
+    if (result.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+      throw new RuntimeException(new IoTDBException(result.getMessage(), result.getCode()));


Use "AccessControlException" instead?

Caideyipi · 2025-01-07T03:14:45Z

...ain/java/org/apache/iotdb/db/queryengine/plan/relational/security/ITableAuthCheckerImpl.java

+      return;
+    }
+    TSStatus result =
+        AuthorityChecker.getTSStatus(


getGrantOptTSStatus?

Caideyipi · 2025-01-07T03:17:35Z

...ain/java/org/apache/iotdb/db/queryengine/plan/relational/security/ITableAuthCheckerImpl.java

+  @Override
+  public void checkTablePrivilegeGrantOption(
+      String userName, QualifiedObjectName tableName, TableModelPrivilege privilege) {
+    TSStatus result =


Why there are no "AuthorityChecker.SUPER_USER.equals(userName)" here...

Caideyipi · 2025-01-07T03:18:05Z

...ain/java/org/apache/iotdb/db/queryengine/plan/relational/security/ITableAuthCheckerImpl.java

+  @Override
+  public void checkDatabasePrivilegeGrantOption(
+      String userName, String databaseName, TableModelPrivilege privilege) {
+    TSStatus result =


Why there are no "AuthorityChecker.SUPER_USER.equals(userName)" here...

Caideyipi · 2025-01-07T03:37:31Z

...ode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/parser/AstBuilder.java

+  @Override
+  public Node visitAlterUserStatement(RelationalSqlParser.AlterUserStatementContext ctx) {
+    RelationalAuthorStatement stmt = new RelationalAuthorStatement(AuthorRType.UPDATE_USER);
+    stmt.setRoleName(ctx.userName.getText());


Why "roleName" here?

Caideyipi · 2025-01-07T04:03:42Z

...ode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/parser/AstBuilder.java

+        if (toTable) {
+          databaseName = clientSession.getDatabaseName();
+          if (databaseName == null) {
+            throw new SemanticException("Database is set yet.");


Caideyipi · 2025-01-07T04:08:53Z

...ode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/parser/AstBuilder.java

+        if (fromTable) {
+          databaseName = clientSession.getDatabaseName();
+          if (databaseName == null) {
+            throw new SemanticException("Database is set yet.");


Same as above

Caideyipi · 2025-01-07T04:22:33Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/PathPrivilege.java

+  private Set<PrivilegeType> privileges;
+  private Set<PrivilegeType> grantOpts;
+
+  private final int PRI_SIZE = PrivilegeType.getPrivilegeCount(PrivilegeModelType.TREE);


May be static

Caideyipi · 2025-01-07T04:28:07Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/entity/PathPrivilege.java

-    for (Integer privilegeId : privileges) {
-      builder.append(" ").append(PrivilegeType.values()[privilegeId]);
-      if (grantOpts.contains(privilegeId)) {
+    List<PrivilegeType> sortedPrivileges = new ArrayList<>(privileges);


Better change line 185 BTW

Caideyipi · 2025-01-07T06:22:42Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/entity/DataBasePrivilegeTest.java

+    dbPrivilege2.revokeTablePrivilege("test", PrivilegeType.SELECT);
+    dbPrivilege2.revokeTableGrantOption("test", PrivilegeType.SELECT);
+    dbPrivilege2.revokeTablePrivilege("test", PrivilegeType.DELETE);
+    Assert.assertEquals(dbPrivilege2.getTablePrivilegeMap().size(), 1);


Notice the order of the expected value and the actual one

Caideyipi · 2025-01-07T06:24:11Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/entity/DataBasePrivilegeTest.java

+    dbPrivilege2.revokeTablePrivilege("test2", PrivilegeType.SELECT);
+    dbPrivilege2.revokeDBPrivilege(PrivilegeType.INSERT);
+    dbPrivilege2.revokeDBGrantOption(PrivilegeType.ALTER);
+    Assert.assertEquals(dbPrivilege2.getTablePrivilegeMap().size(), 0);


Same as above

Caideyipi · 2025-01-07T06:33:49Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/entity/RoleTest.java

+      } else if (item.isRelationalPrivilege()) {
+        databasePrivilege.grantDBPrivilege(item);
+        databasePrivilege.grantDBGrantOption(item);
+        databasePrivilege.grantTablePrivilege("testTable", item);


Better add grant option for admin

Caideyipi · 2025-01-07T06:43:39Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/entity/TablePrivilegeTest.java

+
+public class TablePrivilegeTest {
+  @Test
+  public void testTablePrivilege_Init() {


This may not reflect the real change of table privilege, because the "privilege" and "grant option" seems indepentent here.

Caideyipi · 2025-01-07T06:47:05Z

...tanode/src/test/java/org/apache/iotdb/db/queryengine/plan/parser/StatementGeneratorTest.java

@@ -756,11 +757,12 @@ public void testNormalGrantRevoke() {

    // 3. check simple privilege revoke from user/role on simple path
    for (PrivilegeType type : PrivilegeType.values()) {
+      if (type.isRelationalPrivilege()) continue;


Better place the "continue" in a separate line?

Caideyipi · 2025-01-07T06:57:47Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/authorizer/IAuthorizer.java

-   *     seriesPath-free privilege, this should be "root".
-   * @param privilegeId An integer that represents a privilege.
-   * @param grantOpt Whether the privilege is grant option.
+   * @param userName The username of the user to which the privilege should be added.


Better add a little description fro the "union" field

Caideyipi · 2025-01-07T07:08:55Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/role/LocalFileRoleAccessorTest.java

        roles[i].getPathPrivilegeList().add(pathPrivilege);
-        roles[i].getSysPrivilege().add(i + 4);
+        roles[i].grantSysPrivilege(PrivilegeType.values()[i + 4], false);
+        roles[i].grantDBPrivilege("testdb", PrivilegeType.CREATE, false);


Seemingly the "CREATE" is granted twice

Caideyipi · 2025-01-07T07:30:00Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/entity/UserTest.java

        user1.toString());
-    Assert.assertTrue(user1.equals(user));
+    Assert.assertEquals(user1, user);


Better swap the sequence

Caideyipi · 2025-01-07T07:35:04Z

integration-test/src/test/java/org/apache/iotdb/db/it/auth/IoTDBRelationalAuthIT.java

+import static org.junit.Assert.assertTrue;
+
+@RunWith(IoTDBTestRunner.class)
+@Category({LocalStandaloneIT.class, ClusterIT.class})


@category({TableLocalStandaloneIT.class, TableClusterIT.class})

Caideyipi · 2025-01-07T07:36:24Z

...ration-test/src/test/java/org/apache/iotdb/db/it/auth/IoTDBClusterAuthorityRelationalIT.java

+import static org.junit.Assert.assertTrue;
+
+@RunWith(IoTDBTestRunner.class)
+@Category({ClusterIT.class})


@category({TableClusterIT.class})

Caideyipi · 2025-01-07T07:37:16Z

...ration-test/src/test/java/org/apache/iotdb/db/it/auth/IoTDBClusterAuthorityRelationalIT.java

+public class IoTDBClusterAuthorityRelationalIT {
+  @Before
+  public void setUp() throws Exception {
+    // Init 1C0D environment


Caideyipi · 2025-01-07T07:44:20Z

...ration-test/src/test/java/org/apache/iotdb/db/it/auth/IoTDBClusterAuthorityRelationalIT.java

+    assertEquals(status.getCode(), TSStatusCode.SUCCESS_STATUS.getStatusCode());
+  }
+
+  private void cleanUserAndRole(SyncConfigNodeIServiceClient client) throws TException {


Only "user" is cleared?

Caideyipi · 2025-01-07T07:55:18Z

...ration-test/src/test/java/org/apache/iotdb/db/it/auth/IoTDBClusterAuthorityRelationalIT.java

+              AuthorRType.GRANT_USER_ROLE.ordinal(), "user1", "role1", "", "", "", -1, false));
+      grantSysPrivilegeAndCheck(client, "user1", "role1", true, PrivilegeType.MANAGE_USER, false);
+      grantSysPrivilegeAndCheck(client, "user1", "role1", true, PrivilegeType.MANAGE_ROLE, true);
+      grantSysPrivilegeAndCheck(client, "user1", "role1", false, PrivilegeType.MAINTAIN, true);


Better only specify one of user and role to make it clearer…… And in fact we shall only pass one "name" into the parameter

Caideyipi · 2025-01-07T08:20:02Z

...core/datanode/src/test/java/org/apache/iotdb/db/auth/authorizer/LocalFileAuthorizerTest.java

@@ -101,174 +98,143 @@ public void createAndDeleteUser() throws AuthException {

  @Test
  public void createAndDeleteRole() throws AuthException {
-    authorizer.createRole(user.getName());
+    authorizer.createRole(userName);


Better use "roleName"?

Caideyipi · 2025-01-07T08:21:27Z

...core/datanode/src/test/java/org/apache/iotdb/db/auth/authorizer/LocalFileAuthorizerTest.java

    } catch (AuthException e) {
      assertEquals("Role user already exists", e.getMessage());
    }
-    authorizer.deleteRole(user.getName());
+    authorizer.createUser("test", "password");


Better use "userName"?

Caideyipi · 2025-01-07T08:23:34Z

...core/datanode/src/test/java/org/apache/iotdb/db/auth/authorizer/LocalFileAuthorizerTest.java

    try {
      authorizer.revokePrivilegeFromUser(
-          user.getName(), nodeName, PrivilegeType.READ_DATA.ordinal());
+          userName, new PrivilegeUnion(nodeName, PrivilegeType.READ_DATA));
    } catch (AuthException e) {
      assertEquals("User user does not have READ_DATA on root.laptop.d1", e.getMessage());


This will not be thrown now, do not catch it

Caideyipi · 2025-01-07T09:00:40Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/user/LocalFileUserAccessorTest.java

+    accessor.saveEntry(user);
+    accessor.reset();
+    User loadUser = accessor.loadEntry("test");
+    assertEquals(user, loadUser);


Better reserve the "List" and "Delete" tests.

Caideyipi · 2025-01-09T02:00:24Z

integration-test/src/test/java/org/apache/iotdb/db/it/auth/IoTDBRelationalAuthIT.java

+      List<String> resultLines = Arrays.asList(result.split("\n"));
+      assertEquals(ansLines.size(), resultLines.size());
+      for (String resultLine : resultLines) {
+        assertTrue(ansLines.contains(resultLine));


Why use "contains"? BTW, there is a similar function called TestUtils.assertResultSetEqual().

Caideyipi · 2025-01-09T04:00:54Z

iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/role/BasicRoleManager.java

+    return "No such role %s";
+  }
+
+  protected BasicRoleManager() {


Is it used?

Caideyipi · 2025-01-09T07:49:35Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/AuthorizerManagerTest.java


    // user's priv:
    // 1. MANAGE_DATABASE
    // 2. USE_PIPE with grant option
    // 3. READ_DATA root.d1.**
    // 4. WRITE_SCHEMA root.d1.** with grant option
+    // 5. SELECT on database, ALTER on database with grant option
+    // 6. UPDATE on database.table, DELETE on database.table with grant option

    // role's priv:
    // 1. USE_UDF
    // 2. USE_CQ with grant option
    // 3. READ_DATA root.t9.** with grant option


t9? Database privilege?

Caideyipi · 2025-01-09T07:50:55Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/AuthorizerManagerTest.java

-            "user1", Collections.singletonList(pathRoot), PrivilegeType.MANAGE_USER.ordinal()));
+    Assert.assertEquals(
+        authorityFetcher.checkUserSysPrivilegesGrantOpt("user1", PrivilegeType.USE_PIPE).getCode(),
+        TSStatusCode.SUCCESS_STATUS.getStatusCode());


Put the expected value at the front

Caideyipi · 2025-01-09T07:53:18Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/AuthorizerManagerTest.java

+        authorityFetcher
+            .checkUserPathPrivilegesGrantOpt(
+                "user1",
+                Collections.singletonList(new PartialPath("root.d1.d1.**")),


Above just said "root.d1.d2"

Caideyipi · 2025-01-09T07:59:15Z

iotdb-core/datanode/src/test/java/org/apache/iotdb/db/auth/AuthorizerManagerTest.java

+                Collections.singletonList(new PartialPath("root.**")),
+                PrivilegeType.WRITE_SCHEMA)
+            .getCode(),
+        TSStatusCode.NO_PERMISSION.getStatusCode());
    // reuqire root.d1.d2 with write_schema, return true


Caideyipi · 2025-01-09T08:19:12Z

...ore/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/LocalFileUserAccessor.java

-
-        List<String> roleList = new ArrayList<>();
-        if (roleOfUser.exists()) {
+        Set<String> roleList = new HashSet<>();


Caideyipi · 2025-01-09T08:27:53Z

...ore/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/LocalFileUserAccessor.java

+      assert (tag == VERSION);
+      user.setName(IOUtils.readString(dataInputStream, STRING_ENCODING, strBufferLocal));
+      user.setPassword(IOUtils.readString(dataInputStream, STRING_ENCODING, strBufferLocal));
+      user.setSysPrivilegesWithMask(dataInputStream.readInt());


Why not put this in "loadPrivileges"?

Caideyipi · 2025-01-09T08:30:37Z

...ore/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/LocalFileUserAccessor.java

+      Set<String> roleSet = new HashSet<>();
+      if (roleOfUser.exists()) {
+        inputStream = new FileInputStream(roleOfUser);
+        try (DataInputStream roleInpuStream =


roleInputStream

Caideyipi · 2025-01-09T08:40:21Z

...ore/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/LocalFileUserAccessor.java

-  public List<String> listAllUsers() {
-    File userDir = SystemFileFactory.INSTANCE.getFile(userDirPath);
+  public List<String> listAllEntries() {
+    File userDir = SystemFileFactory.INSTANCE.getFile(entryDirPath);
    String[] names =
        userDir.list(
            (dir, name) ->
                (name.endsWith(IoTDBConstant.PROFILE_SUFFIX)
                        && !name.endsWith(ROLE_SUFFIX + IoTDBConstant.PROFILE_SUFFIX))
                    || (name.endsWith(TEMP_SUFFIX) && !name.endsWith(ROLE_SUFFIX + TEMP_SUFFIX)));


ROLE_SUFFIX + PROFILE_SUFFIX + TEMP_SUFFIX?

ColinLeeo marked this pull request as draft August 14, 2024 02:21

ColinLeeo changed the base branch from ty/TableModelGrammar to master August 14, 2024 02:38

ColinLeeo changed the base branch from master to ty/TableModelGrammar August 14, 2024 02:39

Caideyipi reviewed Sep 11, 2024

View reviewed changes

ColinLeeo force-pushed the colin_table_auth branch from a6077b0 to 5e99d00 Compare December 5, 2024 06:27

ColinLeeo changed the base branch from ty/TableModelGrammar to master December 5, 2024 06:28

ColinLeeo force-pushed the colin_table_auth branch from a5c57ee to 07fd8de Compare December 11, 2024 10:03

ColinLeeo marked this pull request as ready for review December 19, 2024 15:25

ColinLeeo force-pushed the colin_table_auth branch from d36e6a5 to 4496e1a Compare December 19, 2024 15:28

ColinLeeo added 19 commits December 25, 2024 00:06

add auth relation sql.

731562f

add author rpc.

fix code style.

38743fc

fix sql.g4

6e8a706

fix compile.

9dfbf55

fix auth plan.

c45c7c1

fix list.

619e70a

fix auth parser.

de2a9b6

fix tmp.

55f291a

fix anltr for author.

cd59053

fix auth list.

1c5dfc2

fix plan serialize.

33ef097

fix privilege check.

fb944e7

add table privilege checker.

0726bd9

fix ut test.

72e4cd8

fix ut error.

c9819ad

add fix uts.

dcf6933

add check visible.

113bb07

add some tests.

adaa94f

code add.

6748f63

add some uts and its.

db6dd55

ColinLeeo force-pushed the colin_table_auth branch from 7f58b02 to db6dd55 Compare December 25, 2024 19:00

ColinLeeo added 8 commits December 30, 2024 21:34

add its.

2f5a399

add old version user load.

e98179a

fix uts.

0c01e85

fix its.

a50e0b2

fix check remote .

db9161f

remote auth write plan.

c6e8792

fix pipe auth check.

3e52e56

fix some errors.

7eb7211

ColinLeeo commented Jan 2, 2025

View reviewed changes

ColinLeeo requested a review from JackieTien97 January 2, 2025 03:49

Caideyipi reviewed Jan 3, 2025

View reviewed changes

tmp code.

86171e1

Caideyipi reviewed Jan 7, 2025

View reviewed changes

Caideyipi reviewed Jan 8, 2025

View reviewed changes

Caideyipi reviewed Jan 9, 2025

View reviewed changes

tmp code.

bc3c721


		TSStatus checkUserSysPrivileges(String username, int permisssion);
		TSStatus checkUserSysPrivileges(String username, PrivilegeType permisssion);

		@@ -604,7 +605,7 @@ private ColumnHeaderConstant() {
		public static final List<ColumnHeader> LIST_USER_PRIVILEGES_Column_HEADERS =

		@@ -423,6 +465,7 @@ public TAuthizedPatternTreeResp generateAuthizedPTree(String username, int permi
		throws AuthException {
		TAuthizedPatternTreeResp resp = new TAuthizedPatternTreeResp();

Permission for Relation model. #13158

Are you sure you want to change the base?

Permission for Relation model. #13158

Conversation

ColinLeeo commented Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinLeeo Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Caideyipi commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinLeeo commented Aug 14, 2024 •

edited

Loading

ColinLeeo Jan 5, 2025 •

edited

Loading