Fixes #34206 - Fix bool params in global registration template

[stejskalleos]

No description provided.

[theforeman-bot]

Issues: #34206

[ekohl]

I wonder if @param should be normalized in code instead. For example, on line 151 @force is also used but not checked with == 'true'. So it will be inconsistent. Please at least make it consistent, but consider normalizing @force before passing it to the template.

[stejskalleos]

Condition on 151th line updated.

I was thinking about normalizing parameters before passing them to the template, but with extensions from plugins (Katello & REX) it would require quite a lot of changes in foreman core and in the plugins. Updating the global reg. template looks like the easiest fix.

[ekohl]

Slowly starting to get back and I now remembered we also have host_param_true?() which uses Foreman::Cast.to_bool(value). However, I'm not sure if it works since it's really only valid for host params. Do you think this could be used?

I'm worried about the case where it's passed via the API and actually might be a real boolean. Both are specified as real booleans in https://github.com/Katello/katello/blob/8ca9d36efa44b8f2fa0982b066e7483989407fec/app/controllers/katello/concerns/api/v2/registration_commands_controller_extensions.rb#L33-L34 and this patch may cause a regression there.

[stejskalleos]

I'm worried about the case where it's passed via the API and actually might be a real boolean. Both are specified as real booleans in app/controllers/katello/concerns/api/v2/registration_commands_controller_extensions.rb#L33-L34 and this patch may cause a regression there.

This is for generating registration command, not for the /register endpoint where we generate the template, so it should not introduce any regression issues

[ekohl]

I was thinking about normalizing parameters before passing them to the template, but with extensions from plugins (Katello & REX) it would require quite a lot of changes in foreman core and in the plugins. Updating the global reg. template looks like the easiest fix.

Since the workflow wasn't clear, I had to figure this out myself. Noting it down here for others.

What happens is that a Foreman plugin can declare allowed_registration_vars via extend_allowed_registration_vars:

foreman/app/registries/foreman/plugin.rb

Lines 609 to 611 in 5bbd164

	def extend_allowed_registration_vars(var)
	@allowed_registration_vars << var
	end

These allowed params are gathered in the controller:

foreman/app/controllers/concerns/foreman/controller/registration.rb

Lines 13 to 15 in 5bbd164

	permitted = Foreman::Plugin.all
	.map(&:allowed_registration_vars)
	.flatten.compact.uniq

The registration controller then filters the params by calling params.permit(permitted):

foreman/app/controllers/concerns/foreman/controller/registration.rb

Line 37 in 5bbd164

params.permit(permitted)

To properly cast to a bool it would need to modify params which then in turn would probably require the DSL to add casting. That's a non-trivial change.

An alternative would be to introduce a macro to do the comparison. Thinking more about it: perhaps host_param_true? should be generalized. That's a rather large change though.

Having said that, I think this could be OK to go in but I don't want to merge while our test suite is broken. Hopefully #9012 solves that.

[stejskalleos]

An alternative would be to introduce a macro to do the comparison.

I thought the same, see #9013

[ezr-ondrej]

@stejskalleos can you rebase pls, the failures should be fixed

[ekohl]

Thanks!

[ezr-ondrej]

Thanks @stejskalleos ! 👍

[ezr-ondrej]

CP 3.1: 5cb4f6c