-
Notifications
You must be signed in to change notification settings - Fork 30
Fix multiple downtime deletion permission issue #1257
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
Conversation
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @Copilot Details
|
Co-authored-by: nilmerg <[email protected]>
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @Copilot Details
|
1 similar comment
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @Copilot Details
|
nilmerg
left a comment
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.
This does not even remotely resolve the issue. That's a bad workaround (even if, didn't test). #1246 solves a very similar issue and the underlying problem is the same here.
You're right that this is not the proper fix. I can see that PR #1246 addresses the same underlying issue with filter qualification. Could you provide more guidance on the correct approach for handling the bulk deletion case? The problem is that
I want to implement the correct fix rather than a workaround. |
Co-authored-by: nilmerg <[email protected]>
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @Copilot Details
|
|
The filters to remove multiple downtimes, based on a manual selection of multiple downtimes, are not qualified enough. Instead of |
Co-authored-by: nilmerg <[email protected]>
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @Copilot Details
|
Fixed the filter qualification issue. Changed the multiselect filters to use qualified names: 'downtime.name' instead of 'name' for downtimes and 'comment.name' instead of 'name' for comments. This ensures proper filter qualification when multiple downtimes are selected for bulk operations. (4c210fb) |
nilmerg
left a comment
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. Quite surprised of your ability to navigate the code based on such a rough hint. Still, without that hint…
|
I consider this a trivial change, especially since I needed to guide the bot to the fix quite substantially. |
Users with
icingadb/command/downtime/*permission were unable to delete multiple downtimes, receiving "No permission for icingadb/command/downtime/delete" error despite having the required permissions.Problem
When attempting to delete multiple downtimes from the Downtimes view, users would get a permission error even though they had the
icingadb/command/downtime/*permission which should cover downtime deletion:The audit log would show the permission was granted but the deletion still failed:
Root Cause
The issue was in the
DowntimesController::fetchCommandTargets()method. When deleting multiple downtimes, this method returned aQueryobject, which triggered the bulk permission check viaisGrantedOnType(). This bulk check failed because it couldn't properly validate permissions on the underlying host/service objects that the downtimes belong to.Solution
Changed
fetchCommandTargets()to return anarrayinstead of aQuery. This forces the system to use individual permission checks viaisGrantedOn()for each downtime, which properly validates permissions on the underlying host/service objects.Key Changes
fetchCommandTargets()return type fromQuerytoarrayiterator_to_array($downtimes)to convert query results to arrayTechnical Details
When
fetchCommandTargets()returns an array, theCommandActions::isGrantedOnCommandTargets()method iterates through each downtime and callsisGrantedOn()individually. TheisGrantedOn()method inDowntimesControllercorrectly checks permissions on$object->{$object->object_type}(the host or service that the downtime belongs to).This approach is consistent with other controllers like
HostControllerandServiceControllerthat also return arrays fromfetchCommandTargets().Fixes #1256.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.