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

Scott Seago sseago at redhat.com
Thu Jun 7 13:58:16 UTC 2012


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