MTV-1745 | Improve validating webhooks #1338
Conversation
This will be needed to determine permissions for certain operations. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
pkg/forklift-api/webhooks/validating-webhook/admitters/plan-admitter.go
Outdated
Show resolved
Hide resolved
|
@jonner can you squash the commits ? |
Since the targetNamespace field of the plan is used to decide where to create the VM, it is necessary to ensure that a user has permission to create a VirtualMachine object in this namespace. Otherwise we have a security issue where a user can migrate VMS into namespaces that they don't have permissions for. References: https://issues.redhat.com/browse/MTV-1745 Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
If we can't get the source or destination provider, we will not be able to perform appropriate permission checks, so allowing the plan in this case has security risks. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Since the plan just specifies vms by name, we must validate that the user has permission to these vms before creating a migration plan for them. Otherwise the user would be allowed to migrate a VM that they can't normally access to another location. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
I personally prefer more fine-grained commits, but I certainly can squash them. Do you prefer that to using the "squash and merge" feature in github? |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1338 +/- ##
==========================================
- Coverage 15.45% 15.37% -0.08%
==========================================
Files 112 112
Lines 23377 23495 +118
==========================================
Hits 3613 3613
- Misses 19479 19597 +118
Partials 285 285
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
We were not checking if the user had permission to access the provider specified in a given migration plan. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Instead of returning separate values for group and resource, use the kubernetes GroupResource type instead, which encapsulates things a bit more nicely. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
|
I prefer the PR owner to be responsible for the squashing, that way I know if the separate commits are only for reviewing purposes or are a real separation of concerns, because the PR author has the best context and understanding of the changes they've made and can therefore create a clean, logical, and meaningful commit history in the final squashed commit, making it easier for reviewers and future maintainers to understand the evolution of the code. |
My concern with a large number of commits in a pull request is that it complicates back porting. A smaller, concern specific commits can makes it easier to track and manage the back porting. |
yaacov
changed the title
SubjectAccessReviewobjectsref: https://issues.redhat.com/browse/MTV-1745