[PATCH conductor] Refactor SessionEntity model to work properly with API
Scott Seago
sseago at redhat.com
Thu Jun 7 18:39:32 UTC 2012
On 06/07/2012 01:47 PM, Tzu-Mainn Chen wrote:
> ACK, passes tests, permissions seem to work.
>
> Mainn
Pushed
> ----- Original Message -----
>> From: "Scott Seago"<sseago at redhat.com>
>> To: aeolus-devel at lists.fedorahosted.org
>> Cc: "Scott Seago"<sseago at redhat.com>
>> Sent: Thursday, June 7, 2012 9:58:16 AM
>> Subject: [PATCH conductor] Refactor SessionEntity model to work properly with API
>>
>> The direct reference to the Session class in SessionEntity was
>> causing
>> problems for API calls, since the session object wasn't yet in the db
>> (but the in-memory session_id and session data was there).
>>
>> On further investigation, it's considered bad design to reference the
>> session class directly anyway, as it exposes too many impl details
>> of the session to the other models, even if it did work here.
>>
>> I've refactored it to reference the session_id string in the
>> SessionEntity
>> model instead, which appears to work now for both API and Web UI.
>> ---
>> src/app/controllers/application_controller.rb | 18 ++-----
>> src/app/models/permissioned_object.rb | 6 +-
>> src/app/models/session_entity.rb | 13 ++---
>> src/config/initializers/session.rb | 23 ---------
>> src/config/initializers/warden.rb | 3 +
>> ...06200000_fix_session_id_for_session_entities.rb | 51
>> ++++++++++++++++++++
>> src/spec/models/deployment_spec.rb | 24 +++++----
>> src/spec/models/permission_spec.rb | 23 +++++----
>> src/spec/services/registration_service_spec.rb | 5 +-
>> src/spec/spec_helper.rb | 8 ++--
>> 10 files changed, 99 insertions(+), 75 deletions(-)
>> delete mode 100644 src/config/initializers/session.rb
>> create mode 100644
>> src/db/migrate/20120606200000_fix_session_id_for_session_entities.rb
>>
>> diff --git a/src/app/controllers/application_controller.rb
>> b/src/app/controllers/application_controller.rb
>> index 6f280be..aed6b7e 100644
>> --- a/src/app/controllers/application_controller.rb
>> +++ b/src/app/controllers/application_controller.rb
>> @@ -179,9 +179,10 @@ class ApplicationController<
>> ActionController::Base
>> def http_auth_user
>> return unless request.authorization&& request.authorization =~
>> /^Basic (.*)/m
>> authenticate!(:scope => :api)
>> - request.session_options = request.session_options.dup
>> + frozen = request.session_options.frozen?
>> + request.session_options = request.session_options.dup if frozen
>> request.session_options[:expire_after] = 2.minutes
>> - request.session_options.freeze
>> + request.session_options.freeze if frozen
>> # we use :api scope for authentication to avoid saving session.
>> # But it's handy to set authenticated user in default scope, so
>> we
>> # can use current_user, instead of current_user(:api)
>> @@ -190,18 +191,7 @@ class ApplicationController<
>> ActionController::Base
>> end
>>
>> def current_session
>> - @current_session ||= ActiveRecord::SessionStore::Session.
>> - find_by_session_id(request.session_options[:id])
>> - # FIXME: I shouldn't have to reload sessions here, but for some
>> reason
>> - # without reloading it wasn't working for non-admin permissions.
>> - # If we ever need to add session_entities for _other_ users to
>> the
>> - # current session, this won't work
>> - if @current_session and (!@current_session.session_entities.any?
>> or
>> -
>> @current_session.session_entities.first.user
>> != current_user)
>> - SessionEntity.update_session(@current_session, current_user)
>> - @current_session.reload
>> - end
>> - @current_session
>> + @current_session ||= request.session_options[:id]
>> end
>>
>> def require_user
>> diff --git a/src/app/models/permissioned_object.rb
>> b/src/app/models/permissioned_object.rb
>> index 4f881ee..b657054 100644
>> --- a/src/app/models/permissioned_object.rb
>> +++ b/src/app/models/permissioned_object.rb
>> @@ -26,7 +26,7 @@ module PermissionedObject
>> privileges.target_type=:target_type and
>> privileges.action=:action",
>> { :user => user.id,
>> - :session => session.id,
>> + :session => session,
>> :target_type => target_type.name,
>> :action => action}]).any?
>> return true
>> @@ -39,7 +39,7 @@ module PermissionedObject
>> privileges.target_type=:target_type and
>> privileges.action=:action",
>> { :user => user.id,
>> - :session => session.id,
>> + :session => session,
>> :target_type => target_type.name,
>> :action => action}]).any?
>> end
>> @@ -129,7 +129,7 @@ module PermissionedObject
>> privileges.target_type=:target_type and
>> privileges.action=:action",
>> {:user => user.id,
>> - :session => session.id,
>> + :session => session,
>> :target_type => target_type.name,
>> :action => action})
>> end
>> diff --git a/src/app/models/session_entity.rb
>> b/src/app/models/session_entity.rb
>> index e7bf869..8a0b85a 100644
>> --- a/src/app/models/session_entity.rb
>> +++ b/src/app/models/session_entity.rb
>> @@ -16,7 +16,6 @@
>>
>> class SessionEntity< ActiveRecord::Base
>> belongs_to :user
>> - belongs_to :session, :class_name =>
>> "ActiveRecord::SessionStore::Session"
>> belongs_to :entity
>>
>> validates_presence_of :user_id
>> @@ -24,22 +23,22 @@ class SessionEntity< ActiveRecord::Base
>> validates_presence_of :entity_id
>> validates_uniqueness_of :entity_id, :scope => [:user_id,
>> :session_id]
>>
>> - def self.update_session(session, user)
>> + def self.update_session(session_id, user)
>> self.transaction do
>> # skips callbacks, which should be fine here
>> - self.delete_all(:session_id => session.id)
>> - self.add_to_session(session, user)
>> + self.delete_all(:session_id => session_id)
>> + self.add_to_session(session_id, user)
>> end
>> end
>>
>> - def self.add_to_session(session, user)
>> + def self.add_to_session(session_id, user)
>> return unless user
>> # create mapping for user-level permissions
>> - SessionEntity.create!(:session => session, :user => user,
>> + SessionEntity.create!(:session_id => session_id, :user => user,
>> :entity => user.entity)
>> # create mappings for local groups
>> user.user_groups.each do |ug|
>> - SessionEntity.create!(:session => session, :user => user,
>> + SessionEntity.create!(:session_id => session_id, :user =>
>> user,
>> :entity => ug.entity)
>> end
>> # TODO: add entities for ldap groups
>> diff --git a/src/config/initializers/session.rb
>> b/src/config/initializers/session.rb
>> deleted file mode 100644
>> index 6d82968..0000000
>> --- a/src/config/initializers/session.rb
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -#
>> -# Copyright 2012 Red Hat, Inc.
>> -#
>> -# Licensed under the Apache License, Version 2.0 (the "License");
>> -# you may not use this file except in compliance with the License.
>> -# You may obtain a copy of the License at
>> -#
>> -# http://www.apache.org/licenses/LICENSE-2.0
>> -#
>> -# Unless required by applicable law or agreed to in writing,
>> software
>> -# distributed under the License is distributed on an "AS IS"
>> BASIS,
>> -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> implied.
>> -# See the License for the specific language governing permissions
>> and
>> -# limitations under the License.
>> -#
>> -
>> -module ActiveRecord
>> - class SessionStore
>> - class Session
>> - has_many :session_entities, :dependent => :destroy
>> - end
>> - end
>> -end
>> diff --git a/src/config/initializers/warden.rb
>> b/src/config/initializers/warden.rb
>> index 80751ad..368a9a2 100644
>> --- a/src/config/initializers/warden.rb
>> +++ b/src/config/initializers/warden.rb
>> @@ -81,3 +81,6 @@ Warden::Strategies.add(:ldap) do
>> u ? success!(u) : fail!("Username or password is not correct -
>> could not log in")
>> end
>> end
>> +Warden::Manager.after_set_user do |user,auth,opts|
>> + SessionEntity.update_session(auth.request.session_options[:id],
>> user)
>> +end
>> diff --git
>> a/src/db/migrate/20120606200000_fix_session_id_for_session_entities.rb
>> b/src/db/migrate/20120606200000_fix_session_id_for_session_entities.rb
>> new file mode 100644
>> index 0000000..908edae
>> --- /dev/null
>> +++
>> b/src/db/migrate/20120606200000_fix_session_id_for_session_entities.rb
>> @@ -0,0 +1,51 @@
>> +#
>> +# Copyright 2012 Red Hat, Inc.
>> +#
>> +# Licensed under the Apache License, Version 2.0 (the "License");
>> +# you may not use this file except in compliance with the License.
>> +# You may obtain a copy of the License at
>> +#
>> +# http://www.apache.org/licenses/LICENSE-2.0
>> +#
>> +# Unless required by applicable law or agreed to in writing,
>> software
>> +# distributed under the License is distributed on an "AS IS"
>> BASIS,
>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> implied.
>> +# See the License for the specific language governing permissions
>> and
>> +# limitations under the License.
>> +#
>> +class FixSessionIdForSessionEntities< ActiveRecord::Migration
>> + def self.up
>> + rename_column :session_entities, :session_id, :session_db_id
>> + add_column :session_entities, :session_id, :string
>> + SessionEntity.reset_column_information
>> + SessionEntity.all.each do |se|
>> + session =
>> ActiveRecord::SessionStore::Session.find(se.session_db_id)
>> + if session
>> + se.session_id = session.session_id
>> + se.save!
>> + else
>> + se.delete!
>> + end
>> + end
>> + remove_column :session_entities, :session_db_id
>> + change_column :session_entities, :session_id, :string, :null =>
>> false
>> + end
>> +
>> + def self.down
>> + add_column :session_entities, :session_db_id, :integer
>> + SessionEntity.reset_column_information
>> + SessionEntity.each do |se|
>> + session =
>> ActiveRecord::SessionStore::Session.find_by_session_id(se.session_id)
>> + if session
>> + se.session_db_id = session.id
>> + se.save!
>> + else
>> + se.delete!
>> + end
>> + end
>> + remove_column :session_entities, :session_id
>> + rename_column :session_entities, :session_db_id, :session_id
>> + change_column :session_entities, :session_id, :integer, :null =>
>> false
>> +
>> + end
>> +end
>> diff --git a/src/spec/models/deployment_spec.rb
>> b/src/spec/models/deployment_spec.rb
>> index 3ffbae9..47e63ef 100644
>> --- a/src/spec/models/deployment_spec.rb
>> +++ b/src/spec/models/deployment_spec.rb
>> @@ -220,14 +220,15 @@ describe Deployment do
>> @user_for_launch = admin_perms.user
>> @user_for_launch.quota.maximum_running_instances = 1
>> @session = FactoryGirl.create :session
>> - SessionEntity.update_session(@session, @user_for_launch)
>> + @session_id = @session.session_id
>> + SessionEntity.update_session(@session_id, @user_for_launch)
>> @deployment.stub(:common_provider_accounts_for).and_return(["test","test"])
>> end
>>
>> it "return error when user quota was reached" do
>> Instance.any_instance.stub(:matches).and_return(["test","test"])
>> @deployment.stub!(:find_match_with_common_account).and_return([[],
>> true, []])
>> - errors = @deployment.check_assemblies_matches(@session,
>> @user_for_launch)
>> + errors = @deployment.check_assemblies_matches(@session_id,
>> @user_for_launch)
>> errors.should have(1).items
>> errors.last.should include
>> I18n.t('instances.errors.user_quota_reached')
>> end
>> @@ -246,20 +247,21 @@ describe Deployment do
>> admin_perms = FactoryGirl.create :admin_permission
>> @user_for_launch = admin_perms.user
>> @session = FactoryGirl.create :session
>> - SessionEntity.update_session(@session, @user_for_launch)
>> + @session_id = @session.session_id
>> + SessionEntity.update_session(@session_id, @user_for_launch)
>> end
>>
>> it "should return errors when checking assemblies matches which
>> are not launchable" do
>> - @deployment.check_assemblies_matches(@session,
>> @user_for_launch).should be_empty
>> + @deployment.check_assemblies_matches(@session_id,
>> @user_for_launch).should be_empty
>> @deployment.pool.pool_family.provider_accounts.destroy_all
>> - @deployment.check_assemblies_matches(@session,
>> @user_for_launch).should_not be_empty
>> + @deployment.check_assemblies_matches(@session_id,
>> @user_for_launch).should_not be_empty
>> end
>>
>> it "should launch instances when launching deployment" do
>> @deployment.instances.should be_empty
>>
>> Taskomatic.stub!(:create_instance!).and_return(true)
>> - @deployment.create_and_launch(@session, @user_for_launch)
>> + @deployment.create_and_launch(@session_id, @user_for_launch)
>> @deployment.errors.should be_empty
>> @deployment.instances.count.should == 2
>> end
>> @@ -272,7 +274,7 @@ describe Deployment do
>> Taskomatic.stub!(:create_dcloud_instance).and_return(true)
>> Taskomatic.stub!(:handle_dcloud_error).and_return(true)
>> Taskomatic.stub!(:handle_instance_state).and_return(true)
>> - @deployment.create_and_launch(@session, @user_for_launch)
>> + @deployment.create_and_launch(@session_id, @user_for_launch)
>> @deployment.errors.should be_empty
>> @deployment.reload
>> @deployment.instances.count.should == 2
>> @@ -281,7 +283,7 @@ describe Deployment do
>> @provider_account1.priority = 30
>> @provider_account1.save!
>> deployment2 = Factory.create(:deployment, :pool_id =>
>> @pool.id)
>> - deployment2.create_and_launch(@session, @user_for_launch)
>> + deployment2.create_and_launch(@session_id, @user_for_launch)
>> deployment2.errors.should be_empty
>> deployment2.reload
>> deployment2.instances.count.should == 2
>> @@ -300,7 +302,7 @@ describe Deployment do
>> Taskomatic.stub!(:create_dcloud_instance).and_return(true)
>> Taskomatic.stub!(:handle_dcloud_error).and_return(true)
>> Taskomatic.stub!(:handle_instance_state).and_return(true)
>> - @deployment.create_and_launch(@session, @user_for_launch)
>> + @deployment.create_and_launch(@session_id, @user_for_launch)
>> @deployment.errors.should be_empty
>> @deployment.reload
>> @deployment.instances.count.should == 2
>> @@ -312,7 +314,7 @@ describe Deployment do
>> @deployment.instances.should be_empty
>> @deployment.pool.pool_family.provider_accounts.destroy_all
>> Taskomatic.stub!(:create_instance!).and_return(true)
>> - @deployment.create_and_launch(@session, @user_for_launch)
>> + @deployment.create_and_launch(@session_id, @user_for_launch)
>> @deployment.errors.should_not be_empty
>> lambda { Deployment.find(@deployment.id) }.should
>> raise_error(ActiveRecord::RecordNotFound)
>> end
>> @@ -325,7 +327,7 @@ describe Deployment do
>> it "should set create_failed status for instances if
>> instance's launch raises an exception" do
>> @deployment.instances.should be_empty
>> Taskomatic.stub!(:create_dcloud_instance).and_raise("an
>> exception")
>> - @deployment.create_and_launch(@session, @user_for_launch)
>> + @deployment.create_and_launch(@session_id, @user_for_launch)
>> @deployment.reload
>> @deployment.instances.should_not be_empty
>> @deployment.instances.each {|i| i.state.should ==
>> Instance::STATE_CREATE_FAILED}
>> diff --git a/src/spec/models/permission_spec.rb
>> b/src/spec/models/permission_spec.rb
>> index ce92bd6..7f05587 100644
>> --- a/src/spec/models/permission_spec.rb
>> +++ b/src/spec/models/permission_spec.rb
>> @@ -32,53 +32,54 @@ describe Permission do
>> @provider = @provider_admin_permission.provider
>> @pool = @pool_user_permission.pool
>> @session = FactoryGirl.create :session
>> - SessionEntity.update_session(@session, @admin)
>> - SessionEntity.add_to_session(@session, @provider_admin)
>> - SessionEntity.add_to_session(@session, @pool_user)
>> + @session_id = @session.session_id
>> + SessionEntity.update_session(@session_id, @admin)
>> + SessionEntity.add_to_session(@session_id, @provider_admin)
>> + SessionEntity.add_to_session(@session_id, @pool_user)
>> end
>>
>> it "Admin should be able to create users" do
>> -
>> BasePermissionObject.general_permission_scope.has_privilege(@session,
>> +
>> BasePermissionObject.general_permission_scope.has_privilege(@session_id,
>> @admin,
>> Privilege::CREATE,
>> User).should
>> be_true
>> end
>>
>> it "Provider Admin should NOT be able to create users" do
>> -
>> BasePermissionObject.general_permission_scope.has_privilege(@session,
>> +
>> BasePermissionObject.general_permission_scope.has_privilege(@session_id,
>> @provider_admin,
>> Privilege::CREATE,
>> User).should
>> be_false
>> end
>>
>> it "Pool User should NOT be able to create users" do
>> -
>> BasePermissionObject.general_permission_scope.has_privilege(@session,
>> +
>> BasePermissionObject.general_permission_scope.has_privilege(@session_id,
>> @pool_user,
>> Privilege::CREATE,
>> User).should
>> be_false
>> end
>>
>> it "Provider Admin should be able to edit provider" do
>> - @provider.has_privilege(@session, @provider_admin,
>> + @provider.has_privilege(@session_id, @provider_admin,
>> Privilege::MODIFY).should be_true
>> end
>>
>> it "Admin should be able to edit provider" do
>> - @provider.has_privilege(@session, @admin,
>> Privilege::MODIFY).should be_true
>> + @provider.has_privilege(@session_id, @admin,
>> Privilege::MODIFY).should be_true
>> end
>>
>> it "Pool User should NOT be able to edit provider" do
>> - @provider.has_privilege(@session, @pool_user,
>> + @provider.has_privilege(@session_id, @pool_user,
>> Privilege::MODIFY).should be_false
>> end
>>
>> it "Pool User should be able to create instances in @pool" do
>> - @pool.has_privilege(@session, @pool_user,
>> + @pool.has_privilege(@session_id, @pool_user,
>> Privilege::CREATE, Instance).should be_true
>> end
>>
>> it "Pool User should NOT be able to create instances in another
>> pool" do
>> - FactoryGirl.create(:tpool).has_privilege(@session, @pool_user,
>> + FactoryGirl.create(:tpool).has_privilege(@session_id,
>> @pool_user,
>> Privilege::CREATE,
>> Instance).
>> should be_false
>> end
>> diff --git a/src/spec/services/registration_service_spec.rb
>> b/src/spec/services/registration_service_spec.rb
>> index b7c33cc..76c2544 100644
>> --- a/src/spec/services/registration_service_spec.rb
>> +++ b/src/spec/services/registration_service_spec.rb
>> @@ -38,7 +38,8 @@ describe RegistrationService do
>> it "should register a user with default pool/quota/role perms
>> when default settings set" do
>> @user = FactoryGirl.create :user
>> @session = FactoryGirl.create :session
>> - SessionEntity.update_session(@session, @user)
>> + @session_id = @session.session_id
>> + SessionEntity.update_session(@session_id, @user)
>> @pool = MetadataObject.lookup("self_service_default_pool")
>> @role = MetadataObject.lookup("self_service_default_role")
>> @quota = FactoryGirl.create :quota
>> @@ -47,7 +48,7 @@ describe RegistrationService do
>> @registration_service = RegistrationService.new(@user)
>> @registration_service.save
>>
>> - @pools = Pool.list_for_user(@session, @user,
>> Privilege::CREATE, Instance)
>> + @pools = Pool.list_for_user(@session_id, @user,
>> Privilege::CREATE, Instance)
>> @pools.length.should == 1
>> @pools[0].name.should == "Default"
>>
>> diff --git a/src/spec/spec_helper.rb b/src/spec/spec_helper.rb
>> index 372d7b7..cc97efb 100644
>> --- a/src/spec/spec_helper.rb
>> +++ b/src/spec/spec_helper.rb
>> @@ -56,11 +56,11 @@ def mock_warden(user)
>> :authenticate! => user,
>> :user => user,
>> :raw_session => nil)
>> - request.session_options[:id] = 'ee73441902cb9445483e498cb05dc398'
>> - @session = ActiveRecord::SessionStore::Session.
>> - find_by_session_id('ee73441902cb9445483e498cb05dc398')
>> + @session_id = 'ee73441902cb9445483e498cb05dc398'
>> + request.session_options[:id] = @session_id
>> + @session =
>> ActiveRecord::SessionStore::Session.find_by_session_id(@session_id)
>> @session = FactoryGirl.create :session unless @session
>> - SessionEntity.update_session(@session, user) if user
>> + SessionEntity.update_session(@session_id, user) if user
>> end
>>
>> RSpec.configure do |config|
>> --
>> 1.7.6.5
>>
>>
More information about the aeolus-devel
mailing list