[PATCH aeolus-conductor] BZ#786126 Do not mass assign on credentials hash
Jan Provaznik
jprovazn at redhat.com
Thu Feb 2 12:08:36 UTC 2012
On 02/02/2012 11:46 AM, mtaylor at redhat.com wrote:
> From: Martyn Taylor<mtaylor at redhat.com>
>
> ---
> .../controllers/provider_accounts_controller.rb | 7 ++++-
> src/app/models/provider_account.rb | 25 ++++++++++++-------
> .../provider_accounts_controller_spec.rb | 2 +-
> src/spec/models/provider_account_spec.rb | 18 ++++++++++++++
> 4 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/src/app/controllers/provider_accounts_controller.rb b/src/app/controllers/provider_accounts_controller.rb
> index 9b5517d..acfecdb 100644
> --- a/src/app/controllers/provider_accounts_controller.rb
> +++ b/src/app/controllers/provider_accounts_controller.rb
> @@ -79,6 +79,7 @@ class ProviderAccountsController< ApplicationController
> params[:provider_account][:provider_id] = @provider.id
> @providers = Provider.all
> @provider_account = ProviderAccount.new(params[:provider_account])
> + @provider_account.credentials_hash = params[:provider_account][:credentials_hash]
> @provider_account.quota = @quota = Quota.new
> limit = params[:quota][:maximum_running_instances] if params[:quota]
> @provider_account.quota.set_maximum_running_instances(limit)
> @@ -119,10 +120,14 @@ class ProviderAccountsController< ApplicationController
>
> limit = params[:quota][:maximum_running_instances] if params[:quota]
> @provider_account.quota.set_maximum_running_instances(limit)
> - if @provider_account.update_attributes(params[:provider_account])
> + credentials_hash = params[:provider_account].delete(:credentials_hash)
> + @provider_account.attributes = @provider_account.attributes.merge(params[:provider_account])
you can use only @provider_account.attributes =
params[:provider_account] - existing attrs are kept anyway
> + @provider_account.credentials_hash= credentials_hash
> + if @provider_account.save
> flash[:notice] = t"provider_accounts.flash.notice.updated"
> redirect_to edit_provider_path(@provider, :details_tab => 'accounts')
> else
> + puts "==================================================================\n" + @provider_account.errors.inspect
^ forgotten line?
> flash[:error] = t"provider_accounts.flash.error.not_updated"
> render :action => :edit
> end
> diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
> index e772f52..541834e 100644
> --- a/src/app/models/provider_account.rb
> +++ b/src/app/models/provider_account.rb
> @@ -74,6 +74,11 @@ class ProviderAccount< ActiveRecord::Base
>
> scope :enabled, lambda { where(:provider_id => Provider.enabled) }
>
> + # We set credentials hash as protected so that it is not set during mass assign on new
> + # This is to avoid the scenario where the credentials are set before provider which
> + # will result in an exception.
> + attr_protected :credentials_hash
> +
> def validate_presence_of_credentials
> provider.provider_type.credential_definitions.each do |cd|
> errors.add(:base, "#{cd.label} can't be blank") if credentials_hash[cd.name].blank?
> @@ -207,16 +212,18 @@ class ProviderAccount< ActiveRecord::Base
> end
>
> def credentials_hash=(hash={})
> - cred_defs = provider.provider_type.credential_definitions
> - hash.each do |k,v|
> - cred_def = cred_defs.detect {|d| d.name == k.to_s}
> - raise "Key #{k} not found" unless cred_def
> - unless cred = credentials.detect {|c| c.credential_definition_id == cred_def.id}
> - cred = Credential.new(:provider_account_id => id, :credential_definition_id => cred_def.id)
> - credentials<< cred
> + if @provider
it's better to use 'provider' instead of '@provider' (in case provider
getter is redefined in future)
> + cred_defs = provider.provider_type.credential_definitions
> + hash.each do |k,v|
> + cred_def = cred_defs.detect {|d| d.name == k.to_s}
> + raise "Key #{k} not found" unless cred_def
> + unless cred = credentials.detect {|c| c.credential_definition_id == cred_def.id}
> + cred = Credential.new(:provider_account_id => id, :credential_definition_id => cred_def.id)
> + credentials<< cred
> + end
> + # we need to handle uploaded files:
> + cred.value = v.respond_to?(:read) ? v.read : v
> end
> - # we need to handle uploaded files:
> - cred.value = v.respond_to?(:read) ? v.read : v
> end
> end
>
> diff --git a/src/spec/controllers/provider_accounts_controller_spec.rb b/src/spec/controllers/provider_accounts_controller_spec.rb
> index 4675d0a..0b722c9 100644
> --- a/src/spec/controllers/provider_accounts_controller_spec.rb
> +++ b/src/spec/controllers/provider_accounts_controller_spec.rb
> @@ -42,7 +42,7 @@ describe ProviderAccountsController do
>
> it "doesn't allow to save provider's account if not valid credentials" do
> mock_warden(@admin)
> - post :create, :provider_account => {:provider_id => @provider.id}, :provider_id => @provider.id
> + post :create, :provider_account => {:provider_id => @provider.id, :credentials_hash => {}}, :provider_id => @provider.id
> response.should be_success
> response.should render_template("new")
> request.flash[:error].should == "Cannot add the provider account."
> diff --git a/src/spec/models/provider_account_spec.rb b/src/spec/models/provider_account_spec.rb
> index 6a52224..04b4b1e 100644
> --- a/src/spec/models/provider_account_spec.rb
> +++ b/src/spec/models/provider_account_spec.rb
> @@ -153,6 +153,24 @@ describe ProviderAccount do
> @provider_account.should_not be_valid
> end
>
> + it "should not set credentials in intialise" do
> + @provider_account = ProviderAccount.new({:credentials_hash => {"username" => "test", "password" => "test"}})
> + @provider_account.credentials_hash.should == {}
> + end
> +
> + it "should not set credentials before provider is set" do
> + @provider_account = ProviderAccount.new
> + @provider_account.credentials_hash = {"username" => "test", "password" => "test"}
> + @provider_account.credentials_hash.should == {}
> + end
> +
> + it "should set credentials after provider is set" do
> + provider = FactoryGirl.create :mock_provider
> + @provider_account = ProviderAccount.new({:provider => provider})
> + @provider_account.credentials_hash = {"username" => "test", "password" => "test"}
> + @provider_account.credentials_hash.should == {"username" => "test", "password" => "test"}
> + end
> +
> context "validations" do
> context "priority" do
> it "can be positive integer" do
ACK (3 minor nits inline)
Jan
More information about the aeolus-devel
mailing list