Merge branch...

Merge branch '247515-the-ssh-key-page-at-profile-keys-should-mention-that-expired-keys-only-cause-warnings-and' into 'master'

The SSH Key page should mention that expired keys only cause warnings and will continue working

See merge request gitlab-org/gitlab!57494

app/helpers/profiles_helper.rb

@@ -37,4 +37,18 @@ module ProfilesHelper
   def user_status_set_to_busy?(status)
     status&.availability == availability_values[:busy]
   end
+
+  # Overridden in EE::ProfilesHelper#ssh_key_expiration_tooltip
+  def ssh_key_expiration_tooltip(key)
+    return key.errors.full_messages.join(', ') if key.errors.full_messages.any?
+
+    s_('Profiles|Key usable beyond expiration date.') if key.expired?
+  end
+
+  # Overridden in EE::ProfilesHelper#ssh_key_expires_field_description
+  def ssh_key_expires_field_description
+    s_('Profiles|Key can still be used after expiration.')
+  end
 end
+
+ProfilesHelper.prepend_ee_mod

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

@@ -15,11 +15,12 @@
       .col.form-group
         = f.label :expires_at, s_('Profiles|Expires at'), class: 'label-bold'
         = f.date_field :expires_at, class: "form-control input-lg", min: Date.tomorrow, data: { qa_selector: 'key_expiry_date_field' }
+        %p.form-text.text-muted{ data: { qa_selector: 'key_expiry_date_field_description' } }= ssh_key_expires_field_description
 
     .js-add-ssh-key-validation-warning.hide
       .bs-callout.bs-callout-warning{ role: 'alert', aria_live: 'assertive' }
         %strong= _('Oops, are you sure?')
-        %p= s_("Profiles|This doesn't look like a public SSH key, are you sure you want to add it? It will be publicly visible.")
+        %p= s_("Profiles|Publicly visible private SSH keys can compromise your system.")
 
       %button.btn.gl-button.btn-confirm.js-add-ssh-key-validation-confirm-submit= _("Yes, add it")
 

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

+- icon_classes = 'settings-list-icon gl-display-none gl-sm-display-block'
+
 %li.key-list-item
   .gl-display-flex.gl-align-items-flex-start
     .key-list-item-info.gl-w-full.float-none
@@ -5,15 +7,11 @@
         = key.title
 
       .gl-display-flex.gl-align-items-center.gl-mt-2
-        - if key.valid?
-          - if key.expired?
-            %span.gl-display-inline-block.has-tooltip{ title: s_('Profiles|Your key has expired') }
-              = sprite_icon('warning-solid', css_class: 'settings-list-icon gl-display-none gl-sm-display-block')
-          - else
-            = sprite_icon('key', css_class: 'settings-list-icon gl-display-none gl-sm-display-block')
+        - if key.valid? && !key.expired?
+          = sprite_icon('key', css_class: icon_classes)
         - else
-          %span.gl-display-inline-block.has-tooltip{ title: key.errors.full_messages.join(', ') }
-            = sprite_icon('warning-solid', css_class: 'settings-list-icon gl-display-none gl-sm-display-block')
+          %span.gl-display-inline-block.has-tooltip{ title: ssh_key_expiration_tooltip(key) }
+            = sprite_icon('warning-solid', css_class: icon_classes)
 
         %span.gl-text-truncate.gl-sm-ml-3
           = key.fingerprint

ee/app/helpers/ee/profiles_helper.rb

+# frozen_string_literal: true
+
+module EE
+  module ProfilesHelper
+    extend ::Gitlab::Utils::Override
+
+    override :ssh_key_expiration_tooltip
+    def ssh_key_expiration_tooltip(key)
+      return super unless ::Key.expiration_enforced? && key.expired?
+
+      # The key returns a validation error for an invalid keys which is displayed in git related operations.
+      # However, on the UI we don't want to show this message,
+      # so we strip it out and check for any other errors
+      return s_('Profiles|Invalid key.') unless key.errors.full_messages.reject do |m|
+        m.eql?('Key has expired and the instance administrator has enforced expiration')
+      end.empty?
+
+      s_('Profiles|Expired key is not valid.')
+    end
+
+    override :ssh_key_expires_field_description
+    def ssh_key_expires_field_description
+      return super unless ::Key.expiration_enforced?
+
+      s_('Profiles|Key will be deleted on this date.')
+    end
+  end
+end

ee/changelogs/unreleased/247515-the-ssh-key-page-at-profile-keys-should-mention-that-expired-ke.yml

+---
+title: SSH key page text now reflects the expiration enforcement setting being used on the instance
+merge_request: 57494
+author:
+type: changed

ee/spec/helpers/ee/profiles_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ProfilesHelper do
+  before do
+    allow(Key).to receive(:enforce_ssh_key_expiration_feature_available?).and_return(true)
+  end
+
+  describe "#ssh_key_expiration_tooltip" do
+    using RSpec::Parameterized::TableSyntax
+
+    error_message = 'Key type is forbidden. Must be DSA, ECDSA, or ED25519'
+
+    where(:error, :expired, :enforced, :result) do
+      false | false | false | nil
+      true  | false | false | error_message
+      true  | false | true  | error_message
+      true  | true  | false | error_message
+      true  | true  | true  | 'Invalid key.'
+      false | true  | true  | 'Expired key is not valid.'
+      false | true  | false | 'Key usable beyond expiration date.'
+    end
+
+    with_them do
+      let_it_be(:key) { build(:personal_key) }
+
+      it do
+        allow(Key).to receive(:expiration_enforced?).and_return(enforced)
+        key.expires_at = expired ? 2.days.ago : 2.days.from_now
+        key.errors.add(:base, error_message) if error
+
+        expect(helper.ssh_key_expiration_tooltip(key)).to eq(result)
+      end
+    end
+
+    context 'when enforced and expired' do
+      let_it_be(:key) { build(:personal_key) }
+
+      it 'does not return the expiration validation error message', :aggregate_failures do
+        allow(Key).to receive(:expiration_enforced?).and_return(true)
+        key.expires_at = 2.days.ago
+
+        expect(key.invalid?).to eq(true)
+        expect(helper.ssh_key_expiration_tooltip(key)).to eq('Expired key is not valid.')
+      end
+    end
+  end
+
+  describe "#ssh_key_expires_field_description" do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:expiration_enforced, :result) do
+      true  | "Key will be deleted on this date."
+      false | "Key can still be used after expiration."
+    end
+
+    with_them do
+      it do
+        allow(Key).to receive(:expiration_enforced?).and_return(expiration_enforced)
+
+        expect(helper.ssh_key_expires_field_description).to eq(result)
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -23895,6 +23895,9 @@ msgstr ""
 msgid "Profiles|Enter your name, so people you know can recognize you"
 msgstr ""
 
+msgid "Profiles|Expired key is not valid."
+msgstr ""
+
 msgid "Profiles|Expires at"
 msgstr ""
 
@@ -23925,6 +23928,9 @@ msgstr ""
 msgid "Profiles|Increase your account's security by enabling Two-Factor Authentication (2FA)"
 msgstr ""
 
+msgid "Profiles|Invalid key."
+msgstr ""
+
 msgid "Profiles|Invalid password"
 msgstr ""
 
@@ -23934,6 +23940,15 @@ msgstr ""
 msgid "Profiles|Key"
 msgstr ""
 
+msgid "Profiles|Key can still be used after expiration."
+msgstr ""
+
+msgid "Profiles|Key usable beyond expiration date."
+msgstr ""
+
+msgid "Profiles|Key will be deleted on this date."
+msgstr ""
+
 msgid "Profiles|Last used:"
 msgstr ""
 
@@ -23979,6 +23994,9 @@ msgstr ""
 msgid "Profiles|Public email"
 msgstr ""
 
+msgid "Profiles|Publicly visible private SSH keys can compromise your system."
+msgstr ""
+
 msgid "Profiles|Remove avatar"
 msgstr ""
 
@@ -24003,9 +24021,6 @@ msgstr ""
 msgid "Profiles|The maximum file size allowed is 200KB."
 msgstr ""
 
-msgid "Profiles|This doesn't look like a public SSH key, are you sure you want to add it? It will be publicly visible."
-msgstr ""
-
 msgid "Profiles|This email will be displayed on your public profile"
 msgstr ""
 
@@ -24093,9 +24108,6 @@ 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/helpers/profiles_helper_spec.rb

@@ -112,6 +112,46 @@ RSpec.describe ProfilesHelper do
     end
   end
 
+  describe "#ssh_key_expiration_tooltip" do
+    using RSpec::Parameterized::TableSyntax
+
+    before do
+      allow(Key).to receive(:enforce_ssh_key_expiration_feature_available?).and_return(false)
+    end
+
+    error_message = 'Key type is forbidden. Must be DSA, ECDSA, or ED25519'
+
+    where(:error, :expired, :result) do
+      false | false | nil
+      true  | false | error_message
+      false | true  | 'Key usable beyond expiration date.'
+      true  | true  | error_message
+    end
+
+    with_them do
+      let_it_be(:key) do
+        build(:personal_key)
+      end
+
+      it do
+        key.expires_at = expired ? 2.days.ago : 2.days.from_now
+        key.errors.add(:base, error_message) if error
+
+        expect(helper.ssh_key_expiration_tooltip(key)).to eq(result)
+      end
+    end
+  end
+
+  describe "#ssh_key_expires_field_description" do
+    before do
+      allow(Key).to receive(:enforce_ssh_key_expiration_feature_available?).and_return(false)
+    end
+
+    it 'returns the description' do
+      expect(helper.ssh_key_expires_field_description).to eq('Key can still be used after expiration.')
+    end
+  end
+
   def stub_cas_omniauth_provider
     provider = OpenStruct.new(
       'name' => 'cas3',

spec/views/profiles/keys/_form.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'profiles/keys/_form.html.haml' do
+  let_it_be(:key) { Key.new }
+
+  let(:page) { Capybara::Node::Simple.new(rendered) }
+
+  before do
+    assign(:key, key)
+  end
+
+  context 'when the form partial is used' do
+    before do
+      allow(view).to receive(:ssh_key_expires_field_description).and_return('Key can still be used after expiration.')
+
+      render
+    end
+
+    it 'renders the form with the correct action' do
+      expect(page.find('form')['action']).to eq('/-/profile/keys')
+    end
+
+    it 'has the key field', :aggregate_failures do
+      expect(rendered).to have_field('Key', type: 'textarea', placeholder: 'Typically starts with "ssh-ed25519 …" or "ssh-rsa …"')
+      expect(rendered).to have_text("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'. Do not paste your private SSH key, as that can compromise your identity.")
+    end
+
+    it 'has the title field', :aggregate_failures do
+      expect(rendered).to have_field('Title', type: 'text', placeholder: 'e.g. My MacBook key')
+      expect(rendered).to have_text('Give your individual key a title.')
+    end
+
+    it 'has the expires at field', :aggregate_failures do
+      expect(rendered).to have_field('Expires at', type: 'date')
+      expect(page.find_field('Expires at')['min']).to eq(l(1.day.from_now, format: "%Y-%m-%d"))
+      expect(rendered).to have_text('Key can still be used after expiration.')
+    end
+
+    it 'has the validation warning', :aggregate_failures do
+      expect(rendered).to have_text("Oops, are you sure? Publicly visible private SSH keys can compromise your system.")
+      expect(rendered).to have_button('Yes, add it')
+    end
+
+    it 'has the submit button' do
+      expect(rendered).to have_button('Add key')
+    end
+  end
+end

spec/views/profiles/keys/_key.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'profiles/keys/_key.html.haml' do
+  let_it_be(:user) { create(:user) }
+
+  before do
+    allow(view).to receive(:key).and_return(key)
+    allow(view).to receive(:is_admin).and_return(false)
+  end
+
+  context 'when the key partial is used' do
+    let_it_be(:key) do
+      create(:personal_key,
+             user: user,
+             last_used_at: 7.days.ago,
+             expires_at: 2.days.from_now)
+    end
+
+    it 'displays the correct values', :aggregate_failures do
+      render
+
+      expect(rendered).to have_text(key.title)
+      expect(rendered).to have_css('[data-testid="key-icon"]')
+      expect(rendered).to have_text(key.fingerprint)
+      expect(rendered).to have_text(l(key.last_used_at, format: "%b %d, %Y"))
+      expect(rendered).to have_text(l(key.created_at, format: "%b %d, %Y"))
+      expect(rendered).to have_text(key.expires_at.to_date)
+      expect(response).to render_template(partial: 'shared/ssh_keys/_key_delete')
+    end
+
+    context 'when the key has not been used' do
+      let_it_be(:key) do
+        create(:personal_key,
+               user: user,
+               last_used_at: nil)
+      end
+
+      it 'renders "Never" for last used' do
+        render
+
+        expect(rendered).to have_text('Last used: Never')
+      end
+    end
+
+    context 'when the key does not have an expiration date' do
+      let_it_be(:key) do
+        create(:personal_key,
+               user: user,
+               expires_at: nil)
+      end
+
+      it 'renders "Never" for expires' do
+        render
+
+        expect(rendered).to have_text('Expires: Never')
+      end
+    end
+
+    context 'when the key is not deletable' do
+      # Turns out key.can_delete? is only false for LDAP keys
+      # but LDAP keys don't exist outside EE
+      before do
+        allow(key).to receive(:can_delete?).and_return(false)
+      end
+
+      it 'does not render the partial' do
+        render
+
+        expect(response).not_to render_template(partial: 'shared/ssh_keys/_key_delete')
+      end
+    end
+
+    context 'icon tooltip' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:valid, :expiry, :result) do
+        false | 2.days.from_now | 'Key type is forbidden. Must be DSA, ECDSA, or ED25519'
+        false | 2.days.ago      | 'Key type is forbidden. Must be DSA, ECDSA, or ED25519'
+        true  | 2.days.ago      | 'Key usable beyond expiration date.'
+        true  | 2.days.from_now | ''
+      end
+
+      with_them do
+        let_it_be(:key) do
+          create(:personal_key, user: user)
+        end
+
+        it 'renders the correct icon', :aggregate_failures do
+          unless valid
+            stub_application_setting(rsa_key_restriction: ApplicationSetting::FORBIDDEN_KEY_VALUE)
+          end
+
+          key.expires_at = expiry
+
+          render
+
+          if result.empty?
+            expect(rendered).to have_css('[data-testid="key-icon"]')
+          else
+            expect(rendered).to have_css('[data-testid="warning-solid-icon"]')
+            expect(rendered).to have_selector("span.has-tooltip[title='#{result}']")
+          end
+        end
+      end
+    end
+  end
+end