Merge branch 'todos_for_expiring_ssh_keys' into 'master'

Create todos for expiring SSH keys

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/168528



Merged-by: Lukas Eipert <leipert@gitlab.com>
Approved-by: Lukas Eipert <leipert@gitlab.com>
Reviewed-by: Michał Zając <mzajac@gitlab.com>
Co-authored-by: Thomas Hutterer <thutterer@gitlab.com>

app/assets/javascripts/todos/components/todo_item_body.vue

@@ -18,6 +18,7 @@ import {
   TODO_ACTION_TYPE_REVIEW_SUBMITTED,
   TODO_ACTION_TYPE_UNMERGEABLE,
   TODO_ACTION_TYPE_SSH_KEY_EXPIRED,
+  TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON,
 } from '../constants';
 export default {
@@ -51,7 +52,8 @@ export default {
         this.todo.action !== TODO_ACTION_TYPE_BUILD_FAILED &&
         this.todo.action !== TODO_ACTION_TYPE_MERGE_TRAIN_REMOVED &&
         this.todo.action !== TODO_ACTION_TYPE_UNMERGEABLE &&
-        this.todo.action !== TODO_ACTION_TYPE_SSH_KEY_EXPIRED
+        this.todo.action !== TODO_ACTION_TYPE_SSH_KEY_EXPIRED &&
+        this.todo.action !== TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON
       );
     },
     userIsAuthor() {
@@ -133,6 +135,10 @@ export default {
         name = s__('Todos|Your SSH key has expired');
       }
+      if (this.todo.action === TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON) {
+        name = s__('Todos|Your SSH key is expiring soon');
+      }
+
       if (!name) {
         Sentry.captureException(
           new Error(`Encountered unknown TODO_ACTION_TYPE ${this.todo.action}`),

app/assets/javascripts/todos/components/todos_filter_bar.vue

@@ -34,6 +34,7 @@ import {
   TODO_ACTION_TYPE_OKR_CHECKIN_REQUESTED,
   TODO_ACTION_TYPE_ADDED_APPROVER,
   TODO_ACTION_TYPE_SSH_KEY_EXPIRED,
+  TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON,
 } from '../constants';
 import GroupToken from './filtered_search_tokens/group_token.vue';
 import ProjectToken from './filtered_search_tokens/project_token.vue';
@@ -174,6 +175,11 @@ export const ACTION_TYPES = [
     value: TODO_ACTION_TYPE_SSH_KEY_EXPIRED,
     title: s__('Todos|SSH key expired'),
   },
+  {
+    id: '15',
+    value: TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON,
+    title: s__('Todos|SSH key expiring soon'),
+  },
 ];
 const DEFAULT_TOKEN_OPTIONS = {

app/assets/javascripts/todos/constants.js

@@ -25,6 +25,7 @@ export const TODO_ACTION_TYPE_REVIEW_SUBMITTED = 'review_submitted';
 export const TODO_ACTION_TYPE_OKR_CHECKIN_REQUESTED = 'okr_checkin_requested';
 export const TODO_ACTION_TYPE_ADDED_APPROVER = 'added_approver';
 export const TODO_ACTION_TYPE_SSH_KEY_EXPIRED = 'ssh_key_expired';
+export const TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON = 'ssh_key_expiring_soon';
 export const TODO_EMPTY_TITLE_POOL = [
   s__("Todos|Good job! Looks like you don't have anything left on your To-Do List"),

app/graphql/types/todo_action_enum.rb

@@ -16,5 +16,6 @@ class TodoActionEnum < BaseEnum
     value 'okr_checkin_requested', value: 12, description: 'An OKR assigned to the user requires an update.'
     value 'added_approver', value: 13, description: 'User was added as an approver.'
     value 'ssh_key_expired', value: 14, description: 'SSH key of the user has expired.'
+    value 'ssh_key_expiring_soon', value: 15, description: 'SSH key of the user will expire soon.'
   end
 end

app/helpers/todos_helper.rb

@@ -31,6 +31,7 @@ def todo_action_name(todo) # rubocop:disable Metrics/CyclomaticComplexity -- Wil
       s_("Todos|requested an OKR update for %{what}"), what: todo.target.title
     )
     when Todo::SSH_KEY_EXPIRED then s_('Todos|Your SSH key has expired')
+    when Todo::SSH_KEY_EXPIRING_SOON then s_('Todos|Your SSH key is expiring soon')
     end
   end
@@ -205,7 +206,8 @@ def todo_actions_options
       { id: Todo::MARKED, text: s_('Todos|Added') },
       { id: Todo::BUILD_FAILED, text: s_('Todos|Pipelines') },
       { id: Todo::MEMBER_ACCESS_REQUESTED, text: s_('Todos|Member access requested') },
-      { id: Todo::SSH_KEY_EXPIRED, text: s_('Todos|SSH key expired') }
+      { id: Todo::SSH_KEY_EXPIRED, text: s_('Todos|SSH key expired') },
+      { id: Todo::SSH_KEY_EXPIRING_SOON, text: s_('Todos|SSH key expiring soon') }
     ]
   end

app/models/todo.rb

@@ -25,6 +25,7 @@ class Todo < ApplicationRecord
   OKR_CHECKIN_REQUESTED = 12 # This is an EE-only feature
   ADDED_APPROVER = 13 # This is an EE-only feature,
   SSH_KEY_EXPIRED = 14
+  SSH_KEY_EXPIRING_SOON = 15
   ACTION_NAMES = {
     ASSIGNED => :assigned,
@@ -40,7 +41,8 @@ class Todo < ApplicationRecord
     REVIEW_SUBMITTED => :review_submitted,
     OKR_CHECKIN_REQUESTED => :okr_checkin_requested,
     ADDED_APPROVER => :added_approver,
-    SSH_KEY_EXPIRED => :ssh_key_expired
+    SSH_KEY_EXPIRED => :ssh_key_expired,
+    SSH_KEY_EXPIRING_SOON => :ssh_key_expiring_soon
   }.freeze
   ACTIONS_MULTIPLE_ALLOWED = [Todo::MENTIONED, Todo::DIRECTLY_ADDRESSED, Todo::MEMBER_ACCESS_REQUESTED].freeze
@@ -128,6 +130,15 @@ def for_group_ids_and_descendants(group_ids)
         ], remove_duplicates: false)
     end
+    def pending_for_expiring_ssh_keys(ssh_key_ids)
+      where(
+        target_type: Key,
+        target_id: ssh_key_ids,
+        action: ::Todo::SSH_KEY_EXPIRING_SOON,
+        state: :pending
+      )
+    end
+
     # Returns `true` if the current user has any todos for the given target with the optional given state.
     #
     # target - The value of the `target_type` column, such as `Issue`.

app/services/keys/expiry_notification_service.rb

@@ -15,6 +15,7 @@ def execute
       return unless allowed?
       if expiring_soon
+        create_expiring_soon_todos if Feature.enabled?(:todos_for_ssh_key_expiry, user)
         trigger_expiring_soon_notification
       else
         create_expired_todos if Feature.enabled?(:todos_for_ssh_key_expiry, user)
@@ -40,6 +41,10 @@ def trigger_expired_notification
       keys.update_all(expiry_notification_delivered_at: Time.current.utc)
     end
+    def create_expiring_soon_todos
+      todo_service.ssh_key_expiring_soon(keys)
+    end
+
     def create_expired_todos
       todo_service.ssh_key_expired(keys)
     end

app/services/todo_service.rb

@@ -164,23 +164,27 @@ def new_award_emoji(awardable, current_user)
     resolve_todos_for_target(awardable, current_user)
   end
+  # When a SSH key is expiring soon we should:
+  #
+  # * create a todo for the user owning that SSH key
+  #
+  def ssh_key_expiring_soon(ssh_keys)
+    create_ssh_key_todos(Array(ssh_keys), ::Todo::SSH_KEY_EXPIRING_SOON)
+  end
+
   # When a SSH key expired we should:
   #
+  # * resolve any corresponding "expiring soon" todo
   # * create a todo for the user owning that SSH key
   #
   def ssh_key_expired(ssh_keys)
     ssh_keys = Array(ssh_keys)
-    ssh_keys.each do |ssh_key|
-      user = ssh_key.user
-      attributes = {
-        target_id: ssh_key.id,
-        target_type: Key,
-        action: ::Todo::SSH_KEY_EXPIRED,
-        author_id: user.id
-      }
-      create_todos(user, attributes, nil, nil)
-    end
+    # Resolve any pending "expiring soon" todos for these keys
+    expiring_key_todos = ::Todo.pending_for_expiring_ssh_keys(ssh_keys.map(&:id))
+    expiring_key_todos.batch_update(state: :done, resolved_by_action: :system_done)
+
+    create_ssh_key_todos(ssh_keys, ::Todo::SSH_KEY_EXPIRED)
   end
   # When user marks a target as todo
@@ -406,6 +410,19 @@ def create_unmergeable_todo(merge_request, todo_author)
     create_todos(todo_author, attributes, project.namespace, project)
   end
+  def create_ssh_key_todos(ssh_keys, action)
+    ssh_keys.each do |ssh_key|
+      user = ssh_key.user
+      attributes = {
+        target_id: ssh_key.id,
+        target_type: Key,
+        action: action,
+        author_id: user.id
+      }
+      create_todos(user, attributes, nil, nil)
+    end
+  end
+
   def attributes_for_target(target)
     attributes = {
       project_id: target&.project&.id,

doc/api/graphql/reference/index.md

@@ -39370,6 +39370,7 @@ Values for sorting timelogs.
 | <a id="todoactionenumreview_requested"></a>`review_requested` | Review was requested from the user. |
 | <a id="todoactionenumreview_submitted"></a>`review_submitted` | Merge request authored by the user received a review. |
 | <a id="todoactionenumssh_key_expired"></a>`ssh_key_expired` | SSH key of the user has expired. |
+| <a id="todoactionenumssh_key_expiring_soon"></a>`ssh_key_expiring_soon` | SSH key of the user will expire soon. |
 | <a id="todoactionenumunmergeable"></a>`unmergeable` | Merge request authored by the user could not be merged. |
 ### `TodoSort`

locale/gitlab.pot

@@ -57258,6 +57258,9 @@ msgstr ""
 msgid "Todos|SSH key expired"
 msgstr ""
+msgid "Todos|SSH key expiring soon"
+msgstr ""
+
 msgid "Todos|Something went wrong. Please try again."
 msgstr ""
@@ -57297,6 +57300,9 @@ msgstr ""
 msgid "Todos|Your SSH key has expired"
 msgstr ""
+msgid "Todos|Your SSH key is expiring soon"
+msgstr ""
+
 msgid "Todos|Your To-Do List shows what to work on next"
 msgstr ""

spec/frontend/todos/components/todo_item_body_spec.js

@@ -17,6 +17,7 @@ import {
   TODO_ACTION_TYPE_REVIEW_SUBMITTED,
   TODO_ACTION_TYPE_UNMERGEABLE,
   TODO_ACTION_TYPE_SSH_KEY_EXPIRED,
+  TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON,
 } from '~/todos/constants';
 describe('TodoItemBody', () => {
@@ -59,21 +60,22 @@ describe('TodoItemBody', () => {
   describe('correct text for actionName', () => {
     it.each`
-      actionName                                  | text                              | showsAuthor
-      ${TODO_ACTION_TYPE_ADDED_APPROVER}          | ${'set you as an approver.'}      | ${true}
-      ${TODO_ACTION_TYPE_APPROVAL_REQUIRED}       | ${'set you as an approver.'}      | ${true}
-      ${TODO_ACTION_TYPE_ASSIGNED}                | ${'assigned you.'}                | ${true}
-      ${TODO_ACTION_TYPE_BUILD_FAILED}            | ${'The pipeline failed.'}         | ${false}
-      ${TODO_ACTION_TYPE_DIRECTLY_ADDRESSED}      | ${'mentioned you.'}               | ${true}
-      ${TODO_ACTION_TYPE_MARKED}                  | ${'added a to-do item'}           | ${true}
-      ${TODO_ACTION_TYPE_MEMBER_ACCESS_REQUESTED} | ${'has requested access to'}      | ${true}
-      ${TODO_ACTION_TYPE_MENTIONED}               | ${'mentioned you.'}               | ${true}
-      ${TODO_ACTION_TYPE_MERGE_TRAIN_REMOVED}     | ${'Removed from Merge Train.'}    | ${false}
-      ${TODO_ACTION_TYPE_OKR_CHECKIN_REQUESTED}   | ${'requested an OKR update for'}  | ${true}
-      ${TODO_ACTION_TYPE_REVIEW_REQUESTED}        | ${'requested a review.'}          | ${true}
-      ${TODO_ACTION_TYPE_REVIEW_SUBMITTED}        | ${'reviewed your merge request.'} | ${true}
-      ${TODO_ACTION_TYPE_UNMERGEABLE}             | ${'Could not merge.'}             | ${false}
-      ${TODO_ACTION_TYPE_SSH_KEY_EXPIRED}         | ${'Your SSH key has expired.'}    | ${false}
+      actionName                                  | text                                | showsAuthor
+      ${TODO_ACTION_TYPE_ADDED_APPROVER}          | ${'set you as an approver.'}        | ${true}
+      ${TODO_ACTION_TYPE_APPROVAL_REQUIRED}       | ${'set you as an approver.'}        | ${true}
+      ${TODO_ACTION_TYPE_ASSIGNED}                | ${'assigned you.'}                  | ${true}
+      ${TODO_ACTION_TYPE_BUILD_FAILED}            | ${'The pipeline failed.'}           | ${false}
+      ${TODO_ACTION_TYPE_DIRECTLY_ADDRESSED}      | ${'mentioned you.'}                 | ${true}
+      ${TODO_ACTION_TYPE_MARKED}                  | ${'added a to-do item'}             | ${true}
+      ${TODO_ACTION_TYPE_MEMBER_ACCESS_REQUESTED} | ${'has requested access to'}        | ${true}
+      ${TODO_ACTION_TYPE_MENTIONED}               | ${'mentioned you.'}                 | ${true}
+      ${TODO_ACTION_TYPE_MERGE_TRAIN_REMOVED}     | ${'Removed from Merge Train.'}      | ${false}
+      ${TODO_ACTION_TYPE_OKR_CHECKIN_REQUESTED}   | ${'requested an OKR update for'}    | ${true}
+      ${TODO_ACTION_TYPE_REVIEW_REQUESTED}        | ${'requested a review.'}            | ${true}
+      ${TODO_ACTION_TYPE_REVIEW_SUBMITTED}        | ${'reviewed your merge request.'}   | ${true}
+      ${TODO_ACTION_TYPE_UNMERGEABLE}             | ${'Could not merge.'}               | ${false}
+      ${TODO_ACTION_TYPE_SSH_KEY_EXPIRED}         | ${'Your SSH key has expired.'}      | ${false}
+      ${TODO_ACTION_TYPE_SSH_KEY_EXPIRING_SOON}   | ${'Your SSH key is expiring soon.'} | ${false}
     `('renders "$text" for the "$actionName" action', ({ actionName, text, showsAuthor }) => {
       createComponent({ action: actionName });
       expect(wrapper.text()).toContain(text);

spec/helpers/todos_helper_spec.rb

@@ -368,6 +368,7 @@
       Todo::MERGE_TRAIN_REMOVED | true  | s_("Todos|Removed from Merge Train")
       Todo::REVIEW_SUBMITTED    | false | s_('Todos|reviewed your merge request')
       Todo::SSH_KEY_EXPIRED     | true  | s_('Todos|Your SSH key has expired')
+      Todo::SSH_KEY_EXPIRING_SOON | true | s_('Todos|Your SSH key is expiring soon')
     end
     with_them do

spec/models/todo_spec.rb

@@ -552,6 +552,18 @@
     end
   end
+  describe '.pending_for_expiring_ssh_keys' do
+    it 'returns only todos matching the given key ids' do
+      todo1 = create(:todo, target_type: Key, target_id: 1, action: described_class::SSH_KEY_EXPIRING_SOON, state: :pending)
+      todo2 = create(:todo, target_type: Key, target_id: 2, action: described_class::SSH_KEY_EXPIRING_SOON, state: :pending)
+      create(:todo, target_type: Key, target_id: 3, action: described_class::SSH_KEY_EXPIRING_SOON, state: :done)
+      create(:todo, target_type: Issue, target_id: 1, action: described_class::ASSIGNED, state: :pending)
+      todos = described_class.pending_for_expiring_ssh_keys([1, 2, 3])
+
+      expect(todos).to contain_exactly(todo1, todo2)
+    end
+  end
+
   describe '.for_user' do
     it 'returns the expected todos' do
       user1 = create(:user)

spec/services/keys/expiry_notification_service_spec.rb

@@ -101,6 +101,7 @@
     let(:expiring_soon) { true }
     context 'when user has permission to receive notification' do
+      it_behaves_like 'creates todo'
       it_behaves_like 'sends a notification'
       it_behaves_like 'uses notification service to send email to the user', :ssh_key_expiring_soon
@@ -108,11 +109,20 @@
       it 'updates notified column' do
         expect { subject.execute }.to change { key.reload.before_expiry_notification_delivered_at }
       end
+
+      context 'when derisk feature flag is disabled' do
+        before do
+          stub_feature_flags(todos_for_ssh_key_expiry: false)
+        end
+
+        it_behaves_like 'does not create todo'
+      end
     end
     context 'when user does NOT have permission to receive notification' do
       include_context 'block user'
+      it_behaves_like 'does not create todo'
       it_behaves_like 'does not send notification'
       it 'does not update notified column' do

spec/services/todo_service_spec.rb

@@ -1121,6 +1121,31 @@
       end
     end
+    describe '#ssh_key_expiring_soon' do
+      let_it_be(:ssh_key) { create(:key, user: author) }
+
+      context 'when given a single key' do
+        it 'creates a pending todo for the user' do
+          service.ssh_key_expiring_soon(ssh_key)
+
+          should_create_todo(user: author, author: author, target: ssh_key, project: nil, action: Todo::SSH_KEY_EXPIRING_SOON)
+        end
+      end
+
+      context 'when given an array of keys' do
+        let_it_be(:ssh_key_of_member) { create(:key, user: member) }
+        let_it_be(:ssh_key_of_guest) { create(:key, user: guest) }
+
+        it 'creates a pending todo for each key with the correct user' do
+          service.ssh_key_expiring_soon([ssh_key, ssh_key_of_member, ssh_key_of_guest])
+
+          should_create_todo(user: author, author: author, target: ssh_key, project: nil, action: Todo::SSH_KEY_EXPIRING_SOON)
+          should_create_todo(user: member, author: member, target: ssh_key_of_member, project: nil, action: Todo::SSH_KEY_EXPIRING_SOON)
+          should_create_todo(user: guest, author: guest, target: ssh_key_of_guest, project: nil, action: Todo::SSH_KEY_EXPIRING_SOON)
+        end
+      end
+    end
+
     describe '#ssh_key_expired' do
       let_it_be(:ssh_key) { create(:key, user: author) }
@@ -1144,6 +1169,24 @@
           should_create_todo(user: guest, author: guest, target: ssh_key_of_guest, project: nil, action: Todo::SSH_KEY_EXPIRED)
         end
       end
+
+      describe 'auto-resolve behavior' do
+        let_it_be(:ssh_key_2) { create(:key, user: author) }
+        let_it_be(:todo_for_expiring_key_1) { create(:todo, target: ssh_key, action: Todo::SSH_KEY_EXPIRING_SOON, user: author) }
+        let_it_be(:todo_for_expiring_key_2) { create(:todo, target: ssh_key_2, action: Todo::SSH_KEY_EXPIRING_SOON, user: author) }
+
+        it 'resolves the "expiring soon" todo for the same key' do
+          service.ssh_key_expired(ssh_key)
+
+          expect(todo_for_expiring_key_1.reload.state).to eq 'done'
+        end
+
+        it 'does not resolve "expiring soon" todos of other keys' do
+          service.ssh_key_expired(ssh_key)
+
+          expect(todo_for_expiring_key_2.state).to eq 'pending'
+        end
+      end
     end
     describe '#merge_request_build_failed' do