[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