Fixes #39437 - Apply taxonomies to UI bulk actions#11033
Conversation
06bed8b to
9adc2fe
Compare
|
@jeremylenz @lfu @MariaAga This is a very serious issue and we are trying to find a solution. Any opinions? |
|
Adding also @ianballou and @parthaa as they reviewed the original. Was there a reason not to scope by location, and only scope by org? |
Thanks! We were investigating a bit more and sending search strings in general may result in other dangerous behavior. For example, you could:
In the 20 minutes that you were waiting, someone else creates 50 new hosts that would all be deleted. Looking at this, it looks bigger than adding 'Location' to the scope. |
Probably not. We tend to neglect taxonomies other than org, since Katello isn't location-focused. This seems like a needed fix.
This part is just an effect of how scoped search works. Remote Execution has a feature that attempts to get around this by asking the user if you want the host query to be "dynamic" or "static". We could consider expanding that feature to other searches, but I don't consider it part of the scope here. |
Do you think we need the complete path from frontend to backend or is it enough to have the validation part at the backend?
I agree that those are two different issues. Shall I add another issue on Redmine? If you look at the old 'All Hosts' page, it would give you a preview of all the hosts that it would operate bulk actions on. So this seems to be a regression. I know that the old page works on the controller data directly and moving all this data via API calls is tedious. |
Not sure I understand the question completely (I haven't reviewed the code in detail yet), but I would expect it to respect the UI location context selector the same as it does for org. As for new Redmine issues, I don't think it's a regression that searches are not static, but you could feel free to add that as an RFE. What specific behavior do you see as a regression? We could add a modal or wizard step that shows you selected hosts, but that too would be dynamic. And I think we decided with @MariSvirik not to add it because you've just selected hosts, so it would be redundant to show you the hosts you've just selected. |
In this PR, I added a part to the backend that copies the behavior of organization for location. I also added that both are explicitly passed from the frontend to the backend. Not sure whether we need that second part. |
A part of me thinks this is intended behavior. You wanted all of the hosts deleted - you selected "Select all". That means all the hosts will be deleted when you hit submit. The intention to destroy everything is there. If you linger on a form for too long, the intent hasn't gone away to delete all hosts. Now, could we make that behavior easier to understand? Perhaps, yeah. But I think it's a good thing to shelf for later. |
|
|
||
| def_param_group :bulk_host_ids do | ||
| param :organization_id, :number, :required => true, :desc => N_("ID of the organization") | ||
| param :location_id, :number, :required => true, :desc => N_("ID of the location") |
There was a problem hiding this comment.
Since this line is being added - neither the location nor the organization are actually required. We could probably take the required flag out for both.
There was a problem hiding this comment.
There was a problem hiding this comment.
I like the default "Set the location context.." better anyway. 👍
Somehow. As the user A fall asleep for 20minutes new hosts we're added by another user. User A then press select all and starts to delete the hosts via the bulk action. The bulk action shows that x hosts were selected - which is the amount of hosts before sleeping for 20minutes on the all hosts page. As no host ids are send to the backend but search queries, it can always be, that the user deletes wrong hosts because in the meantime something has happened to the hosts. Maybe a user searches for all hosts with OS Alma and even excluded 4 hosts - in the meantime more hosts were added (with FAM) and they are deleted, too. I have worked on a guard which uses digests of user ID, location ID, organization ID, and all host IDs a user has access. This digest is passed to the UI and forwarded to 'some' bulk actions like delete, power off which can really 'destroy' a datacenter managed by foreman. In the bulk action the hash is recalculated and only if the hash stays the same, the bulk action is executed. Not perfect, but I think it solves both issues. In my point of view, the root cause is, that search queries (dynamic queries) are somehow send to the backend instead of host IDs. Only if the user would see which hosts are affected by a bulk action and sending these host IDs to the backend would really solve the issue . |
|
I think we can agree that this particular PR targets to fix the location(/organization?) scope mismatch issue between UI and the backend. In parallel, I will create an RFC that outlines the broader problem with the dynamically scoped search queries. |
|
PR with the guard I mentioned: #11038 |
9adc2fe to
83b75b5
Compare
|
CI is green, this is ready for review :) |
jeremylenz
left a comment
There was a problem hiding this comment.
This seems to be working well! Sorry for the delay in review
- I have 4 hosts in Default Org/Default Location (location_id 2)
- I have 0 hosts in Loc2
- on All Hosts page, I select all
- in another tab, I change current location to Loc2
- In the original tab, I perform a bulk action (I tried power state and build)
Even though current location is set to Loc2 (thru the other tab), the request still sends location_id 2 / Default Location, and it properly finds my hosts.
Basically it now behaves the way you would think it should - it respects the location you have in front of your eyes, rather than the more-recently-changed one in the other tab.
83b75b5 to
32f9854
Compare
32f9854 to
bdb805f
Compare
Before this patch, the Foreman UI did not send any information about taxonomies on the 'All Hosts' page for bulk actions. This could cause bulk actions to be applied to different hosts than the ones that were actually selected in the UI because the session in the backend would calculate the hosts independent from UI input. Assisted-by: OpenAI Codex
bdb805f to
41bf182
Compare
jeremylenz
left a comment
There was a problem hiding this comment.
Thanks @nadjaheitmann!
ACK 👍
|
Thanks @jeremylenz , that was fast! Should we have a cherry-pick for 3.18 and 3.19? I am not sure if this is the best solution to the problem but I think it is better having this than the bug 🙃 |
|
@jeremylenz @ianballou I have created an RFC in the community forum for awareness and further discussion: https://community.theforeman.org/t/rfc-destructive-bug-with-bulk-actions-on-all-hosts-ui-page/46920 |
|
+1 to cherry-picking. |
Who decides that and when? What is the procedure here? |
|
The last I've heard is that in Foreman (unlike Katello) you're supposed to open your own cherry-pick PRs as soon as possible. @ofedoren is that right? |
I was not aware of that, otherwise I would have opened a cherry-pick immediately. 3.18.2 was just released... Anyways, I am surprised about the low attention that this bug receives. |
|
perhaps I got the wrong "O" name and should have tagged @ogajduse .. |

The error is caused by a mismatch between what is shown in the UI and what the backend resolved as current organization/location.
The API request that is made from the frontend does not contain any information which organization/location the action should be performed on. It only sends an empty search string when an 'Select All' is activated. This can cause serious issue if for - whatever reason - there is a mismatch between the frontend location and the backend scope.
(this did not happen on the old All Hosts page - it would try to show the affected hosts beforehand and just show an empty page).
Even worse, the new page shows the correct number of hosts in the confirmation window, so there is no way for the user to know that something might go wrong!!!
We managed to reproduce the error when doing the following:
(however, this does not work every time, it's a bit random)