[ISSUE #5]Add permission control when loginRequired is true. #6

zhangjidi2016 · 2021-08-17T09:27:03Z

What is the purpose of the change

When the login user calls all interfaces, the permission of each interface is verified by AOP, by checking whether the role to which the user belongs has the access permission of the interface. All interface urls that a common user role has access permissions are saved in a file. The file is hot updated to facilitate adding or deleting permissions.

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

StyleTang

@zhangjidi2016 LGTM
Coverage decreased (-0.3%) to 79.213% Could you add more UT for this PR?

StyleTang · 2021-08-17T14:55:11Z

src/main/java/org/apache/rocketmq/dashboard/permisssion/PermissionAspect.java

+            }
+            int type = userInfo.getUser().getType();
+            // 1:admin    0:ordinary
+            String loginUserRole = type == 1 ? ADMIN : ORDINARY;


Can we add a UserRoleEnum, it seems so many place use it.

StyleTang · 2021-08-17T15:07:59Z

src/main/java/org/apache/rocketmq/dashboard/permisssion/PermissionAspect.java

+            if (loginUserRole == ADMIN) {
+                return joinPoint.proceed();
+            }
+            Map<String, List<String>> map = permissionService.queryRolePerms();


The permission check logic should not be here. It should belongs to PermissionService.
suggest to use

permissionService.checkUriAvailable(uri , userInfo);

In this permission check aspect, just invoke it.

StyleTang · 2021-08-17T15:10:38Z

src/main/java/org/apache/rocketmq/dashboard/service/impl/PermissionServiceImpl.java

+    RMQConfigure configure;
+
+    FileBasedPermissionStore fileBasedPermissionStore;


suggest to use private but not default.

StyleTang · 2021-08-17T15:15:49Z

src/main/java/org/apache/rocketmq/dashboard/service/impl/PermissionServiceImpl.java

+        }
+    }
+
+    public static class FileBasedPermissionStore {


There are some duplicate code here, it would be better if we can reuse it.

org.apache.rocketmq.dashboard.service.impl.UserServiceImpl.FileBasedUserInfoStore

Okay, I'll re-optimize the code as suggested.

vongosling · 2021-08-18T02:04:02Z

doc/1_0_0/UserGuide_CN.md

+```
+* 2.确保${rocketmq.config.dataPath}定义的目录存在，并且该目录下创建登录配置文件"role-permission.yml", 
+如果该目录下不存在此文件，则默认使用resources/role-permission.yml文件。改文件保存了普通用户角色所有能访问的接口地址。


改-> 该

如果resources/role-permission.yml也不存在，会怎样？请把错误的退避规则描述的更精致一些：-）

vongosling · 2021-08-18T02:06:11Z

doc/1_0_0/UserGuide_CN.md

+    - /dashboard/topic.query
+    - /dashboard/topicCurrent
+    ....


考虑通配支持，后续维护太复杂否则

Ok, the interface path in the authentication file is added with wildcard characters

vongosling · 2021-08-18T02:06:51Z

doc/1_0_0/UserGuide_CN.md

+    ....
+```
+* 3.前端页面显示上，为了更好区分普通用户和admin用户权限，关于资源的删除、更新等操作按钮不对普通用户角色显示。


考虑重签功能，要么退出换角色登入，要么支持管理员到普通账户的一键切换

vongosling · 2021-08-18T02:08:00Z

doc/1_0_0/UserGuide_EN.md

+  ordinary:
+    - /rocketmq/nsaddr
+    - /ops/homepage.query


Change description corresponds with CN.

vongosling · 2021-08-18T02:09:11Z

src/main/java/org/apache/rocketmq/dashboard/controller/ConsumerController.java

@@ -37,6 +38,7 @@

 @Controller
 @RequestMapping("/consumer")
+@Permission


I still recommend using the unified configuration, which can be unified in one directory, without annotating each method.

The @Permission annotation is on a class in which all the methods of the class will implement aop methods. There is no need to annotate each method

vongosling · 2021-08-18T02:11:12Z

src/main/java/org/apache/rocketmq/dashboard/permisssion/Permission.java

+@Retention(RetentionPolicy.RUNTIME)
+public @interface Permission {
+
+    String value() default "";


It would be helpful if we could leave the concrete field in here. such as read-only, write...

This value attribute is not used, then delete

vongosling · 2021-08-18T02:16:01Z

src/main/java/org/apache/rocketmq/dashboard/permisssion/PermissionAspect.java

+     * can also be applied to methods in a class for fine control
+     */
+    @Pointcut("@annotation(org.apache.rocketmq.dashboard.permisssion.Permission) || @within(org.apache.rocketmq.dashboard.permisssion.Permission)")
+    private void permission() {


Does within[1] annotation work well here?

[1] https://docs.spring.io/spring-framework/docs/2.0.x/reference/aop.html

Yes，it can work，@Within - limits matching to join points within types that have the given annotation (the execution of methods declared in types with the given annotation when using Spring AOP).

vongosling · 2021-08-18T02:22:36Z

src/main/java/org/apache/rocketmq/dashboard/service/impl/PermissionServiceImpl.java

+                    load(inputStream);
+                }
+            } else {
+                log.info(String.format("User Permission configure file is %s", filePath));


Any resource leak if we do not use try with?

Yes, I will close the InputStream after use

vongosling · 2021-08-19T04:31:10Z

@zhangjidi2016 there are many places waiting to be clarified. BTW, do we have a better way to support here. do not link permission to URL directly?

zhangjidi2016 · 2021-08-19T09:49:39Z

@zhangjidi2016 there are many places waiting to be clarified. BTW, do we have a better way to support here. do not link permission to URL directly?

I haven't thought of any other good authentication methods yet. However, regardless of the authentication method, we need to maintain the role-permission mapping, because rocketMQ-Dashboard deployment does not depend on any external storage system, only to save the mapping to a disk file.
Do you have any good suggestions for improving authentication？@vongosling @StyleTang @zongtanghu

vongosling · 2021-08-19T12:25:50Z

One trick is you could store it in one topic if it is necessary, but it is not friendly for updates, it invokes me to think about our compact topic design...

StyleTang · 2021-08-19T12:59:51Z

I come up with an idea for improving authentication.
For now, the relationship is

user -> role 
role -> uri

Maybe we can introduce permission

user -> role 
role -> permission 

(URI/button/tab) requires permission

user have role, role contains several permissions. A login user can have a permission set.
for example we have a topic_write permission.

For backend: invoke create topic method requires topic_write permission ==> (we can add @Permission(value = "topic_write") to the create topic method)

For front end: show create topic button requires topic_write permission ==> (we can get permissions after login,only show create topic button if the user has topic_write permission)

Do you have any suggestions?

StyleTang · 2021-08-19T13:13:20Z

Local file storage is indeed a problem. Because of it, we need to stand-alone deployment, or the config can not sync.

It is better to have a remote shared storage, Maybe we should abstract the storage interface, local file storage is an implementation(for now), mysql is another implementation, MQ message implementation etc.

vongosling · 2021-08-19T14:09:17Z

I come up with an idea for improving authentication.
For now, the relationship is
user -> role 
role -> uri
Maybe we can introduce permission
user -> role 
role -> permission 

(URI/button/tab) requires permission
user have role, role contains several permissions. A login user can have a permission set.
for example we have a topic_write permission.

For backend: invoke create topic method requires topic_write permission ==> (we can add @Permission(value = "topic_write") to the create topic method)

For front end: show create topic button requires topic_write permission ==> (we can get permissions after login,only show create topic button if the user has topic_write permission)

Do you have any suggestions?

Agree, specifically speaking, we could learn lessons from RBAC, here resources may have different operations

StyleTang · 2021-08-30T11:55:24Z

@zhangjidi2016
Do you have any further updates for this PR.
If no further updates, since the feature works well, I'm OK to merge it and we can refactor it in the future.

zhangjidi2016 · 2021-08-30T12:31:09Z

@zhangjidi2016
Do you have any further updates for this PR.
If no further updates, since the feature works well, I'm OK to merge it and we can refactor it in the future.

Well, we can merge this PR first, and verify permissions according to the interface path in the first version, and then optimize this problem again according to the structure of RBAC.

StyleTang · 2021-08-31T13:14:03Z

@zhangjidi2016
This branch has conflicts with master, could you resolve the conflicts.
Thanks.

…into add_permission

zhangjidi2016 · 2021-09-01T04:45:36Z

@zhangjidi2016
This branch has conflicts with master, could you resolve the conflicts.
Thanks.

OK，code conflicts have been resolved

[ISSUE apache#5]Add permission control when loginRequired is true.

c0709d3

StyleTang reviewed Aug 17, 2021

View reviewed changes

optimize the code

9827f75

vongosling requested changes Aug 18, 2021

View reviewed changes

StyleTang approved these changes Aug 18, 2021

View reviewed changes

Wildcard characters are supported

da89e46

zhangjidi added 2 commits September 1, 2021 09:31

Merge branch 'master' of https://github.com/apache/rocketmq-dashboard …

fabe15b

…into add_permission

add Apache License headers

c9d83bb

StyleTang merged commit 5b2a027 into apache:master Sep 1, 2021

zhangjidi2016 deleted the add_permission branch September 3, 2021 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #5]Add permission control when loginRequired is true. #6

[ISSUE #5]Add permission control when loginRequired is true. #6

zhangjidi2016 commented Aug 17, 2021

StyleTang left a comment

StyleTang Aug 17, 2021

StyleTang Aug 17, 2021

StyleTang Aug 17, 2021

StyleTang Aug 17, 2021

zhangjidi2016 Aug 18, 2021

vongosling Aug 18, 2021

vongosling Aug 18, 2021

zhangjidi2016 Aug 19, 2021

vongosling Aug 18, 2021

vongosling Aug 18, 2021

vongosling Aug 18, 2021

zhangjidi2016 Aug 18, 2021

vongosling Aug 18, 2021

zhangjidi2016 Aug 19, 2021

vongosling Aug 18, 2021

zhangjidi2016 Aug 18, 2021

vongosling Aug 18, 2021

zhangjidi2016 Aug 19, 2021

vongosling commented Aug 19, 2021

zhangjidi2016 commented Aug 19, 2021

vongosling commented Aug 19, 2021 •

edited

StyleTang commented Aug 19, 2021

StyleTang commented Aug 19, 2021

vongosling commented Aug 19, 2021

StyleTang commented Aug 30, 2021

zhangjidi2016 commented Aug 30, 2021

StyleTang commented Aug 31, 2021

zhangjidi2016 commented Sep 1, 2021

		RMQConfigure configure;

		FileBasedPermissionStore fileBasedPermissionStore;

[ISSUE #5]Add permission control when loginRequired is true. #6

[ISSUE #5]Add permission control when loginRequired is true. #6

Conversation

zhangjidi2016 commented Aug 17, 2021

What is the purpose of the change

Brief changelog

Verifying this change

StyleTang left a 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

vongosling commented Aug 19, 2021

zhangjidi2016 commented Aug 19, 2021

vongosling commented Aug 19, 2021 • edited

StyleTang commented Aug 19, 2021

StyleTang commented Aug 19, 2021

vongosling commented Aug 19, 2021

StyleTang commented Aug 30, 2021

zhangjidi2016 commented Aug 30, 2021

StyleTang commented Aug 31, 2021

zhangjidi2016 commented Sep 1, 2021

vongosling commented Aug 19, 2021 •

edited