Mo,
Thanks for reviewing this so quickly, I've replied to your comments inline,
Cheers
Martyn ----- Original Message ----- From: "Mohammed Morsi" mmorsi@redhat.com To: mtaylor@redhat.com Cc: deltacloud-devel@lists.fedorahosted.org Sent: Monday, May 10, 2010 8:45:15 PM GMT +00:00 GMT Britain, Ireland, Portugal Subject: Re: [deltacloud-devel] [PATCH] Added Validation for Cloud Account Provider Credentials
On 05/10/2010 09:56 AM, mtaylor@redhat.com wrote:
From: martyntaylormtaylor@redhat.com
src/app/controllers/cloud_accounts_controller.rb | 2 +- src/app/controllers/provider_controller.rb | 11 ++++++++--- src/app/models/cloud_account.rb | 16 ++++++++++++++++ src/app/views/cloud_accounts/new.html.erb | 1 + src/features/support/env.rb | 6 ++++++ .../controllers/cloud_accounts_controller_spec.rb | 7 +++++++ src/spec/spec_helper.rb | 7 +++++++ 7 files changed, 46 insertions(+), 4 deletions(-)
Tried it out and it looks good for the most part, though there are some caveats below.
diff --git a/src/app/controllers/cloud_accounts_controller.rb b/src/app/controllers/cloud_accounts_controller.rb index 05e024e..23b3113 100644 --- a/src/app/controllers/cloud_accounts_controller.rb +++ b/src/app/controllers/cloud_accounts_controller.rb @@ -95,4 +95,4 @@ class CloudAccountsController< ApplicationController end redirect_to :controller => 'provider', :action => 'accounts', :id => provider.id end -end +end \ No newline at end of file
-What is the change here? I'm not even seeing a whitespace difference. -Though that being said, I was unable to apply your patch until I ran -dos2unix on it. I'm not sure whose end this is a problem on, I remember -having this issue in the past, but thought I had resolved it. Could you -tell me if either of your ~/.gitconfig or your aggregator checkout -.git/config has a CRLF option configured, and what it is set to. If not, -we should see if anyone else has a problem applying it to see who has -the issue.
OK, I dont actually think there is a change here. I'm not sure if this is a result of my improper use of GIT or whether this has occurred due to some obscure reason. Anyways, I think it is safe to ignore this.
Myself and Marios have tried applying the patch on a fresh Aggregator and it seems to apply OK for us.
diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index 23b5ba0..092054c 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -76,8 +76,13 @@ class ProviderController< ApplicationController require_privilege(Privilege::ACCOUNT_MODIFY) @acct = CloudAccount.find_or_create(params[:cloud_account]) @provider = Provider.find(params[:cloud_account][:provider_id])
@provider.cloud_accounts<< @acct
redirect_to :action => 'accounts', :id => @provider.id
if @acct.save
@provider.cloud_accounts<< @acct
redirect_to :action => 'accounts', :id => @provider.id
else
flash[:notice] = @acct.errors.full_messages[0]
redirect_to :action => 'new_account', :id => @provider.id
endend
-end +end \ No newline at end of file diff --git a/src/app/models/cloud_account.rb b/src/app/models/cloud_account.rb index 1d6b3e9..f04187d 100644 --- a/src/app/models/cloud_account.rb +++ b/src/app/models/cloud_account.rb @@ -67,4 +67,20 @@ class CloudAccount< ActiveRecord::Base def name username end
- protected
- def validate
- errors.add_to_base("Login Credentials are Invalid for this Provider") unless valid_credentials?
- end
- private
- def valid_credentials?
- begin
deltacloud = DeltaCloud.new(username, password, provider.url)
deltacloud.instances
- rescue Exception => e
return false
- end
- return true
- end end
-Why not just use the 'connect' method defined above in the CloudAccount -class, it does the exact same thing. The Provider validate method does -it that way.
We can't just use the connect method here because the connect method only connects to the Provider, it does not perform any action that requires authentication. Since we are checking the validation of login credentials we need to test for an AuthenticationException to see if they are valid. To do this I've tried listing the instances (since this needs authentication). However, there was some discussion on IRC yesterday about possibly adding a "test credentials" type method to the API.
diff --git a/src/app/views/cloud_accounts/new.html.erb b/src/app/views/cloud_accounts/new.html.erb index 40f32cb..8d4fcc8 100644 --- a/src/app/views/cloud_accounts/new.html.erb +++ b/src/app/views/cloud_accounts/new.html.erb @@ -4,6 +4,7 @@
<h2 class="greeting">New Cloud Account</h2>
<div class="dcloud_form"> +<%= error_messages_for 'cloud_account' %> <% form_for @cloud_account, :url => { :action => "create" } do |f| %> <%= f.error_messages %> <%= render :partial => "form", :object => f %> diff --git a/src/features/support/env.rb b/src/features/support/env.rb index 2d6e9a1..91327a8 100644 --- a/src/features/support/env.rb +++ b/src/features/support/env.rb @@ -30,6 +30,12 @@ Provider.class_eval do end end
+CloudAccount.class_eval do
- alias_method :original_validate, :validate
- def validate
- end
+end
-I'm still pretty new to rspec / cucumber, but is it wise to overload the -validate method at this level? eg in other specs / features we might -want to test the validate method itself, especially if it gets expanded. -What is the benefit of doing this here?
Good point here, I was in some deliberation over this. After giving this some thought I decided to go ahead with overriding the method here. I think however, that it maybe better to simply override the valid_credentials? method.
The reasoning behind doing this, is that each time a CloudAccount is created it tries to validate the credentials. For our tests, we don't have a core setup to be able to do this, (the same applies for Provider validation). An alternative way to do this is to use mock objects and override the method on the individual object. This is how Tomas, overcome the problem in some of the spec tests. The problem here is that when running a test that uses a REST, generally a new object is created from the parameters specified in the call. Since this new object is different from the object we stubbed, the method is not overidden.
Also if we really did want to test "valid_credentials?" and also the "connect" method on the provider, we need some form of integrated testing, so we can integrate core and aggregator, which we currently don't have.
I'm open to suggestion on this tho, so if you can think of a better way to do this, it would be great :-)
# If you set this to false, any error raised from within your app will bubble # up to your step definition and out to cucumber unless you catch it somewhere # on the way. You can make Rails rescue errors and render error pages on a diff --git a/src/spec/controllers/cloud_accounts_controller_spec.rb b/src/spec/controllers/cloud_accounts_controller_spec.rb index f1d4a0d..4dbac78 100644 --- a/src/spec/controllers/cloud_accounts_controller_spec.rb +++ b/src/spec/controllers/cloud_accounts_controller_spec.rb @@ -48,4 +48,11 @@ describe CloudAccountsController do response.should_not be_success end
- it "should fail to create a cloud account if the provider credentials are invalid" do
- cloud_account = Factory.build(:mock_cloud_account)
- cloud_account.stub!(:valid_credentials?).and_return(false)
- cloud_account.save.should == false
- end
- end
diff --git a/src/spec/spec_helper.rb b/src/spec/spec_helper.rb index 5181389..b880a52 100644 --- a/src/spec/spec_helper.rb +++ b/src/spec/spec_helper.rb @@ -21,6 +21,13 @@ Spec::Runner.configure do |config| config.use_instantiated_fixtures = false config.fixture_path = RAILS_ROOT + '/spec/fixtures/'
+CloudAccount.class_eval do
- alias_method :original_valid_credentials?, :valid_credentials?
- def valid_credentials?
- return true
- end
+end
Same comment as above as far as overriding the validation methods.
-Mo
On 05/11/2010 05:35 AM, Martyn Taylor wrote:
Mo,
Thanks for reviewing this so quickly, I've replied to your comments inline,
Cheers
Martyn ----- Original Message ----- From: "Mohammed Morsi"mmorsi@redhat.com To: mtaylor@redhat.com Cc: deltacloud-devel@lists.fedorahosted.org Sent: Monday, May 10, 2010 8:45:15 PM GMT +00:00 GMT Britain, Ireland, Portugal Subject: Re: [deltacloud-devel] [PATCH] Added Validation for Cloud Account Provider Credentials
On 05/10/2010 09:56 AM, mtaylor@redhat.com wrote:
From: martyntaylormtaylor@redhat.com
src/app/controllers/cloud_accounts_controller.rb | 2 +- src/app/controllers/provider_controller.rb | 11 ++++++++--- src/app/models/cloud_account.rb | 16 ++++++++++++++++ src/app/views/cloud_accounts/new.html.erb | 1 + src/features/support/env.rb | 6 ++++++ .../controllers/cloud_accounts_controller_spec.rb | 7 +++++++ src/spec/spec_helper.rb | 7 +++++++ 7 files changed, 46 insertions(+), 4 deletions(-)
Tried it out and it looks good for the most part, though there are some caveats below.
diff --git a/src/app/controllers/cloud_accounts_controller.rb b/src/app/controllers/cloud_accounts_controller.rb index 05e024e..23b3113 100644 --- a/src/app/controllers/cloud_accounts_controller.rb +++ b/src/app/controllers/cloud_accounts_controller.rb @@ -95,4 +95,4 @@ class CloudAccountsController< ApplicationController end redirect_to :controller => 'provider', :action => 'accounts', :id => provider.id end -end +end \ No newline at end of file
-What is the change here? I'm not even seeing a whitespace difference. -Though that being said, I was unable to apply your patch until I ran -dos2unix on it. I'm not sure whose end this is a problem on, I remember -having this issue in the past, but thought I had resolved it. Could you -tell me if either of your ~/.gitconfig or your aggregator checkout -.git/config has a CRLF option configured, and what it is set to. If not, -we should see if anyone else has a problem applying it to see who has -the issue.
OK, I dont actually think there is a change here. I'm not sure if this is a result of my improper use of GIT or whether this has occurred due to some obscure reason. Anyways, I think it is safe to ignore this.
Myself and Marios have tried applying the patch on a fresh Aggregator and it seems to apply OK for us.
Since there is no actual change can you resend the patch with this omitted. I ask because when someone is looking at the git log for the src/app/controllers/cloud_accounts_controller.rb file, they will see this commit, which should not be there.
diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index 23b5ba0..092054c 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -76,8 +76,13 @@ class ProviderController< ApplicationController require_privilege(Privilege::ACCOUNT_MODIFY) @acct = CloudAccount.find_or_create(params[:cloud_account]) @provider = Provider.find(params[:cloud_account][:provider_id])
@provider.cloud_accounts<< @acct
redirect_to :action => 'accounts', :id => @provider.id
if @acct.save
@provider.cloud_accounts<< @acct
redirect_to :action => 'accounts', :id => @provider.id
else
flash[:notice] = @acct.errors.full_messages[0]
redirect_to :action => 'new_account', :id => @provider.id
endend
-end +end \ No newline at end of file diff --git a/src/app/models/cloud_account.rb b/src/app/models/cloud_account.rb index 1d6b3e9..f04187d 100644 --- a/src/app/models/cloud_account.rb +++ b/src/app/models/cloud_account.rb @@ -67,4 +67,20 @@ class CloudAccount< ActiveRecord::Base def name username end
- protected
- def validate
- errors.add_to_base("Login Credentials are Invalid for this Provider") unless valid_credentials?
- end
- private
- def valid_credentials?
- begin
deltacloud = DeltaCloud.new(username, password, provider.url)
deltacloud.instances
- rescue Exception => e
return false
- end
- return true
- end end
-Why not just use the 'connect' method defined above in the CloudAccount -class, it does the exact same thing. The Provider validate method does -it that way.
We can't just use the connect method here because the connect method only connects to the Provider, it does not perform any action that requires authentication. Since we are checking the validation of login credentials we need to test for an AuthenticationException to see if they are valid. To do this I've tried listing the instances (since this needs authentication). However, there was some discussion on IRC yesterday about possibly adding a "test credentials" type method to the API.
OK that sounds reasonable. Could you please include a TODO or a FIXME comment right above the call to "instances" saying that it should be replaced eventually with the test_credentials call.
diff --git a/src/app/views/cloud_accounts/new.html.erb b/src/app/views/cloud_accounts/new.html.erb index 40f32cb..8d4fcc8 100644 --- a/src/app/views/cloud_accounts/new.html.erb +++ b/src/app/views/cloud_accounts/new.html.erb @@ -4,6 +4,7 @@
<h2 class="greeting">New Cloud Account</h2>
<div class="dcloud_form"> +<%= error_messages_for 'cloud_account' %> <% form_for @cloud_account, :url => { :action => "create" } do |f| %> <%= f.error_messages %> <%= render :partial => "form", :object => f %> diff --git a/src/features/support/env.rb b/src/features/support/env.rb index 2d6e9a1..91327a8 100644 --- a/src/features/support/env.rb +++ b/src/features/support/env.rb @@ -30,6 +30,12 @@ Provider.class_eval do end end
+CloudAccount.class_eval do
- alias_method :original_validate, :validate
- def validate
- end
+end
-I'm still pretty new to rspec / cucumber, but is it wise to overload the -validate method at this level? eg in other specs / features we might -want to test the validate method itself, especially if it gets expanded. -What is the benefit of doing this here?
Good point here, I was in some deliberation over this. After giving this some thought I decided to go ahead with overriding the method here. I think however, that it maybe better to simply override the valid_credentials? method.
The reasoning behind doing this, is that each time a CloudAccount is created it tries to validate the credentials. For our tests, we don't have a core setup to be able to do this, (the same applies for Provider validation). An alternative way to do this is to use mock objects and override the method on the individual object. This is how Tomas, overcome the problem in some of the spec tests. The problem here is that when running a test that uses a REST, generally a new object is created from the parameters specified in the call. Since this new object is different from the object we stubbed, the method is not overidden.
Also if we really did want to test "valid_credentials?" and also the "connect" method on the provider, we need some form of integrated testing, so we can integrate core and aggregator, which we currently don't have.
I'm open to suggestion on this tho, so if you can think of a better way to do this, it would be great :-)
OK I see what your doing here and agree this is probably the best approach for now for getting around the problem (though once again a comment above the block explaining this is in order). Ultimately this is because the mock core api driver will thrown an exception when trying to pass it credentials correct? (else why don't we just use the core api that needs to be started for the spec suite to run successfully anyways).
-Mo
deltacloud-devel@lists.fedorahosted.org