dashboard: add authority for archive experiments and events #1261

WangXiangUSTC · 2020-12-09T09:09:58Z

What problem does this PR solve?

add authority for archive experiments and events

What is changed and how does it work?

when querying the archive experiments and events, will use SelfSubjectAccessReviews to judge whether the token can list the information.

Checklist

Tests

Unit test
E2E test
Manual test (add detailed scripts or steps below)
No code

Side effects

Breaking backward compatibility

Related changes

Need to update the documentation

Does this PR introduce a user-facing change?

NONE

Signed-off-by: xiang <[email protected]>

WangXiangUSTC · 2020-12-16T09:30:43Z

@g1eny0ung I update the HTTP request on front-end at 408ef7d, PTAL

WangXiangUSTC · 2020-12-16T09:34:31Z

/run-e2e-tests

pkg/apiserver/event/event.go

ui/src/api/events.ts

Signed-off-by: xiang <[email protected]>

codecov-io · 2020-12-16T11:03:25Z

Codecov Report

Merging #1261 (d053930) into master (7e9ff3f) will decrease coverage by 4.20%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##           master    #1261      +/-   ##
==========================================
- Coverage   55.78%   51.57%   -4.21%     
==========================================
  Files          68       78      +10     
  Lines        4383     5012     +629     
==========================================
+ Hits         2445     2585     +140     
- Misses       1768     2159     +391     
- Partials      170      268      +98

Impacted Files	Coverage Δ
api/v1alpha1/common_types.go	`0.00% <0.00%> (ø)`
api/v1alpha1/common_webhook.go	`100.00% <ø> (ø)`
api/v1alpha1/dnschaos_type.go	`0.00% <0.00%> (ø)`
api/v1alpha1/dnschaos_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/httpchaos_types.go	`0.00% <0.00%> (ø)`
api/v1alpha1/iochaos_types.go	`0.00% <ø> (-40.00%)`	⬇️
api/v1alpha1/jvmchaos_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/kernelchaos_types.go	`0.00% <ø> (-20.00%)`	⬇️
api/v1alpha1/kernelchaos_webhook.go	`100.00% <ø> (+14.81%)`	⬆️
api/v1alpha1/kinds.go	`27.27% <ø> (+0.60%)`	⬆️
... and 128 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 ec70532...d053930. Read the comment docs.

g1eny0ung

LGTM

WangXiangUSTC · 2020-12-17T06:55:19Z

@STRRL PTAL

STRRL

How about printing some log when utils.CanListChaos return false.

The current implementation is NOT so good for presenting authorization failed with HTTP API:

It seems just to return empty results(not empty array[]) with HTTP status 200, and the frontend needs more logic to resolve it.
We should return 403 with authorization failed.

If this implementation is a temporary solution, just appending some logs is OK to me.

WangXiangUSTC · 2020-12-25T03:51:15Z

How about printing some log when utils.CanListChaos return false.

The current implementation is NOT so good for presenting authorization failed with HTTP API:

It seems just to return empty results(not empty array[]) with HTTP status 200, and the frontend needs more logic to resolve it.

We should return 403 with authorization failed.

If this implementation is a temporary solution, just appending some logs is OK to me.

I take the gin.Context as a parameter of CanListChaos, and will take the error info to gin.Context, if CanListChaos return false, will print the error log in dashboard @STRRL

STRRL · 2020-12-25T05:28:36Z

Oh, I missed it.🙈

But I think making errors in CanListChaos is not so good. 😰 Any operations about HTTP requests/response should be restricted in Controllers.

cwen0 · 2020-12-28T03:18:37Z

Why not implementing a specific middleware to define this authority?

fewdan · 2020-12-28T09:50:51Z

pkg/apiserver/archive/archive.go

@@ -93,6 +93,11 @@ func (s *Service) list(c *gin.Context) {
 	name := c.Query("name")
 	ns := c.Query("namespace")

+	canList := utils.CanListChaos(c, ns)
+	if !canList {
+		return


Is it more reasonable to return some errors here, such as returning error codes (403 and so on?) ?

yes, you are right, I will refine this logic, maybe use the middleware @cwen0 mentioned

Signed-off-by: xiang <[email protected]>

WangXiangUSTC · 2020-12-29T07:03:02Z

Why not implementing a specific middleware to define this authority?

refine this logic by using the middleware in 584aa18, PTAL again @STRRL @cwen0 @g1eny0ung @fewdan

Signed-off-by: xiang <[email protected]>

fewdan

LGTM

g1eny0ung

LGTM

g1eny0ung · 2020-12-30T09:17:22Z

/merge

ti-srebot · 2020-12-30T09:25:11Z

/run-all-tests

WangXiangUSTC · 2020-12-30T09:25:15Z

/run-e2e-tests

WangXiangUSTC added 4 commits December 9, 2020 15:52

add authority client

62e9b00

Signed-off-by: xiang <[email protected]>

add auth for archive and event

a25670c

Signed-off-by: xiang <[email protected]>

minor fix

1aff56f

Signed-off-by: xiang <[email protected]>

use lru for auth client

23c57d3

Signed-off-by: xiang <[email protected]>

WangXiangUSTC added the status/WIP label Dec 9, 2020

WangXiangUSTC added 5 commits December 9, 2020 17:28

minor fix

add7ff6

Signed-off-by: xiang <[email protected]>

fix unit test

74df6d5

Signed-off-by: xiang <[email protected]>

minor fix

f233eb0

Signed-off-by: xiang <[email protected]>

fix test

303a07f

Signed-off-by: xiang <[email protected]>

add namespace as parameter for http request

408ef7d

Signed-off-by: xiang <[email protected]>

WangXiangUSTC added component/dashboard status/PTAL and removed status/WIP labels Dec 16, 2020

WangXiangUSTC requested review from STRRL and g1eny0ung and removed request for STRRL December 16, 2020 09:31

g1eny0ung reviewed Dec 16, 2020

View reviewed changes

pkg/apiserver/event/event.go Outdated Show resolved Hide resolved

ui/src/api/events.ts Outdated Show resolved Hide resolved

minor fix

fc4611e

Signed-off-by: xiang <[email protected]>

g1eny0ung previously approved these changes Dec 17, 2020

View reviewed changes

WangXiangUSTC added status/LGT1 and removed status/PTAL labels Dec 17, 2020

g1eny0ung mentioned this pull request Dec 17, 2020

refactor: dashboard homepage and some visible changes #1285

Merged

13 tasks

STRRL reviewed Dec 25, 2020

View reviewed changes

fewdan reviewed Dec 28, 2020

View reviewed changes

WangXiangUSTC added 2 commits December 29, 2020 10:51

merge master && resolve conflicts

51601ab

Signed-off-by: xiang <[email protected]>

use middleware in gin for authority

584aa18

Signed-off-by: xiang <[email protected]>

WangXiangUSTC dismissed g1eny0ung’s stale review via 584aa18 December 29, 2020 06:53

minor fix

6b7adbb

Signed-off-by: xiang <[email protected]>

minor fix

2f31325

Signed-off-by: xiang <[email protected]>

cwen0 requested review from STRRL, g1eny0ung, fewdan and cwen0 December 30, 2020 04:03

fewdan approved these changes Dec 30, 2020

View reviewed changes

g1eny0ung approved these changes Dec 30, 2020

View reviewed changes

ti-srebot added the status/can-merge label Dec 30, 2020

Merge branch 'master' into xiang/dashboard_token_refine

d053930

ti-srebot merged commit f34ed1e into chaos-mesh:master Dec 30, 2020

WangXiangUSTC deleted the xiang/dashboard_token_refine branch December 30, 2020 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dashboard: add authority for archive experiments and events #1261

dashboard: add authority for archive experiments and events #1261

WangXiangUSTC commented Dec 9, 2020 •

edited

Loading

WangXiangUSTC commented Dec 16, 2020

WangXiangUSTC commented Dec 16, 2020

codecov-io commented Dec 16, 2020 •

edited

Loading

g1eny0ung left a comment

WangXiangUSTC commented Dec 17, 2020

STRRL left a comment

WangXiangUSTC commented Dec 25, 2020

STRRL commented Dec 25, 2020 •

edited

Loading

cwen0 commented Dec 28, 2020

fewdan Dec 28, 2020

WangXiangUSTC Dec 28, 2020

WangXiangUSTC commented Dec 29, 2020

fewdan left a comment

g1eny0ung left a comment

g1eny0ung commented Dec 30, 2020

ti-srebot commented Dec 30, 2020

WangXiangUSTC commented Dec 30, 2020

dashboard: add authority for archive experiments and events #1261

dashboard: add authority for archive experiments and events #1261

Conversation

WangXiangUSTC commented Dec 9, 2020 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Checklist

Does this PR introduce a user-facing change?

WangXiangUSTC commented Dec 16, 2020

WangXiangUSTC commented Dec 16, 2020

codecov-io commented Dec 16, 2020 • edited Loading

Codecov Report

g1eny0ung left a comment

Choose a reason for hiding this comment

WangXiangUSTC commented Dec 17, 2020

STRRL left a comment

Choose a reason for hiding this comment

WangXiangUSTC commented Dec 25, 2020

STRRL commented Dec 25, 2020 • edited Loading

cwen0 commented Dec 28, 2020

fewdan Dec 28, 2020

Choose a reason for hiding this comment

WangXiangUSTC Dec 28, 2020

Choose a reason for hiding this comment

WangXiangUSTC commented Dec 29, 2020

fewdan left a comment

Choose a reason for hiding this comment

g1eny0ung left a comment

Choose a reason for hiding this comment

g1eny0ung commented Dec 30, 2020

ti-srebot commented Dec 30, 2020

WangXiangUSTC commented Dec 30, 2020

WangXiangUSTC commented Dec 9, 2020 •

edited

Loading

codecov-io commented Dec 16, 2020 •

edited

Loading

STRRL commented Dec 25, 2020 •

edited

Loading