Re: [PATCH conductor] Refactor SessionEntity model to work properly with API

ACK, passes tests, permissions seem to work. Mainn

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