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 2022/05/19 02:48:05 UTC
[GitHub] [iotdb] SpriCoder commented on a diff in pull request #5943: [IOTDB-3083] DataNode AuthorInfo cache
SpriCoder commented on code in PR #5943:
URL: https://github.com/apache/iotdb/pull/5943#discussion_r876515377
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/Manager.java:
##########
@@ -183,11 +184,19 @@ public interface Manager {
*/
DataSet queryPermission(ConfigRequest configRequest);
- /** login */
- TSStatus login(String username, String password);
+ /**
+ * login
+ *
+ * @return PermissionCacheInfo
+ */
+ TPermissionInfoResp login(String username, String password);
- /** Check User Privileges */
- TSStatus checkUserPrivileges(String username, List<String> paths, int permission);
+ /**
+ * Check User Privileges
+ *
+ * @return PermissionCacheInfo
+ */
Review Comment:
If there are no description of return parameter, I think line 196-197 can be removed.
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/Manager.java:
##########
@@ -183,11 +184,19 @@ public interface Manager {
*/
DataSet queryPermission(ConfigRequest configRequest);
- /** login */
- TSStatus login(String username, String password);
+ /**
+ * login
+ *
+ * @return PermissionCacheInfo
+ */
Review Comment:
If there are no description of return parameter, I think line 189-190 can be removed.
##########
confignode/src/main/java/org/apache/iotdb/confignode/manager/PermissionManager.java:
##########
@@ -44,7 +57,21 @@ public PermissionManager(Manager configManager, AuthorInfo authorInfo) {
* @return TSStatus
*/
public TSStatus operatePermission(AuthorReq authorReq) {
- return getConsensusManager().write(authorReq).getStatus();
+ TSStatus status = getConsensusManager().write(authorReq).getStatus();
+ if (status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ try {
+ // If the permissions change, clear the cache content affected by the operation
+ if (authorReq.getAuthorType() != ConfigRequestType.CreateUser
+ || authorReq.getAuthorType() != ConfigRequestType.CreateRole) {
+ authorInfo.invalidateCache(env, authorReq.getUserName(), authorReq.getRoleName());
+ }
+ } catch (IOException | TException e) {
+ logger.error(
+ "Failed to initialize cache,the initialization operation is {}",
+ authorReq.getAuthorType());
Review Comment:
maybe print the message in `e` can help us to find the problem？
##########
server/src/main/java/org/apache/iotdb/db/auth/AuthorizerManager.java:
##########
@@ -42,6 +57,19 @@ public class AuthorizerManager implements IAuthorizer {
private IAuthorizer iAuthorizer;
private ReentrantReadWriteLock snapshotLock;
+ private TPermissionInfoResp tPermissionInfoResp;
+
+ private LoadingCache<String, User> userCache =
+ Caffeine.newBuilder()
+ .maximumSize(100)
+ .expireAfterAccess(30, TimeUnit.MINUTES)
+ .build(this::cacheUser);
+
+ private LoadingCache<String, Role> roleCache =
+ Caffeine.newBuilder()
+ .maximumSize(100)
+ .expireAfterAccess(30, TimeUnit.MINUTES)
+ .build(this::cacheRole);
Review Comment:
there are some magic numbers, maybe some of these number can be config param? What's your opinion?
##########
server/src/main/java/org/apache/iotdb/db/auth/AuthorizerManager.java:
##########
@@ -342,4 +370,179 @@ public void processLoadSnapshot(File snapshotDir) throws TException, IOException
snapshotLock.writeLock().unlock();
}
}
+
+ public TSStatus checkPath(String username, List<String> allPath, int permission)
+ throws AuthException {
+ snapshotLock.writeLock().lock();
+ try {
+ User user = userCache.getIfPresent(username);
+ boolean status = true;
+ if (user != null) {
+ for (String path : allPath) {
+ if (!user.checkPrivilege(path, permission)) {
+ for (String roleName : user.getRoleList()) {
+ Role role = roleCache.getIfPresent(roleName);
+ // It is detected that the role of the user does not exist in the cache, indicating
+ // that the permission information of the role has changed.
+ // The user cache needs to be initialized
+ if (role == null) {
+ invalidateCache(username, "");
+ status = false;
+ break;
+ }
+ if (!role.checkPrivilege(path, permission)) {
+ status = false;
+ }
Review Comment:
why break when role is null, but not break when role checkPrivilege failed?
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/AuthorInfo.java:
##########
@@ -362,4 +384,70 @@ public void clear() throws AuthException {
}
authorizer.reset();
}
+
+ /**
+ * When the permission information of a user or role is changed, the permission cache information
+ * of all datanode is initialized, and only the affected cache is initialized.
+ *
+ * @param env
+ * @param username
+ * @param roleName
+ * @throws IOException
+ * @throws TException
+ */
+ public void invalidateCache(ConfigNodeProcedureEnv env, String username, String roleName)
+ throws IOException, TException {
+ List<TDataNodeInfo> allDataNodes =
+ env.getConfigManager().getNodeManager().getOnlineDataNodes(-1);
+ TInvalidatePermissionCacheReq req = new TInvalidatePermissionCacheReq();
+ req.setUsername(username);
+ req.setRoleName(roleName);
+ for (TDataNodeInfo dataNodeInfo : allDataNodes) {
+ env.getDataNodeClient(dataNodeInfo.getLocation()).invalidatePermissionCache(req);
+ }
+ }
+
+ /**
+ * Save the user's permission information,Bring back the datanode for caching
+ *
+ * @param username The username of the user that needs to be cached
+ * @return TPermissionInfoResp
+ * @throws AuthException
Review Comment:
when there are no description of parameters, these lines can be removed.
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/AuthorInfo.java:
##########
@@ -362,4 +384,70 @@ public void clear() throws AuthException {
}
authorizer.reset();
}
+
+ /**
+ * When the permission information of a user or role is changed, the permission cache information
+ * of all datanode is initialized, and only the affected cache is initialized.
+ *
+ * @param env
+ * @param username
+ * @param roleName
+ * @throws IOException
+ * @throws TException
+ */
+ public void invalidateCache(ConfigNodeProcedureEnv env, String username, String roleName)
+ throws IOException, TException {
+ List<TDataNodeInfo> allDataNodes =
+ env.getConfigManager().getNodeManager().getOnlineDataNodes(-1);
+ TInvalidatePermissionCacheReq req = new TInvalidatePermissionCacheReq();
+ req.setUsername(username);
+ req.setRoleName(roleName);
+ for (TDataNodeInfo dataNodeInfo : allDataNodes) {
+ env.getDataNodeClient(dataNodeInfo.getLocation()).invalidatePermissionCache(req);
+ }
+ }
+
+ /**
+ * Save the user's permission information,Bring back the datanode for caching
+ *
+ * @param username The username of the user that needs to be cached
+ * @return TPermissionInfoResp
+ * @throws AuthException
+ */
+ public TPermissionInfoResp getUserPermissionInfo(String username) throws AuthException {
+ TPermissionInfoResp result = new TPermissionInfoResp();
+ TUserResp tUserResp = new TUserResp();
+ TRoleResp tRoleResp = new TRoleResp();
+ Map<String, TRoleResp> tRoleRespMap = new HashMap();
+ List<String> userPrivilegeList = new ArrayList<>();
+ List<String> rolePrivilegeList = new ArrayList<>();
+
+ // User permission information
+ User user = authorizer.getUser(username);
+ for (PathPrivilege pathPrivilege : user.getPrivilegeList()) {
+ userPrivilegeList.add(pathPrivilege.getPath());
+ String privilegeIdList = pathPrivilege.getPrivileges().toString();
+ userPrivilegeList.add(privilegeIdList.substring(1, privilegeIdList.length() - 1));
Review Comment:
Please check if `privilegeIdList` can be null or shorter than 1.
##########
confignode/src/main/java/org/apache/iotdb/confignode/persistence/AuthorInfo.java:
##########
@@ -362,4 +384,70 @@ public void clear() throws AuthException {
}
authorizer.reset();
}
+
+ /**
+ * When the permission information of a user or role is changed, the permission cache information
+ * of all datanode is initialized, and only the affected cache is initialized.
+ *
+ * @param env
+ * @param username
+ * @param roleName
+ * @throws IOException
+ * @throws TException
Review Comment:
when there are no description of parameters, these lines can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org