[ISSUE #70] The rocketmq-dashboard supports ACL configuration #71

xxd763795151 · 2022-01-30T15:55:08Z

What is the purpose of the change

The rocketmq-dashboard supports ACL configuration, such as following:

Or

Feature descirbe:

the 'CRUD operation' of topic permission for an accessKey
the 'CRUD operation' of group permission for an accessKey
add/delete host white list
the acl info can be synchronized in the cluster from a broker to all broker when it is inconsistent(Why it may be inconsistent? For example, the acl info is add by manual in all brokers, one broker config error or forget to config; when we config acl, one broker is down in the cluster.)

**Note: the Acl menu only appears when the accessKey and secretKey is not empty in the application.yml. **

By the way, the last topic or group permission can not be deleted if there is only one topic or group permission for an access key because of the rocketmq`s implemention.

Brief changelog

XXXX

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.

xxd763795151 · 2022-01-30T16:19:27Z

Sorry, I forget unit tests. I need spend a little time to add some unit tests.

StyleTang · 2022-02-13T02:46:22Z

@xxd763795151
Thanks for your contribution, I have some urgent things to do recently, will review your PR later.

vongosling · 2022-02-14T00:59:29Z

Security key should be hidden by asterisks :-)

vongosling · 2022-02-14T01:08:13Z

NIce job ~~~ I'd like to add another reviewer @zhangjidi2016 for this feature :-)

zhangjidi2016 · 2022-02-14T06:33:05Z

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

+
+ @PostMapping("/add.do")
+ public Object addAclConfig(@RequestBody PlainAccessConfig config) {
+ Preconditions.checkArgument(config.getAccessKey() != null && !config.getAccessKey().isEmpty(), "accessKey is null");


Use StringUtils.isNotEmpty(config.getAccessKey()) instead

src/main/java/org/apache/rocketmq/dashboard/service/client/MQAdminExtImpl.java

zhangjidi2016 · 2022-02-14T06:49:40Z

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

+
+@RestController
+@RequestMapping("/acl")
+public class AclController {


Add permission control for ACL-related operation interfaces for different login role

Oh, oh, okay. I will make some adjustments according to your suggestions later.
@zhangjidi2016

zhangjidi2016 · 2022-02-14T06:51:17Z

src/main/resources/static/view/pages/acl.html

+ confirmed-click="deleteAclConfig(item.accessKey)">{{'DELETE' | translate}}
+ </button>
+ <button class="btn btn-raised btn-sm btn-danger" type="button"
+ ng-click="synchronizeData(item)">{{'SYNCHRONIZE' | translate}}


Hide action buttons based on login roles

xxd763795151 · 2022-02-14T07:50:54Z

Security key should be hidden by asterisks :-)

About this, I hope you could think it about more. If it is necessary, I will adjust later. Such as , it will show '********' , that hidden by asterisks, and there is a button on the side, click visible. Or hide this column , and add a 'Secret key ' button.

codecov-commenter · 2022-02-17T07:25:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6da21f4). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #71   +/-   ##
=========================================
  Coverage          ?   76.94%           
  Complexity        ?      821           
=========================================
  Files             ?       91           
  Lines             ?     2837           
  Branches          ?      278           
=========================================
  Hits              ?     2183           
  Misses            ?      500           
  Partials          ?      154

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6da21f4...5ed049c. Read the comment docs.

xxd763795151 · 2022-02-17T07:32:11Z

Now, when enable login controle, different roles are treated differently, as following:
the admin role:

the normal role:

@zhangjidi2016 @vongosling
Please review code again, thanks.

vongosling · 2022-02-18T01:42:44Z

Security key should be hidden by asterisks :-)

About this, I hope you could think it about more. If it is necessary, I will adjust later. Such as , it will show '********' , that hidden by asterisks, and there is a button on the side, click visible. Or hide this column, and add a 'Secret key ' button.

As I said, Open Source projects also consider the common safety problem. Do you really see the downside of displaying security keys in clear text by default?

…admin in the background.

xxd763795151 · 2022-02-18T08:15:27Z

Security key should be hidden by asterisks :-)

About this, I hope you could think it about more. If it is necessary, I will adjust later. Such as , it will show '********' , that hidden by asterisks, and there is a button on the side, click visible. Or hide this column, and add a 'Secret key ' button.

As I said, Open Source projects also consider the common safety problem. Do you really see the downside of displaying security keys in clear text by default?

@vongosling Yeah, u are right. I just made some adjustments for it. Such as following:

src/main/java/org/apache/rocketmq/dashboard/model/request/AclRequest.java

StyleTang · 2022-02-20T10:08:14Z

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

+ Preconditions.checkArgument(StringUtils.isNotEmpty(config.getAccessKey()), "accessKey is null");
+ Preconditions.checkArgument(StringUtils.isNotEmpty(config.getSecretKey()), "secretKey is null");
+ aclService.addAclConfig(config);
+ return new JsonResult(0, "");


We can just return true.

return true;

I will adjust it later. When I optimize code first time, I try return true, but it occur json error. So I use this style as return value. But using this style, it doesn't seem to be a problem right now, I will optimize these code again.

StyleTang · 2022-02-20T10:08:51Z

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

+ public Object deleteAclConfig(@RequestBody PlainAccessConfig config) {
+ Preconditions.checkArgument(StringUtils.isNotEmpty(config.getAccessKey()), "accessKey is null");
+ aclService.deleteAclConfig(config);
+ return new JsonResult(0, "");


StyleTang · 2022-02-20T10:09:59Z

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

+ for (String addr :
+ masterSet) {


can be on the same line.

xxd763795151 added 2 commits January 26, 2022 20:30

Add Acl menu, support config acl.

d5f9403

Optimize one line code.

df4b639

Add some unit tests for acl.

aaa199b

zhangjidi2016 reviewed Feb 14, 2022

View reviewed changes

src/main/java/org/apache/rocketmq/dashboard/service/client/MQAdminExtImpl.java Outdated Show resolved Hide resolved

zhangjidi2016 reviewed Feb 14, 2022

View reviewed changes

Add permission control by role and optimize some code.

b1d436c

zhangjidi2016 approved these changes Feb 18, 2022

View reviewed changes

xxd763795151 added 2 commits February 18, 2022 13:52

The secret keys are hidden by asterisks.

5ed049c

Search acl data will exclude secretKey info if the login role is not …

fade103

…admin in the background.

StyleTang approved these changes Feb 20, 2022

View reviewed changes

xxd763795151 added 2 commits February 21, 2022 11:47

Optimize some code again.

f6dae9d

recover default application.yml config

181db5a

StyleTang merged commit 4269879 into apache:master Mar 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #70] The rocketmq-dashboard supports ACL configuration #71

[ISSUE #70] The rocketmq-dashboard supports ACL configuration #71

xxd763795151 commented Jan 30, 2022 •

edited

Loading

xxd763795151 commented Jan 30, 2022

StyleTang commented Feb 13, 2022

vongosling commented Feb 14, 2022

vongosling commented Feb 14, 2022

zhangjidi2016 Feb 14, 2022

zhangjidi2016 Feb 14, 2022

xxd763795151 Feb 14, 2022

zhangjidi2016 Feb 14, 2022

xxd763795151 commented Feb 14, 2022

codecov-commenter commented Feb 17, 2022 •

edited

Loading

xxd763795151 commented Feb 17, 2022

vongosling commented Feb 18, 2022

xxd763795151 commented Feb 18, 2022

StyleTang Feb 20, 2022

xxd763795151 Feb 20, 2022

StyleTang Feb 20, 2022

StyleTang Feb 20, 2022

[ISSUE #70] The rocketmq-dashboard supports ACL configuration #71

[ISSUE #70] The rocketmq-dashboard supports ACL configuration #71

Conversation

xxd763795151 commented Jan 30, 2022 • edited Loading

What is the purpose of the change

Brief changelog

Verifying this change

xxd763795151 commented Jan 30, 2022

StyleTang commented Feb 13, 2022

vongosling commented Feb 14, 2022

vongosling commented Feb 14, 2022

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

xxd763795151 commented Feb 14, 2022

codecov-commenter commented Feb 17, 2022 • edited Loading

Codecov Report

xxd763795151 commented Feb 17, 2022

vongosling commented Feb 18, 2022

xxd763795151 commented Feb 18, 2022

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

xxd763795151 commented Jan 30, 2022 •

edited

Loading

codecov-commenter commented Feb 17, 2022 •

edited

Loading