----- Original Message -----
> From: "Scott Seago"<sseago(a)redhat.com>
> To: aeolus-devel(a)lists.fedorahosted.org
> Cc: "Scott Seago"<sseago(a)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 (!(a)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((a)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
>
>