[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