-
Notifications
You must be signed in to change notification settings - Fork 262
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
Add test for checking user-facing resources can be manipulated by non-cluster-admin #3033
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @awels |
tests/rbac_test.go
Outdated
outputAPIResources, err := f.RunKubectlCommand("api-resources", "--namespaced", "-o", "name") | ||
Expect(err).ToNot(HaveOccurred(), "ERR: %s, OUT: %s", err, outputAPIResources) | ||
for _, apiResource := range strings.Split(strings.TrimSpace(outputAPIResources), "\n") { | ||
if strings.Contains(apiResource, "cdi.kubevirt.io") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use HasSuffix
instead of Contains
. limits the matches a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated for now
} | ||
}, | ||
Entry("[test_id:XXXX]for admin", "admin"), | ||
Entry("[test_id:XXXX]for edit", "edit"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the view
user? They have much more limited access. But probably should still have get/list/watch on most resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I took a stab at that and only "uploadtokenrequests" diverges. The cluster wide resources may need extra adjustments too
@@ -147,6 +149,34 @@ var _ = Describe("Aggregated role in-action tests", Serial, func() { | |||
Entry("[test_id:3949]can do everything with edit", "edit"), | |||
) | |||
|
|||
DescribeTable("check all user facing resources can be manipulated by non-cluster-admin", func(user string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe pass in a map or list of resources and rbac rules that a user should be allowed to do. This way we are checking all the aggregate roles and not just the admin ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to avoid passing a list of resources, and by doing that, we protect the project from introducing unusable new APIs in the future
4adb2dc
to
7930e75
Compare
/hold let's see if we can gate against view user and thus cluster wide resources too |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
This should gate us from introducing user-facing resources that cannot be manipulated by non-cluster-admin. Signed-off-by: Alex Kalenyuk <[email protected]>
7930e75
to
9d801f3
Compare
Rotten issues close after 30d of inactivity. /close |
@kubevirt-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/reopen |
@akalenyu: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Rotten issues close after 30d of inactivity. /close |
@kubevirt-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/reopen |
@akalenyu: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Rotten issues close after 30d of inactivity. /close |
@kubevirt-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/reopen |
@akalenyu: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@akalenyu: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What this PR does / why we need it:
This should keep us from introducing user-facing resources that cannot be
manipulated by non-cluster-admin.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: