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

[Martyn Taylor]

From: Martyn Taylor <mtaylor(a)redhat.com&gt;

---
 .../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])
+    @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
       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
+      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
-- 
1.7.6.4

[Jan Provazník]

On 02/02/2012 11:46 AM, mtaylor(a)redhat.com wrote:
>  From: Martyn Taylor<mtaylor(a)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

On Thursday 02 February 2012 13:08:36 Jan Provaznik wrote:
>  On 02/02/2012 11:46 AM, mtaylor(a)redhat.com wrote:
>  > From: Martyn Taylor<mtaylor(a)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