Merge branch '222734-add-alert-to-personal-access-tokens' into 'master'

Add expired alert to personal access tokens [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!66915

app/helpers/user_callouts_helper.rb

@@ -62,4 +62,4 @@ module UserCalloutsHelper
   end
 end
 
-UserCalloutsHelper.prepend_mod_with('UserCalloutsHelper')
+UserCalloutsHelper.prepend_mod

app/models/concerns/expirable.rb

@@ -13,6 +13,9 @@ module Expirable
     expires? && expires_at <= Time.current
   end
 
+  # Used in subclasses that override expired?
+  alias_method :expired_original?, :expired?
+
   def expires?
     expires_at.present?
   end

app/models/personal_access_token.rb

@@ -47,6 +47,10 @@ class PersonalAccessToken < ApplicationRecord
     !revoked? && !expired?
   end
 
+  def expired_but_not_enforced?
+    false
+  end
+
   def self.redis_getdel(user_id)
     Gitlab::Redis::SharedState.with do |redis|
       redis_key = redis_shared_state_key(user_id)

app/models/user_callout.rb

@@ -34,7 +34,8 @@ class UserCallout < ApplicationRecord
     cloud_licensing_subscription_activation_banner: 33, # EE-only
     trial_status_reminder_d14: 34,             # EE-only
     trial_status_reminder_d3: 35,              # EE-only
-    security_configuration_devops_alert: 36    # EE-only
+    security_configuration_devops_alert: 36,   # EE-only
+    profile_personal_access_token_expiry: 37   # EE-only
   }
 
   validates :user, presence: true

app/views/shared/access_tokens/_table.html.haml

@@ -7,6 +7,7 @@
 
 %h5
   = _('Active %{type} (%{token_length})') % { type: type_plural, token_length: active_tokens.length }
+
 - if personal && !personal_access_token_expiration_enforced?
   %p.profile-settings-content
     = _("Personal access tokens are not revoked upon expiration.")
@@ -14,6 +15,9 @@
   %p.profile-settings-content
     = _("To see all the user's personal access tokens you must impersonate them first.")
 
+- if personal
+  = render_if_exists 'profiles/personal_access_tokens/token_expiry_notification', active_tokens: active_tokens
+
 - if active_tokens.present?
   .table-responsive
     %table.table.active-tokens
@@ -42,7 +46,7 @@
                 %span.token-never-used-label= _('Never')
             %td
               - if token.expires?
-                - if token.expires_at.past? || token.expires_at.today?
+                - if token.expired? || token.expired_but_not_enforced?
                   %span{ class: 'text-danger has-tooltip', title: _('Token valid until revoked') }
                     = _('Expired')
                 - else

doc/api/graphql/reference/index.md

@@ -15685,6 +15685,7 @@ Name of the feature that the callout is for.
 | <a id="usercalloutfeaturenameenumpersonal_access_token_expiry"></a>`PERSONAL_ACCESS_TOKEN_EXPIRY` | Callout feature name for personal_access_token_expiry. |
 | <a id="usercalloutfeaturenameenumpipeline_needs_banner"></a>`PIPELINE_NEEDS_BANNER` | Callout feature name for pipeline_needs_banner. |
 | <a id="usercalloutfeaturenameenumpipeline_needs_hover_tip"></a>`PIPELINE_NEEDS_HOVER_TIP` | Callout feature name for pipeline_needs_hover_tip. |
+| <a id="usercalloutfeaturenameenumprofile_personal_access_token_expiry"></a>`PROFILE_PERSONAL_ACCESS_TOKEN_EXPIRY` | Callout feature name for profile_personal_access_token_expiry. |
 | <a id="usercalloutfeaturenameenumregistration_enabled_callout"></a>`REGISTRATION_ENABLED_CALLOUT` | Callout feature name for registration_enabled_callout. |
 | <a id="usercalloutfeaturenameenumsecurity_configuration_devops_alert"></a>`SECURITY_CONFIGURATION_DEVOPS_ALERT` | Callout feature name for security_configuration_devops_alert. |
 | <a id="usercalloutfeaturenameenumsecurity_configuration_upgrade_banner"></a>`SECURITY_CONFIGURATION_UPGRADE_BANNER` | Callout feature name for security_configuration_upgrade_banner. |

ee/app/helpers/ee/user_callouts_helper.rb

@@ -14,6 +14,7 @@ module EE
     EOA_BRONZE_PLAN_BANNER         = 'eoa_bronze_plan_banner'
     EOA_BRONZE_PLAN_END_DATE       = '2022-01-26'
     CL_SUBSCRIPTION_ACTIVATION = 'cloud_licensing_subscription_activation_banner'
+    PROFILE_PERSONAL_ACCESS_TOKEN_EXPIRY = 'profile_personal_access_token_expiry'
 
     def render_enable_hashed_storage_warning
       return unless show_enable_hashed_storage_warning?
@@ -71,6 +72,10 @@ module EE
         !user_dismissed?(PERSONAL_ACCESS_TOKEN_EXPIRY, 1.week.ago)
     end
 
+    def show_profile_token_expiry_notification?
+      !token_expiration_enforced? && !user_dismissed?(PROFILE_PERSONAL_ACCESS_TOKEN_EXPIRY, 1.day.ago)
+    end
+
     def show_new_user_signups_cap_reached?
       return false unless current_user&.admin?
       return false if user_dismissed?(NEW_USER_SIGNUPS_CAP_REACHED)

ee/app/models/ee/personal_access_token.rb

@@ -59,6 +59,11 @@ module EE
       false
     end
 
+    override :expired_but_not_enforced?
+    def expired_but_not_enforced?
+      expired_original? && !self.class.expiration_enforced?
+    end
+
     override :revoke!
     def revoke!
       clear_rotation_notification_cache

ee/app/views/profiles/personal_access_tokens/_token_expiry_notification.html.haml

+- return unless show_profile_token_expiry_notification?
+
+- expired_tokens = active_tokens.select(&:expired_but_not_enforced?)
+- return unless expired_tokens.present?
+
+.gl-alert.gl-alert-danger.js-token-expiry-callout.gl-mb-3{ role: 'alert', data: { feature_id: "profile_personal_access_token_expiry", dismiss_endpoint: user_callouts_path, defer_links: "true" } }
+  .gl-alert-container
+    = sprite_icon('error', css_class: 'gl-icon s16 gl-alert-icon')
+    %button.js-close.btn.gl-dismiss-btn.btn-default.btn-sm.gl-button.btn-default-tertiary.btn-icon{ type: 'button', 'aria-label' => _('Dismiss') }
+      = sprite_icon('close', css_class: 'gl-button-icon gl-icon s16')
+    .gl-alert-content
+      %h4.gl-alert-title= n_('%d token has expired', '%d tokens have expired', expired_tokens.size) % expired_tokens.size
+      .gl-alert-body
+        = _('Until revoked, expired personal access tokens pose a security risk.')

ee/spec/helpers/ee/user_callouts_helper_spec.rb

@@ -257,6 +257,29 @@ RSpec.describe EE::UserCalloutsHelper do
     end
   end
 
+  describe '.show_profile_token_expiry_notification?' do
+    subject { helper.show_profile_token_expiry_notification? }
+
+    let_it_be(:user) { create(:user) }
+
+    where(:expiration_enforced?, :dismissed_callout?, :result) do
+      true  | true  | false
+      true  | false | false
+      false | true  | false
+      false | false | true
+    end
+
+    with_them do
+      before do
+        allow(helper).to receive(:current_user).and_return(user)
+        allow(helper).to receive(:token_expiration_enforced?).and_return(expiration_enforced?)
+        allow(helper).to receive(:user_dismissed?).and_return(dismissed_callout?)
+      end
+
+      it { is_expected.to be result }
+    end
+  end
+
   describe '.show_new_user_signups_cap_reached?' do
     subject { helper.show_new_user_signups_cap_reached? }
 

ee/spec/models/ee/personal_access_token_spec.rb

@@ -243,6 +243,33 @@ RSpec.describe PersonalAccessToken do
     it_behaves_like 'enforcement of personal access token expiry'
   end
 
+  describe '#expired_but_not_enforced?' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { expired_token.expired_but_not_enforced? }
+
+    where(:enforced?, :expires_at, :result) do
+      true    | 1.week.ago      | false
+      true    | Time.current    | false
+      true    | 1.week.from_now | false
+      false   | 1.week.ago      | true
+      false   | Time.current    | true
+      false   | 1.week.from_now | false
+    end
+
+    with_them do
+      let_it_be(:expired_token) { build(:personal_access_token) }
+
+      before do
+        allow(described_class).to receive(:expiration_enforced?).and_return(enforced?)
+
+        expired_token.expires_at = expires_at
+      end
+
+      it { expect(subject).to be result }
+    end
+  end
+
   describe '.enforce_pat_expiration_feature_available?' do
     using RSpec::Parameterized::TableSyntax
 

ee/spec/views/profiles/personal_access_tokens/_token_expiry_notification.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'profiles/personal_access_tokens/_token_expiry_notification.html.haml' do
+  let_it_be(:user) { build(:user) }
+  let_it_be(:active_tokens) { build_list(:personal_access_token, 2) }
+  let_it_be(:expired_tokens) { build_list(:personal_access_token, 2, expires_at: 5.days.ago) }
+  let_it_be(:one_expired_token) { [build(:personal_access_token, expires_at: 5.days.ago)] }
+
+  context 'when the notification should be shown' do
+    before do
+      stub_licensed_features(enforce_personal_access_token_expiration: true)
+      allow(Gitlab::CurrentSettings).to receive(:enforce_pat_expiration?).and_return(false)
+      allow(view).to receive(:show_profile_token_expiry_notification?).and_return(true)
+    end
+
+    context 'when there are expired tokens' do
+      before do
+        render 'profiles/personal_access_tokens/token_expiry_notification', active_tokens: expired_tokens
+      end
+
+      it 'contains the correct content', :aggregate_failures do
+        expect(rendered).to have_selector '[data-feature-id="profile_personal_access_token_expiry"]'
+        expect(rendered).to match /<use xlink:href=".+?icons-.+?#error">/
+        expect(rendered).to have_content '2 tokens have expired'
+        expect(rendered).to have_content 'Until revoked, expired personal access tokens pose a security risk.'
+      end
+    end
+
+    context 'when there is one expired token' do
+      before do
+        render 'profiles/personal_access_tokens/token_expiry_notification', active_tokens: one_expired_token
+      end
+
+      it 'has the singular title' do
+        expect(rendered).to have_content '1 token has expired'
+      end
+    end
+  end
+end

ee/spec/views/shared/access_tokens/_table.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'shared/access_tokens/_table.html.haml' do
+  let(:type) { 'token' }
+  let(:type_plural) { 'tokens' }
+  let(:token_expiry_enforced?) { false }
+  let(:impersonation) { false }
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:tokens) { [create(:personal_access_token, user: user)] }
+  let_it_be(:project) { false }
+
+  before do
+    stub_licensed_features(enforce_personal_access_token_expiration: true)
+    allow(Gitlab::CurrentSettings).to receive(:enforce_pat_expiration?).and_return(false)
+
+    allow(view).to receive(:personal_access_token_expiration_enforced?).and_return(token_expiry_enforced?)
+    allow(view).to receive(:show_profile_token_expiry_notification?).and_return(true)
+    allow(view).to receive(:distance_of_time_in_words_to_now).and_return('4 days')
+
+    if project
+      project.add_maintainer(user)
+    end
+
+    locals = {
+      type: type,
+      type_plural: type_plural,
+      active_tokens: tokens,
+      project: project,
+      impersonation: impersonation,
+      revoke_route_helper: ->(token) { 'path/' }
+    }
+
+    render partial: 'shared/access_tokens/table', locals: locals
+  end
+
+  shared_examples 'does not show the token expiry notification' do
+    it do
+      expect(rendered).not_to have_content 'tokens have expired'
+    end
+  end
+
+  context 'if impersonation' do
+    let(:impersonation) { true }
+
+    it_behaves_like 'does not show the token expiry notification'
+  end
+
+  context 'if project' do
+    let_it_be(:project) { create(:project) }
+
+    it_behaves_like 'does not show the token expiry notification'
+  end
+
+  context 'without tokens' do
+    let_it_be(:tokens) { [] }
+
+    it_behaves_like 'does not show the token expiry notification'
+  end
+
+  context 'with tokens' do
+    let_it_be(:tokens) do
+      [
+        create(:personal_access_token, user: user, name: 'Access token', last_used_at: 1.day.ago, expires_at: nil),
+        create(:personal_access_token, user: user, expires_at: 5.days.ago),
+        create(:personal_access_token, user: user, expires_at: Time.now),
+        create(:personal_access_token, user: user, expires_at: 5.days.from_now, scopes: [:read_api, :read_user])
+      ]
+    end
+
+    it 'shows the token expiry notification', :aggregate_failures do
+      expect(rendered).to render_template('profiles/personal_access_tokens/_token_expiry_notification', active_tokens: tokens)
+      expect(rendered).to have_content '2 tokens have expired'
+    end
+  end
+end

locale/gitlab.pot

@@ -370,6 +370,11 @@ msgid_plural "%d tags per image name"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "%d token has expired"
+msgid_plural "%d tokens have expired"
+msgstr[0] ""
+msgstr[1] ""
+
 msgid "%d unassigned issue"
 msgid_plural "%d unassigned issues"
 msgstr[0] ""
@@ -35599,6 +35604,9 @@ msgstr ""
 msgid "Until"
 msgstr ""
 
+msgid "Until revoked, expired personal access tokens pose a security risk."
+msgstr ""
+
 msgid "Unused"
 msgstr ""
 

spec/models/personal_access_token_spec.rb

@@ -73,6 +73,14 @@ RSpec.describe PersonalAccessToken do
     end
   end
 
+  describe '#expired_but_not_enforced?' do
+    let(:token) { build(:personal_access_token) }
+
+    it 'returns false', :aggregate_failures do
+      expect(token).not_to be_expired_but_not_enforced
+    end
+  end
+
   describe 'Redis storage' do
     let(:user_id) { 123 }
     let(:token) { 'KS3wegQYXBLYhQsciwsj' }

spec/views/shared/access_tokens/_table.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'shared/access_tokens/_table.html.haml' do
+  let(:type) { 'token' }
+  let(:type_plural) { 'tokens' }
+  let(:empty_message) { nil }
+  let(:token_expiry_enforced?) { false }
+  let(:impersonation) { false }
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:tokens) { [create(:personal_access_token, user: user)] }
+  let_it_be(:project) { false }
+
+  before do
+    stub_licensed_features(enforce_personal_access_token_expiration: true)
+    allow(Gitlab::CurrentSettings).to receive(:enforce_pat_expiration?).and_return(false)
+
+    allow(view).to receive(:personal_access_token_expiration_enforced?).and_return(token_expiry_enforced?)
+    allow(view).to receive(:show_profile_token_expiry_notification?).and_return(true)
+    allow(view).to receive(:distance_of_time_in_words_to_now).and_return('4 days')
+
+    if project
+      project.add_maintainer(user)
+    end
+
+    # Forcibly removing scopes from one token as it's not possible to do with the current modal on creation
+    # But the check exists in the template (it may be there for legacy reasons), so we should test the outcome
+    if tokens.size > 1
+      tokens[1].scopes = []
+    end
+
+    locals = {
+      type: type,
+      type_plural: type_plural,
+      active_tokens: tokens,
+      project: project,
+      impersonation: impersonation,
+      revoke_route_helper: ->(token) { 'path/' }
+    }
+
+    if empty_message
+      locals[:no_active_tokens_message] = empty_message
+    end
+
+    render partial: 'shared/access_tokens/table', locals: locals
+  end
+
+  context 'if personal' do
+    it 'does not show non-personal content', :aggregate_failures do
+      expect(rendered).not_to have_content 'To see all the user\'s personal access tokens you must impersonate them first.'
+      expect(rendered).not_to have_selector 'th', text: 'Role'
+    end
+
+    context 'if token expiration is enforced' do
+      let(:token_expiry_enforced?) { true }
+
+      it 'does not show the subtext' do
+        expect(rendered).not_to have_content 'Personal access tokens are not revoked upon expiration.'
+      end
+    end
+
+    context 'if token expiration is not enforced' do
+      let(:token_expiry_enforced?) { false }
+
+      it 'does show the subtext' do
+        expect(rendered).to have_content 'Personal access tokens are not revoked upon expiration.'
+      end
+    end
+  end
+
+  context 'if impersonation' do
+    let(:impersonation) { true }
+
+    it 'shows the impersonation content', :aggregate_failures do
+      expect(rendered).to have_content 'To see all the user\'s personal access tokens you must impersonate them first.'
+
+      expect(rendered).not_to have_content 'Personal access tokens are not revoked upon expiration.'
+      expect(rendered).not_to have_selector 'th', text: 'Role'
+    end
+  end
+
+  context 'if project' do
+    let_it_be(:project) { create(:project) }
+
+    it 'shows the project content', :aggregate_failures do
+      expect(rendered).to have_selector 'th', text: 'Role'
+      expect(rendered).to have_selector 'td', text: 'Maintainer'
+
+      expect(rendered).not_to have_content 'Personal access tokens are not revoked upon expiration.'
+      expect(rendered).not_to have_content 'To see all the user\'s personal access tokens you must impersonate them first.'
+    end
+  end
+
+  context 'without tokens' do
+    let_it_be(:tokens) { [] }
+
+    it 'has the correct content', :aggregate_failures do
+      expect(rendered).to have_content 'Active tokens (0)'
+      expect(rendered).to have_content 'This user has no active tokens.'
+    end
+
+    context 'with a custom empty text' do
+      let(:empty_message) { 'Custom empty message' }
+
+      it 'shows the custom empty text' do
+        expect(rendered).to have_content empty_message
+      end
+    end
+  end
+
+  context 'with tokens' do
+    let_it_be(:tokens) do
+      [
+        create(:personal_access_token, user: user, name: 'Access token', last_used_at: 1.day.ago, expires_at: nil),
+        create(:personal_access_token, user: user, expires_at: 5.days.ago),
+        create(:personal_access_token, user: user, expires_at: Time.now),
+        create(:personal_access_token, user: user, expires_at: 5.days.from_now, scopes: [:read_api, :read_user])
+      ]
+    end
+
+    it 'has the correct content', :aggregate_failures do
+      # Heading content
+      expect(rendered).to have_content 'Active tokens (4)'
+
+      # Table headers
+      expect(rendered).to have_selector 'th', text: 'Token name'
+      expect(rendered).to have_selector 'th', text: 'Scopes'
+      expect(rendered).to have_selector 'th', text: 'Created'
+      expect(rendered).to have_selector 'th', text: 'Last Used'
+      expect(rendered).to have_selector 'th', text: 'Expires'
+
+      # Table contents
+      expect(rendered).to have_content 'Access token'
+      expect(rendered).to have_content 'read_api, read_user'
+      expect(rendered).to have_content 'no scopes selected'
+      expect(rendered).to have_content Time.now.to_date.to_s(:medium)
+      expect(rendered).to have_content l(1.day.ago, format: "%b %d, %Y")
+
+      # Expiry
+      expect(rendered).to have_content 'Expired', count: 2
+      expect(rendered).to have_content 'In 4 days'
+
+      # Revoke buttons
+      expect(rendered).to have_link 'Revoke', href: 'path/', class: 'btn-danger-secondary', count: 1
+      expect(rendered).to have_link 'Revoke', href: 'path/', count: 4
+    end
+
+    context 'without the last used time' do
+      let_it_be(:tokens) { [create(:personal_access_token, user: user, expires_at: 5.days.ago)] }
+
+      it 'shows the last used empty text' do
+        expect(rendered).to have_content 'Never'
+      end
+    end
+
+    context 'without expired at' do
+      let_it_be(:tokens) { [create(:personal_access_token, user: user, expires_at: nil, last_used_at: 1.day.ago)] }
+
+      it 'shows the expired at empty text' do
+        expect(rendered).to have_content 'Never'
+      end
+    end
+  end
+end