[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