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

Fixes #37640 - Apply Loc/Org filter to Host Registration form #10240

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

nadjaheitmann
Copy link
Contributor

@nadjaheitmann nadjaheitmann commented Jul 10, 2024

Select organization A. Then go to Register Host page and Select organization B in the Host Registration form and choose a smart-proxy that belongs to B. Choose activation key and click generate. That gives you an error because organizations are not matching. The curl command fails to render.

Fixed it by not allowing to choose other locations/organizations than the one selected for Foreman previously. If no location/organization is selected in Foreman, any organization can be selected in the Host Registration form.

bug

@sbernhard
Copy link
Contributor

@jeremylenz Interested to review this?

@dosas
Copy link
Contributor

dosas commented Jul 15, 2024

Would it be possible to add a tests for this?

@ekohl
Copy link
Member

ekohl commented Jul 15, 2024

If you only allow a single value, would it make more sense to hide the fields completely?

@nadjaheitmann
Copy link
Contributor Author

If you only allow a single value, would it make more sense to hide the fields completely?

The way it is implemented now, you can still choose 'Not specified' as the second option.

@nadjaheitmann
Copy link
Contributor Author

Would it be possible to add a tests for this?

As far as I can see, there are no tests at all for the Host Registration page (yet).

@m-bucher
Copy link
Contributor

Test added.

@ekohl
Copy link
Member

ekohl commented Jul 20, 2024

If you only allow a single value, would it make more sense to hide the fields completely?

The way it is implemented now, you can still choose 'Not specified' as the second option.

But not specified results in the same, right? The selected org/loc is used

@nadjaheitmann
Copy link
Contributor Author

But not specified results in the same, right? The selected org/loc is used

If the location/organization parameter is not specified, it is not part of the registration command.

Screenshot from 2024-07-22 09-41-05

Screenshot from 2024-07-22 09-40-46

Copy link
Contributor

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbernhard sbernhard merged commit 79d11c4 into theforeman:develop Jul 22, 2024
66 of 67 checks passed
@nadjaheitmann nadjaheitmann deleted the 37640_host_reg_loc_org branch July 22, 2024 16:15
@ekohl
Copy link
Member

ekohl commented Jul 22, 2024

Wasn't it possible to use this in Foreman with nested organizations? I know those aren't a thing in Katello but they are in vanilla Foreman.

I also wonder about the case where the user has selected an organization but not a location. Can't you still end up in the situation where the combination is invalid?

@nadjaheitmann
Copy link
Contributor Author

Wasn't it possible to use this in Foreman with nested organizations? I know those aren't a thing in Katello but they are in vanilla Foreman.

You mean that you would select a parent organization at the top and then see the child organizations in the selection? The way it was is that you did not have any restrictions to the selection at all.

I also wonder about the case where the user has selected an organization but not a location. Can't you still end up in the situation where the combination is invalid?

Actually yes: If you have a smart-proxy which is under a given organization and you have a location selected at the top which does not match. Not sure what is the best way to prevent this.

This is executed before the command is generated:
https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/registration_commands_controller.rb#L7
https://github.com/theforeman/foreman/blob/develop/app/controllers/concerns/foreman/controller/registration_commands.rb#L80

I suppose that the upper level location is used for finding the proxy. And if we want to generate a registration command for a proxy that is not found, there is an error.

@ekohl
Copy link
Member

ekohl commented Jul 23, 2024

I think it might be needed to update the list of available proxies when you change the organization and/or location.

@nadjaheitmann
Copy link
Contributor Author

At the moment, the list is updated according to what you select in the registration form but not according to what is selected outside of the form.

@sbernhard
Copy link
Contributor

sbernhard commented Jul 23, 2024

I think it might be needed to update the list of available proxies when you change the organization and/or location.

This is done. I'm currently debugging it.

This method https://github.com/theforeman/foreman/blob/develop/app/controllers/registration_commands_controller.rb#L55 returns smart_proxies which can be used for the selected HostRegistartion Formular -> Organization / Location

If you then press "Generate" this method is used to find the SmartProxy with a given ID but it validates if the SmartProxy is available at the Page Organization/Location:
https://github.com/theforeman/foreman/blob/develop/app/controllers/concerns/foreman/controller/registration_commands.rb#L81

Update:
I added some debug code here:
https://github.com/theforeman/foreman/blob/develop/app/controllers/registration_commands_controller.rb#L55

    Rails.logger.warn("params: #{params}")
    Rails.logger.warn("org: #{Organization.current.id}")
    Rails.logger.warn("loc: #{Location.current.id}")

If I select organization 1 and location 2 in the HostRegistartion from, I get the following debug output:

2024-07-23T16:59:01 [W|app|fbe5335f] params: {"organization_id"=>"1", "location_id"=>"2", "controller"=>"registration_commands", "action"=>"form_data", "locale"=>nil}
2024-07-23T16:59:01 [W|app|fbe5335f] org: 1
2024-07-23T16:59:01 [W|app|fbe5335f] loc: 2

So, the HostRegistration formular data changes the org/loc. To actually find the SmartProxy which will be used to generate the curl command, the page Org/Loc is used:
https://github.com/theforeman/foreman/blob/develop/app/controllers/concerns/foreman/controller/registration_commands.rb#L81

@sbernhard
Copy link
Contributor

Created https://projects.theforeman.org/issues/37682 for this issue.

@ekohl
Copy link
Member

ekohl commented Jul 25, 2024

Thanks, I think that properly summarizes the issues. I left a comment that when working on that we should consider reverting the behavior introduced here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants