Merge branch 'force_user_to_reenter_integration_password_299323' into 'master'

Force user to re-enter integration password

See merge request gitlab-org/gitlab!65569

app/models/integrations/bamboo.rb

@@ -18,7 +18,7 @@ module Integrations
 
     attr_accessor :response
 
-    before_update :reset_password
+    before_validation :reset_password
 
     def reset_password
       if bamboo_url_changed? && !password_touched?

app/models/integrations/jenkins.rb

@@ -8,7 +8,7 @@ module Integrations
 
     prop_accessor :jenkins_url, :project_name, :username, :password
 
-    before_update :reset_password
+    before_validation :reset_password
 
     validates :jenkins_url, presence: true, addressable_url: true, if: :activated?
     validates :project_name, presence: true, if: :activated?

app/models/integrations/jira.rb

@@ -33,7 +33,7 @@ module Integrations
     data_field :username, :password, :url, :api_url, :jira_issue_transition_automatic, :jira_issue_transition_id, :project_key, :issues_enabled,
       :vulnerabilities_enabled, :vulnerabilities_issuetype
 
-    before_update :reset_password
+    before_validation :reset_password
     after_commit :update_deployment_type, on: [:create, :update], if: :update_deployment_type?
 
     enum comment_detail: {
@@ -65,7 +65,10 @@ module Integrations
     end
 
     def reset_password
-      data_fields.password = nil if reset_password?
+      return unless reset_password?
+
+      data_fields.password = nil
+      properties.delete('password') if properties
     end
 
     def set_default_data

app/models/integrations/teamcity.rb

@@ -18,7 +18,7 @@ module Integrations
 
     attr_accessor :response
 
-    before_update :reset_password
+    before_validation :reset_password
 
     class << self
       def to_param

ee/spec/serializers/integrations/jira_serializers/issue_detail_entity_spec.rb

@@ -131,7 +131,7 @@ RSpec.describe Integrations::JiraSerializers::IssueDetailEntity do
     context 'with only url' do
       before do
         stub_jira_integration_test
-        jira_integration.update!(api_url: nil)
+        jira_integration.update!(api_url: nil, password: 'password')
       end
 
       it 'returns URLs with the web url' do

ee/spec/serializers/integrations/jira_serializers/issue_entity_spec.rb

@@ -95,7 +95,7 @@ RSpec.describe Integrations::JiraSerializers::IssueEntity do
     context 'with only url' do
       before do
         stub_jira_integration_test
-        jira_integration.update!(api_url: nil)
+        jira_integration.update!(api_url: nil, password: 'password')
       end
 
       it 'returns URLs with the web url' do

spec/controllers/admin/integrations_controller_spec.rb

@@ -43,15 +43,15 @@ RSpec.describe Admin::IntegrationsController do
       stub_jira_integration_test
       allow(PropagateIntegrationWorker).to receive(:perform_async)
 
-      put :update, params: { id: integration.class.to_param, service: { url: url } }
+      put :update, params: { id: integration.class.to_param, service: params }
     end
 
     context 'valid params' do
-      let(:url) { 'https://jira.gitlab-example.com' }
+      let(:params) { { url: 'https://jira.gitlab-example.com', password: 'password' } }
 
       it 'updates the integration' do
         expect(response).to have_gitlab_http_status(:found)
-        expect(integration.reload.url).to eq(url)
+        expect(integration.reload).to have_attributes(params)
       end
 
       it 'calls to PropagateIntegrationWorker' do
@@ -60,12 +60,12 @@ RSpec.describe Admin::IntegrationsController do
     end
 
     context 'invalid params' do
-      let(:url) { 'invalid' }
+      let(:params) { { url: 'invalid', password: 'password' } }
 
       it 'does not update the integration' do
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to render_template(:edit)
-        expect(integration.reload.url).not_to eq(url)
+        expect(integration.reload).not_to have_attributes(params)
       end
 
       it 'does not call to PropagateIntegrationWorker' do

spec/controllers/groups/settings/integrations_controller_spec.rb

@@ -69,25 +69,25 @@ RSpec.describe Groups::Settings::IntegrationsController do
       group.add_owner(user)
       stub_jira_integration_test
 
-      put :update, params: { group_id: group, id: integration.class.to_param, service: { url: url } }
+      put :update, params: { group_id: group, id: integration.class.to_param, service: params }
     end
 
     context 'valid params' do
-      let(:url) { 'https://jira.gitlab-example.com' }
+      let(:params) { { url: 'https://jira.gitlab-example.com', password: 'password' } }
 
       it 'updates the integration' do
         expect(response).to have_gitlab_http_status(:found)
-        expect(integration.reload.url).to eq(url)
+        expect(integration.reload).to have_attributes(params)
       end
     end
 
     context 'invalid params' do
-      let(:url) { 'invalid' }
+      let(:params) { { url: 'invalid', password: 'password' } }
 
       it 'does not update the integration' do
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to render_template(:edit)
-        expect(integration.reload.url).not_to eq(url)
+        expect(integration.reload).not_to have_attributes(params)
       end
     end
   end

spec/controllers/projects/services_controller_spec.rb

@@ -239,22 +239,39 @@ RSpec.describe Projects::ServicesController do
       end
 
       context 'when update succeeds' do
-        let(:integration_params) { { url: 'http://example.com' } }
+        let(:integration_params) { { url: 'http://example.com', password: 'password' } }
 
-        it 'returns JSON response with no errors' do
+        it 'returns success response' do
           expect(response).to be_successful
-          expect(json_response).to include('active' => true, 'errors' => {})
+          expect(json_response).to include(
+            'active' => true,
+            'errors' => {}
+          )
+        end
+      end
+
+      context 'when update fails with missing password' do
+        let(:integration_params) { { url: 'http://example.com' } }
+
+        it 'returns JSON response errors' do
+          expect(response).not_to be_successful
+          expect(json_response).to include(
+            'active' => true,
+            'errors' => {
+              'password' => ["can't be blank"]
+            }
+          )
         end
       end
 
-      context 'when update fails' do
-        let(:integration_params) { { url: '' } }
+      context 'when update fails with invalid URL' do
+        let(:integration_params) { { url: '', password: 'password' } }
 
         it 'returns JSON response with errors' do
           expect(response).to have_gitlab_http_status(:unprocessable_entity)
           expect(json_response).to include(
             'active' => true,
-            'errors' => { 'url' => ['must be a valid URL', %(can't be blank)] }
+            'errors' => { 'url' => ['must be a valid URL', "can't be blank"] }
           )
         end
       end

spec/factories/integrations.rb

@@ -53,7 +53,7 @@ FactoryBot.define do
     transient do
       create_data { true }
       url { 'https://jira.example.com' }
-      api_url { nil }
+      api_url { '' }
       username { 'jira_username' }
       password { 'jira_password' }
       jira_issue_transition_automatic { false }

spec/fixtures/trace/sample_trace

@@ -2592,7 +2592,7 @@ TeamcityService
       should not validate that :username cannot be empty/falsy
       should not validate that :password cannot be empty/falsy
   Callbacks
-    before_update :reset_password
+    before_validation :reset_password
       saves password if new url is set together with password when no password was previously set
       when a password was previously set
         resets password if url changed

spec/models/integrations/bamboo_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Integrations::Bamboo, :use_clean_rails_memory_store_caching do
 
   subject(:integration) do
     described_class.create!(
+      active: true,
       project: project,
       properties: {
         bamboo_url: bamboo_url,
@@ -74,27 +75,27 @@ RSpec.describe Integrations::Bamboo, :use_clean_rails_memory_store_caching do
   end
 
   describe 'Callbacks' do
-    describe 'before_update :reset_password' do
+    describe 'before_validation :reset_password' do
       context 'when a password was previously set' do
         it 'resets password if url changed' do
           integration.bamboo_url = 'http://gitlab1.com'
-          integration.save!
 
+          expect(integration).not_to be_valid
           expect(integration.password).to be_nil
         end
 
         it 'does not reset password if username changed' do
           integration.username = 'some_name'
-          integration.save!
 
+          expect(integration).to be_valid
           expect(integration.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
           integration.bamboo_url = 'http://gitlab_edited.com'
           integration.password = 'password'
-          integration.save!
 
+          expect(integration).to be_valid
           expect(integration.password).to eq('password')
           expect(integration.bamboo_url).to eq('http://gitlab_edited.com')
         end
@@ -107,8 +108,10 @@ RSpec.describe Integrations::Bamboo, :use_clean_rails_memory_store_caching do
         integration.password = 'password'
         integration.save!
 
-        expect(integration.password).to eq('password')
-        expect(integration.bamboo_url).to eq('http://gitlab_edited.com')
+        expect(integration.reload).to have_attributes(
+          bamboo_url: 'http://gitlab_edited.com',
+          password: 'password'
+        )
       end
     end
   end

spec/models/integrations/jenkins_spec.rb

@@ -200,21 +200,21 @@ RSpec.describe Integrations::Jenkins do
 
       it 'resets password if url changed' do
         jenkins_integration.jenkins_url = 'http://jenkins-edited.example.com/'
-        jenkins_integration.save!
+        jenkins_integration.valid?
 
         expect(jenkins_integration.password).to be_nil
       end
 
       it 'resets password if username is blank' do
         jenkins_integration.username = ''
-        jenkins_integration.save!
+        jenkins_integration.valid?
 
         expect(jenkins_integration.password).to be_nil
       end
 
       it 'does not reset password if username changed' do
         jenkins_integration.username = 'some_name'
-        jenkins_integration.save!
+        jenkins_integration.valid?
 
         expect(jenkins_integration.password).to eq('password')
       end
@@ -222,7 +222,7 @@ RSpec.describe Integrations::Jenkins do
       it 'does not reset password if new url is set together with password, even if it\'s the same password' do
         jenkins_integration.jenkins_url = 'http://jenkins_edited.example.com/'
         jenkins_integration.password = 'password'
-        jenkins_integration.save!
+        jenkins_integration.valid?
 
         expect(jenkins_integration.password).to eq('password')
         expect(jenkins_integration.jenkins_url).to eq('http://jenkins_edited.example.com/')
@@ -231,7 +231,7 @@ RSpec.describe Integrations::Jenkins do
       it 'resets password if url changed, even if setter called multiple times' do
         jenkins_integration.jenkins_url = 'http://jenkins1.example.com/'
         jenkins_integration.jenkins_url = 'http://jenkins1.example.com/'
-        jenkins_integration.save!
+        jenkins_integration.valid?
 
         expect(jenkins_integration.password).to be_nil
       end
@@ -253,8 +253,10 @@ RSpec.describe Integrations::Jenkins do
         jenkins_integration.password = 'password'
         jenkins_integration.save!
 
-        expect(jenkins_integration.password).to eq('password')
-        expect(jenkins_integration.jenkins_url).to eq('http://jenkins_edited.example.com/')
+        expect(jenkins_integration.reload).to have_attributes(
+          jenkins_url: 'http://jenkins_edited.example.com/',
+          password: 'password'
+        )
       end
     end
   end

spec/models/integrations/jira_spec.rb

@@ -280,7 +280,7 @@ RSpec.describe Integrations::Jira do
 
               expect(integration.jira_tracker_data.deployment_server?).to be_truthy
 
-              integration.update!(api_url: 'http://another.url')
+              integration.update!(api_url: 'http://another.url', password: password)
               integration.jira_tracker_data.reload
 
               expect(integration.jira_tracker_data.deployment_cloud?).to be_truthy
@@ -301,13 +301,13 @@ RSpec.describe Integrations::Jira do
           end
 
           it 'calls serverInfo for url' do
-            integration.update!(url: 'http://first.url')
+            integration.update!(url: 'http://first.url', password: password)
 
             expect(WebMock).to have_requested(:get, /serverInfo/)
           end
 
           it 'calls serverInfo for api_url' do
-            integration.update!(api_url: 'http://another.url')
+            integration.update!(api_url: 'http://another.url', password: password)
 
             expect(WebMock).to have_requested(:get, /serverInfo/)
           end
@@ -358,33 +358,33 @@ RSpec.describe Integrations::Jira do
               it 'resets password if url changed' do
                 integration
                 integration.url = 'http://jira_edited.example.com'
-                integration.save!
 
-                expect(integration.reload.url).to eq('http://jira_edited.example.com')
+                expect(integration).not_to be_valid
+                expect(integration.url).to eq('http://jira_edited.example.com')
                 expect(integration.password).to be_nil
               end
 
               it 'does not reset password if url "changed" to the same url as before' do
                 integration.url = 'http://jira.example.com'
-                integration.save!
 
-                expect(integration.reload.url).to eq('http://jira.example.com')
+                expect(integration).to be_valid
+                expect(integration.url).to eq('http://jira.example.com')
                 expect(integration.password).not_to be_nil
               end
 
               it 'resets password if url not changed but api url added' do
                 integration.api_url = 'http://jira_edited.example.com/rest/api/2'
-                integration.save!
 
-                expect(integration.reload.api_url).to eq('http://jira_edited.example.com/rest/api/2')
+                expect(integration).not_to be_valid
+                expect(integration.api_url).to eq('http://jira_edited.example.com/rest/api/2')
                 expect(integration.password).to be_nil
               end
 
               it 'does not reset password if new url is set together with password, even if it\'s the same password' do
                 integration.url = 'http://jira_edited.example.com'
                 integration.password = password
-                integration.save!
 
+                expect(integration).to be_valid
                 expect(integration.password).to eq(password)
                 expect(integration.url).to eq('http://jira_edited.example.com')
               end
@@ -392,32 +392,32 @@ RSpec.describe Integrations::Jira do
               it 'resets password if url changed, even if setter called multiple times' do
                 integration.url = 'http://jira1.example.com/rest/api/2'
                 integration.url = 'http://jira1.example.com/rest/api/2'
-                integration.save!
 
+                expect(integration).not_to be_valid
                 expect(integration.password).to be_nil
               end
 
               it 'does not reset password if username changed' do
                 integration.username = 'some_name'
-                integration.save!
 
-                expect(integration.reload.password).to eq(password)
+                expect(integration).to be_valid
+                expect(integration.password).to eq(password)
               end
 
               it 'does not reset password if password changed' do
                 integration.url = 'http://jira_edited.example.com'
                 integration.password = 'new_password'
-                integration.save!
 
-                expect(integration.reload.password).to eq('new_password')
+                expect(integration).to be_valid
+                expect(integration.password).to eq('new_password')
               end
 
               it 'does not reset password if the password is touched and same as before' do
                 integration.url = 'http://jira_edited.example.com'
                 integration.password = password
-                integration.save!
 
-                expect(integration.reload.password).to eq(password)
+                expect(integration).to be_valid
+                expect(integration.password).to eq(password)
               end
             end
 
@@ -432,22 +432,23 @@ RSpec.describe Integrations::Jira do
 
               it 'resets password if api url changed' do
                 integration.api_url = 'http://jira_edited.example.com/rest/api/2'
-                integration.save!
 
+                expect(integration).not_to be_valid
                 expect(integration.password).to be_nil
               end
 
               it 'does not reset password if url changed' do
                 integration.url = 'http://jira_edited.example.com'
-                integration.save!
 
+                expect(integration).to be_valid
                 expect(integration.password).to eq(password)
               end
 
               it 'resets password if api url set to empty' do
-                integration.update!(api_url: '')
+                integration.api_url = ''
 
-                expect(integration.reload.password).to be_nil
+                expect(integration).not_to be_valid
+                expect(integration.password).to be_nil
               end
             end
           end
@@ -463,8 +464,11 @@ RSpec.describe Integrations::Jira do
               integration.url = 'http://jira_edited.example.com/rest/api/2'
               integration.password = 'password'
               integration.save!
-              expect(integration.reload.password).to eq('password')
-              expect(integration.reload.url).to eq('http://jira_edited.example.com/rest/api/2')
+
+              expect(integration.reload).to have_attributes(
+                url: 'http://jira_edited.example.com/rest/api/2',
+                password: 'password'
+              )
             end
           end
         end
@@ -492,7 +496,7 @@ RSpec.describe Integrations::Jira do
     context 'when data are stored in both properties and separated fields' do
       let(:properties) { data_params }
       let(:integration) do
-        create(:jira_integration, :without_properties_callback, active: false, properties: properties).tap do |integration|
+        create(:jira_integration, :without_properties_callback, properties: properties).tap do |integration|
           create(:jira_tracker_data, data_params.merge(integration: integration))
         end
       end

spec/models/integrations/teamcity_spec.rb

@@ -76,18 +76,18 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do
   describe 'Callbacks' do
     let(:teamcity_integration) { integration }
 
-    describe 'before_update :reset_password' do
+    describe 'before_validation :reset_password' do
       context 'when a password was previously set' do
         it 'resets password if url changed' do
           teamcity_integration.teamcity_url = 'http://gitlab1.com'
-          teamcity_integration.save!
+          teamcity_integration.valid?
 
           expect(teamcity_integration.password).to be_nil
         end
 
         it 'does not reset password if username changed' do
           teamcity_integration.username = 'some_name'
-          teamcity_integration.save!
+          teamcity_integration.valid?
 
           expect(teamcity_integration.password).to eq('password')
         end
@@ -95,7 +95,7 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do
         it "does not reset password if new url is set together with password, even if it's the same password" do
           teamcity_integration.teamcity_url = 'http://gitlab_edited.com'
           teamcity_integration.password = 'password'
-          teamcity_integration.save!
+          teamcity_integration.valid?
 
           expect(teamcity_integration.password).to eq('password')
           expect(teamcity_integration.teamcity_url).to eq('http://gitlab_edited.com')
@@ -109,8 +109,10 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do
         teamcity_integration.password = 'password'
         teamcity_integration.save!
 
-        expect(teamcity_integration.password).to eq('password')
-        expect(teamcity_integration.teamcity_url).to eq('http://gitlab_edited.com')
+        expect(teamcity_integration.reload).to have_attributes(
+          teamcity_url: 'http://gitlab_edited.com',
+          password: 'password'
+        )
       end
     end
   end