Fixes #36753 - Only allow coherent default CVE in global registration form #10764

jeremylenz · 2023-10-09T20:04:40Z

What are the changes introduced in this pull request?

Now that content views and lifecycle environments are assigned as a single unit (CVE / Content View Environment), it doesn't make sense to be able to select lifecycle environment separately. Also, the web UI was allowing you to select a non-library LCE and attempt to use it with the 'Default Organization View' content view, which won't work. So:

Remove 'Lifecycle Environment' field from global registration form
Add a Rails validator preventing content view environments from using a non-default lifecycle environment with the Default Organization View.

Considerations taken when implementing this change?

It's always been the case that you can't create a content view environment with Default Organization View and a non-library LCE. This just improves the error messaging and UI.

What are the testing steps for this pull request?

Before checking out the PR:

Create an activation key assigning Library environment and Default Organization View
Create at least one lifecycle environment
Hosts > Register Host > Generate a registration command with the following properties:
- Insecure
- Activation keys: <the one you created>
- Lifecycle Environment: <the one you created>
- Force: true (more convenient if you're using an existing host)

Run the registration command on the host. You'll get

The system has been registered with ID: 8b3a0960-9dbe-4428-9ac5-661e1ca892c3
The registered system name is: rhel8.fedora.example.com
ERROR: standard_error
PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint index_cve_cp_id
DETAIL:  Key (cp_id)=(64c2cce3716a5bf67b0413fac95e5daa) already exists.

Now, check out the PR and use the same generated registration command (this is where the "force" comes in handy :)

Now you'll get

The system has been registered with ID: 895e054c-9e47-42b2-a690-9c94b65bea1d
The registered system name is: rhel8.fedora.example.com
ERROR: Validation failed: Lifecycle environment 'dev2' cannot be used with content view 'Default Organization View'

Now go to the web UI - Hosts > Register Host

Verify that the 'Lifecycle Environment' field is now gone
verify that generating commands still works as expected

theforeman-bot · 2023-10-09T20:04:42Z

Issues: #36753

jeremylenz · 2023-10-09T20:11:26Z

app/lib/katello/validators/content_view_environment_validator.rb

@@ -1,5 +1,6 @@
 module Katello
  module Validators
+    # used by activation key and content facet


I first said to myself, Why isn't it already hitting this? but then I realized this validator is only used by ActivationKey and ContentFacet records, not ContentViewEnvironment itself. So I made a separate one for this case.

in global registration form - remove 'Lifecycle Environment' field from global registration form - add Rails validator preventing content view environments from using a non-default lifecycle environment with the Default Organization View

jeremylenz · 2023-10-10T13:38:14Z

[test katello]

jeremylenz · 2023-10-10T14:03:47Z

[test katello]

jeremylenz · 2023-10-13T13:18:18Z

[test katello]

ianballou · 2023-10-16T20:38:04Z

I hit another bug, unrelated(?), but good to note:

Validation failed: Host centos9-stream.example.com: Cannot add content view environment to content facet. The host's content source 'centos8-proxy-devel.example.com' does not sync lifecycle environment 'Library'. (HTTP error code 422: Unprocessable Entity)

My host exists and was previously given a different content source... now it seems passing no content source is not defaulting to using the primary smart proxy.

I also hit this when trying to assign a smart proxy that doesn't sync library

Host [centos9-stream.example.com] successfully configured.
Validation failed: Content view environment content facets is invalid (HTTP error code 422: Unprocessable Entity)

ianballou · 2023-10-16T20:40:13Z

Aside from my comment above it works when not trying to change the content source! I just need to review the code a bit more to look for corner cases.

jeremylenz · 2023-10-17T17:07:29Z

My host exists and was previously given a different content source.

ah, so you were checking the Force checkbox? That's probably worth a redmine issue. Katello should clear out a host's content source when unregistering.

jeremylenz · 2023-10-17T17:27:59Z

I also hit this when trying to assign a smart proxy that doesn't sync library

This is intentional (#10524), but might be nice to not use the Rails default error message there.

ianballou · 2023-10-17T18:18:53Z

ah, so you were checking the Force checkbox? That's probably worth a redmine issue. Katello should clear out a host's content source when unregistering.

Made a redmine for it: https://projects.theforeman.org/issues/36840

ianballou

Pretty much looking good! Just some small comments.

ianballou · 2023-10-17T18:30:12Z

app/lib/katello/validators/content_view_environment_coherent_default_validator.rb

+        #support lifecycle_environment_id for foreman models
+        environment_id = record.respond_to?(:lifecycle_environment_id) ? record.lifecycle_environment_id : record.environment_id
+        if record.content_view_id
+          view = ContentView.where(:id => record.content_view_id).first


Perhaps a bit clearer:

Suggested change

view = ContentView.where(:id => record.content_view_id).first

view = ContentView.find(record.content_view_id)

I realized this was copy/pasted from the other validator, so up to you if you wanna take the suggestions.

.where ... .first will give you nil if it doesn't find it, while .find will error.

I guess find_by(id: 42) will also give you nil, but it seems like we use the .where ... .first pattern a lot anyway 🤷

works for me

app/lib/katello/validators/content_view_environment_coherent_default_validator.rb

ianballou · 2023-10-17T18:34:08Z

app/lib/katello/validators/content_view_environment_coherent_default_validator.rb

+      def validate(record)
+        #support lifecycle_environment_id for foreman models
+        environment_id = record.respond_to?(:lifecycle_environment_id) ? record.lifecycle_environment_id : record.environment_id
+        if record.content_view_id


Would it be possible for a record to have an LCE but not a CV? I feel like everything with an LCE ID should also have a content view ID, but I'm not certain.

... I suppose it doesn't really matter because this validator is used only for content view environments, and those should always have an environment. I guess in that case the respond_to? check is a bit irrelevant, but again, maybe there is some benefit in keeping it exactly the same as the other related validator.

Okay, took me a minute to remember, but I remembered the reason 😄

It's about making sure we raise the correct errors.

Content view environments already have these validations:

validates :environment_id, uniqueness: {scope: :content_view_id}, presence: true validates :content_view_id, presence: true

Omitting one of them would normally cause an error like environment_id must be specified, because of the presence: true validation.

But let's say these checks aren't there. You might get a NilClass error, or if we fix that with safe navigation, you'd get both the correct and incorrect validation error:

environment_id must be specified, Lifecycle environment '' cannot be used with content view 'myCV'

Or, if we switched back to find, it'd be even worse, with

ActiveRecord::RecordNotFound: Couldn't find Katello::ContentViewVersion with 'id'=42

makes sense!

ianballou

Looking good 👍

Gauravtalreja1 · 2023-12-07T20:37:07Z

@jeremylenz I'm still seeing lifecycle-environment option in hammer, so just wondering if do you've any PR open for removing this for Hammer/API too?

# hammer host-registration generate-command --help | grep lifecycle
 --lifecycle-environment[-id] VALUE/NUMBER Lifecycle environment for the host.

jeremylenz · 2023-12-07T20:47:59Z

I'm still seeing lifecycle-environment option in hammer, so just wondering if do you've any PR open for removing this for Hammer/API too?

@Gauravtalreja1 Good catch, this only removed it from the web UI and added the validations.

Now that we have the validation, we could leave the option in hammer and you'd get the error in the PR description if you try to register the host while assigning an invalid LCE. Or we could remove it from hammer for consistency. I'll leave it up to you whether to open a BZ.

Gauravtalreja1 · 2023-12-08T08:58:39Z

@jeremylenz Thanks for the confirming, opened the BZ 2253618 (CC'ed you) for its removal for consistency.

pr-processor bot added the Not yet reviewed label Oct 9, 2023

jeremylenz commented Oct 9, 2023

View reviewed changes

Fixes #36753 - Only allow coherent default CVE

73b586e

in global registration form - remove 'Lifecycle Environment' field from global registration form - add Rails validator preventing content view environments from using a non-default lifecycle environment with the Default Organization View

jeremylenz force-pushed the 36753-only-coherent-cves-from-now-on branch from 21dc7a8 to 73b586e Compare October 9, 2023 20:14

ianballou reviewed Oct 17, 2023

View reviewed changes

ianballou approved these changes Oct 18, 2023

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 18, 2023

jeremylenz merged commit 337b5d6 into Katello:master Oct 18, 2023
5 checks passed

Gauravtalreja1 mentioned this pull request Dec 7, 2023

Remove LCE field from HostRegisterView SatelliteQE/airgun#1069

Merged

Gauravtalreja1 mentioned this pull request Dec 8, 2023

Remove LCE option from register helper and assert in form test SatelliteQE/robottelo#13341

Merged

This was referenced Dec 12, 2023

[6.15.z] Remove LCE option from register helper and assert in form test SatelliteQE/robottelo#13385

Merged

[6.15.z] Remove LCE field from HostRegisterView SatelliteQE/airgun#1075

Merged

chris1984 mentioned this pull request Jan 11, 2024

Fixes #37044 - Remove LCE option from registration API #10844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36753 - Only allow coherent default CVE in global registration form #10764

Fixes #36753 - Only allow coherent default CVE in global registration form #10764

jeremylenz commented Oct 9, 2023 •

edited

Loading

theforeman-bot commented Oct 9, 2023

jeremylenz Oct 9, 2023

jeremylenz commented Oct 10, 2023

jeremylenz commented Oct 10, 2023

jeremylenz commented Oct 13, 2023

ianballou commented Oct 16, 2023 •

edited

Loading

ianballou commented Oct 16, 2023

jeremylenz commented Oct 17, 2023

jeremylenz commented Oct 17, 2023

ianballou commented Oct 17, 2023

ianballou left a comment

ianballou Oct 17, 2023

ianballou Oct 17, 2023

jeremylenz Oct 18, 2023

jeremylenz Oct 18, 2023

ianballou Oct 18, 2023

ianballou Oct 17, 2023

ianballou Oct 18, 2023

jeremylenz Oct 18, 2023 •

edited

Loading

ianballou Oct 18, 2023

ianballou left a comment

Gauravtalreja1 commented Dec 7, 2023

jeremylenz commented Dec 7, 2023

Gauravtalreja1 commented Dec 8, 2023

	view = ContentView.where(:id => record.content_view_id).first
	view = ContentView.find(record.content_view_id)

Fixes #36753 - Only allow coherent default CVE in global registration form #10764

Fixes #36753 - Only allow coherent default CVE in global registration form #10764

Conversation

jeremylenz commented Oct 9, 2023 • edited Loading

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

theforeman-bot commented Oct 9, 2023

Choose a reason for hiding this comment

jeremylenz commented Oct 10, 2023

jeremylenz commented Oct 10, 2023

jeremylenz commented Oct 13, 2023

ianballou commented Oct 16, 2023 • edited Loading

ianballou commented Oct 16, 2023

jeremylenz commented Oct 17, 2023

jeremylenz commented Oct 17, 2023

ianballou commented Oct 17, 2023

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

Gauravtalreja1 commented Dec 7, 2023

jeremylenz commented Dec 7, 2023

Gauravtalreja1 commented Dec 8, 2023

jeremylenz commented Oct 9, 2023 •

edited

Loading

ianballou commented Oct 16, 2023 •

edited

Loading

jeremylenz Oct 18, 2023 •

edited

Loading