RFC re: 798960 -- session timeout-induced bug
by Matt Wagner
Hi folks,
The other day, https://bugzilla.redhat.com/show_bug.cgi?id=794536 was
entered -- "conductor should have an inactivity timeout."
I only noticed it after it was pushed, and I remarked on IRC that it
sounded more like an irritant than a good feature. Further, I suggested
that it was a new feature, not a bug, so it didn't belong in this
sprint. It sounded like there was some consensus on at least the second
point, but more in a "going forward" sense than in relation to this
specific bugfix.
I just noticed https://bugzilla.redhat.com/show_bug.cgi?id=798960 as I
was packing up for the day, which is marked as an urgent bug that is
wreaking havoc on QE. While it's a legitimate bug, it seems like it's
directly caused by the patch in #794536.
At the risk of sounding like a curmudgeon, I would propose that we
should revert the patch for 794536. The patch itself was good, but its
inclusion was out-of-scope for the sprint, and it's now causing a
high-priority bug.
Thoughts?
-- Matt
12 years, 1 month
[PATCH conductor] BZ 798515 - Render correct error message when deleting user
by Matt Wagner
We were displaying the entirely wrong flash message and passing in the wrong
argument for it, causing an interpolation error.
Resolves https://bugzilla.redhat.com/show_bug.cgi?id=798515
---
src/app/controllers/users_controller.rb | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/users_controller.rb b/src/app/controllers/users_controller.rb
index 493fb4d..93918bc 100644
--- a/src/app/controllers/users_controller.rb
+++ b/src/app/controllers/users_controller.rb
@@ -139,7 +139,7 @@ class UsersController < ApplicationController
require_privilege(Privilege::MODIFY, User)
user = User.find(params[:id])
if user == current_user
- flash[:warning] = "#{t('users.flash.warning.not_delete', :login => "#{user.login}")}"
+ flash[:warning] = "#{t('users.flash.warning.not_delete_same_user', :login => "#{user.login}")}"
else
user.destroy
flash[:notice] = t"users.flash.notice.deleted"
--
1.7.7.6
12 years, 1 month
[PATCH multi 0/2] BZ #761279 - Better handling of import errors
by Matt Wagner
This is really a speculative fix, as I could not reproduce the described issue. It appears that the most likely explanation is an image which failed to import fully, so I modified the code slightly to clean up better in that case.
I get 3 failing Cucumber tests with or without this patch.
-- Matt
12 years, 1 month
[PATCH] BZ 798347 added breadcrumb back to realms index
by Tzu-Mainn Chen
---
src/app/views/realms/show.html.haml | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/app/views/realms/show.html.haml b/src/app/views/realms/show.html.haml
index b3ef52d..133731f 100644
--- a/src/app/views/realms/show.html.haml
+++ b/src/app/views/realms/show.html.haml
@@ -2,6 +2,9 @@
%header.admin-page-header
%h1{:class => controller.controller_name}= @title
#obj_actions.button-container
+ .return_to
+ =t'return_to'
+ = link_to t('realms.realms'), realms_path
- if check_privilege(Privilege::CREATE, Realm)
= link_to t('realms.new_realm'), new_realm_url, :class => 'button primary', :id => 'new_realm_button'
.button-group
--
1.7.6.5
12 years, 1 month
[PATCH conductor 0/1] #799050 - Fix nil.[] reference
by Matt Wagner
The effect of this patch is basically just to move a block of code up a little bit, outside of a conditional. Otherwise, the "format.json" section blows up.
A simpler test than described in the BZ: /conductor/pools.json should give you an error without this patch, and a JSON listing of pools with the patch.
-- Matt
12 years, 1 month
[PATCH] BZ 784479 add range validation to priority
by Tzu-Mainn Chen
---
src/app/models/provider_account.rb | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index eff59f9..49f8e67 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -66,7 +66,7 @@ class ProviderAccount < ActiveRecord::Base
validate :validate_presence_of_credentials
validate :validate_credentials
validate :validate_unique_username
- validates :priority, :numericality => { :only_integer => true}, :allow_blank => true
+ validates :priority, :numericality => { :only_integer => true, :greater_than_or_equal_to => -100000, :less_than_or_equal_to => 100000}, :allow_blank => true
before_create :populate_profiles_and_validate
after_create :populate_realms_and_validate
--
1.7.6.5
12 years, 1 month
[PATCH conductor 1/6] bug 788148: Take environment into account for permission inheritance
by Scott Seago
https://bugzilla.redhat.com/show_bug.cgi?id=788148
Add 'pool family' to permission inheritance for various types. In addition, refactor permission queries to reduce the join complexity:
1) denormalize 'pool family' association into deployments, instances, etc. -- once created it never changes, so this reduces the number of joins for permissions
2) pre-generate a list of roles that include the desired privilege -- this keeps the permission queries from having to join permissions, roles, and privileges tables (all 3 tables) multiple times in a single query. Sometimes two queries are better than one. This resulted in significant speedup in queries where the user was granted permissions via the pool family.
---
src/app/models/catalog.rb | 27 +++++++++++++++
src/app/models/deployable.rb | 34 ++++++++++++++++++-
src/app/models/deployment.rb | 20 ++++++++---
src/app/models/instance.rb | 23 ++++++++-----
src/app/models/permissioned_object.rb | 8 ++--
src/app/models/pool.rb | 19 +++++++++++
src/app/models/provider_account.rb | 7 +---
.../20120227150102_add_pool_family_to_instance.rb | 32 ++++++++++++++++++
8 files changed, 144 insertions(+), 26 deletions(-)
create mode 100644 src/db/migrate/20120227150102_add_pool_family_to_instance.rb
diff --git a/src/app/models/catalog.rb b/src/app/models/catalog.rb
index 3472c59..307c4dd 100644
--- a/src/app/models/catalog.rb
+++ b/src/app/models/catalog.rb
@@ -32,6 +32,7 @@ class Catalog < ActiveRecord::Base
include PermissionedObject
belongs_to :pool
+ belongs_to :pool_family
has_many :catalog_entries, :dependent => :destroy
has_many :deployables, :through => :catalog_entries
has_many :permissions, :as => :permission_object, :dependent => :destroy,
@@ -39,12 +40,34 @@ class Catalog < ActiveRecord::Base
:order => "permissions.id ASC"
before_destroy :destroy_deployables_related_only_to_self
+ before_create :set_pool_family
validates_presence_of :pool
validates_presence_of :name
validates_uniqueness_of :name
validates_length_of :name, :maximum => 1024
+ def object_list
+ super + [pool, pool_family]
+ end
+ class << self
+ alias orig_list_for_user_include list_for_user_include
+ alias orig_list_for_user_conditions list_for_user_conditions
+ end
+
+ def self.list_for_user_include
+ orig_list_for_user_include + [ {:pool => :permissions},
+ {:pool_family => :permissions} ]
+ end
+
+ def self.list_for_user_conditions
+ "(#{orig_list_for_user_conditions}) or
+ (permissions_pools.user_id=:user and
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
+ end
+
PRESET_FILTERS_OPTIONS = [
{:title => I18n.t("catalogs.preset_filters.belongs_to_default_pool"), :id => "belongs_to_default_pool", :query => includes(:pool).where("pools.name" => "Default")}
]
@@ -53,6 +76,10 @@ class Catalog < ActiveRecord::Base
deployables.each {|d| d.destroy if d.catalogs.count == 1}
end
+ def set_pool_family
+ self[:pool_family_id] = pool.pool_family_id
+ end
+
private
def self.apply_search_filter(search)
diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index c77f094..ad537a7 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -34,9 +34,11 @@ class Deployable < ActiveRecord::Base
:order => "permissions.id ASC"
has_many :catalog_entries, :dependent => :delete_all
has_many :catalogs, :through => :catalog_entries
+ belongs_to :pool_family
belongs_to :owner, :class_name => "User", :foreign_key => "owner_id"
after_create "assign_owner_roles(owner)"
+ before_create :set_pool_family
scope :without_catalog, lambda {
deployable_ids_in_association = CatalogEntry.select(:deployable_id).map(&:deployable_id)
@@ -45,6 +47,30 @@ class Deployable < ActiveRecord::Base
PRESET_FILTERS_OPTIONS = []
+ def object_list
+ super + catalogs + catalogs.collect{|c| c.pool} + [pool_family]
+ end
+ class << self
+ alias orig_list_for_user_include list_for_user_include
+ alias orig_list_for_user_conditions list_for_user_conditions
+ end
+
+ def self.list_for_user_include
+ orig_list_for_user_include + [ {:catalogs => [:permissions,
+ {:pool => :permissions}]},
+ {:pool_family => :permissions} ]
+ end
+
+ def self.list_for_user_conditions
+ "(#{orig_list_for_user_conditions}) or
+ (permissions_catalogs.user_id=:user and
+ permissions_catalogs.role_id in (:role_ids)) or
+ (permissions_pools.user_id=:user and
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
+ end
+
def valid_deployable_xml?
begin
deployable_xml = DeployableXML.new(xml)
@@ -133,12 +159,12 @@ class Deployable < ActiveRecord::Base
:assembly => assembly.name,
:uuid => assembly.image_id)
else
- if image.environment != catalogs.first.pool.pool_family.name
+ if image.environment != pool_family.name
deployable_errors << I18n.t("deployables.flash.error.wrong_environment",
:assembly => assembly.name,
:uuid => assembly.image_id,
:wrong_env => image.environment,
- :environment => catalogs.first.pool.pool_family.name)
+ :environment => pool_family.name)
end
images << image
assembly_hash[:build_and_target_uuids] = get_build_and_target_uuids(image)
@@ -186,6 +212,10 @@ class Deployable < ActiveRecord::Base
(latest_build ? latest_build.target_images.collect { |ti| ti.uuid} : nil)]
end
+ def set_pool_family
+ self[:pool_family_id] = catalogs.first.pool.pool_family_id
+ end
+
private
def self.apply_search_filter(search)
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index ec66fdb..c0c88b2 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -42,6 +42,7 @@ class Deployment < ActiveRecord::Base
end
belongs_to :pool
+ belongs_to :pool_family
has_many :instances, :dependent => :destroy
@@ -71,6 +72,8 @@ class Deployment < ActiveRecord::Base
before_create :inject_launch_parameters
before_create :generate_uuid
before_save :replace_special_characters_in_name
+ before_create :set_pool_family
+
USER_MUTABLE_ATTRS = ['name']
STATE_MIXED = "mixed"
@@ -105,7 +108,7 @@ class Deployment < ActiveRecord::Base
end
def object_list
- super << pool
+ super + [pool, pool_family]
end
class << self
alias orig_list_for_user_include list_for_user_include
@@ -113,16 +116,16 @@ class Deployment < ActiveRecord::Base
end
def self.list_for_user_include
- includes = orig_list_for_user_include
- includes << { :pool => {:permissions => {:role => :privileges}}}
- includes
+ orig_list_for_user_include + [ {:pool => :permissions},
+ {:pool_family => :permissions} ]
end
def self.list_for_user_conditions
"(#{orig_list_for_user_conditions}) or
(permissions_pools.user_id=:user and
- privileges_roles.target_type=:target_type and
- privileges_roles.action=:action)"
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
end
def get_action_list(user=nil)
@@ -596,4 +599,9 @@ class Deployment < ActiveRecord::Base
def replace_special_characters_in_name
name.gsub!(/[^a-zA-Z0-9]+/, '-')
end
+
+ def set_pool_family
+ self[:pool_family_id] = pool.pool_family_id
+ end
+
end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index 13ff13b..e03052a 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -70,6 +70,7 @@ class Instance < ActiveRecord::Base
@@per_page = 15
belongs_to :pool
+ belongs_to :pool_family
belongs_to :provider_account
belongs_to :deployment
@@ -100,6 +101,7 @@ class Instance < ActiveRecord::Base
validates_length_of :name, :maximum => 1024
before_create :generate_uuid
+ before_create :set_pool_family
STATE_NEW = "new"
STATE_PENDING = "pending"
@@ -144,7 +146,7 @@ class Instance < ActiveRecord::Base
USER_MUTABLE_ATTRS = ['name']
def object_list
- super + [pool, deployment]
+ super + [deployment, pool, pool_family]
end
class << self
alias orig_list_for_user_include list_for_user_include
@@ -152,20 +154,19 @@ class Instance < ActiveRecord::Base
end
def self.list_for_user_include
- includes = orig_list_for_user_include
- includes << { :pool => {:permissions => {:role => :privileges}},
- :deployment => {:permissions => {:role => :privileges}}}
- includes
+ orig_list_for_user_include + [ {:deployment => :permissions},
+ {:pool => :permissions},
+ {:pool_family => :permissions} ]
end
def self.list_for_user_conditions
"(#{orig_list_for_user_conditions}) or
(permissions_deployments.user_id=:user and
- privileges_roles.target_type=:target_type and
- privileges_roles.action=:action) or
+ permissions_deployments.role_id in (:role_ids)) or
(permissions_pools.user_id=:user and
- privileges_roles_2.target_type=:target_type and
- privileges_roles_2.action=:action)"
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
end
def get_action_list(user=nil)
@@ -511,6 +512,10 @@ class Instance < ActiveRecord::Base
self[:uuid] = UUIDTools::UUID.timestamp_create.to_s
end
+ def set_pool_family
+ self[:pool_family_id] = pool.pool_family_id
+ end
+
def do_operation(user, operation)
@task = self.queue_action(user, operation)
unless @task
diff --git a/src/app/models/permissioned_object.rb b/src/app/models/permissioned_object.rb
index 36a2f25..c534261 100644
--- a/src/app/models/permissioned_object.rb
+++ b/src/app/models/permissioned_object.rb
@@ -59,20 +59,20 @@ module PermissionedObject
self.name.constantize
end
def self.list_for_user_include
- [{:permissions => {:role => :privileges}}]
+ [:permissions]
end
def self.list_for_user_conditions
"permissions.user_id=:user and
- privileges.target_type=:target_type and
- privileges.action=:action"
+ permissions.role_id in (:role_ids)"
end
def self.list_for_user(user, action, target_type=self.default_privilege_target_type)
return where("1=0") if user.nil? or action.nil? or target_type.nil?
if BasePermissionObject.general_permission_scope.has_privilege(user, action, target_type)
scoped
else
+ role_ids = Role.includes(:privileges).where("privileges.target_type" => target_type, "privileges.action" => action).collect {|r| r.id}
include_clause = self.list_for_user_include
- conditions_hash = {:user => user.id, :target_type => target_type.name, :action => action}
+ conditions_hash = {:user => user.id, :target_type => target_type.name, :action => action, :role_ids => role_ids}
conditions_str = self.list_for_user_conditions
includes(include_clause).where(conditions_str, conditions_hash)
end
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index be3d156..3fce49b 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -137,6 +137,25 @@ class Pool < ActiveRecord::Base
{:title => I18n.t("pools.preset_filters.with_stopped_instances"), :id => "with_stopped_instances", :query => includes(:deployments => :instances).where("instances.state" => "stopped")}
]
+ def object_list
+ super + [pool_family]
+ end
+ class << self
+ alias orig_list_for_user_include list_for_user_include
+ alias orig_list_for_user_conditions list_for_user_conditions
+ end
+
+ def self.list_for_user_include
+ orig_list_for_user_include + [ {:pool_family => :permissions} ]
+ end
+
+ def self.list_for_user_conditions
+ "(#{orig_list_for_user_conditions}) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
+ end
+
+
private
def self.apply_search_filter(search)
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index eff59f9..1e0ed69 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -115,16 +115,13 @@ class ProviderAccount < ActiveRecord::Base
end
def self.list_for_user_include
- includes = orig_list_for_user_include
- includes << { :provider => {:permissions => {:role => :privileges}}}
- includes
+ orig_list_for_user_include + [{ :provider => :permissions}]
end
def self.list_for_user_conditions
"(#{orig_list_for_user_conditions}) or
(permissions_providers.user_id=:user and
- privileges_roles.target_type=:target_type and
- privileges_roles.action=:action)"
+ permissions_providers.role_id in (:role_ids))"
end
def destroyable?
diff --git a/src/db/migrate/20120227150102_add_pool_family_to_instance.rb b/src/db/migrate/20120227150102_add_pool_family_to_instance.rb
new file mode 100644
index 0000000..00848ba
--- /dev/null
+++ b/src/db/migrate/20120227150102_add_pool_family_to_instance.rb
@@ -0,0 +1,32 @@
+class AddPoolFamilyToInstance < ActiveRecord::Migration
+ def self.up
+ add_column :instances, :pool_family_id, :integer
+ add_column :deployments, :pool_family_id, :integer
+ add_column :catalogs, :pool_family_id, :integer
+ add_column :deployables, :pool_family_id, :integer
+
+ Deployment.all.each do |deployment|
+ deployment.pool_family_id = deployment.pool.pool_family_id
+ deployment.save!
+ end
+ Instance.all.each do |instance|
+ instance.pool_family_id = instance.pool.pool_family_id
+ instance.save!
+ end
+ Catalog.all.each do |catalog|
+ catalog.pool_family_id = catalog.pool.pool_family_id
+ catalog.save!
+ end
+ Deployable.all.each do |deployable|
+ deployable.pool_family_id = deployable.catalogs.first.pool_family_id unless deployable.catalogs.empty?
+ deployable.save!
+ end
+ end
+
+ def self.down
+ remove_column :instances, :pool_family_id
+ remove_column :deployments, :pool_family_id
+ remove_column :catalogs, :pool_family_id
+ remove_column :deployables, :pool_family_id
+ end
+end
--
1.7.6.4
12 years, 1 month