Bug #17157

inconsitent mapping of host owner

Added by Martin Schurz 7 months ago. Updated 6 months ago.

Status:Closed
Priority:High
Assigned To:Kavita Gaikwad
Category:Organizations and Locations
Target version:Team Anurag Iteration 6
Difficulty:easy Bugzilla link:
Found in release: Pull request:https://github.com/theforeman/foreman/pull/4069
Story points-
Velocity based estimate-
Release1.14.0Release relationshipAuto

Description

A host owner can either be a user or a usergroup, this is implemented in the database with two columns.

MariaDB [foreman]> desc hosts;
+----------------------+--------------+------+-----+---------+----------------+
| Field                | Type         | Null | Key | Default | Extra          |
+----------------------+--------------+------+-----+---------+----------------+
...
| owner_id             | int(11)      | YES  |     | NULL    |                |
| owner_type           | varchar(255) | YES  |     | NULL    |                |
...
+----------------------+--------------+------+-----+---------+----------------+

The owner_type checking seems to be inconsistent in the ruby code (not everywhere implemented where needed). I found one case where I can produce an obvious error, while tracking this error through the code I found some other possible errors.

obvious error

steps to reproduce:
  • need to have organizations enabled
  • create a sampe organization
  • create a user with a specific user id (eg. 25)
  • create a group with the same id
  • set the group as owner for a host
  • set the sample organzation for the host
found error:
  • navigate to Administer->Users
  • open the user
  • open the Organizations part
  • the user has the organization assigned and is is grey (Message: "this is used by a host")

I know the code error should be in app/models/concerns/taxonomix.rb:get_taxonomy_ids, but my understanding of ruby is to limited to code a solution for this. :(

possible errors
in app/models/user.rb:

has_many :direct_hosts,      :class_name => 'Host',    :as => :owner

I think this is missing the where condition to check for the correct type.

This should be:

has_many :direct_hosts,      -> { where(:owner_type => 'User') }, :class_name => 'Host',    :as => :owner

in app/models/usergroup.rb

has_many_hosts :as => :owner

Same as above.

I can generate a pull request for the simple things, but I need help to create a correct patch for taxonomix.rb

Associated revisions

Revision dd17d270
Added by Kavita Gaikwad 6 months ago

fixes #17157 - inconsistent mapping of host owner

There is user_group which is assigned to one host as owner
with id=x. For organization assigned to user with same id=x,
on edit page showing a grey color with message
"this is used by a host".
With this commit, it should not show message to organization
assigned to user with same id as of user_groups.

Also, refactored the association code for
(User, Usergroup) -> UserRoles.

History

#1 Updated by Ohad Levy 7 months ago

first, thanks for the detalied response!

secondly, just commenting that:

has_many :direct_hosts,      :class_name => 'Host',    :as => :owner

is actually correct, the :as keyword defines a polymorphic relationship (how rails calls this kind of db layout) and does the correct SQL

see http://guides.rubyonrails.org/association_basics.html#polymorphic-associations

#2 Updated by Martin Schurz 7 months ago

Ohad Levy wrote:

is actually correct, the :as keyword defines a polymorphic relationship (how rails calls this kind of db layout) and does the correct SQL

see http://guides.rubyonrails.org/association_basics.html#polymorphic-associations

oh thanks, now I understand this.
I was looking at the relation (User, Usergroup) -> Role and this is modeled with the where condition, maybe this sould be refactored for the sake of consistency

#3 Updated by Kavita Gaikwad 6 months ago

  • Assigned To set to Kavita Gaikwad
  • Target version set to Team Anurag Iteration 6

#4 Updated by The Foreman Bot 6 months ago

  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/4069 added

#5 Updated by Kavita Gaikwad 6 months ago

  • Status changed from Ready For Testing to Closed
  • % Done changed from 0 to 100

#6 Updated by Dominic Cleal 6 months ago

  • Release set to 1.14.0

Also available in: Atom PDF