Merge branch 'fix/improve_reset_service_password' into 'master'

Improve invalidation of stored service password if the endpoint URL is changed

A number of issues were found in !1490 and !1558 (triggered by support request 7395)

* It is not possible to set a new URL and a password at the same time (new password is ignored)
* An error occurs on the Service Templates admin pages (prop_updated? was referencing the service's project, which is not defined for templates)
* Passwords are reset on every save in Service Templates admin pages

This should fix these 3 issues by respectively:
* Differentiating a property that has been assigned a new value (regardless of the new value) and a property that has been assigned a new value that is different from the old one
* Providing an alternate implementation to detected updated properties, not relying on the service's project
* Filtering an empty password parameter passed to the Service Templates admin page like on the project service page

See merge request !1583

app/controllers/admin/services_controller.rb

@@ -39,7 +39,13 @@ class Admin::ServicesController < Admin::ApplicationController
   end
 
   def application_services_params
-    params.permit(:id,
+    application_services_params = params.permit(:id,
       service: Projects::ServicesController::ALLOWED_PARAMS)
+    if application_services_params[:service].is_a?(Hash)
+      Projects::ServicesController::FILTER_BLANK_PARAMS.each do |param|
+        application_services_params[:service].delete(param) if application_services_params[:service][param].blank? 
+      end
+    end
+    application_services_params
   end
 end

app/controllers/projects/services_controller.rb

@@ -9,6 +9,10 @@ class Projects::ServicesController < Projects::ApplicationController
                     :note_events, :send_from_committer_email, :disable_diffs, :external_wiki_url,
                     :notify, :color,
                     :server_host, :server_port, :default_irc_uri, :enable_ssl_verification]
+
+  # Parameters to ignore if no value is specified
+  FILTER_BLANK_PARAMS = [:password]
+  
   # Authorize
   before_action :authorize_admin_project!
   before_action :service, only: [:edit, :update, :test]
@@ -59,7 +63,9 @@ class Projects::ServicesController < Projects::ApplicationController
 
   def service_params
     service_params = params.require(:service).permit(ALLOWED_PARAMS)
-    service_params.delete("password") if service_params["password"].blank?
+    FILTER_BLANK_PARAMS.each do |param|
+      service_params.delete(param) if service_params[param].blank?
+    end
     service_params
   end
 end

app/models/project_services/bamboo_service.rb

@@ -48,7 +48,7 @@ class BambooService < CiService
   end
 
   def reset_password
-    if prop_updated?(:bamboo_url)
+    if prop_modified?(:bamboo_url) && !prop_updated?(:password)
       self.password = nil
     end
   end

app/models/project_services/teamcity_service.rb

@@ -45,7 +45,7 @@ class TeamcityService < CiService
   end
 
   def reset_password
-    if prop_updated?(:teamcity_url)
+    if prop_modified?(:teamcity_url) && !prop_updated?(:password)
       self.password = nil
     end
   end

app/models/service.rb

@@ -33,6 +33,8 @@ class Service < ActiveRecord::Base
 
   after_initialize :initialize_properties
 
+  after_commit :reset_updated_properties
+
   belongs_to :project
   has_one :service_hook
 
@@ -103,6 +105,7 @@ class Service < ActiveRecord::Base
 
   # Provide convenient accessor methods
   # for each serialized property.
+  # Also keep track of updated properties.
   def self.prop_accessor(*args)
     args.each do |arg|
       class_eval %{
@@ -111,19 +114,36 @@ class Service < ActiveRecord::Base
         end
 
         def #{arg}=(value)
+          updated_properties['#{arg}'] = #{arg} unless updated_properties.include?('#{arg}') 
           self.properties['#{arg}'] = value
         end
       }
     end
   end
 
-  # ActiveRecord does not provide a mechanism to track changes in serialized keys.
-  # This is why we need to perform extra query to do it mannually.
+  # Returns a hash of the properties that have been assigned a new value since last save,
+  # indicating their original values (attr => original value).
+  # ActiveRecord does not provide a mechanism to track changes in serialized keys, 
+  # so we need a specific implementation for service properties.
+  # This allows to track changes to properties set with the accessor methods,
+  # but not direct manipulation of properties hash.
+  def updated_properties
+    @updated_properties ||= ActiveSupport::HashWithIndifferentAccess.new
+  end
+
   def prop_updated?(prop_name)
-    relation_name = self.type.underscore
-    previous_value = project.send(relation_name).send(prop_name)
-    return false if previous_value.nil?
-    previous_value != send(prop_name)
+    # Check if a new value was set.
+    # The new value may or may not be the same as the old value
+    updated_properties.include?(prop_name)
+  end
+
+  def prop_modified?(prop_name)
+    # Check if new value was set and it is different from the old value
+    prop_updated?(prop_name) && updated_properties[prop_name] != send(prop_name)
+  end
+
+  def reset_updated_properties
+    @updated_properties = nil
   end
 
   def async_execute(data)

spec/models/service_spec.rb

@@ -116,14 +116,47 @@ describe Service do
       )
     end
 
-    it "returns false" do
+    it "returns false when the property has not been assigned a new value" do
       service.username = "key_changed"
       expect(service.prop_updated?(:bamboo_url)).to be_falsy
     end
 
-    it "returns true" do
-      service.bamboo_url = "http://other.com"
+    it "returns true when the property has been assigned a different value" do
+      service.bamboo_url = "http://example.com"
       expect(service.prop_updated?(:bamboo_url)).to be_truthy
     end
+
+    it "returns true when the property has been re-assigned the same value" do
+      service.bamboo_url = 'http://gitlab.com'
+      expect(service.prop_updated?(:bamboo_url)).to be_truthy
+    end
+  end
+
+  describe "#prop_modified?" do
+    let(:service) do
+      BambooService.create(
+        project: create(:project),
+        properties: {
+          bamboo_url: 'http://gitlab.com',
+          username: 'mic',
+          password: "password"
+        }
+      )
+    end
+
+    it "returns false when the property has not been assigned a new value" do
+      service.username = "key_changed"
+      expect(service.prop_modified?(:bamboo_url)).to be_falsy
+    end
+
+    it "returns true when the property has been assigned a different value" do
+      service.bamboo_url = "http://example.com"
+      expect(service.prop_modified?(:bamboo_url)).to be_truthy
+    end
+
+    it "returns false when the property has been re-assigned the same value" do
+      service.bamboo_url = 'http://gitlab.com'
+      expect(service.prop_modified?(:bamboo_url)).to be_falsy
+    end
   end
 end