From 61b5d757066717372aa4cba0bc60ed280a9f59eb Mon Sep 17 00:00:00 2001 From: manojmj Date: Tue, 3 Mar 2020 11:26:16 +0530 Subject: [PATCH 01/15] Introduce expiry date for SSH keys This change introduces expiry dates for SSH keys. --- app/controllers/profiles/keys_controller.rb | 2 +- app/models/key.rb | 1 + app/views/profiles/keys/_form.html.haml | 13 +++++++++---- app/views/profiles/keys/_key.html.haml | 11 +++++++++-- app/views/profiles/keys/_key_details.html.haml | 3 +++ ...uce-an-optional-expiration-date-for-ssh-keys.yml | 5 +++++ db/migrate/20200303055348_add_expires_at_to_keys.rb | 9 +++++++++ db/schema.rb | 1 + 8 files changed, 38 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/36243-introduce-an-optional-expiration-date-for-ssh-keys.yml create mode 100644 db/migrate/20200303055348_add_expires_at_to_keys.rb diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb index 055d900eecef6a..b9cb71ae89a003 100644 --- a/app/controllers/profiles/keys_controller.rb +++ b/app/controllers/profiles/keys_controller.rb @@ -55,6 +55,6 @@ def get_keys private def key_params - params.require(:key).permit(:title, :key) + params.require(:key).permit(:title, :key, :expires_at) end end diff --git a/app/models/key.rb b/app/models/key.rb index afa0d489ef6572..18fa8aaaa16e49 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -6,6 +6,7 @@ class Key < ApplicationRecord include AfterCommitQueue include Sortable include Sha256Attribute + include Expirable sha256_attribute :fingerprint_sha256 diff --git a/app/views/profiles/keys/_form.html.haml b/app/views/profiles/keys/_form.html.haml index 63ef5eaa17214c..9f93415f0f2fbd 100644 --- a/app/views/profiles/keys/_form.html.haml +++ b/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 - = 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') + .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') + + .col.form-group + = f.label :expires_at, _('Expires at'), class: 'label-bold' + = f.date_field :expires_at, class: "form-control input-lg qa-key-expiry-field", min: Date.current .js-add-ssh-key-validation-warning.hide .bs-callout.bs-callout-warning{ role: 'alert', aria_live: 'assertive' } diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index 0e94e6563fd613..94e671d6eedae5 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -1,7 +1,11 @@ %li.key-list-item .float-left.append-right-10 - if key.valid? - = icon 'key', class: 'settings-list-icon d-none d-sm-block' + - if key.expired? + = icon 'exclamation-triangle', class: 'settings-list-icon d-none d-sm-block has-tooltip', + title: _('Your key has expired') + - else + = icon 'key', 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(', ') @@ -13,8 +17,11 @@ %span.text-truncate = key.fingerprint .last-used-at - last used: + = _('Last used:') = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : 'n/a' + + = _('Expires:') + = key.expires_at ? key.expires_at : 'n/a' .float-right %span.key-created-at = s_('Profiles|Created %{time_ago}'.html_safe) % { time_ago:time_ago_with_tooltip(key.created_at)} diff --git a/app/views/profiles/keys/_key_details.html.haml b/app/views/profiles/keys/_key_details.html.haml index 02f1a267044609..c176bc91d7b44a 100644 --- a/app/views/profiles/keys/_key_details.html.haml +++ b/app/views/profiles/keys/_key_details.html.haml @@ -11,6 +11,9 @@ %li %span.light= _('Created on:') %strong= @key.created_at.to_s(:medium) + %li + %span.light= _('Expires on:') + %strong= @key.expires_at.try(:to_s, :medium) || 'N/A' %li %span.light= _('Last used on:') %strong= @key.last_used_at.try(:to_s, :medium) || 'N/A' diff --git a/changelogs/unreleased/36243-introduce-an-optional-expiration-date-for-ssh-keys.yml b/changelogs/unreleased/36243-introduce-an-optional-expiration-date-for-ssh-keys.yml new file mode 100644 index 00000000000000..1a51203095f85d --- /dev/null +++ b/changelogs/unreleased/36243-introduce-an-optional-expiration-date-for-ssh-keys.yml @@ -0,0 +1,5 @@ +--- +title: Introduce optional expiry date for SSH Keys +merge_request: 26351 +author: +type: added diff --git a/db/migrate/20200303055348_add_expires_at_to_keys.rb b/db/migrate/20200303055348_add_expires_at_to_keys.rb new file mode 100644 index 00000000000000..973e554f03ff1e --- /dev/null +++ b/db/migrate/20200303055348_add_expires_at_to_keys.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddExpiresAtToKeys < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column :keys, :expires_at, :date + end +end diff --git a/db/schema.rb b/db/schema.rb index 06f2011c568890..f2a33d682c21e1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2291,6 +2291,7 @@ t.boolean "public", default: false, null: false t.datetime "last_used_at" t.binary "fingerprint_sha256" + t.date "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)" -- GitLab From 2c43b63c36b8e463733efb6decfc2bf84d47c1c8 Mon Sep 17 00:00:00 2001 From: manojmj Date: Tue, 3 Mar 2020 15:03:20 +0530 Subject: [PATCH 02/15] Add console messaging when using expired key This change adds console messaging to inform a user about usage of expired key --- ee/lib/ee/gitlab/geo_git_access.rb | 6 +++--- ee/lib/ee/gitlab/git_access.rb | 2 +- lib/gitlab/git_access.rb | 16 ++++++++++++++-- locale/gitlab.pot | 12 ++++++++++++ 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/ee/lib/ee/gitlab/geo_git_access.rb b/ee/lib/ee/gitlab/geo_git_access.rb index 609337c4908360..95f4fa9491e75d 100644 --- a/ee/lib/ee/gitlab/geo_git_access.rb +++ b/ee/lib/ee/gitlab/geo_git_access.rb @@ -41,11 +41,11 @@ def custom_action_for(cmd) def messages messages = proxying_to_primary_message - lag_message = current_replication_lag_message + console_messages = check_for_console_messages - return messages unless lag_message + return messages unless console_messages - messages + ['', lag_message] + messages + ['', *console_messages] end def push_to_read_only_message diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index af845723b442c6..6c890844490a12 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -43,7 +43,7 @@ def check_custom_action(cmd) end override :check_for_console_messages - def check_for_console_messages(cmd) + def check_for_console_messages super.push( *current_replication_lag_message ) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 7a13a35c096d5c..fac945ba45076d 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -119,10 +119,22 @@ def check_custom_action(cmd) nil end - def check_for_console_messages(cmd) + def check_for_console_messages + return expired_key_message if key_expired? + [] end + def expired_key_message + <<~STR.split("\n") + INFO: Your SSH key has expired. Please generate a new key. + STR + end + + def key_expired? + actor.is_a?(Key) && actor.expired? + end + def check_valid_actor! return unless actor.is_a?(Key) @@ -375,7 +387,7 @@ def receive_pack_disabled_over_http? protected def success_result(cmd) - ::Gitlab::GitAccessResult::Success.new(console_messages: check_for_console_messages(cmd)) + ::Gitlab::GitAccessResult::Success.new(console_messages: check_for_console_messages) end def changes_list diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9684efba728214..ffae81acfb312b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8215,6 +8215,12 @@ msgstr "" msgid "Expires in %{expires_at}" msgstr "" +msgid "Expires on:" +msgstr "" + +msgid "Expires:" +msgstr "" + msgid "Explain the problem. If appropriate, provide a link to the relevant issue or comment." msgstr "" @@ -11503,6 +11509,9 @@ msgstr "" msgid "Last used on:" msgstr "" +msgid "Last used:" +msgstr "" + msgid "LastCommit|authored" msgstr "" @@ -23150,6 +23159,9 @@ msgstr "" msgid "Your issues will be imported in the background. Once finished, you'll get a confirmation email." msgstr "" +msgid "Your key has expired" +msgstr "" + msgid "Your license is valid from" msgstr "" -- GitLab From 31e63979a72f1f912700fc72e9385195717f58b8 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Tue, 3 Mar 2020 11:59:00 +0000 Subject: [PATCH 03/15] Realigned SSH keys list so it works responsively The existing SSH keys list went a bit bonkers on smaller screens, so we've readjusted the system to use flexbox and be more responsive. It also now lines up all the lower items (last used, expires, created at) in a much more visually pleasing way --- app/assets/stylesheets/pages/profile.scss | 27 ++++++++++++++++++++ app/views/profiles/keys/_key.html.haml | 27 ++++++++++---------- app/views/profiles/keys/_key_table.html.haml | 2 +- 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index 08796742f089e0..6ae8722f977768 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -118,6 +118,33 @@ } } +.ssh-keys .key-list-item { + align-content: flex-start; + display: flex; + justify-content: flex-start; + + .key-list-item-info { + float: unset; + width: 100%; + } + + .key-list-item-dates { + align-items: flex-start; + display: flex; + justify-content: space-between; + } + + .last-used-at, + .expires, + .key-created-at { + line-height: 32px; + } + + .key-delete-btn { + vertical-align: baseline; + } +} + .key-created-at { line-height: 42px; } diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index 94e671d6eedae5..1b0517aa7c2387 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -1,5 +1,5 @@ %li.key-list-item - .float-left.append-right-10 + .append-right-10 - if key.valid? - if key.expired? = icon 'exclamation-triangle', class: 'settings-list-icon d-none d-sm-block has-tooltip', @@ -16,16 +16,17 @@ = 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' - = _('Expires:') - = key.expires_at ? key.expires_at : 'n/a' - .float-right - %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 - %span.sr-only= _('Remove') - = icon('trash') + .key-list-item-dates + %span.last-used-at.append-right-10 + = _('Last used:') + = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : 'n/a' + %span.expires.append-right-10 + = _('Expires:') + = key.expires_at ? key.expires_at : 'n/a' + %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: "key-delete-btn btn btn-transparent prepend-left-10" do + %span.sr-only= _('Remove') + = icon('trash') diff --git a/app/views/profiles/keys/_key_table.html.haml b/app/views/profiles/keys/_key_table.html.haml index 8b8625226451af..ba6cee9a4427a9 100644 --- a/app/views/profiles/keys/_key_table.html.haml +++ b/app/views/profiles/keys/_key_table.html.haml @@ -1,7 +1,7 @@ - is_admin = local_assigns.fetch(:admin, false) - if @keys.any? - %ul.content-list{ data: { qa_selector: 'ssh_keys_list' } } + %ul.content-list.ssh-keys{ data: { qa_selector: 'ssh_keys_list' } } = render partial: 'profiles/keys/key', collection: @keys, locals: { is_admin: is_admin } - else %p.settings-message.text-center -- GitLab From 02b2f0bb3d4ec2e8a16a5c760dd12165d8045792 Mon Sep 17 00:00:00 2001 From: manojmj Date: Wed, 4 Mar 2020 09:55:29 +0530 Subject: [PATCH 04/15] Fix failing tests This commit fixes some failing tests and also does some refactoring --- app/views/profiles/keys/_form.html.haml | 2 +- lib/gitlab/git_access.rb | 9 ++++----- spec/requests/api/internal/base_spec.rb | 1 - 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/views/profiles/keys/_form.html.haml b/app/views/profiles/keys/_form.html.haml index 9f93415f0f2fbd..fe48a6aece072b 100644 --- a/app/views/profiles/keys/_form.html.haml +++ b/app/views/profiles/keys/_form.html.haml @@ -14,7 +14,7 @@ .col.form-group = f.label :expires_at, _('Expires at'), class: 'label-bold' - = f.date_field :expires_at, class: "form-control input-lg qa-key-expiry-field", min: Date.current + = 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' } diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index fac945ba45076d..eab8bfee8ac71a 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -120,15 +120,14 @@ def check_custom_action(cmd) end def check_for_console_messages - return expired_key_message if key_expired? + console_messages = [] + console_messages.push(expired_key_message) if key_expired? - [] + console_messages end def expired_key_message - <<~STR.split("\n") - INFO: Your SSH key has expired. Please generate a new key. - STR + 'INFO: Your SSH key has expired. Please generate a new key.' end def key_expired? diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index 95513776f398bd..6ce4192d339d65 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -591,7 +591,6 @@ 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) end -- GitLab From fc93534c31b53847d53bb4e7ae9d76e330c3ebb0 Mon Sep 17 00:00:00 2001 From: manojmj Date: Wed, 4 Mar 2020 11:40:48 +0530 Subject: [PATCH 05/15] Add messaging for key expiring soon --- lib/gitlab/git_access.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index eab8bfee8ac71a..3fa44cc9dba70a 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -121,19 +121,29 @@ def check_custom_action(cmd) def check_for_console_messages console_messages = [] - console_messages.push(expired_key_message) if key_expired? + + console_messages.push(key_expired_message) if key_expired? + console_messages.push(key_expires_soon_message) if key_expires_soon? console_messages end - def expired_key_message + def key_expired_message 'INFO: Your SSH key has expired. Please generate a new key.' end + def key_expires_soon_message + 'INFO: Your SSH key is expiring soon. Please generate a new key.' + end + def key_expired? actor.is_a?(Key) && actor.expired? end + def key_expires_soon? + actor.is_a?(Key) && actor.expires_soon? + end + def check_valid_actor! return unless actor.is_a?(Key) -- GitLab From 5371e702b15ebd1212498595ae6ca587ba32009b Mon Sep 17 00:00:00 2001 From: manojmj Date: Thu, 5 Mar 2020 10:29:12 +0530 Subject: [PATCH 06/15] Refactor GitAccess code This change refactors some code by moving it to a new class --- app/views/profiles/keys/_key.html.haml | 4 +- .../profiles/keys/_key_details.html.haml | 4 +- lib/gitlab/auth/key_status_checker.rb | 44 ++++++++++++ lib/gitlab/git_access.rb | 33 ++++----- locale/gitlab.pot | 9 +++ .../gitlab/auth/key_status_checker_spec.rb | 68 +++++++++++++++++++ 6 files changed, 139 insertions(+), 23 deletions(-) create mode 100644 lib/gitlab/auth/key_status_checker.rb create mode 100644 spec/lib/gitlab/auth/key_status_checker_spec.rb diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index 1b0517aa7c2387..23c04f517cfe3d 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -20,10 +20,10 @@ .key-list-item-dates %span.last-used-at.append-right-10 = _('Last used:') - = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : 'n/a' + = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : _('Never') %span.expires.append-right-10 = _('Expires:') - = key.expires_at ? key.expires_at : 'n/a' + = key.expires_at ? key.expires_at : _('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? diff --git a/app/views/profiles/keys/_key_details.html.haml b/app/views/profiles/keys/_key_details.html.haml index c176bc91d7b44a..b6f631bf332029 100644 --- a/app/views/profiles/keys/_key_details.html.haml +++ b/app/views/profiles/keys/_key_details.html.haml @@ -13,10 +13,10 @@ %strong= @key.created_at.to_s(:medium) %li %span.light= _('Expires on:') - %strong= @key.expires_at.try(:to_s, :medium) || 'N/A' + %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? diff --git a/lib/gitlab/auth/key_status_checker.rb b/lib/gitlab/auth/key_status_checker.rb new file mode 100644 index 00000000000000..0a836d0dd42b5b --- /dev/null +++ b/lib/gitlab/auth/key_status_checker.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Gitlab + module Auth + class KeyStatusChecker + include Gitlab::Utils::StrongMemoize + + def initialize(key) + @key = key + end + + def active? + key_status == :active + end + + def status_message + case key_status + when :expired + _('INFO: Your SSH key has expired. Please generate a new key.') + when :expiring_soon + _('INFO: Your SSH key is expiring soon. Please generate a new key.') + when :active + _('INFO: Your SSH key is currently active.') + end + end + + private + + attr_reader :key + + def key_status + strong_memoize(:key_status) do + if key.expired? + :expired + elsif key.expires_soon? + :expiring_soon + else + :active + end + end + end + end + end +end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 3fa44cc9dba70a..0eddc0d36420bc 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -120,32 +120,23 @@ def check_custom_action(cmd) end def check_for_console_messages - console_messages = [] + return console_messages unless key? - console_messages.push(key_expired_message) if key_expired? - console_messages.push(key_expires_soon_message) if key_expires_soon? + key_status = Gitlab::Auth::KeyStatusChecker.new(actor) - console_messages - end - - def key_expired_message - 'INFO: Your SSH key has expired. Please generate a new key.' - end - - def key_expires_soon_message - 'INFO: Your SSH key is expiring soon. Please generate a new key.' - end - - def key_expired? - actor.is_a?(Key) && actor.expired? + if key_status.active? + console_messages + else + console_messages.push(key_status.status_message) + end end - def key_expires_soon? - actor.is_a?(Key) && actor.expires_soon? + 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}." @@ -361,6 +352,10 @@ def ci? actor == :ci end + def key? + actor.is_a?(Key) + end + def can_read_project? if deploy_key? deploy_key.has_access_to?(project) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index ffae81acfb312b..b50504bf72b2d4 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -10506,6 +10506,15 @@ 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 currently active." +msgstr "" + +msgid "INFO: Your SSH key is expiring soon. Please generate a new key." +msgstr "" + msgid "IP Address" msgstr "" diff --git a/spec/lib/gitlab/auth/key_status_checker_spec.rb b/spec/lib/gitlab/auth/key_status_checker_spec.rb new file mode 100644 index 00000000000000..ff122b05fc342c --- /dev/null +++ b/spec/lib/gitlab/auth/key_status_checker_spec.rb @@ -0,0 +1,68 @@ +# 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 '#active?' do + subject { key_status_checker.active? } + + context 'for an expired key' do + let(:key) { expired_key } + + it { is_expected.to be_falsey } + end + + context 'for a key expiring in the next 7 days' do + let(:key) { expiring_soon_key } + + it { is_expected.to be_falsey } + end + + context 'for a key expiring after the next 7 days' do + let(:key) { expires_in_future_key } + + it { is_expected.to be_truthy } + end + + context 'for a key that never expires' do + let(:key) { never_expires_key } + + it { is_expected.to be_truthy } + end + end + + describe '#status_message?' do + subject { key_status_checker.status_message } + + context 'for an expired key' do + let(:key) { expired_key } + + it { is_expected.to match /has expired/ } + end + + context 'for a key expiring in the next 7 days' do + let(:key) { expiring_soon_key } + + it { is_expected.to match /expiring soon/ } + end + + context 'for a key expiring after the next 7 days' do + let(:key) { expires_in_future_key } + + it { is_expected.to match /active/ } + end + + context 'for a key that never expires' do + let(:key) { never_expires_key } + + it { is_expected.to match /active/ } + end + end +end -- GitLab From 274e21839508ea2bf7f7bad6be8cb08af6d421ad Mon Sep 17 00:00:00 2001 From: manojmj Date: Thu, 5 Mar 2020 14:24:50 +0530 Subject: [PATCH 07/15] Add expiry to API This change adds expires_at to the API --- doc/api/keys.md | 2 ++ lib/api/entities/ssh_key.rb | 2 +- .../controllers/profiles/keys_controller_spec.rb | 16 ++++++++++++++++ spec/requests/api/keys_spec.rb | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/doc/api/keys.md b/doc/api/keys.md index 05933e5a1d188d..f0776d23db25ce 100644 --- a/doc/api/keys.md +++ b/doc/api/keys.md @@ -24,6 +24,7 @@ curl --header "PRIVATE-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-03-06" "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-03-06" "user": { "id": 1, "name": "Administrator", diff --git a/lib/api/entities/ssh_key.rb b/lib/api/entities/ssh_key.rb index 0e2f6ebae8c3fe..aae216173c7f2d 100644 --- a/lib/api/entities/ssh_key.rb +++ b/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 diff --git a/spec/controllers/profiles/keys_controller_spec.rb b/spec/controllers/profiles/keys_controller_spec.rb index 3bed117deb0797..6e7c6c358de901 100644 --- a/spec/controllers/profiles/keys_controller_spec.rb +++ b/spec/controllers/profiles/keys_controller_spec.rb @@ -5,6 +5,22 @@ 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.to_date + + 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 eq(expires_at) + end + end + describe "#get_keys" do describe "non existent user" do it "does not generally work" do diff --git a/spec/requests/api/keys_spec.rb b/spec/requests/api/keys_spec.rb index df0bae603b167f..896995afe8c927 100644 --- a/spec/requests/api/keys_spec.rb +++ b/spec/requests/api/keys_spec.rb @@ -5,7 +5,7 @@ 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 @@ get api("/keys/#{key.id}", admin) expect(response).to have_gitlab_http_status(:ok) expect(json_response['title']).to eq(key.title) + expect(json_response['expires_at']).to eq(key.expires_at.to_s) expect(json_response['user']['id']).to eq(user.id) expect(json_response['user']['username']).to eq(user.username) end -- GitLab From 0c3366b50282ead044134069441f5dfc98036317 Mon Sep 17 00:00:00 2001 From: manojmj Date: Thu, 5 Mar 2020 15:30:02 +0530 Subject: [PATCH 08/15] Correct some code This change refactors some code --- .../profiles/keys/_key_details.html.haml | 2 +- ee/lib/ee/gitlab/geo_git_access.rb | 6 ++-- ee/lib/gitlab/git_access_design.rb | 2 +- lib/gitlab/auth/key_status_checker.rb | 2 +- lib/gitlab/git_access.rb | 12 ++++---- locale/gitlab.pot | 3 -- .../gitlab/auth/key_status_checker_spec.rb | 4 +-- spec/requests/api/internal/base_spec.rb | 30 +++++++++++-------- 8 files changed, 33 insertions(+), 28 deletions(-) diff --git a/app/views/profiles/keys/_key_details.html.haml b/app/views/profiles/keys/_key_details.html.haml index b6f631bf332029..88deb0f11cb167 100644 --- a/app/views/profiles/keys/_key_details.html.haml +++ b/app/views/profiles/keys/_key_details.html.haml @@ -12,7 +12,7 @@ %span.light= _('Created on:') %strong= @key.created_at.to_s(:medium) %li - %span.light= _('Expires on:') + %span.light= _('Expires:') %strong= @key.expires_at.try(:to_s, :medium) || _('Never') %li %span.light= _('Last used on:') diff --git a/ee/lib/ee/gitlab/geo_git_access.rb b/ee/lib/ee/gitlab/geo_git_access.rb index 95f4fa9491e75d..609337c4908360 100644 --- a/ee/lib/ee/gitlab/geo_git_access.rb +++ b/ee/lib/ee/gitlab/geo_git_access.rb @@ -41,11 +41,11 @@ def custom_action_for(cmd) def messages messages = proxying_to_primary_message - console_messages = check_for_console_messages + lag_message = current_replication_lag_message - return messages unless console_messages + return messages unless lag_message - messages + ['', *console_messages] + messages + ['', lag_message] end def push_to_read_only_message diff --git a/ee/lib/gitlab/git_access_design.rb b/ee/lib/gitlab/git_access_design.rb index fb6e91cc0ef501..41bd0bd8d50270 100644 --- a/ee/lib/gitlab/git_access_design.rb +++ b/ee/lib/gitlab/git_access_design.rb @@ -8,7 +8,7 @@ def check(cmd, _changes) check_can_create_design! end - success_result(cmd) + success_result end private diff --git a/lib/gitlab/auth/key_status_checker.rb b/lib/gitlab/auth/key_status_checker.rb index 0a836d0dd42b5b..04aafdfc03c272 100644 --- a/lib/gitlab/auth/key_status_checker.rb +++ b/lib/gitlab/auth/key_status_checker.rb @@ -13,7 +13,7 @@ def active? key_status == :active end - def status_message + def message case key_status when :expired _('INFO: Your SSH key has expired. Please generate a new key.') diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 0eddc0d36420bc..c2c447fd4d109d 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -81,7 +81,7 @@ def check(cmd, changes) check_push_access! end - success_result(cmd) + success_result end def guest_can_download_code? @@ -122,15 +122,17 @@ def check_custom_action(cmd) def check_for_console_messages return console_messages unless key? - key_status = Gitlab::Auth::KeyStatusChecker.new(actor) - if key_status.active? console_messages else - console_messages.push(key_status.status_message) + console_messages.push(key_status.message) end end + def key_status + Gitlab::Auth::KeyStatusChecker.new(actor) + end + def console_messages [] end @@ -390,7 +392,7 @@ def receive_pack_disabled_over_http? protected - def success_result(cmd) + def success_result ::Gitlab::GitAccessResult::Success.new(console_messages: check_for_console_messages) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b50504bf72b2d4..c2575ee1a0e198 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8215,9 +8215,6 @@ msgstr "" msgid "Expires in %{expires_at}" msgstr "" -msgid "Expires on:" -msgstr "" - msgid "Expires:" msgstr "" diff --git a/spec/lib/gitlab/auth/key_status_checker_spec.rb b/spec/lib/gitlab/auth/key_status_checker_spec.rb index ff122b05fc342c..2812c0854ca224 100644 --- a/spec/lib/gitlab/auth/key_status_checker_spec.rb +++ b/spec/lib/gitlab/auth/key_status_checker_spec.rb @@ -38,8 +38,8 @@ end end - describe '#status_message?' do - subject { key_status_checker.status_message } + describe '#message' do + subject { key_status_checker.message } context 'for an expired key' do let(:key) { expired_key } diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index 6ce4192d339d65..be592ac6a5c925 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -575,29 +575,35 @@ 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) - .and_return(console_messages) - end + 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 -- GitLab From e2dc335974db3da1ebabf83c255f6cae199c2c32 Mon Sep 17 00:00:00 2001 From: Robert Hunt Date: Thu, 5 Mar 2020 15:28:52 +0000 Subject: [PATCH 09/15] Moved styles over to utility classes Changed the style class to ssh-keys-list to keep it distinct --- app/assets/stylesheets/pages/profile.scss | 21 +------------------- app/views/profiles/keys/_key.html.haml | 8 ++++---- app/views/profiles/keys/_key_table.html.haml | 2 +- 3 files changed, 6 insertions(+), 25 deletions(-) diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index 6ae8722f977768..43cf0d4bd70d36 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -118,31 +118,12 @@ } } -.ssh-keys .key-list-item { - align-content: flex-start; - display: flex; - justify-content: flex-start; - - .key-list-item-info { - float: unset; - width: 100%; - } - - .key-list-item-dates { - align-items: flex-start; - display: flex; - justify-content: space-between; - } - +.ssh-keys-list { .last-used-at, .expires, .key-created-at { line-height: 32px; } - - .key-delete-btn { - vertical-align: baseline; - } } .key-created-at { diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index 23c04f517cfe3d..a479b5906aa759 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -1,4 +1,4 @@ -%li.key-list-item +%li.key-list-item.d-flex.align-items-start.justify-content-start .append-right-10 - if key.valid? - if key.expired? @@ -11,13 +11,13 @@ title: key.errors.full_messages.join(', ') - .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 - .key-list-item-dates + .key-list-item-dates.d-flex.align-items-start.justify-content-between %span.last-used-at.append-right-10 = _('Last used:') = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : _('Never') @@ -27,6 +27,6 @@ %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: "key-delete-btn 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') diff --git a/app/views/profiles/keys/_key_table.html.haml b/app/views/profiles/keys/_key_table.html.haml index ba6cee9a4427a9..176d7a42002dff 100644 --- a/app/views/profiles/keys/_key_table.html.haml +++ b/app/views/profiles/keys/_key_table.html.haml @@ -1,7 +1,7 @@ - is_admin = local_assigns.fetch(:admin, false) - if @keys.any? - %ul.content-list.ssh-keys{ 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 -- GitLab From 65a0e20c948f79879a7b36a1af2843d343d79a0b Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 6 Mar 2020 11:43:21 +0530 Subject: [PATCH 10/15] Address code review remarks This change makes some changes as per the code review --- lib/gitlab/auth/key_status_checker.rb | 15 +++++------- lib/gitlab/git_access.rb | 12 ++++------ locale/gitlab.pot | 3 --- .../gitlab/auth/key_status_checker_spec.rb | 24 +++++++++---------- 4 files changed, 23 insertions(+), 31 deletions(-) diff --git a/lib/gitlab/auth/key_status_checker.rb b/lib/gitlab/auth/key_status_checker.rb index 04aafdfc03c272..aeeb9bf2d3a3f2 100644 --- a/lib/gitlab/auth/key_status_checker.rb +++ b/lib/gitlab/auth/key_status_checker.rb @@ -9,18 +9,17 @@ def initialize(key) @key = key end - def active? - key_status == :active + def show_console_message? + key_status == :expired || + key_status == :expires_soon end - def message + def console_message case key_status when :expired _('INFO: Your SSH key has expired. Please generate a new key.') - when :expiring_soon + when :expires_soon _('INFO: Your SSH key is expiring soon. Please generate a new key.') - when :active - _('INFO: Your SSH key is currently active.') end end @@ -33,9 +32,7 @@ def key_status if key.expired? :expired elsif key.expires_soon? - :expiring_soon - else - :active + :expires_soon end end end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index c2c447fd4d109d..7361ccf4aa2db8 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -122,17 +122,15 @@ def check_custom_action(cmd) def check_for_console_messages return console_messages unless key? - if key_status.active? - console_messages + key_status = Gitlab::Auth::KeyStatusChecker.new(actor) + + if key_status.show_console_message? + console_messages.push(key_status.console_message) else - console_messages.push(key_status.message) + console_messages end end - def key_status - Gitlab::Auth::KeyStatusChecker.new(actor) - end - def console_messages [] end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c2575ee1a0e198..38fd1111b7b95e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -10506,9 +10506,6 @@ msgstr "" msgid "INFO: Your SSH key has expired. Please generate a new key." msgstr "" -msgid "INFO: Your SSH key is currently active." -msgstr "" - msgid "INFO: Your SSH key is expiring soon. Please generate a new key." msgstr "" diff --git a/spec/lib/gitlab/auth/key_status_checker_spec.rb b/spec/lib/gitlab/auth/key_status_checker_spec.rb index 2812c0854ca224..b1a540eae81c3a 100644 --- a/spec/lib/gitlab/auth/key_status_checker_spec.rb +++ b/spec/lib/gitlab/auth/key_status_checker_spec.rb @@ -10,59 +10,59 @@ let(:key_status_checker) { described_class.new(key) } - describe '#active?' do - subject { key_status_checker.active? } + 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 be_falsey } + 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 be_falsey } + 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 be_truthy } + it { is_expected.to eq(false) } end context 'for a key that never expires' do let(:key) { never_expires_key } - it { is_expected.to be_truthy } + it { is_expected.to eq(false) } end end - describe '#message' do - subject { key_status_checker.message } + describe '#console_message' do + subject { key_status_checker.console_message } context 'for an expired key' do let(:key) { expired_key } - it { is_expected.to match /has expired/ } + 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 match /expiring soon/ } + 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 match /active/ } + it { is_expected.to be_nil } end context 'for a key that never expires' do let(:key) { never_expires_key } - it { is_expected.to match /active/ } + it { is_expected.to be_nil } end end end -- GitLab From e5b9c26e2564bad369871b2df1408d60677a80ce Mon Sep 17 00:00:00 2001 From: manojmj Date: Mon, 9 Mar 2020 15:02:09 +0530 Subject: [PATCH 11/15] Make expires_at datetime data type This change changes the `expires_at` column from date to datetime --- app/views/profiles/keys/_key.html.haml | 2 +- db/migrate/20200303055348_add_expires_at_to_keys.rb | 2 +- db/schema.rb | 2 +- doc/api/keys.md | 4 ++-- spec/controllers/profiles/keys_controller_spec.rb | 4 ++-- spec/requests/api/keys_spec.rb | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index a479b5906aa759..79ff852cee4069 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -23,7 +23,7 @@ = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : _('Never') %span.expires.append-right-10 = _('Expires:') - = key.expires_at ? key.expires_at : _('Never') + = 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? diff --git a/db/migrate/20200303055348_add_expires_at_to_keys.rb b/db/migrate/20200303055348_add_expires_at_to_keys.rb index 973e554f03ff1e..ef7b813a2ef3b8 100644 --- a/db/migrate/20200303055348_add_expires_at_to_keys.rb +++ b/db/migrate/20200303055348_add_expires_at_to_keys.rb @@ -4,6 +4,6 @@ class AddExpiresAtToKeys < ActiveRecord::Migration[6.0] DOWNTIME = false def change - add_column :keys, :expires_at, :date + add_column :keys, :expires_at, :datetime_with_timezone end end diff --git a/db/schema.rb b/db/schema.rb index f2a33d682c21e1..47063201be438a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2291,7 +2291,7 @@ t.boolean "public", default: false, null: false t.datetime "last_used_at" t.binary "fingerprint_sha256" - t.date "expires_at" + 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)" diff --git a/doc/api/keys.md b/doc/api/keys.md index f0776d23db25ce..d4eb1161a97fc2 100644 --- a/doc/api/keys.md +++ b/doc/api/keys.md @@ -24,7 +24,7 @@ curl --header "PRIVATE-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-03-06" + "expires_at": "2020-05-05T00:00:00.000Z" "user": { "name": "John Smith", "username": "john_smith", @@ -93,7 +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-03-06" + "expires_at": "2020-05-05T00:00:00.000Z" "user": { "id": 1, "name": "Administrator", diff --git a/spec/controllers/profiles/keys_controller_spec.rb b/spec/controllers/profiles/keys_controller_spec.rb index 6e7c6c358de901..8582ecbb06dc65 100644 --- a/spec/controllers/profiles/keys_controller_spec.rb +++ b/spec/controllers/profiles/keys_controller_spec.rb @@ -11,13 +11,13 @@ end it 'creates a new key' do - expires_at = 3.days.from_now.to_date + 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 eq(expires_at) + expect(Key.last.expires_at).to be_like_time(expires_at) end end diff --git a/spec/requests/api/keys_spec.rb b/spec/requests/api/keys_spec.rb index 896995afe8c927..089ee22982c4be 100644 --- a/spec/requests/api/keys_spec.rb +++ b/spec/requests/api/keys_spec.rb @@ -28,7 +28,7 @@ get api("/keys/#{key.id}", admin) expect(response).to have_gitlab_http_status(:ok) expect(json_response['title']).to eq(key.title) - expect(json_response['expires_at']).to eq(key.expires_at.to_s) + 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 -- GitLab From 408bb2107ceeb7ebfdc43cbdfb2c4f8f429ebb73 Mon Sep 17 00:00:00 2001 From: manojmj Date: Mon, 9 Mar 2020 15:30:36 +0530 Subject: [PATCH 12/15] Add docs for key expiry --- doc/ssh/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/ssh/README.md b/doc/ssh/README.md index fa4a5bdccdfab1..38d6cbe8ce22c2 100644 --- a/doc/ssh/README.md +++ b/doc/ssh/README.md @@ -186,6 +186,7 @@ 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. NOTE: **Note:** -- GitLab From 273b7cafd2689f6f2ec04a033c9dd70ee8037ca8 Mon Sep 17 00:00:00 2001 From: manojmj Date: Tue, 10 Mar 2020 12:10:44 +0530 Subject: [PATCH 13/15] Address code review comments This change refactors and make changes as per code review comments --- app/views/profiles/keys/_form.html.haml | 4 ++-- app/views/profiles/keys/_key.html.haml | 6 +++--- lib/gitlab/auth/key_status_checker.rb | 24 ++++++------------------ locale/gitlab.pot | 24 +++++++++++++++--------- 4 files changed, 26 insertions(+), 32 deletions(-) diff --git a/app/views/profiles/keys/_form.html.haml b/app/views/profiles/keys/_form.html.haml index fe48a6aece072b..34e812853289bc 100644 --- a/app/views/profiles/keys/_form.html.haml +++ b/app/views/profiles/keys/_form.html.haml @@ -10,10 +10,10 @@ .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, _('Expires at'), class: 'label-bold' + = 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 diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index 79ff852cee4069..cc6e93b168990e 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -3,7 +3,7 @@ - if key.valid? - if key.expired? = icon 'exclamation-triangle', class: 'settings-list-icon d-none d-sm-block has-tooltip', - title: _('Your key has expired') + title: s_('Profiles|Your key has expired') - else = icon 'key', class: 'settings-list-icon d-none d-sm-block' - else @@ -19,10 +19,10 @@ .key-list-item-dates.d-flex.align-items-start.justify-content-between %span.last-used-at.append-right-10 - = _('Last used:') + = s_('Profiles|Last used:') = key.last_used_at ? time_ago_with_tooltip(key.last_used_at) : _('Never') %span.expires.append-right-10 - = _('Expires:') + = 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)} diff --git a/lib/gitlab/auth/key_status_checker.rb b/lib/gitlab/auth/key_status_checker.rb index aeeb9bf2d3a3f2..af654c0caad4b5 100644 --- a/lib/gitlab/auth/key_status_checker.rb +++ b/lib/gitlab/auth/key_status_checker.rb @@ -5,34 +5,22 @@ module Auth class KeyStatusChecker include Gitlab::Utils::StrongMemoize + attr_reader :key + def initialize(key) @key = key end def show_console_message? - key_status == :expired || - key_status == :expires_soon + console_message.present? end def console_message - case key_status - when :expired - _('INFO: Your SSH key has expired. Please generate a new key.') - when :expires_soon - _('INFO: Your SSH key is expiring soon. Please generate a new key.') - end - end - - private - - attr_reader :key - - def key_status - strong_memoize(:key_status) do + strong_memoize(:console_message) do if key.expired? - :expired + _('INFO: Your SSH key has expired. Please generate a new key.') elsif key.expires_soon? - :expires_soon + _('INFO: Your SSH key is expiring soon. Please generate a new key.') end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 38fd1111b7b95e..a8f81d2714bdd4 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -11512,9 +11512,6 @@ msgstr "" msgid "Last used on:" msgstr "" -msgid "Last used:" -msgstr "" - msgid "LastCommit|authored" msgstr "" @@ -12879,9 +12876,6 @@ msgstr "" msgid "Name new label" msgstr "" -msgid "Name your individual key via a title" -msgstr "" - msgid "Name:" msgstr "" @@ -14869,12 +14863,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 "" @@ -14896,6 +14899,9 @@ msgstr "" msgid "Profiles|Key" msgstr "" +msgid "Profiles|Last used:" +msgstr "" + msgid "Profiles|Learn more" msgstr "" @@ -15052,6 +15058,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 "" @@ -23162,9 +23171,6 @@ msgstr "" msgid "Your issues will be imported in the background. Once finished, you'll get a confirmation email." msgstr "" -msgid "Your key has expired" -msgstr "" - msgid "Your license is valid from" msgstr "" -- GitLab From 61016d5aec12cc140957b8e5810dbc57bcf731ee Mon Sep 17 00:00:00 2001 From: manojmj Date: Wed, 11 Mar 2020 17:10:44 +0530 Subject: [PATCH 14/15] Change CSS classes --- app/views/profiles/keys/_key.html.haml | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index cc6e93b168990e..b227041c9de2b5 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -1,15 +1,14 @@ -%li.key-list-item.d-flex.align-items-start.justify-content-start +%li.d-flex.align-items-center.key-list-item .append-right-10 - if key.valid? - if key.expired? - = icon 'exclamation-triangle', class: 'settings-list-icon d-none d-sm-block has-tooltip', - title: s_('Profiles|Your key has 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 'key', class: 'settings-list-icon d-none d-sm-block' + = sprite_icon('key', 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(', ') - + %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.w-100.float-none = link_to path_to_key(key, is_admin), class: "title" do @@ -29,4 +28,4 @@ - 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 align-baseline" do %span.sr-only= _('Remove') - = icon('trash') + = sprite_icon('remove', size: 16) -- GitLab From aaa43b8bb0a2abad6e24b5bdb6449a3baaf07db8 Mon Sep 17 00:00:00 2001 From: Manoj M J Date: Thu, 12 Mar 2020 04:16:35 +0000 Subject: [PATCH 15/15] Add documentation changes --- doc/ssh/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/ssh/README.md b/doc/ssh/README.md index 38d6cbe8ce22c2..c08e83677eafea 100644 --- a/doc/ssh/README.md +++ b/doc/ssh/README.md @@ -189,6 +189,8 @@ Now, it's time to add the newly created public key to your GitLab account. 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. -- GitLab