Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the ACL checking mechanism of Sentinel dashboard #1042

Merged
merged 5 commits into from
Dec 2, 2019
Merged

Improve the ACL checking mechanism of Sentinel dashboard #1042

merged 5 commits into from
Dec 2, 2019

Conversation

lkxiaolou
Copy link
Contributor

Describe what this PR does / why we need it

Fix the dashboard privileges check bug, from the interface AuthService, there is a comment on the authTarget function

@return if current user has the specific privileges to the target, return true, otherwise return false.

but in the dashboard controllers, there is not check on the authTarget's return value to verify privileges.

and update the fastjson version because of the security, see https://github.com/alibaba/fastjson/releases

Does this pull request fix one issue?

NONE

Describe how you did it

Add checks for every authTarget's return.

refer #1035

@codecov-io
Copy link

codecov-io commented Sep 12, 2019

Codecov Report

Merging #1042 into master will decrease coverage by 0.36%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1042      +/-   ##
============================================
- Coverage     43.03%   42.67%   -0.37%     
+ Complexity     1568     1473      -95     
============================================
  Files           337      317      -20     
  Lines          9877     9286     -591     
  Branches       1332     1267      -65     
============================================
- Hits           4251     3963     -288     
+ Misses         5097     4831     -266     
+ Partials        529      492      -37
Impacted Files Coverage Δ Complexity Δ
...sp/sentinel/slots/system/SystemBlockException.java 0% <0%> (-44.45%) 0% <0%> (-2%)
...a/csp/sentinel/slots/system/SystemRuleManager.java 43.16% <0%> (-21.84%) 7% <0%> (-12%)
.../alibaba/csp/sentinel/slots/system/SystemRule.java 28.57% <0%> (-16.33%) 8% <0%> (-4%)
...libaba/csp/sentinel/slotchain/ResourceWrapper.java 71.42% <0%> (-15.24%) 3% <0%> (-3%)
...m/alibaba/csp/sentinel/node/metric/MetricNode.java 51.94% <0%> (-13.32%) 18% <0%> (-5%)
.../java/com/alibaba/csp/sentinel/util/SpiLoader.java 8.64% <0%> (-7.69%) 2% <0%> (-2%)
...nel/adapter/reactor/SentinelReactorSubscriber.java 80% <0%> (-6.89%) 19% <0%> (-2%)
...baba/csp/sentinel/slotchain/SlotChainProvider.java 63.15% <0%> (-6.85%) 5% <0%> (+2%)
...aba/csp/sentinel/adapter/servlet/CommonFilter.java 84.61% <0%> (-3.19%) 10% <0%> (-1%)
...a/csp/sentinel/slots/statistic/base/LeapArray.java 67.32% <0%> (-2.98%) 33% <0%> (-1%)
... and 53 more

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 20ad3c8...c46e84a. Read the comment docs.

@sczyh30 sczyh30 added the area/dashboard Issues or PRs about Sentinel Dashboard label Sep 12, 2019
pom.xml Outdated Show resolved Hide resolved
@@ -69,7 +69,9 @@
@RequestParam String ip,
@RequestParam Integer port) {
AuthUser authUser = authService.getAuthUser(request);
authUser.authTarget(app, PrivilegeType.READ_RULE);
if (!authUser.authTarget(app, PrivilegeType.READ_RULE)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing the duplicate boilerplate code in every controller handler is tedious :(
Maybe we need a unified, simplified filter or other mechanisms to achieve this. See #745 for more discussions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I try to improve it

Copy link
Contributor Author

@lkxiaolou lkxiaolou Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing the duplicate boilerplate code in every controller handler is tedious :(
Maybe we need a unified, simplified filter or other mechanisms to achieve this. See #745 for more discussions.

I have pushed the new implementation.
Use AuthInterceptor to intercept the AuthAction annotation

@Retention(RetentionPolicy.RUNTIME)
@Documented
@Target({ElementType.METHOD})
public @interface AuthAction {

    AuthService.PrivilegeType value();

    String targetName() default "app";

    String message() default "No privilege";
}

Value is the privilege.
TargetName is the auth target, from the request parameter(post or get), usually app or ip, default app.
Message is the response message when do action without privilege.

It work looks like this
image

@sczyh30
Copy link
Member

sczyh30 commented Sep 15, 2019

Any ideas about this PR? @jasonjoo2010

@sczyh30 sczyh30 changed the title Fix the dashboard privileges check bug and update fastjson version Improve the ACL checking mechanism of Sentinel dashboard Sep 18, 2019
@sczyh30 sczyh30 added the to-review To review label Sep 18, 2019
Copy link
Collaborator

@jasonjoo2010 jasonjoo2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brief: EL expression to support JSON format requests.

@sczyh30 @lkxiaolou

@sczyh30 sczyh30 added this to the 1.7.1 milestone Nov 7, 2019
Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@jasonjoo2010 jasonjoo2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter in JSON type would be improved later.

Now everything LGTM except it.

@sczyh30 sczyh30 merged commit e8a01e2 into alibaba:master Dec 2, 2019
@sczyh30
Copy link
Member

sczyh30 commented Dec 2, 2019

Nice work. Thanks for contributing!

@sczyh30 sczyh30 added kind/enhancement Category issues or prs related to enhancement. and removed to-review To review labels Dec 2, 2019
hughpearse pushed a commit to hughpearse/Sentinel that referenced this pull request Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dashboard Issues or PRs about Sentinel Dashboard kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants