[PATCH aeolus-conductor] BZ#786126 Do not mass assign on credentials hash

Jozef Zigmund jzigmund at redhat.com
Thu Feb 2 22:36:01 UTC 2012


On Thursday 02 February 2012 13:08:36 Jan Provaznik wrote:
> 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?

One notice, maybe you missed it because I see it in staging :)
> 
> >         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