Merge branch...

Merge branch '216103-personal-access-token-pat-expiry-is-notifying-a-for-impersonation-tokens-b-at-premium' into 'master'

Prevent emails to user on expiry of impersonation token

See merge request gitlab-org/gitlab!32140

app/models/user.rb

@@ -343,6 +343,7 @@ class User < ApplicationRecord
     where('EXISTS (?)',
           ::PersonalAccessToken
             .where('personal_access_tokens.user_id = users.id')
+            .without_impersonation
             .expiring_and_not_notified(at).select(1))
   end
   scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) }

app/workers/personal_access_tokens/expiring_worker.rb

@@ -15,9 +15,9 @@ module PersonalAccessTokens
         with_context(user: user) do
           notification_service.access_token_about_to_expire(user)
 
-          Rails.logger.info "#{self.class}: Notifying User #{user.id} about expiring tokens" # rubocop:disable Gitlab/RailsLogger
+          Gitlab::AppLogger.info "#{self.class}: Notifying User #{user.id} about expiring tokens"
 
-          user.personal_access_tokens.expiring_and_not_notified(limit_date).update_all(expire_notification_delivered: true)
+          user.personal_access_tokens.without_impersonation.expiring_and_not_notified(limit_date).update_all(expire_notification_delivered: true)
         end
       end
     end

changelogs/unreleased/216103-personal-access-token-pat-expiry-is-notifying-a-for-impersonation-.yml

+---
+title: Prevent emails to user on expiry of impersonation token
+merge_request: 32140
+author:
+type: fixed

db/migrate/20200518091745_add_index_to_personal_access_token_impersonation.rb

+# frozen_string_literal: true
+
+class AddIndexToPersonalAccessTokenImpersonation < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_expired_and_not_notified_personal_access_tokens'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(
+      :personal_access_tokens,
+      [:id, :expires_at],
+      where: "impersonation = FALSE AND revoked = FALSE AND expire_notification_delivered = FALSE",
+      name: INDEX_NAME
+    )
+  end
+
+  def down
+    remove_concurrent_index_by_name(
+      :personal_access_tokens,
+      name: INDEX_NAME
+    )
+  end
+end

db/structure.sql

@@ -9666,6 +9666,8 @@ CREATE INDEX index_events_on_target_type_and_target_id ON public.events USING bt
 
 CREATE INDEX index_evidences_on_release_id ON public.evidences USING btree (release_id);
 
+CREATE INDEX index_expired_and_not_notified_personal_access_tokens ON public.personal_access_tokens USING btree (id, expires_at) WHERE ((impersonation = false) AND (revoked = false) AND (expire_notification_delivered = false));
+
 CREATE UNIQUE INDEX index_external_pull_requests_on_project_and_branches ON public.external_pull_requests USING btree (project_id, source_branch, target_branch);
 
 CREATE UNIQUE INDEX index_feature_flag_scopes_on_flag_id_and_environment_scope ON public.operations_feature_flag_scopes USING btree (feature_flag_id, environment_scope);
@@ -13947,6 +13949,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200514000132
 20200514000340
 20200515155620
+20200518091745
 20200519115908
 20200519171058
 20200519194042

spec/models/personal_access_token_spec.rb

@@ -178,6 +178,15 @@ describe PersonalAccessToken do
         end
       end
     end
+
+    describe '.without_impersonation' do
+      let_it_be(:impersonation_token) { create(:personal_access_token, :impersonation) }
+      let_it_be(:personal_access_token) { create(:personal_access_token) }
+
+      it 'returns only non-impersonation tokens' do
+        expect(described_class.without_impersonation).to contain_exactly(personal_access_token)
+      end
+    end
   end
 
   describe '.simple_sorts' do

spec/models/user_spec.rb

@@ -817,6 +817,7 @@ describe User do
 
       let_it_be(:expired_token) { create(:personal_access_token, user: user1, expires_at: 2.days.ago) }
       let_it_be(:revoked_token) { create(:personal_access_token, user: user1, revoked: true) }
+      let_it_be(:impersonation_token) { create(:personal_access_token, :impersonation, user: user1, expires_at: 2.days.from_now) }
       let_it_be(:valid_token_and_notified) { create(:personal_access_token, user: user2, expires_at: 2.days.from_now, expire_notification_delivered: true) }
       let_it_be(:valid_token1) { create(:personal_access_token, user: user2, expires_at: 2.days.from_now) }
       let_it_be(:valid_token2) { create(:personal_access_token, user: user2, expires_at: 2.days.from_now) }

spec/workers/personal_access_tokens/expiring_worker_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do
 
   describe '#perform' do
     context 'when a token needs to be notified' do
-      let!(:pat) { create(:personal_access_token, expires_at: 5.days.from_now) }
+      let_it_be(:pat) { create(:personal_access_token, expires_at: 5.days.from_now) }
 
       it 'uses notification service to send the email' do
         expect_next_instance_of(NotificationService) do |notification_service|
@@ -23,7 +23,7 @@ RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do
     end
 
     context 'when no tokens need to be notified' do
-      let!(:pat) { create(:personal_access_token, expires_at: 5.days.from_now, expire_notification_delivered: true) }
+      let_it_be(:pat) { create(:personal_access_token, expires_at: 5.days.from_now, expire_notification_delivered: true) }
 
       it "doesn't use notification service to send the email" do
         expect_next_instance_of(NotificationService) do |notification_service|
@@ -33,7 +33,23 @@ RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do
         worker.perform
       end
 
-      it "doesn't change the notificationd delivered of the token" do
+      it "doesn't change the notification delivered of the token" do
+        expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered }
+      end
+    end
+
+    context 'when a token is an impersonation token' do
+      let_it_be(:pat) { create(:personal_access_token, :impersonation, expires_at: 5.days.from_now) }
+
+      it "doesn't use notification service to send the email" do
+        expect_next_instance_of(NotificationService) do |notification_service|
+          expect(notification_service).not_to receive(:access_token_about_to_expire).with(pat.user)
+        end
+
+        worker.perform
+      end
+
+      it "doesn't change the notification delivered of the token" do
         expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered }
       end
     end