This is an automated email from the ASF dual-hosted git repository. yiguolei pushed a commit to branch branch-2.1 in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-2.1 by this push: new 45e1f9f4c4c [branch-2.1](cherry-pick)Role comment 2.1 #32837 45e1f9f4c4c is described below commit 45e1f9f4c4c596708a5a467395d2f47c5f3e3366 Author: zhangdong <493738...@qq.com> AuthorDate: Wed Mar 27 08:33:54 2024 +0800 [branch-2.1](cherry-pick)Role comment 2.1 #32837 --- fe/fe-core/src/main/cup/sql_parser.cup | 27 +++++- .../{CreateRoleStmt.java => AlterRoleStmt.java} | 32 +++---- .../org/apache/doris/analysis/AlterUserStmt.java | 17 +++- .../org/apache/doris/analysis/CreateRoleStmt.java | 20 +++- .../org/apache/doris/analysis/CreateUserStmt.java | 16 +++- .../org/apache/doris/analysis/ShowRolesStmt.java | 1 + .../org/apache/doris/common/proc/AuthProcDir.java | 5 +- .../org/apache/doris/journal/JournalEntity.java | 1 + .../org/apache/doris/mysql/privilege/Auth.java | 105 +++++++++++++++------ .../org/apache/doris/mysql/privilege/Role.java | 16 ++++ .../apache/doris/mysql/privilege/RoleManager.java | 1 + .../org/apache/doris/mysql/privilege/User.java | 13 ++- .../apache/doris/mysql/privilege/UserManager.java | 10 +- .../doris/persist/AlterUserOperationLog.java | 10 +- .../java/org/apache/doris/persist/EditLog.java | 9 ++ .../org/apache/doris/persist/OperationType.java | 2 + .../java/org/apache/doris/persist/PrivInfo.java | 17 ++++ .../main/java/org/apache/doris/qe/DdlExecutor.java | 3 + .../org/apache/doris/mysql/privilege/AuthTest.java | 4 +- .../suites/account_p0/test_account.groovy | 23 ++++- .../suites/account_p0/test_revoke_role.groovy | 4 +- regression-test/suites/account_p0/test_role.groovy | 19 ++++ 22 files changed, 280 insertions(+), 75 deletions(-) diff --git a/fe/fe-core/src/main/cup/sql_parser.cup b/fe/fe-core/src/main/cup/sql_parser.cup index f907b762407..8d6c43ff6c2 100644 --- a/fe/fe-core/src/main/cup/sql_parser.cup +++ b/fe/fe-core/src/main/cup/sql_parser.cup @@ -953,7 +953,7 @@ nonterminal JobName job_name; nonterminal Map<String, String> with_analysis_properties; nonterminal List<Map<String, String>> opt_with_analysis_properties; -nonterminal String opt_db, procedure_or_function, opt_comment, opt_engine; +nonterminal String opt_db, procedure_or_function, opt_comment, opt_comment_null, opt_engine; nonterminal ColumnDef.DefaultValue opt_default_value; nonterminal Boolean opt_if_exists, opt_if_not_exists; nonterminal Boolean opt_external; @@ -1427,8 +1427,9 @@ alter_stmt ::= opt_if_exists:ifExists grant_user:user opt_password_option:passwdOptions + opt_comment_null:comment {: - RESULT = new AlterUserStmt(ifExists, user, null, passwdOptions); + RESULT = new AlterUserStmt(ifExists, user, null, passwdOptions, comment); :} | KW_ALTER KW_REPOSITORY ident:repoName opt_properties:properties {: @@ -1877,8 +1878,9 @@ create_stmt ::= grant_user:user opt_user_role:userRole opt_password_option:passwdOptions + opt_comment:comment {: - RESULT = new CreateUserStmt(ifNotExists, user, userRole, passwdOptions); + RESULT = new CreateUserStmt(ifNotExists, user, userRole, passwdOptions, comment); :} | KW_CREATE KW_VIEW opt_if_not_exists:ifNotExists table_name:viewName opt_col_with_comment_list:columns opt_comment:comment KW_AS query_stmt:view_def @@ -1889,9 +1891,13 @@ create_stmt ::= {: RESULT = new CreateRepositoryStmt(isReadOnly, repoName, storage); :} - | KW_CREATE KW_ROLE opt_if_not_exists:ifNotExists ident:role + | KW_CREATE KW_ROLE opt_if_not_exists:ifNotExists ident:role opt_comment:comment {: - RESULT = new CreateRoleStmt(ifNotExists, role); + RESULT = new CreateRoleStmt(ifNotExists, role, comment); + :} + | KW_ALTER KW_ROLE ident:role KW_COMMENT STRING_LITERAL:comment + {: + RESULT = new AlterRoleStmt(role, comment); :} | KW_CREATE KW_FILE STRING_LITERAL:fileName opt_db:db KW_PROPERTIES LPAREN key_value_map:properties RPAREN {: @@ -3800,6 +3806,17 @@ opt_comment ::= :} ; +opt_comment_null ::= + /* empty */ + {: + RESULT = null; + :} + | KW_COMMENT STRING_LITERAL:comment + {: + RESULT = comment; + :} + ; + opt_index_type ::= {: RESULT = null; diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateRoleStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/AlterRoleStmt.java similarity index 75% copy from fe/fe-core/src/main/java/org/apache/doris/analysis/CreateRoleStmt.java copy to fe/fe-core/src/main/java/org/apache/doris/analysis/AlterRoleStmt.java index bd19325f1ef..75e86a4b6f0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateRoleStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/AlterRoleStmt.java @@ -20,46 +20,46 @@ package org.apache.doris.analysis; import org.apache.doris.catalog.Env; import org.apache.doris.common.ErrorCode; import org.apache.doris.common.ErrorReport; -import org.apache.doris.common.FeNameFormat; import org.apache.doris.common.UserException; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; -public class CreateRoleStmt extends DdlStmt { +import com.google.common.base.Strings; + +public class AlterRoleStmt extends DdlStmt { - private boolean ifNotExists; private String role; - public CreateRoleStmt(String role) { - this.role = role; - } + private String comment; - public CreateRoleStmt(boolean ifNotExists, String role) { - this.ifNotExists = ifNotExists; + public AlterRoleStmt(String role, String comment) { this.role = role; - } - - public boolean isSetIfNotExists() { - return ifNotExists; + this.comment = Strings.nullToEmpty(comment); } public String getRole() { return role; } + public String getComment() { + return comment; + } + @Override public void analyze(Analyzer analyzer) throws UserException { super.analyze(analyzer); - FeNameFormat.checkRoleName(role, false /* can not be admin */, "Can not create role"); - // check if current user has GRANT priv on GLOBAL level. if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ConnectContext.get(), PrivPredicate.GRANT)) { - ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR, "CREATE ROLE"); + ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR, "ALTER ROLE"); } } @Override public String toSql() { - return "CREATE ROLE " + role; + StringBuilder sb = new StringBuilder(); + sb.append("ALTER ROLE "); + sb.append(role); + sb.append(" COMMENT \"").append(comment).append("\""); + return sb.toString(); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/AlterUserStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/AlterUserStmt.java index 6495f73fc94..544a106b3ba 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/AlterUserStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/AlterUserStmt.java @@ -44,22 +44,27 @@ public class AlterUserStmt extends DdlStmt { private String role; private PasswordOptions passwordOptions; + private String comment; + // Only support doing one of these operation at one time. public enum OpType { SET_PASSWORD, SET_ROLE, SET_PASSWORD_POLICY, LOCK_ACCOUNT, - UNLOCK_ACCOUNT + UNLOCK_ACCOUNT, + MODIFY_COMMENT } private Set<OpType> ops = Sets.newHashSet(); - public AlterUserStmt(boolean ifExist, UserDesc userDesc, String role, PasswordOptions passwordOptions) { + public AlterUserStmt(boolean ifExist, UserDesc userDesc, String role, PasswordOptions passwordOptions, + String comment) { this.ifExist = ifExist; this.userDesc = userDesc; this.role = role; this.passwordOptions = passwordOptions; + this.comment = comment; } public boolean isIfExist() { @@ -90,6 +95,10 @@ public class AlterUserStmt extends DdlStmt { return ops.iterator().next(); } + public String getComment() { + return comment; + } + @Override public void analyze(Analyzer analyzer) throws UserException { super.analyze(analyzer); @@ -104,6 +113,10 @@ public class AlterUserStmt extends DdlStmt { ops.add(OpType.SET_ROLE); } + // may be set comment to "", so not use `Strings.isNullOrEmpty` + if (comment != null) { + ops.add(OpType.MODIFY_COMMENT); + } passwordOptions.analyze(); if (passwordOptions.getAccountUnlocked() == FailedLoginPolicy.LOCK_ACCOUNT) { throw new AnalysisException("Not support lock account now"); diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateRoleStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateRoleStmt.java index bd19325f1ef..0a60a3060c3 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateRoleStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateRoleStmt.java @@ -25,18 +25,24 @@ import org.apache.doris.common.UserException; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; +import com.google.common.base.Strings; +import org.apache.commons.lang3.StringUtils; + public class CreateRoleStmt extends DdlStmt { private boolean ifNotExists; private String role; + private String comment; + public CreateRoleStmt(String role) { this.role = role; } - public CreateRoleStmt(boolean ifNotExists, String role) { + public CreateRoleStmt(boolean ifNotExists, String role, String comment) { this.ifNotExists = ifNotExists; this.role = role; + this.comment = Strings.nullToEmpty(comment); } public boolean isSetIfNotExists() { @@ -47,6 +53,10 @@ public class CreateRoleStmt extends DdlStmt { return role; } + public String getComment() { + return comment; + } + @Override public void analyze(Analyzer analyzer) throws UserException { super.analyze(analyzer); @@ -60,6 +70,12 @@ public class CreateRoleStmt extends DdlStmt { @Override public String toSql() { - return "CREATE ROLE " + role; + StringBuilder sb = new StringBuilder(); + sb.append("CREATE ROLE "); + sb.append(role); + if (!StringUtils.isEmpty(comment)) { + sb.append(" COMMENT \"").append(comment).append("\""); + } + return sb.toString(); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateUserStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateUserStmt.java index e3d5dc5556d..d8c589bf0b7 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateUserStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateUserStmt.java @@ -27,6 +27,7 @@ import org.apache.doris.mysql.privilege.Role; import org.apache.doris.qe.ConnectContext; import com.google.common.base.Strings; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -52,6 +53,7 @@ public class CreateUserStmt extends DdlStmt { private PassVar passVar; private String role; private PasswordOptions passwordOptions; + private String comment; public CreateUserStmt() { } @@ -65,10 +67,11 @@ public class CreateUserStmt extends DdlStmt { } public CreateUserStmt(boolean ifNotExist, UserDesc userDesc, String role) { - this(ifNotExist, userDesc, role, null); + this(ifNotExist, userDesc, role, null, null); } - public CreateUserStmt(boolean ifNotExist, UserDesc userDesc, String role, PasswordOptions passwordOptions) { + public CreateUserStmt(boolean ifNotExist, UserDesc userDesc, String role, PasswordOptions passwordOptions, + String comment) { this.ifNotExist = ifNotExist; userIdent = userDesc.getUserIdent(); passVar = userDesc.getPassVar(); @@ -77,6 +80,7 @@ public class CreateUserStmt extends DdlStmt { if (this.passwordOptions == null) { this.passwordOptions = PasswordOptions.UNSET_OPTION; } + this.comment = Strings.nullToEmpty(comment); } public boolean isIfNotExist() { @@ -104,6 +108,10 @@ public class CreateUserStmt extends DdlStmt { return passwordOptions; } + public String getComment() { + return comment; + } + @Override public void analyze(Analyzer analyzer) throws UserException { super.analyze(analyzer); @@ -149,7 +157,9 @@ public class CreateUserStmt extends DdlStmt { if (passwordOptions != null) { sb.append(passwordOptions.toSql()); } - + if (!StringUtils.isEmpty(comment)) { + sb.append(" COMMENT \"").append(comment).append("\""); + } return sb.toString(); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowRolesStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowRolesStmt.java index fdeaeaea23d..52c5bd75765 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowRolesStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowRolesStmt.java @@ -34,6 +34,7 @@ public class ShowRolesStmt extends ShowStmt { ShowResultSetMetaData.Builder builder = ShowResultSetMetaData.builder(); builder.addColumn(new Column("Name", ScalarType.createVarchar(100))); + builder.addColumn(new Column("Comment", ScalarType.createVarchar(100))); builder.addColumn(new Column("Users", ScalarType.createVarchar(100))); builder.addColumn(new Column("GlobalPrivs", ScalarType.createVarchar(300))); builder.addColumn(new Column("CatalogPrivs", ScalarType.createVarchar(300))); diff --git a/fe/fe-core/src/main/java/org/apache/doris/common/proc/AuthProcDir.java b/fe/fe-core/src/main/java/org/apache/doris/common/proc/AuthProcDir.java index 6adc86ee269..95434fec723 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/common/proc/AuthProcDir.java +++ b/fe/fe-core/src/main/java/org/apache/doris/common/proc/AuthProcDir.java @@ -31,8 +31,9 @@ import com.google.common.collect.ImmutableList; */ public class AuthProcDir implements ProcDirInterface { public static final ImmutableList<String> TITLE_NAMES = new ImmutableList.Builder<String>() - .add("UserIdentity").add("Password").add("Roles").add("GlobalPrivs").add("CatalogPrivs") - .add("DatabasePrivs").add("TablePrivs").add("ColPrivs").add("ResourcePrivs").add("WorkloadGroupPrivs") + .add("UserIdentity").add("Comment").add("Password").add("Roles").add("GlobalPrivs").add("CatalogPrivs") + .add("DatabasePrivs").add("TablePrivs").add("ColPrivs").add("ResourcePrivs").add("CloudClusterPrivs") + .add("WorkloadGroupPrivs") .build(); private Auth auth; diff --git a/fe/fe-core/src/main/java/org/apache/doris/journal/JournalEntity.java b/fe/fe-core/src/main/java/org/apache/doris/journal/JournalEntity.java index 8f0d2b54580..1c3f7444dd9 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/journal/JournalEntity.java +++ b/fe/fe-core/src/main/java/org/apache/doris/journal/JournalEntity.java @@ -386,6 +386,7 @@ public class JournalEntity implements Writable { case OperationType.OP_REVOKE_PRIV: case OperationType.OP_SET_PASSWORD: case OperationType.OP_CREATE_ROLE: + case OperationType.OP_ALTER_ROLE: case OperationType.OP_DROP_ROLE: { data = PrivInfo.read(in); isRead = true; diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java index eab88b43189..182863e7ac9 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java @@ -17,6 +17,7 @@ package org.apache.doris.mysql.privilege; +import org.apache.doris.analysis.AlterRoleStmt; import org.apache.doris.analysis.AlterUserStmt; import org.apache.doris.analysis.AlterUserStmt.OpType; import org.apache.doris.analysis.CreateRoleStmt; @@ -94,7 +95,7 @@ public class Auth implements Writable { public static final String UNKNOWN_USER = "unknown"; public static final String DEFAULT_CATALOG = InternalCatalog.INTERNAL_CATALOG_NAME; - //There is no concurrency control logic inside roleManager,userManager,userRoleManage and rpropertyMgr, + // There is no concurrency control logic inside roleManager,userManager,userRoleManage and rpropertyMgr, // and it is completely managed by Auth. // Therefore, their methods cannot be directly called outside, and should be called indirectly through Auth. private RoleManager roleManager = new RoleManager(); @@ -423,20 +424,21 @@ public class Auth implements Writable { // create user public void createUser(CreateUserStmt stmt) throws DdlException { createUserInternal(stmt.getUserIdent(), stmt.getQualifiedRole(), - stmt.getPassword(), stmt.isIfNotExist(), stmt.getPasswordOptions(), false); + stmt.getPassword(), stmt.isIfNotExist(), stmt.getPasswordOptions(), stmt.getComment(), false); } public void replayCreateUser(PrivInfo privInfo) { try { createUserInternal(privInfo.getUserIdent(), privInfo.getRole(), privInfo.getPasswd(), false, - privInfo.getPasswordOptions(), true); + privInfo.getPasswordOptions(), privInfo.getComment(), true); } catch (DdlException e) { LOG.error("should not happen", e); } } private void createUserInternal(UserIdentity userIdent, String roleName, byte[] password, - boolean ignoreIfExists, PasswordOptions passwordOptions, boolean isReplay) throws DdlException { + boolean ignoreIfExists, PasswordOptions passwordOptions, String comment, boolean isReplay) + throws DdlException { writeLock(); try { // check if role exist @@ -460,7 +462,7 @@ public class Auth implements Writable { // create user try { // we should not throw AnalysisException at here,so transfer it - userManager.createUser(userIdent, password, null, false); + userManager.createUser(userIdent, password, null, false, comment); } catch (PatternMatcherException e) { throw new DdlException("create user failed,", e); } @@ -468,7 +470,7 @@ public class Auth implements Writable { // save password to password history passwdPolicyManager.updatePassword(userIdent, password); } - //4.create defaultRole + // 4.create defaultRole Role defaultRole = roleManager.createDefaultRole(userIdent); // 5.create user role userRoleManager.addUserRole(userIdent, defaultRole.getRoleName()); @@ -482,7 +484,7 @@ public class Auth implements Writable { passwdPolicyManager.updatePolicy(userIdent, password, passwordOptions); if (!isReplay) { - PrivInfo privInfo = new PrivInfo(userIdent, null, password, roleName, passwordOptions); + PrivInfo privInfo = new PrivInfo(userIdent, null, password, roleName, passwordOptions, comment); Env.getCurrentEnv().getEditLog().logCreateUser(privInfo); } LOG.info("finished to create user: {}, is replay: {}", userIdent, isReplay); @@ -517,7 +519,7 @@ public class Auth implements Writable { // drop default role roleManager.removeDefaultRole(userIdent); - //drop user role + // drop user role userRoleManager.dropUser(userIdent); passwdPolicyManager.dropUser(userIdent); userManager.removeUser(userIdent); @@ -576,7 +578,7 @@ public class Auth implements Writable { } // grant for TablePattern - //if no role,role is default role of userIdent + // if no role,role is default role of userIdent private void grantInternal(UserIdentity userIdent, String role, TablePattern tblPattern, PrivBitSet privs, Map<ColPrivilegeKey, Set<String>> colPrivileges, boolean errOnNonExist, boolean isReplay) throws DdlException { @@ -650,7 +652,7 @@ public class Auth implements Writable { if (userManager.getUserByUserIdentity(userIdent) == null) { throw new DdlException("user: " + userIdent + " does not exist"); } - //roles must exist + // roles must exist for (String roleName : roles) { if (roleManager.getRole(roleName) == null) { throw new DdlException("role:" + roleName + " does not exist"); @@ -794,7 +796,7 @@ public class Auth implements Writable { if (userManager.getUserByUserIdentity(userIdent) == null) { throw new DdlException("user: " + userIdent + " does not exist"); } - //roles must exist + // roles must exist for (String roleName : roles) { if (roleManager.getRole(roleName) == null) { throw new DdlException("role:" + roleName + " does not exist"); @@ -873,19 +875,44 @@ public class Auth implements Writable { // create role public void createRole(CreateRoleStmt stmt) throws DdlException { - createRoleInternal(stmt.getRole(), stmt.isSetIfNotExists(), false); + createRoleInternal(stmt.getRole(), stmt.isSetIfNotExists(), stmt.getComment(), false); + } + + public void alterRole(AlterRoleStmt stmt) throws DdlException { + alterRoleInternal(stmt.getRole(), stmt.getComment(), false); } public void replayCreateRole(PrivInfo info) { try { - createRoleInternal(info.getRole(), false, true); + createRoleInternal(info.getRole(), false, info.getComment(), true); } catch (DdlException e) { LOG.error("should not happened", e); } } - private void createRoleInternal(String role, boolean ignoreIfExists, boolean isReplay) throws DdlException { - Role emptyPrivsRole = new Role(role); + public void replayAlterRole(PrivInfo info) { + try { + alterRoleInternal(info.getRole(), info.getComment(), true); + } catch (DdlException e) { + LOG.error("should not happened", e); + } + } + + private void alterRoleInternal(String roleName, String comment, boolean isReplay) throws DdlException { + Role role = roleManager.getRole(roleName); + if (role == null) { + throw new DdlException("Role " + roleName + " not exist"); + } + role.setComment(comment); + if (!isReplay) { + PrivInfo info = new PrivInfo(roleName, comment); + Env.getCurrentEnv().getEditLog().logAlterRole(info); + } + } + + private void createRoleInternal(String role, boolean ignoreIfExists, String comment, boolean isReplay) + throws DdlException { + Role emptyPrivsRole = new Role(role, comment); writeLock(); try { if (ignoreIfExists && roleManager.getRole(role) != null) { @@ -896,7 +923,7 @@ public class Auth implements Writable { roleManager.addOrMergeRole(emptyPrivsRole, true /* err on exist */); if (!isReplay) { - PrivInfo info = new PrivInfo(null, null, null, role, null); + PrivInfo info = new PrivInfo(role, comment); Env.getCurrentEnv().getEditLog().logCreateRole(info); } } finally { @@ -1132,6 +1159,8 @@ public class Auth implements Writable { // ================= UserIdentity ======================= userAuthInfo.add(userIdent.toString()); if (isLdapAuthEnabled() && ldapManager.doesUserExist(userIdent.getQualifiedUser())) { + // ============== Comment ============== + userAuthInfo.add(FeConstants.null_string); LdapUserInfo ldapUserInfo = ldapManager.getUserInfo(userIdent.getQualifiedUser()); // ============== Password ============== userAuthInfo.add(ldapUserInfo.isSetPasswd() ? "Yes" : "No"); @@ -1143,7 +1172,10 @@ public class Auth implements Writable { if (user == null) { userAuthInfo.add(FeConstants.null_string); userAuthInfo.add(FeConstants.null_string); + userAuthInfo.add(FeConstants.null_string); } else { + // ============== Comment ============== + userAuthInfo.add(user.getComment()); // ============== Password ============== userAuthInfo.add(user.hasPassword() ? "Yes" : "No"); // ============== Roles ============== @@ -1321,11 +1353,11 @@ public class Auth implements Writable { UserIdentity rootUser = new UserIdentity(ROOT_USER, "%"); rootUser.setIsAnalyzed(); createUserInternal(rootUser, Role.OPERATOR_ROLE, new byte[0], - false /* ignore if exists */, PasswordOptions.UNSET_OPTION, true /* is replay */); + false /* ignore if exists */, PasswordOptions.UNSET_OPTION, "ROOT", true /* is replay */); UserIdentity adminUser = new UserIdentity(ADMIN_USER, "%"); adminUser.setIsAnalyzed(); createUserInternal(adminUser, Role.ADMIN_ROLE, new byte[0], - false /* ignore if exists */, PasswordOptions.UNSET_OPTION, true /* is replay */); + false /* ignore if exists */, PasswordOptions.UNSET_OPTION, "ADMIN", true /* is replay */); } catch (DdlException e) { LOG.error("should not happened", e); } @@ -1524,20 +1556,20 @@ public class Auth implements Writable { public void alterUser(AlterUserStmt stmt) throws DdlException { alterUserInternal(stmt.isIfExist(), stmt.getOpType(), stmt.getUserIdent(), stmt.getPassword(), stmt.getRole(), - stmt.getPasswordOptions(), false); + stmt.getPasswordOptions(), stmt.getComment(), false); } public void replayAlterUser(AlterUserOperationLog log) { try { alterUserInternal(true, log.getOp(), log.getUserIdent(), log.getPassword(), log.getRole(), - log.getPasswordOptions(), true); + log.getPasswordOptions(), log.getComment(), true); } catch (DdlException e) { LOG.error("should not happen", e); } } private void alterUserInternal(boolean ifExists, OpType opType, UserIdentity userIdent, byte[] password, - String role, PasswordOptions passwordOptions, boolean isReplay) throws DdlException { + String role, PasswordOptions passwordOptions, String comment, boolean isReplay) throws DdlException { writeLock(); try { if (!doesUserExist(userIdent)) { @@ -1559,6 +1591,9 @@ public class Auth implements Writable { case UNLOCK_ACCOUNT: passwdPolicyManager.unlockUser(userIdent); break; + case MODIFY_COMMENT: + modifyComment(userIdent, comment); + break; default: throw new DdlException("Unknown alter user operation type: " + opType.name()); } @@ -1566,7 +1601,7 @@ public class Auth implements Writable { // For SET_PASSWORD: // the edit log is wrote in "setPasswordInternal" AlterUserOperationLog log = new AlterUserOperationLog(opType, userIdent, password, role, - passwordOptions); + passwordOptions, comment); Env.getCurrentEnv().getEditLog().logAlterUser(log); } } finally { @@ -1574,7 +1609,7 @@ public class Auth implements Writable { } } - //tmp for current user can only has one role + // tmp for current user can only has one role private void setRoleToUser(UserIdentity userIdent, String role) throws DdlException { // 1. check if role exist Role newRole = roleManager.getRole(role); @@ -1586,6 +1621,14 @@ public class Auth implements Writable { userRoleManager.addUserRole(userIdent, roleManager.getUserDefaultRoleName(userIdent)); } + private void modifyComment(UserIdentity userIdent, String comment) throws DdlException { + User user = userManager.getUserByUserIdentity(userIdent); + if (user == null) { + throw new DdlException("UserIdentity " + userIdent + " does not exist"); + } + user.setComment(comment); + } + public Set<String> getAllUser() { return userManager.getNameToUsers().keySet(); } @@ -1663,15 +1706,15 @@ public class Auth implements Writable { private void upgradeToVersion116(UserPrivTable userPrivTable, CatalogPrivTable catalogPrivTable, DbPrivTable dbPrivTable, TablePrivTable tablePrivTable, ResourcePrivTable resourcePrivTable) throws AnalysisException, DdlException, PatternMatcherException { - //OPERATOR and Admin role not save users,if not inituser,root will do not have admin role + // OPERATOR and Admin role not save users,if not inituser,root will do not have admin role initUser(); for (Entry<String, UserProperty> entry : propertyMgr.propertyMap.entrySet()) { for (Entry<String, byte[]> userEntry : entry.getValue().getWhiteList().getPasswordMap().entrySet()) { - //create user + // create user User user = userManager .createUser(UserIdentity.createAnalyzedUserIdentWithDomain(entry.getKey(), userEntry.getKey()), - userEntry.getValue(), null, false); - //create default role + userEntry.getValue(), null, false, ""); + // create default role Role defaultRole = roleManager.createDefaultRole(user.getUserIdentity()); userRoleManager .addUserRole(user.getUserIdentity(), defaultRole.getRoleName()); @@ -1680,18 +1723,18 @@ public class Auth implements Writable { List<PrivEntry> userPrivTableEntries = userPrivTable.getEntries(); for (PrivEntry privEntry : userPrivTableEntries) { GlobalPrivEntry globalPrivEntry = (GlobalPrivEntry) privEntry; - //may repeat with created user from propertyMgr,but no influence + // may repeat with created user from propertyMgr,but no influence User user = userManager .createUser(globalPrivEntry.userIdentity, globalPrivEntry.password, globalPrivEntry.domainUserIdent, - globalPrivEntry.isSetByDomainResolver); - //create default role + globalPrivEntry.isSetByDomainResolver, ""); + // create default role Role defaultRole = roleManager.createDefaultRole(user.getUserIdentity()); userRoleManager .addUserRole(user.getUserIdentity(), defaultRole.getRoleName()); if (globalPrivEntry.privSet.isEmpty()) { continue; } - //grant global auth + // grant global auth if (globalPrivEntry.privSet.containsResourcePriv()) { roleManager.addOrMergeRole(new Role(roleManager.getUserDefaultRoleName(user.getUserIdentity()), ResourcePattern.ALL, PrivBitSet.of(Privilege.USAGE_PRIV)), false); diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java index 59a2316dadc..f59cfd699f0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Role.java @@ -78,6 +78,9 @@ public class Role implements Writable, GsonPostProcessable { @SerializedName(value = "roleName") private String roleName; + + @SerializedName(value = "comment") + private String comment; // Will be persisted @SerializedName(value = "tblPatternToPrivs") private Map<TablePattern, PrivBitSet> tblPatternToPrivs = Maps.newConcurrentMap(); @@ -110,7 +113,12 @@ public class Role implements Writable, GsonPostProcessable { } public Role(String roleName) { + this(roleName, ""); + } + + public Role(String roleName, String comment) { this.roleName = roleName; + this.comment = comment; } public Role(String roleName, TablePattern tablePattern, PrivBitSet privs) throws DdlException { @@ -484,6 +492,14 @@ public class Role implements Writable, GsonPostProcessable { return workloadGroupPrivTable; } + public String getComment() { + return comment; + } + + public void setComment(String comment) { + this.comment = comment; + } + public boolean checkCanEnterCluster(String clusterName) { if (checkGlobalPriv(PrivPredicate.ALL)) { return true; diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java index 7f48ed87cfa..e5654b6598c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/RoleManager.java @@ -150,6 +150,7 @@ public class RoleManager implements Writable, GsonPostProcessable { } List<String> info = Lists.newArrayList(); info.add(role.getRoleName()); + info.add(role.getComment()); info.add(Joiner.on(", ").join(Env.getCurrentEnv().getAuth().getRoleUsers(role.getRoleName()))); Map<PrivLevel, String> infoMap = Stream.concat( diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/User.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/User.java index afac0767e07..c48d0605c5f 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/User.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/User.java @@ -47,12 +47,14 @@ public class User implements Comparable<User>, Writable, GsonPostProcessable { protected boolean isAnyHost = false; @SerializedName(value = "password") private Password password; + @SerializedName(value = "comment") + private String comment; public User() { } public User(UserIdentity userIdent, byte[] pwd, boolean setByResolver, UserIdentity domainUserIdent, - PatternMatcher hostPattern) { + PatternMatcher hostPattern, String comment) { this.isAnyHost = userIdent.getHost().equals(UserManager.ANY_HOST); this.userIdentity = userIdent; this.password = new Password(pwd); @@ -62,6 +64,7 @@ public class User implements Comparable<User>, Writable, GsonPostProcessable { Preconditions.checkNotNull(domainUserIdent); this.domainUserIdentity = domainUserIdent; } + this.comment = comment; } @@ -126,6 +129,14 @@ public class User implements Comparable<User>, Writable, GsonPostProcessable { return password != null && password.getPassword() != null && password.getPassword().length != 0; } + public String getComment() { + return comment; + } + + public void setComment(String comment) { + this.comment = comment; + } + @Override public int compareTo(@NotNull User o) { return -userIdentity.getHost().compareTo(o.userIdentity.getHost()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java index e1c0faa1677..6d677779639 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java +++ b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java @@ -54,7 +54,7 @@ public class UserManager implements Writable, GsonPostProcessable { public static final String ANY_HOST = "%"; private static final Logger LOG = LogManager.getLogger(UserManager.class); // Concurrency control is delegated by Auth, so not concurrentMap - //One name may have multiple User,because host can be different + // One name may have multiple User,because host can be different @SerializedName(value = "nameToUsers") private Map<String, List<User>> nameToUsers = Maps.newHashMap(); @@ -181,7 +181,8 @@ public class UserManager implements Writable, GsonPostProcessable { } - public User createUser(UserIdentity userIdent, byte[] pwd, UserIdentity domainUserIdent, boolean setByResolver) + public User createUser(UserIdentity userIdent, byte[] pwd, UserIdentity domainUserIdent, boolean setByResolver, + String comment) throws PatternMatcherException { if (userIdentityExist(userIdent, true)) { User userByUserIdentity = getUserByUserIdentity(userIdent); @@ -192,13 +193,14 @@ public class UserManager implements Writable, GsonPostProcessable { return userByUserIdentity; } userByUserIdentity.setPassword(pwd); + userByUserIdentity.setComment(comment); userByUserIdentity.setSetByDomainResolver(setByResolver); return userByUserIdentity; } PatternMatcher hostPattern = PatternMatcher .createMysqlPattern(userIdent.getHost(), CaseSensibility.HOST.getCaseSensibility()); - User user = new User(userIdent, pwd, setByResolver, domainUserIdent, hostPattern); + User user = new User(userIdent, pwd, setByResolver, domainUserIdent, hostPattern, comment); List<User> nameToLists = nameToUsers.get(userIdent.getQualifiedUser()); if (CollectionUtils.isEmpty(nameToLists)) { nameToLists = Lists.newArrayList(user); @@ -286,7 +288,7 @@ public class UserManager implements Writable, GsonPostProcessable { byte[] password = domainUser.getPassword().getPassword(); Preconditions.checkNotNull(password, entry.getKey()); try { - createUser(userIdent, password, domainUser.getUserIdentity(), true); + createUser(userIdent, password, domainUser.getUserIdentity(), true, ""); } catch (PatternMatcherException e) { LOG.info("failed to create user for user ident: {}, {}", userIdent, e.getMessage()); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/persist/AlterUserOperationLog.java b/fe/fe-core/src/main/java/org/apache/doris/persist/AlterUserOperationLog.java index 5d4ea006885..472cda7874e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/persist/AlterUserOperationLog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/persist/AlterUserOperationLog.java @@ -43,13 +43,17 @@ public class AlterUserOperationLog implements Writable { @SerializedName(value = "op") private AlterUserStmt.OpType op; + @SerializedName(value = "comment") + private String comment; + public AlterUserOperationLog(OpType opType, UserIdentity userIdent, byte[] password, - String role, PasswordOptions passwordOptions) { + String role, PasswordOptions passwordOptions, String comment) { this.op = opType; this.userIdent = userIdent; this.password = password; this.role = role; this.passwordOptions = passwordOptions; + this.comment = comment; } public OpType getOp() { @@ -72,6 +76,10 @@ public class AlterUserOperationLog implements Writable { return passwordOptions; } + public String getComment() { + return comment; + } + @Override public void write(DataOutput out) throws IOException { Text.writeString(out, GsonUtils.GSON.toJson(this)); diff --git a/fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java b/fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java index d21012dd658..ffae17c6b1c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java @@ -479,6 +479,11 @@ public class EditLog { env.getAuth().replayCreateRole(privInfo); break; } + case OperationType.OP_ALTER_ROLE: { + PrivInfo privInfo = (PrivInfo) journal.getData(); + env.getAuth().replayAlterRole(privInfo); + break; + } case OperationType.OP_DROP_ROLE: { PrivInfo privInfo = (PrivInfo) journal.getData(); env.getAuth().replayDropRole(privInfo); @@ -1504,6 +1509,10 @@ public class EditLog { logEdit(OperationType.OP_CREATE_ROLE, info); } + public void logAlterRole(PrivInfo info) { + logEdit(OperationType.OP_ALTER_ROLE, info); + } + public void logDropRole(PrivInfo info) { logEdit(OperationType.OP_DROP_ROLE, info); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/persist/OperationType.java b/fe/fe-core/src/main/java/org/apache/doris/persist/OperationType.java index c88dd029502..f5c71e58b72 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/persist/OperationType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/persist/OperationType.java @@ -382,6 +382,8 @@ public class OperationType { public static final short OP_DROP_PLSQL_PACKAGE = 474; + public static final short OP_ALTER_ROLE = 475; + /** * Get opcode name by op code. **/ diff --git a/fe/fe-core/src/main/java/org/apache/doris/persist/PrivInfo.java b/fe/fe-core/src/main/java/org/apache/doris/persist/PrivInfo.java index 6cb11f8cb2c..ca5a35de0f5 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/persist/PrivInfo.java +++ b/fe/fe-core/src/main/java/org/apache/doris/persist/PrivInfo.java @@ -56,6 +56,8 @@ public class PrivInfo implements Writable, GsonPostProcessable { private byte[] passwd; @SerializedName(value = "role") private String role; + @SerializedName(value = "comment") + private String comment; @SerializedName(value = "colPrivileges") private Map<ColPrivilegeKey, Set<String>> colPrivileges; @SerializedName(value = "passwordOptions") @@ -71,6 +73,11 @@ public class PrivInfo implements Writable, GsonPostProcessable { // For create user/set password/create role/drop role public PrivInfo(UserIdentity userIdent, PrivBitSet privs, byte[] passwd, String role, PasswordOptions passwordOptions) { + this(userIdent, privs, passwd, role, passwordOptions, null); + } + + public PrivInfo(UserIdentity userIdent, PrivBitSet privs, byte[] passwd, String role, + PasswordOptions passwordOptions, String comment) { this.userIdent = userIdent; this.tblPattern = null; this.resourcePattern = null; @@ -78,6 +85,12 @@ public class PrivInfo implements Writable, GsonPostProcessable { this.passwd = passwd; this.role = role; this.passwordOptions = passwordOptions; + this.comment = comment; + } + + public PrivInfo(String role, String comment) { + this.role = role; + this.comment = comment; } // For grant/revoke @@ -150,6 +163,10 @@ public class PrivInfo implements Writable, GsonPostProcessable { return role; } + public String getComment() { + return comment; + } + public PasswordOptions getPasswordOptions() { return passwordOptions == null ? PasswordOptions.UNSET_OPTION : passwordOptions; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/DdlExecutor.java b/fe/fe-core/src/main/java/org/apache/doris/qe/DdlExecutor.java index ef6a47de691..35632c5e430 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/qe/DdlExecutor.java +++ b/fe/fe-core/src/main/java/org/apache/doris/qe/DdlExecutor.java @@ -41,6 +41,7 @@ import org.apache.doris.analysis.AlterJobStatusStmt; import org.apache.doris.analysis.AlterPolicyStmt; import org.apache.doris.analysis.AlterRepositoryStmt; import org.apache.doris.analysis.AlterResourceStmt; +import org.apache.doris.analysis.AlterRoleStmt; import org.apache.doris.analysis.AlterRoutineLoadStmt; import org.apache.doris.analysis.AlterSqlBlockRuleStmt; import org.apache.doris.analysis.AlterSystemStmt; @@ -228,6 +229,8 @@ public class DdlExecutor { env.getAuth().revoke(stmt); } else if (ddlStmt instanceof CreateRoleStmt) { env.getAuth().createRole((CreateRoleStmt) ddlStmt); + } else if (ddlStmt instanceof AlterRoleStmt) { + env.getAuth().alterRole((AlterRoleStmt) ddlStmt); } else if (ddlStmt instanceof DropRoleStmt) { env.getAuth().dropRole((DropRoleStmt) ddlStmt); } else if (ddlStmt instanceof SetUserPropertyStmt) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java b/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java index 0bbc84b5b1a..bf52ea76225 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java @@ -888,7 +888,7 @@ public class AuthTest { } // 24.1 create role again with IF NOT EXISTS - roleStmt = new CreateRoleStmt(true, "role1"); + roleStmt = new CreateRoleStmt(true, "role1", ""); try { roleStmt.analyze(analyzer); } catch (UserException e1) { @@ -904,7 +904,7 @@ public class AuthTest { } // 24.2 create role again without IF NOT EXISTS - roleStmt = new CreateRoleStmt(false, "role1"); + roleStmt = new CreateRoleStmt(false, "role1", ""); try { roleStmt.analyze(analyzer); } catch (UserException e1) { diff --git a/regression-test/suites/account_p0/test_account.groovy b/regression-test/suites/account_p0/test_account.groovy index 1eb07e259cf..accea7bb8c5 100644 --- a/regression-test/suites/account_p0/test_account.groovy +++ b/regression-test/suites/account_p0/test_account.groovy @@ -14,12 +14,27 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -suite("test_account") { - // todo: test account management, such as role, user, grant, revoke ... - sql "show roles" +import org.junit.Assert; + +suite("test_account") { + // test comment + def user = "test_account_comment_user"; + sql """drop user if exists ${user}""" + // create user with comment + sql """create user ${user} comment 'test_account_comment_user_comment_create'""" + def user_create = sql "show grants for ${user}" + logger.info("user_create: " + user_create.toString()) + assertTrue(user_create.toString().contains("test_account_comment_user_comment_create")) + // alter user comment + sql """alter user ${user} comment 'test_account_comment_user_comment_alter'""" + def user_alter = sql "show grants for ${user}" + logger.info("user_alter: " + user_alter.toString()) + assertTrue(user_alter.toString().contains("test_account_comment_user_comment_alter")) + // drop user + sql """drop user if exists ${user}""" try { - sql "show grants for 'non_existent_user_1'" + sql "show grants for ${user}" fail() } catch (Exception e) { log.info(e.getMessage()) diff --git a/regression-test/suites/account_p0/test_revoke_role.groovy b/regression-test/suites/account_p0/test_revoke_role.groovy index 3e244232973..5aa9f77206a 100644 --- a/regression-test/suites/account_p0/test_revoke_role.groovy +++ b/regression-test/suites/account_p0/test_revoke_role.groovy @@ -35,12 +35,12 @@ suite("test_revoke_role", "account") { def result = sql """ SHOW GRANTS FOR ${user} """ assertEquals(result.size(), 1) - assertTrue(result[0][5].contains("internal.${dbName}: Select_priv")) + assertTrue(result[0][6].contains("internal.${dbName}: Select_priv")) sql """REVOKE '${role}' from ${user}""" result = sql """ SHOW GRANTS FOR ${user} """ assertEquals(result.size(), 1) - assertFalse(result[0][5].contains("internal.${dbName}: Select_priv")) + assertFalse(result[0][6].contains("internal.${dbName}: Select_priv")) sql """DROP USER ${user}""" sql """DROP ROLE ${role}""" diff --git a/regression-test/suites/account_p0/test_role.groovy b/regression-test/suites/account_p0/test_role.groovy index e3b227ea004..6d88238ebf5 100644 --- a/regression-test/suites/account_p0/test_role.groovy +++ b/regression-test/suites/account_p0/test_role.groovy @@ -15,6 +15,8 @@ // specific language governing permissions and limitations // under the License. +import org.junit.Assert; + suite("test_role", "account") { def role= 'account_role_test' def user = 'acount_role_user_test' @@ -44,5 +46,22 @@ suite("test_role", "account") { sql """DROP USER ${user}""" sql """DROP ROLE ${role}""" sql """DROP DATABASE ${dbName}""" + + // test comment + // create role with comment + sql """CREATE ROLE ${role} comment 'account_p0_account_role_test_comment_create'""" + def roles_create = sql """show roles""" + logger.info("roles_create: " + roles_create.toString()) + assertTrue(roles_create.toString().contains("account_p0_account_role_test_comment_create")) + // alter role with comment + sql """ALTER ROLE ${role} comment 'account_p0_account_role_test_comment_alter'""" + def roles_alter = sql """show roles""" + logger.info("roles_alter: " + roles_alter.toString()) + assertTrue(roles_alter.toString().contains("account_p0_account_role_test_comment_alter")) + // drop role + sql """DROP ROLE ${role}""" + def roles_drop = sql """show roles""" + logger.info("roles_drop: " + roles_drop.toString()) + assertFalse(roles_drop.toString().contains("account_p0_account_role_test_comment_alter")) } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org