Merge branch '36243-introduce-an-optional-expiration-date-for-ssh-keys' into 'master'

Resolve "Introduce an optional expiration date for SSH keys"

See merge request gitlab-org/gitlab!26351

app/assets/stylesheets/pages/profile.scss

@@ -118,6 +118,14 @@
   }
 }
 
+.ssh-keys-list {
+  .last-used-at,
+  .expires,
+  .key-created-at {
+    line-height: 32px;
+  }
+}
+
 .key-created-at {
   line-height: 42px;
 }

app/controllers/profiles/keys_controller.rb

@@ -55,6 +55,6 @@ class Profiles::KeysController < Profiles::ApplicationController
   private
 
   def key_params
-    params.require(:key).permit(:title, :key)
+    params.require(:key).permit(:title, :key, :expires_at)
   end
 end

app/models/key.rb

@@ -6,6 +6,7 @@ class Key < ApplicationRecord
   include AfterCommitQueue
   include Sortable
   include Sha256Attribute
+  include Expirable
 
   sha256_attribute :fingerprint_sha256
 

app/views/profiles/keys/_form.html.haml

@@ -6,10 +6,15 @@
       = f.label :key, s_('Profiles|Key'), class: 'label-bold'
       %p= _("Paste your public SSH key, which is usually contained in the file '~/.ssh/id_ed25519.pub' or '~/.ssh/id_rsa.pub' and begins with 'ssh-ed25519' or 'ssh-rsa'. Don't use your private SSH key.")
       = f.text_area :key, class: "form-control js-add-ssh-key-validation-input qa-key-public-key-field", rows: 8, required: true, placeholder: s_('Profiles|Typically starts with "ssh-ed25519 …" or "ssh-rsa …"')
-    .form-group
+    .form-row
+      .col.form-group
         = f.label :title, _('Title'), class: 'label-bold'
         = f.text_field :title, class: "form-control input-lg qa-key-title-field", required: true, placeholder: s_('Profiles|e.g. My MacBook key')
-      %p.form-text.text-muted= _('Name your individual key via a title')
+        %p.form-text.text-muted= s_('Profiles|Give your individual key a title')
+
+      .col.form-group
+        = f.label :expires_at, s_('Profiles|Expires at'), class: 'label-bold'
+        = f.date_field :expires_at, class: "form-control input-lg qa-key-expiry-field", min: Date.tomorrow
 
     .js-add-ssh-key-validation-warning.hide
       .bs-callout.bs-callout-warning{ role: 'alert', aria_live: 'assertive' }

app/views/profiles/keys/_key.html.haml

-%li.key-list-item
-  .float-left.append-right-10
+%li.d-flex.align-items-center.key-list-item
+  .append-right-10
     - if key.valid?
-      = icon 'key', class: 'settings-list-icon d-none d-sm-block'
+      - if key.expired?
+        %span.d-inline-block.has-tooltip{ title: s_('Profiles|Your key has expired') }
+          = sprite_icon('warning-solid', size: 16, css_class: 'settings-list-icon d-none d-sm-block')
       - else
-      = icon 'exclamation-triangle', class: 'settings-list-icon d-none d-sm-block has-tooltip',
-        title: key.errors.full_messages.join(', ')
-
+        = sprite_icon('key', size: 16, css_class: 'settings-list-icon d-none d-sm-block ')
+    - else
+      %span.d-inline-block.has-tooltip{ title: key.errors.full_messages.join(', ') }
+        = sprite_icon('warning-solid', size: 16, css_class: 'settings-list-icon d-none d-sm-block')
 
-  .key-list-item-info
+  .key-list-item-info.w-100.float-none
     = link_to path_to_key(key, is_admin), class: "title" do
       = key.title
     %span.text-truncate
       = key.fingerprint
-    .last-used-at
-      last used:
-      = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : 'n/a'
-  .float-right
+
+    .key-list-item-dates.d-flex.align-items-start.justify-content-between
+      %span.last-used-at.append-right-10
+        = s_('Profiles|Last used:')
+        = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : _('Never')
+      %span.expires.append-right-10
+        = s_('Profiles|Expires:')
+        = key.expires_at ? key.expires_at.to_date : _('Never')
       %span.key-created-at
         = s_('Profiles|Created %{time_ago}'.html_safe) % { time_ago:time_ago_with_tooltip(key.created_at)}
         - if key.can_delete?
-      = link_to path_to_key(key, is_admin), data: { confirm: _('Are you sure?')}, method: :delete, class: "btn btn-transparent prepend-left-10" do
+          = link_to path_to_key(key, is_admin), data: { confirm: _('Are you sure?')}, method: :delete, class: "btn btn-transparent prepend-left-10 align-baseline" do
             %span.sr-only= _('Remove')
-        = icon('trash')
+            = sprite_icon('remove', size: 16)

app/views/profiles/keys/_key_details.html.haml

@@ -11,9 +11,12 @@
         %li
           %span.light= _('Created on:')
           %strong= @key.created_at.to_s(:medium)
+        %li
+          %span.light= _('Expires:')
+          %strong= @key.expires_at.try(:to_s, :medium) || _('Never')
         %li
           %span.light= _('Last used on:')
-          %strong= @key.last_used_at.try(:to_s, :medium) || 'N/A'
+          %strong= @key.last_used_at.try(:to_s, :medium) || _('Never')
 
   .col-md-8
     = form_errors(@key, type: 'key') unless @key.valid?

app/views/profiles/keys/_key_table.html.haml

 - is_admin = local_assigns.fetch(:admin, false)
 
 - if @keys.any?
-  %ul.content-list{ data: { qa_selector: 'ssh_keys_list' } }
+  %ul.content-list.ssh-keys-list{ data: { qa_selector: 'ssh_keys_list' } }
     = render partial: 'profiles/keys/key', collection: @keys, locals: { is_admin: is_admin }
 - else
   %p.settings-message.text-center

changelogs/unreleased/36243-introduce-an-optional-expiration-date-for-ssh-keys.yml

+---
+title: Introduce optional expiry date for SSH Keys
+merge_request: 26351
+author:
+type: added

db/migrate/20200303055348_add_expires_at_to_keys.rb

+# frozen_string_literal: true
+
+class AddExpiresAtToKeys < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :keys, :expires_at, :datetime_with_timezone
+  end
+end

db/schema.rb

@@ -2291,6 +2291,7 @@ ActiveRecord::Schema.define(version: 2020_03_11_165635) do
     t.boolean "public", default: false, null: false
     t.datetime "last_used_at"
     t.binary "fingerprint_sha256"
+    t.datetime_with_timezone "expires_at"
     t.index ["fingerprint"], name: "index_keys_on_fingerprint", unique: true
     t.index ["fingerprint_sha256"], name: "index_keys_on_fingerprint_sha256"
     t.index ["id", "type"], name: "index_on_deploy_keys_id_and_type_and_public", unique: true, where: "(public = true)"

doc/api/keys.md

@@ -24,6 +24,7 @@ curl --header "PRIVATE-TOKEN: <your_access_token>" 'https://gitlab.example.com/a
   "title": "Sample key 25",
   "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt1256k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=",
   "created_at": "2015-09-03T07:24:44.627Z",
+  "expires_at": "2020-05-05T00:00:00.000Z"
   "user": {
     "name": "John Smith",
     "username": "john_smith",
@@ -92,6 +93,7 @@ Example response:
   "title": "Sample key 1",
   "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt1016k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=",
   "created_at": "2019-11-14T15:11:13.222Z",
+  "expires_at": "2020-05-05T00:00:00.000Z"
   "user": {
     "id": 1,
     "name": "Administrator",

doc/ssh/README.md

@@ -186,8 +186,11 @@ Now, it's time to add the newly created public key to your GitLab account.
    1. Navigating to **SSH Keys** and pasting your **public** key from the clipboard into the **Key** field. If you:
       - Created the key with a comment, this will appear in the **Title** field.
       - Created the key without a comment, give your key an identifiable title like _Work Laptop_ or _Home Workstation_.
+   1. Choose an (optional) expiry date for the key under "Expires at" section. (Introduced in [GitLab 12.9](https://gitlab.com/gitlab-org/gitlab/-/issues/36243))
    1. Click the **Add key** button.
 
+SSH keys that have "expired" using this procedure will still be valid in GitLab workflows. As the GitLab-configured expiration date is not included in the SSH key itself, you can still export public SSH keys as needed.
+
    NOTE: **Note:**
    If you manually copied your public SSH key make sure you copied the entire
    key starting with `ssh-ed25519` (or `ssh-rsa`) and ending with your email.

ee/lib/ee/gitlab/git_access.rb

@@ -43,7 +43,7 @@ module EE
       end
 
       override :check_for_console_messages
-      def check_for_console_messages(cmd)
+      def check_for_console_messages
         super.push(
           *current_replication_lag_message
         )

ee/lib/gitlab/git_access_design.rb

@@ -8,7 +8,7 @@ module Gitlab
         check_can_create_design!
       end
 
-      success_result(cmd)
+      success_result
     end
 
     private

lib/api/entities/ssh_key.rb

@@ -3,7 +3,7 @@
 module API
   module Entities
     class SSHKey < Grape::Entity
-      expose :id, :title, :key, :created_at
+      expose :id, :title, :key, :created_at, :expires_at
     end
   end
 end

lib/gitlab/auth/key_status_checker.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    class KeyStatusChecker
+      include Gitlab::Utils::StrongMemoize
+
+      attr_reader :key
+
+      def initialize(key)
+        @key = key
+      end
+
+      def show_console_message?
+        console_message.present?
+      end
+
+      def console_message
+        strong_memoize(:console_message) do
+          if key.expired?
+            _('INFO: Your SSH key has expired. Please generate a new key.')
+          elsif key.expires_soon?
+            _('INFO: Your SSH key is expiring soon. Please generate a new key.')
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/git_access.rb

@@ -81,7 +81,7 @@ module Gitlab
         check_push_access!
       end
 
-      success_result(cmd)
+      success_result
     end
 
     def guest_can_download_code?
@@ -119,12 +119,24 @@ module Gitlab
       nil
     end
 
-    def check_for_console_messages(cmd)
+    def check_for_console_messages
+      return console_messages unless key?
+
+      key_status = Gitlab::Auth::KeyStatusChecker.new(actor)
+
+      if key_status.show_console_message?
+        console_messages.push(key_status.console_message)
+      else
+        console_messages
+      end
+    end
+
+    def console_messages
       []
     end
 
     def check_valid_actor!
-      return unless actor.is_a?(Key)
+      return unless key?
 
       unless actor.valid?
         raise ForbiddenError, "Your SSH key #{actor.errors[:key].first}."
@@ -340,6 +352,10 @@ module Gitlab
       actor == :ci
     end
 
+    def key?
+      actor.is_a?(Key)
+    end
+
     def can_read_project?
       if deploy_key?
         deploy_key.has_access_to?(project)
@@ -374,8 +390,8 @@ module Gitlab
 
     protected
 
-    def success_result(cmd)
-      ::Gitlab::GitAccessResult::Success.new(console_messages: check_for_console_messages(cmd))
+    def success_result
+      ::Gitlab::GitAccessResult::Success.new(console_messages: check_for_console_messages)
     end
 
     def changes_list

locale/gitlab.pot

@@ -8212,6 +8212,9 @@ msgstr ""
 msgid "Expires in %{expires_at}"
 msgstr ""
 
+msgid "Expires:"
+msgstr ""
+
 msgid "Explain the problem. If appropriate, provide a link to the relevant issue or comment."
 msgstr ""
 
@@ -10497,6 +10500,12 @@ msgstr ""
 msgid "IDE|This option is disabled because you don't have write permissions for the current branch."
 msgstr ""
 
+msgid "INFO: Your SSH key has expired. Please generate a new key."
+msgstr ""
+
+msgid "INFO: Your SSH key is expiring soon. Please generate a new key."
+msgstr ""
+
 msgid "IP Address"
 msgstr ""
 
@@ -12864,9 +12873,6 @@ msgstr ""
 msgid "Name new label"
 msgstr ""
 
-msgid "Name your individual key via a title"
-msgstr ""
-
 msgid "Name:"
 msgstr ""
 
@@ -14854,12 +14860,21 @@ msgstr ""
 msgid "Profiles|Enter your name, so people you know can recognize you"
 msgstr ""
 
+msgid "Profiles|Expires at"
+msgstr ""
+
+msgid "Profiles|Expires:"
+msgstr ""
+
 msgid "Profiles|Feed token was successfully reset"
 msgstr ""
 
 msgid "Profiles|Full name"
 msgstr ""
 
+msgid "Profiles|Give your individual key a title"
+msgstr ""
+
 msgid "Profiles|Impersonation"
 msgstr ""
 
@@ -14881,6 +14896,9 @@ msgstr ""
 msgid "Profiles|Key"
 msgstr ""
 
+msgid "Profiles|Last used:"
+msgstr ""
+
 msgid "Profiles|Learn more"
 msgstr ""
 
@@ -15037,6 +15055,9 @@ msgstr ""
 msgid "Profiles|Your email address was automatically set based on your %{provider_label} account"
 msgstr ""
 
+msgid "Profiles|Your key has expired"
+msgstr ""
+
 msgid "Profiles|Your location was automatically set based on your %{provider_label} account"
 msgstr ""
 

spec/controllers/profiles/keys_controller_spec.rb

@@ -5,6 +5,22 @@ require 'spec_helper'
 describe Profiles::KeysController do
   let(:user) { create(:user) }
 
+  describe 'POST #create' do
+    before do
+      sign_in(user)
+    end
+
+    it 'creates a new key' do
+      expires_at = 3.days.from_now
+
+      expect do
+        post :create, params: { key: build(:key, expires_at: expires_at).attributes }
+      end.to change { Key.count }.by(1)
+
+      expect(Key.last.expires_at).to be_like_time(expires_at)
+    end
+  end
+
   describe "#get_keys" do
     describe "non existent user" do
       it "does not generally work" do

spec/lib/gitlab/auth/key_status_checker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::KeyStatusChecker do
+  let_it_be(:never_expires_key) { build(:personal_key, expires_at: nil) }
+  let_it_be(:expired_key) { build(:personal_key, expires_at: 3.days.ago) }
+  let_it_be(:expiring_soon_key) { build(:personal_key, expires_at: 3.days.from_now) }
+  let_it_be(:expires_in_future_key) { build(:personal_key, expires_at: 14.days.from_now) }
+
+  let(:key_status_checker) { described_class.new(key) }
+
+  describe '#show_console_message?' do
+    subject { key_status_checker.show_console_message? }
+
+    context 'for an expired key' do
+      let(:key) { expired_key }
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'for a key expiring in the next 7 days' do
+      let(:key) { expiring_soon_key }
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'for a key expiring after the next 7 days' do
+      let(:key) { expires_in_future_key }
+
+      it { is_expected.to eq(false) }
+    end
+
+    context 'for a key that never expires' do
+      let(:key) { never_expires_key }
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
+  describe '#console_message' do
+    subject { key_status_checker.console_message }
+
+    context 'for an expired key' do
+      let(:key) { expired_key }
+
+      it { is_expected.to eq('INFO: Your SSH key has expired. Please generate a new key.') }
+    end
+
+    context 'for a key expiring in the next 7 days' do
+      let(:key) { expiring_soon_key }
+
+      it { is_expected.to eq('INFO: Your SSH key is expiring soon. Please generate a new key.') }
+    end
+
+    context 'for a key expiring after the next 7 days' do
+      let(:key) { expires_in_future_key }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'for a key that never expires' do
+      let(:key) { never_expires_key }
+
+      it { is_expected.to be_nil }
+    end
+  end
+end

spec/requests/api/internal/base_spec.rb

@@ -575,30 +575,35 @@ describe API::Internal::Base do
         project.add_developer(user)
       end
 
-      context "git pull" do
-        context "with no console message" do
-          it "has the correct payload" do
+      context 'git pull' do
+        context 'with a key that has expired' do
+          let(:key) { create(:key, user: user, expires_at: 2.days.ago) }
+
+          it 'includes the `key expired` message in the response' do
             pull(key, project)
 
             expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response['gl_console_messages']).to eq([])
+            expect(json_response['gl_console_messages']).to eq(['INFO: Your SSH key has expired. Please generate a new key.'])
           end
         end
 
-        context "with a console message" do
-          let(:console_messages) { ['message for the console'] }
+        context 'with a key that will expire in the next 7 days' do
+          let(:key) { create(:key, user: user, expires_at: 2.days.from_now) }
 
-          it "has the correct payload" do
-            expect_next_instance_of(Gitlab::GitAccess) do |access|
-              expect(access).to receive(:check_for_console_messages)
-                                  .with('git-upload-pack')
-                                  .and_return(console_messages)
+          it 'includes the `key expiring soon` message in the response' do
+            pull(key, project)
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response['gl_console_messages']).to eq(['INFO: Your SSH key is expiring soon. Please generate a new key.'])
+          end
         end
 
+        context 'with a key that has no expiry' do
+          it 'does not include any message in the response' do
             pull(key, project)
 
             expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response['gl_console_messages']).to eq(console_messages)
+            expect(json_response['gl_console_messages']).to eq([])
           end
         end
       end

spec/requests/api/keys_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe API::Keys do
   let(:user)  { create(:user) }
   let(:admin) { create(:admin) }
-  let(:key)   { create(:key, user: user) }
+  let(:key)   { create(:key, user: user, expires_at: 1.day.from_now) }
   let(:email) { create(:email, user: user) }
 
   describe 'GET /keys/:uid' do
@@ -28,6 +28,7 @@ describe API::Keys do
         get api("/keys/#{key.id}", admin)
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['title']).to eq(key.title)
+        expect(Time.parse(json_response['expires_at'])).to be_like_time(key.expires_at)
         expect(json_response['user']['id']).to eq(user.id)
         expect(json_response['user']['username']).to eq(user.username)
       end