Skip to content
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

Cockpit: you can delete an host used into host-group #6735

Closed
federicoballarini opened this issue Mar 16, 2023 · 13 comments
Closed

Cockpit: you can delete an host used into host-group #6735

federicoballarini opened this issue Mar 16, 2023 · 13 comments
Labels
bug A defect of the software verified All test cases were verified successfully

Comments

@federicoballarini
Copy link
Member

Steps to reproduce

  • Create an host
  • Add it to an host group
  • Use the host group into one module like Web Proxy
  • Try to delete the host

Expected behavior

  • You cannot delete the host

Actual behavior

  • Host delete is permitted

Components

nethserver-firewall-base

@federicoballarini federicoballarini added the bug A defect of the software label Mar 16, 2023
federicoballarini added a commit to federicoballarini/nethserver-firewall-base that referenced this issue Mar 16, 2023
@gsanchietti
Copy link
Member

Thanks for reporting!

Current validator has been added to avoid dealing with host groups without any members.
We could extend it, but I'm not sure it's a good idea.
Usually, the expansion of a group should be smart enough to ignore non-existing hosts.

What was the original problem? Can we eventually fix it without a system-wide change?

@DavidePrincipi
Copy link
Member

DavidePrincipi commented Mar 17, 2023

I agree, in general the expected behavior is not what happens in similar situations. Think about a group of users: nothing forbids to remove a user account from the system if it belongs to some group.

The current validator forbids the deletion of the last group member, to avoid other implementation issues (I can't remember what they are)

@federicoballarini
Copy link
Member Author

Thanks for reporting!

Current validator has been added to avoid dealing with host groups without any members. We could extend it, but I'm not sure it's a good idea. Usually, the expansion of a group should be smart enough to ignore non-existing hosts.

What was the original problem? Can we eventually fix it without a system-wide change?

Hi @gsanchietti,
the actual validator doesn't work if you have more than one members inside the group, and you can always delete an used host, if it's used in a group with two or more members.

This row:

if(($record->prop('Members') || '') eq $hostKey) {

will never work, except for host-group with one item. If I want to delete host2, I will be in this situation:

$record->prop('Members') =  host1,host2,host3
$hostKey = host2

The condition cannot be verified if you don't use a split or something like this.
What do you think about changing my code and add a check to avoid the last member deletion?
I think it's helpful for sysadmins to check if an host is used into a group...

@gsanchietti
Copy link
Member

gsanchietti commented Mar 17, 2023

the actual validator doesn't work if you have more than one members inside the group, and you can always delete an used host, if it's used in a group with two or more members.

Exactly. Looking to the original commit this is the purpose of the validator.

will never work, except for host-group with one item. If I want to delete host2, I will be in this situation:

I know and I'm reticent to change the behavior of the validator because it's in place for a long time and I fear regressions.

What is the original problem that led you changing the validator? My guess: you had issues with the proxy.

@federicoballarini
Copy link
Member Author

What is the original problem that led you changing the validator? My guess: you had issues with the proxy.

The problem is that if you have an host used into an host-group you can delete the host and this remains linked in the host group.

@gsanchietti
Copy link
Member

I get it.
But does the dangling reference has any side-effect?

@federicoballarini
Copy link
Member Author

The problem I see is this:

  • Create an host test
  • Create a group test-group and add test to it
  • Add test-group to proxy bypass
  • Delete test host (all is fine)

After some time:

  • Create another host called test
  • It will bypass the proxy because you forgot to delete it from the group

@gsanchietti
Copy link
Member

I see your point.
The proposed solution seems good for this usage scenario.

What do you think about changing my code and add a check to avoid the last member deletion?

Yes, please add also this condition.

Also make sure that dangling references inside the group does not cause any other issue.
Let's hope no regression will be introduced with this change.

gsanchietti added a commit to NethServer/nethserver-firewall-base that referenced this issue Mar 23, 2023
@gsanchietti
Copy link
Member

gsanchietti commented Mar 23, 2023

Proposed solution should work in most case.
Let's hope it will not impact too much on daily firewall configuration.

Test case 1

  • Check the bug is not reproducible

Test case 2

  • Try to delete an host which is the last one in the group
  • Verify the host can't be deleted

Test case 3

  • Add an host to a group
  • Verify the host can be added to a group

Test case 4

  • Try to remove an host not belonging to any group
  • Verify the host can be removed

@nethbot
Copy link
Member

nethbot commented Mar 23, 2023

in 7.9.2009/testing:

@gsanchietti gsanchietti added the testing Packages are available from testing repositories label Mar 23, 2023
@cotosso
Copy link

cotosso commented Mar 24, 2023

Everything works as expected.

  • Test Case 1 : OK

image

  • Test Case 2 : OK (host2 is the last one in the group)
    image

  • Test Case 3 : OK (host5 is added to grp1)
    image
    image

  • Test Case 4 : OK (host 5 is deleted)
    image

@cotosso cotosso added verified All test cases were verified successfully and removed testing Packages are available from testing repositories labels Mar 24, 2023
@nethbot
Copy link
Member

nethbot commented Mar 27, 2023

in 7.9.2009/updates:

@gsanchietti
Copy link
Member

Thank you for the work, released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect of the software verified All test cases were verified successfully
Projects
None yet
Development

No branches or pull requests

5 participants