Merge branch 'fix/improve_reset_service_password_v2' into 'master'

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

New version of !1583, using the same failproof logic but this time mirroring the name and behaviour of the change-tracking methods of ActiveModel::Dirty in order to make it clearer and more natural.

Added more tests to clarify the expected behaviour.

This is an alternative to !1594

/cc @vsizov @rspeicher

See merge request !1600

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 bamboo_url_changed? && !password_touched?
       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 teamcity_url_changed? && !password_touched?
       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 in a similar way as ActiveModel::Dirty
   def self.prop_accessor(*args)
     args.each do |arg|
       class_eval %{
@@ -111,21 +114,39 @@ class Service < ActiveRecord::Base
         end
 
         def #{arg}=(value)
+          updated_properties['#{arg}'] = #{arg} unless #{arg}_changed?
           self.properties['#{arg}'] = value
         end
+
+        def #{arg}_changed?
+          #{arg}_touched? && #{arg} != #{arg}_was
+        end
+
+        def #{arg}_touched?
+          updated_properties.include?('#{arg}')
+        end
+
+        def #{arg}_was
+          updated_properties['#{arg}']
+        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.
-  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)
+  # 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 reset_updated_properties
+    @updated_properties = nil
+  end
+  
   def async_execute(data)
     return unless supported_events.include?(data[:object_kind])
 

spec/models/project_services/bamboo_service_spec.rb

@@ -30,27 +30,65 @@ describe BambooService, models: true do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }
 
-    before do
-      @bamboo_service = BambooService.create(
-        project: create(:project),
-        properties: {
-          bamboo_url: 'http://gitlab.com',
-          username: 'mic',
-          password: "password"
-        }
-      )
-    end
+    context "when a password was previously set" do
+      before do
+        @bamboo_service = BambooService.create(
+          project: create(:project),
+          properties: {
+            bamboo_url: 'http://gitlab.com',
+            username: 'mic',
+            password: "password"
+          }
+        )
+      end
+  
+      it "reset password if url changed" do
+        @bamboo_service.bamboo_url = 'http://gitlab1.com'
+        @bamboo_service.save
+        expect(@bamboo_service.password).to be_nil
+      end
+  
+      it "does not reset password if username changed" do
+        @bamboo_service.username = "some_name"
+        @bamboo_service.save
+        expect(@bamboo_service.password).to eq("password")
+      end
+
+      it "does not reset password if new url is set together with password, even if it's the same password" do
+        @bamboo_service.bamboo_url = 'http://gitlab_edited.com'
+        @bamboo_service.password = 'password'
+        @bamboo_service.save
+        expect(@bamboo_service.password).to eq("password")
+        expect(@bamboo_service.bamboo_url).to eq("http://gitlab_edited.com")
+      end
 
-    it "reset password if url changed" do
-      @bamboo_service.bamboo_url = 'http://gitlab1.com'
-      @bamboo_service.save
-      expect(@bamboo_service.password).to be_nil
+      it "should reset password if url changed, even if setter called multiple times" do
+        @bamboo_service.bamboo_url = 'http://gitlab1.com'
+        @bamboo_service.bamboo_url = 'http://gitlab1.com'
+        @bamboo_service.save
+        expect(@bamboo_service.password).to be_nil
+      end
     end
+    
+    context "when no password was previously set" do
+      before do
+        @bamboo_service = BambooService.create(
+          project: create(:project),
+          properties: {
+            bamboo_url: 'http://gitlab.com',
+            username: 'mic'
+          }
+        )
+      end
+
+      it "saves password if new url is set together with password" do
+        @bamboo_service.bamboo_url = 'http://gitlab_edited.com'
+        @bamboo_service.password = 'password'
+        @bamboo_service.save
+        expect(@bamboo_service.password).to eq("password")
+        expect(@bamboo_service.bamboo_url).to eq("http://gitlab_edited.com")
+      end
 
-    it "does not reset password if username changed" do
-      @bamboo_service.username = "some_name"
-      @bamboo_service.save
-      expect(@bamboo_service.password).to eq("password")
     end
   end
 end

spec/models/project_services/teamcity_service_spec.rb

@@ -30,27 +30,64 @@ describe TeamcityService, models: true do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }
 
-    before do
-      @teamcity_service = TeamcityService.create(
-        project: create(:project),
-        properties: {
-          teamcity_url: 'http://gitlab.com',
-          username: 'mic',
-          password: "password"
-        }
-      )
-    end
+    context "when a password was previously set" do
+      before do
+        @teamcity_service = TeamcityService.create(
+          project: create(:project),
+          properties: {
+            teamcity_url: 'http://gitlab.com',
+            username: 'mic',
+            password: "password"
+          }
+        )
+      end
+  
+      it "reset password if url changed" do
+        @teamcity_service.teamcity_url = 'http://gitlab1.com'
+        @teamcity_service.save
+        expect(@teamcity_service.password).to be_nil
+      end
+  
+      it "does not reset password if username changed" do
+        @teamcity_service.username = "some_name"
+        @teamcity_service.save
+        expect(@teamcity_service.password).to eq("password")
+      end
+
+      it "does not reset password if new url is set together with password, even if it's the same password" do
+        @teamcity_service.teamcity_url = 'http://gitlab_edited.com'
+        @teamcity_service.password = 'password'
+        @teamcity_service.save
+        expect(@teamcity_service.password).to eq("password")
+        expect(@teamcity_service.teamcity_url).to eq("http://gitlab_edited.com")
+      end
 
-    it "reset password if url changed" do
-      @teamcity_service.teamcity_url = 'http://gitlab1.com'
-      @teamcity_service.save
-      expect(@teamcity_service.password).to be_nil
+      it "should reset password if url changed, even if setter called multiple times" do
+        @teamcity_service.teamcity_url = 'http://gitlab1.com'
+        @teamcity_service.teamcity_url = 'http://gitlab1.com'
+        @teamcity_service.save
+        expect(@teamcity_service.password).to be_nil
+      end
     end
+    
+    context "when no password was previously set" do
+      before do
+        @teamcity_service = TeamcityService.create(
+          project: create(:project),
+          properties: {
+            teamcity_url: 'http://gitlab.com',
+            username: 'mic'
+          }
+        )
+      end
 
-    it "does not reset password if username changed" do
-      @teamcity_service.username = "some_name"
-      @teamcity_service.save
-      expect(@teamcity_service.password).to eq("password")
+      it "saves password if new url is set together with password" do
+        @teamcity_service.teamcity_url = 'http://gitlab_edited.com'
+        @teamcity_service.password = 'password'
+        @teamcity_service.save
+        expect(@teamcity_service.password).to eq("password")
+        expect(@teamcity_service.teamcity_url).to eq("http://gitlab_edited.com")
+      end
     end
   end
 end

spec/models/service_spec.rb

@@ -104,7 +104,7 @@ describe Service do
     end
   end
 
-  describe "#prop_updated?" do
+  describe "{property}_changed?" do
     let(:service) do
       BambooService.create(
         project: create(:project),
@@ -116,14 +116,112 @@ 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
+      expect(service.bamboo_url_changed?).to be_falsy
     end
 
-    it "returns true" do
-      service.bamboo_url = "http://other.com"
-      expect(service.prop_updated?(:bamboo_url)).to be_truthy
+    it "returns true when the property has been assigned a different value" do
+      service.bamboo_url = "http://example.com"
+      expect(service.bamboo_url_changed?).to be_truthy
+    end
+
+    it "returns true when the property has been assigned a different value twice" do
+      service.bamboo_url = "http://example.com"
+      service.bamboo_url = "http://example.com"
+      expect(service.bamboo_url_changed?).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.bamboo_url_changed?).to be_falsy
+    end
+
+    it "returns false when the property has been assigned a new value then saved" do
+      service.bamboo_url = 'http://example.com'
+      service.save
+      expect(service.bamboo_url_changed?).to be_falsy
+    end
+  end
+
+  describe "{property}_touched?" 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.bamboo_url_touched?).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.bamboo_url_touched?).to be_truthy
+    end
+
+    it "returns true when the property has been assigned a different value twice" do
+      service.bamboo_url = "http://example.com"
+      service.bamboo_url = "http://example.com"
+      expect(service.bamboo_url_touched?).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.bamboo_url_touched?).to be_truthy
+    end
+
+    it "returns false when the property has been assigned a new value then saved" do
+      service.bamboo_url = 'http://example.com'
+      service.save
+      expect(service.bamboo_url_changed?).to be_falsy
+    end
+  end
+
+  describe "{property}_was" do
+    let(:service) do
+      BambooService.create(
+        project: create(:project),
+        properties: {
+          bamboo_url: 'http://gitlab.com',
+          username: 'mic',
+          password: "password"
+        }
+      )
+    end
+
+
+    it "returns nil when the property has not been assigned a new value" do
+      service.username = "key_changed"
+      expect(service.bamboo_url_was).to be_nil
+    end
+
+    it "returns the previous value when the property has been assigned a different value" do
+      service.bamboo_url = "http://example.com"
+      expect(service.bamboo_url_was).to eq('http://gitlab.com')
+    end
+
+    it "returns initial value when the property has been re-assigned the same value" do
+      service.bamboo_url = 'http://gitlab.com'
+      expect(service.bamboo_url_was).to eq('http://gitlab.com')
+    end
+
+    it "returns initial value when the property has been assigned multiple values" do
+      service.bamboo_url = "http://example.com"
+      service.bamboo_url = "http://example2.com"
+      expect(service.bamboo_url_was).to eq('http://gitlab.com')
+    end
+
+    it "returns nil when the property has been assigned a new value then saved" do
+      service.bamboo_url = 'http://example.com'
+      service.save
+      expect(service.bamboo_url_was).to be_nil
     end
   end
 end