Merge branch '11-4-stable-patch-9' into '11-4-stable'

Prepare 11.4.9 release

See merge request gitlab-org/gitlab-ce!23473

20 files changed, 150 insertions, 33 deletions

diff --git a/app/controllers/admin/impersonation_tokens_controller.rb b/app/controllers/admin/impersonation_tokens_controller.rb
index f5825ecb19a..706bcc1e549 100644
--- a/app/controllers/admin/impersonation_tokens_controller.rb
+++ b/app/controllers/admin/impersonation_tokens_controller.rb
@@ -11,6 +11,7 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController
     @impersonation_token = finder.build(impersonation_token_params)
 
     if @impersonation_token.save
+      PersonalAccessToken.redis_store!(current_user.id, @impersonation_token.token)
       redirect_to admin_user_impersonation_tokens_path, notice: "A new impersonation token has been created."
     else
       set_index_vars
@@ -53,6 +54,8 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController
     @impersonation_token ||= finder.build
     @inactive_impersonation_tokens = finder(state: 'inactive').execute
     @active_impersonation_tokens = finder(state: 'active').execute.order(:expires_at)
+
+    @new_impersonation_token = PersonalAccessToken.redis_getdel(current_user.id)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end
diff --git a/app/views/admin/impersonation_tokens/index.html.haml b/app/views/admin/impersonation_tokens/index.html.haml
index 9e490713ef3..8e869fb4b71 100644
--- a/app/views/admin/impersonation_tokens/index.html.haml
+++ b/app/views/admin/impersonation_tokens/index.html.haml
@@ -5,6 +5,11 @@
 
 .row.prepend-top-default
   .col-lg-12
+    - if @new_impersonation_token
+      = render "shared/personal_access_tokens_created_container", new_token_value: @new_impersonation_token,
+                                                                 container_title: 'Your New Impersonation Token',
+                                                                 clipboard_button_title: 'Copy impersonation token to clipboard'
+
     = render "shared/personal_access_tokens_form", path: admin_user_impersonation_tokens_path, impersonation: true, token: @impersonation_token, scopes: @scopes
 
     = render "shared/personal_access_tokens_table", impersonation: true, active_tokens: @active_impersonation_tokens, inactive_tokens: @inactive_impersonation_tokens
diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml
index c10d4ea1a4d..c1e1eaff942 100644
--- a/app/views/profiles/personal_access_tokens/index.html.haml
+++ b/app/views/profiles/personal_access_tokens/index.html.haml
@@ -14,17 +14,7 @@
 
   .col-lg-8
     - if @new_personal_access_token
-      .created-personal-access-token-container
-        %h5.prepend-top-0
-          Your New Personal Access Token
-        .form-group
-          .input-group
-            = text_field_tag 'created-personal-access-token', @new_personal_access_token, readonly: true, class: "form-control js-select-on-focus", 'aria-describedby' => "created-personal-access-token-help-block"
-            %span.input-group-append
-              = clipboard_button(text: @new_personal_access_token, title: "Copy personal access token to clipboard", placement: "left", class: "input-group-text btn-default btn-clipboard")
-          %span#created-personal-access-token-help-block.form-text.text-muted.text-danger Make sure you save it - you won't be able to access it again.
-
-      %hr
+      = render "shared/personal_access_tokens_created_container", new_token_value: @new_personal_access_token
 
     = render "shared/personal_access_tokens_form", path: profile_personal_access_tokens_path, impersonation: false, token: @personal_access_token, scopes: @scopes
 
diff --git a/app/views/shared/_personal_access_tokens_created_container.html.haml b/app/views/shared/_personal_access_tokens_created_container.html.haml
new file mode 100644
index 00000000000..3150d39b84a
--- /dev/null
+++ b/app/views/shared/_personal_access_tokens_created_container.html.haml
@@ -0,0 +1,14 @@
+- container_title = local_assigns.fetch(:container_title, 'Your New Personal Access Token')
+- clipboard_button_title = local_assigns.fetch(:clipboard_button_title, 'Copy personal access token to clipboard')
+
+.created-personal-access-token-container
+  %h5.prepend-top-0
+    = container_title
+  .form-group
+    .input-group
+      = text_field_tag 'created-personal-access-token', new_token_value, readonly: true, class: "form-control js-select-on-focus", 'aria-describedby' => "created-token-help-block"
+      %span.input-group-append
+        = clipboard_button(text: new_token_value, title: clipboard_button_title, placement: "left", class: "input-group-text btn-default btn-clipboard")
+    %span#created-token-help-block.form-text.text-muted.text-danger Make sure you save it - you won't be able to access it again.
+
+%hr
diff --git a/app/views/shared/_personal_access_tokens_table.html.haml b/app/views/shared/_personal_access_tokens_table.html.haml
index cadac1cc99d..2efd03d4867 100644
--- a/app/views/shared/_personal_access_tokens_table.html.haml
+++ b/app/views/shared/_personal_access_tokens_table.html.haml
@@ -15,8 +15,6 @@
           %th Created
           %th Expires
           %th Scopes
-          - if impersonation
-            %th Token
           %th
       %tbody
         - active_tokens.each do |token|
@@ -30,10 +28,6 @@
               - else
                 %span.token-never-expires-label Never
             %td= token.scopes.present? ? token.scopes.join(", ") : "<no scopes selected>"
-            - if impersonation
-              %td.token-token-container
-                = text_field_tag 'impersonation-token-token', token.token, readonly: true, class: "form-control"
-                = clipboard_button(text: token.token)
             - path = impersonation ? revoke_admin_user_impersonation_token_path(token.user, token) : revoke_profile_personal_access_token_path(token)
             %td= link_to "Revoke", path, method: :put, class: "btn btn-danger float-right", data: { confirm: "Are you sure you want to revoke this #{type} Token? This action cannot be undone." }
 - else
diff --git a/changelogs/unreleased/53763-fix-encrypt-columns-data-loss.yml b/changelogs/unreleased/53763-fix-encrypt-columns-data-loss.yml
new file mode 100644
index 00000000000..44362a8622e
--- /dev/null
+++ b/changelogs/unreleased/53763-fix-encrypt-columns-data-loss.yml
@@ -0,0 +1,5 @@
+---
+title: Correctly handle data-loss scenarios when encrypting columns
+merge_request: 23306
+author:
+type: fixed
diff --git a/changelogs/unreleased/ce-53347_fix_impersonation_tokens.yml b/changelogs/unreleased/ce-53347_fix_impersonation_tokens.yml
new file mode 100644
index 00000000000..6cc743d6f3a
--- /dev/null
+++ b/changelogs/unreleased/ce-53347_fix_impersonation_tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Display impersonation token value only after creation
+merge_request: 22916
+author:
+type: fixed
diff --git a/config/initializers/attr_encrypted_no_db_connection.rb b/config/initializers/attr_encrypted_no_db_connection.rb
index e007666b852..7ad458929db 100644
--- a/config/initializers/attr_encrypted_no_db_connection.rb
+++ b/config/initializers/attr_encrypted_no_db_connection.rb
@@ -1,7 +1,18 @@
 module AttrEncrypted
   module Adapters
     module ActiveRecord
-      module DBConnectionQuerier
+      module GitlabMonkeyPatches
+        # Prevent attr_encrypted from defining virtual accessors for encryption
+        # data when the code and schema are out of sync. See this issue for more
+        # details: https://github.com/attr-encrypted/attr_encrypted/issues/332
+        def attribute_instance_methods_as_symbols_available?
+          false
+        end
+
+        # Prevent attr_encrypted from checking out a database connection
+        # indefinitely. The result of this method is only used when the former
+        # is true, but it is called unconditionally, so there is still value to
+        # ensuring the connection is released
         def attribute_instance_methods_as_symbols
           # Use with_connection so the connection doesn't stay pinned to the thread.
           connected = ::ActiveRecord::Base.connection_pool.with_connection(&:active?) rescue false
@@ -15,7 +26,16 @@ module AttrEncrypted
           end
         end
       end
-      prepend DBConnectionQuerier
     end
   end
 end
+
+# As of v3.1.0, the attr_encrypted gem defines the AttrEncrypted and
+# AttrEncrypted::Adapters::ActiveRecord modules, and uses "extend" to mix them
+# into the ActiveRecord::Base class. This intervention overrides utility methods
+# defined by attr_encrypted to fix two bugs, as detailed above.
+#
+# The methods are used here: https://github.com/attr-encrypted/attr_encrypted/blob/3.1.0/lib/attr_encrypted.rb#L145-158
+ActiveSupport.on_load(:active_record) do
+  extend AttrEncrypted::Adapters::ActiveRecord::GitlabMonkeyPatches
+end
diff --git a/doc/api/users.md b/doc/api/users.md
index 07f03f9c827..b6fb520d6ea 100644
--- a/doc/api/users.md
+++ b/doc/api/users.md
@@ -1069,7 +1069,6 @@ Example response:
 [
    {
       "active" : true,
-      "token" : "EsMo-vhKfXGwX9RKrwiy",
       "scopes" : [
          "api"
       ],
@@ -1086,7 +1085,6 @@ Example response:
          "read_user"
       ],
       "revoked" : true,
-      "token" : "ZcZRpLeEuQRprkRjYydY",
       "name" : "mytoken2",
       "created_at" : "2017-03-17T17:19:28.697Z",
       "id" : 3,
@@ -1122,7 +1120,6 @@ Example response:
 ```json
 {
    "active" : true,
-   "token" : "EsMo-vhKfXGwX9RKrwiy",
    "scopes" : [
       "api"
    ],
@@ -1139,6 +1136,8 @@ Example response:
 
 > Requires admin permissions.
 
+> Token values are returned once. Make sure you save it - you won't be able to access it again.
+
 It creates a new impersonation token. Note that only administrators can do this.
 You are only able to create impersonation tokens to impersonate the user and perform
 both API calls and Git reads and writes. The user will not see these tokens in their profile
diff --git a/ee/changelogs/unreleased/sh-fix-issue-54189.yml b/ee/changelogs/unreleased/sh-fix-issue-54189.yml
deleted file mode 100644
index eee743aa5d9..00000000000
--- a/ee/changelogs/unreleased/sh-fix-issue-54189.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-title: Prevent templated services from being imported
-merge_request:
-author:
-type: security
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 120545792f2..680f2615c79 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1246,7 +1246,11 @@ module API
       expose :token
     end
 
-    class ImpersonationToken < PersonalAccessTokenWithToken
+    class ImpersonationToken < PersonalAccessToken
+      expose :impersonation
+    end
+
+    class ImpersonationTokenWithToken < PersonalAccessTokenWithToken
       expose :impersonation
     end
 
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 501c5cf1df3..431cfb366f7 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -535,7 +535,7 @@ module API
 
           desc 'Create a impersonation token. Available only for admins.' do
             detail 'This feature was introduced in GitLab 9.0'
-            success Entities::ImpersonationToken
+            success Entities::ImpersonationTokenWithToken
           end
           params do
             requires :name, type: String, desc: 'The name of the impersonation token'
@@ -546,7 +546,7 @@ module API
             impersonation_token = finder.build(declared_params(include_missing: false))
 
             if impersonation_token.save
-              present impersonation_token, with: Entities::ImpersonationToken
+              present impersonation_token, with: Entities::ImpersonationTokenWithToken
             else
               render_validation_error!(impersonation_token)
             end
diff --git a/lib/gitlab/background_migration/encrypt_columns.rb b/lib/gitlab/background_migration/encrypt_columns.rb
index 0d333e47e7b..bd5f12276ab 100644
--- a/lib/gitlab/background_migration/encrypt_columns.rb
+++ b/lib/gitlab/background_migration/encrypt_columns.rb
@@ -17,6 +17,12 @@ module Gitlab
     class EncryptColumns
       def perform(model, attributes, from, to)
         model = model.constantize if model.is_a?(String)
+
+        # If sidekiq hasn't undergone a restart, its idea of what columns are
+        # present may be inaccurate, so ensure this is as fresh as possible
+        model.reset_column_information
+        model.define_attribute_methods
+
         attributes = expand_attributes(model, Array(attributes).map(&:to_sym))
 
         model.transaction do
@@ -41,6 +47,14 @@ module Gitlab
           raise "Couldn't determine encrypted column for #{klass}##{attribute}" if
             crypt_column_name.nil?
 
+          raise "#{klass} source column: #{attribute} is missing" unless
+            klass.column_names.include?(attribute.to_s)
+
+          # Running the migration without the destination column being present
+          # leads to data loss
+          raise "#{klass} destination column: #{crypt_column_name} is missing" unless
+            klass.column_names.include?(crypt_column_name.to_s)
+
           [attribute, crypt_column_name]
         end
 
diff --git a/qa/qa/page/profile/personal_access_tokens.rb b/qa/qa/page/profile/personal_access_tokens.rb
index f5ae47dadd0..2d9a0e14f30 100644
--- a/qa/qa/page/profile/personal_access_tokens.rb
+++ b/qa/qa/page/profile/personal_access_tokens.rb
@@ -8,7 +8,7 @@ module QA
           element :scopes_api_radios, "label :scopes"
         end
 
-        view 'app/views/profiles/personal_access_tokens/index.html.haml' do
+        view 'app/views/shared/_personal_access_tokens_created_container.html.haml' do
           element :create_token_field, "text_field_tag 'created-personal-access-token'"
         end
 
diff --git a/spec/controllers/profiles/personal_access_tokens_controller_spec.rb b/spec/controllers/profiles/personal_access_tokens_controller_spec.rb
index ed08a4c1bf2..f5860d4296b 100644
--- a/spec/controllers/profiles/personal_access_tokens_controller_spec.rb
+++ b/spec/controllers/profiles/personal_access_tokens_controller_spec.rb
@@ -39,8 +39,10 @@ describe Profiles::PersonalAccessTokensController do
     let!(:active_personal_access_token) { create(:personal_access_token, user: user) }
     let!(:inactive_personal_access_token) { create(:personal_access_token, :revoked, user: user) }
     let!(:impersonation_personal_access_token) { create(:personal_access_token, :impersonation, user: user) }
+    let(:token_value) { 's3cr3t' }
 
     before do
+      PersonalAccessToken.redis_store!(user.id, token_value)
       get :index
     end
 
@@ -56,5 +58,9 @@ describe Profiles::PersonalAccessTokensController do
       expect(assigns(:active_personal_access_tokens)).not_to include(impersonation_personal_access_token)
       expect(assigns(:inactive_personal_access_tokens)).not_to include(impersonation_personal_access_token)
     end
+
+    it "retrieves newly created personal access token value" do
+      expect(assigns(:new_personal_access_token)).to eql(token_value)
+    end
   end
 end
diff --git a/spec/features/admin/admin_users_impersonation_tokens_spec.rb b/spec/features/admin/admin_users_impersonation_tokens_spec.rb
index e16eae219a4..c7860bebb06 100644
--- a/spec/features/admin/admin_users_impersonation_tokens_spec.rb
+++ b/spec/features/admin/admin_users_impersonation_tokens_spec.rb
@@ -12,6 +12,10 @@ describe 'Admin > Users > Impersonation Tokens', :js do
     find(".settings-message")
   end
 
+  def created_impersonation_token
+    find("#created-personal-access-token").value
+  end
+
   before do
     sign_in(admin)
   end
@@ -39,6 +43,7 @@ describe 'Admin > Users > Impersonation Tokens', :js do
       expect(active_impersonation_tokens).to have_text('api')
       expect(active_impersonation_tokens).to have_text('read_user')
       expect(PersonalAccessTokensFinder.new(impersonation: true).execute.count).to equal(1)
+      expect(created_impersonation_token).not_to be_empty
     end
   end
 
diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb
index 8461cd0027c..dee213a11d4 100644
--- a/spec/features/profiles/personal_access_tokens_spec.rb
+++ b/spec/features/profiles/personal_access_tokens_spec.rb
@@ -43,10 +43,12 @@ describe 'Profile > Personal Access Tokens', :js do
       check "read_user"
 
       click_on "Create personal access token"
+
       expect(active_personal_access_tokens).to have_text(name)
       expect(active_personal_access_tokens).to have_text('In')
       expect(active_personal_access_tokens).to have_text('api')
       expect(active_personal_access_tokens).to have_text('read_user')
+      expect(created_personal_access_token).not_to be_empty
     end
 
     context "when creation fails" do
@@ -57,6 +59,7 @@ describe 'Profile > Personal Access Tokens', :js do
 
         expect { click_on "Create personal access token" }.not_to change { PersonalAccessToken.count }
         expect(page).to have_content("Name cannot be nil")
+        expect(page).not_to have_selector("#created-personal-access-token")
       end
     end
   end
diff --git a/spec/initializers/attr_encrypted_no_db_connection_spec.rb b/spec/initializers/attr_encrypted_no_db_connection_spec.rb
new file mode 100644
index 00000000000..2da9f1cbd96
--- /dev/null
+++ b/spec/initializers/attr_encrypted_no_db_connection_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe 'GitLab monkey-patches to AttrEncrypted' do
+  describe '#attribute_instance_methods_as_symbols_available?' do
+    it 'returns false' do
+      expect(ActiveRecord::Base.__send__(:attribute_instance_methods_as_symbols_available?)).to be_falsy
+    end
+
+    it 'does not define virtual attributes' do
+      klass = Class.new(ActiveRecord::Base) do
+        # We need some sort of table to work on
+        self.table_name = 'projects'
+
+        attr_encrypted :foo
+      end
+
+      instance = klass.new
+
+      aggregate_failures do
+        %w[
+          encrypted_foo encrypted_foo=
+          encrypted_foo_iv encrypted_foo_iv=
+          encrypted_foo_salt encrypted_foo_salt=
+        ].each do |method_name|
+          expect(instance).not_to respond_to(method_name)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/background_migration/encrypt_columns_spec.rb b/spec/lib/gitlab/background_migration/encrypt_columns_spec.rb
index 2a869446753..1d9bac79dcd 100644
--- a/spec/lib/gitlab/background_migration/encrypt_columns_spec.rb
+++ b/spec/lib/gitlab/background_migration/encrypt_columns_spec.rb
@@ -65,5 +65,30 @@ describe Gitlab::BackgroundMigration::EncryptColumns, :migration, schema: 201809
 
       expect(hook).to have_attributes(values)
     end
+
+    it 'reloads the model column information' do
+      expect(model).to receive(:reset_column_information).and_call_original
+      expect(model).to receive(:define_attribute_methods).and_call_original
+
+      subject.perform(model, [:token, :url], 1, 1)
+    end
+
+    it 'fails if a source column is not present' do
+      columns = model.columns.reject { |c| c.name == 'url' }
+      allow(model).to receive(:columns) { columns }
+
+      expect do
+        subject.perform(model, [:token, :url], 1, 1)
+      end.to raise_error(/source column: url is missing/)
+    end
+
+    it 'fails if a destination column is not present' do
+      columns = model.columns.reject { |c| c.name == 'encrypted_url' }
+      allow(model).to receive(:columns) { columns }
+
+      expect do
+        subject.perform(model, [:token, :url], 1, 1)
+      end.to raise_error(/destination column: encrypted_url is missing/)
+    end
   end
 end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 09c1d016081..95a7d74278c 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -1975,11 +1975,11 @@ describe API::Users do
       expect(json_response['message']).to eq('403 Forbidden')
     end
 
-    it 'returns a personal access token' do
+    it 'returns an impersonation token' do
       get api("/users/#{user.id}/impersonation_tokens/#{impersonation_token.id}", admin)
 
       expect(response).to have_gitlab_http_status(200)
-      expect(json_response['token']).to be_present
+      expect(json_response['token']).not_to be_present
       expect(json_response['impersonation']).to be_truthy
     end
   end