Merge branch '27518-revoke-active-sessions' into 'master'

Feature for #27518, users can revoke active sessions again.

Closes #27518

See merge request gitlab-org/gitlab!17462

app/controllers/profiles/active_sessions_controller.rb

@@ -4,4 +4,13 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController
   def index
     @sessions = ActiveSession.list(current_user).reject(&:is_impersonated)
   end
+
+  def destroy
+    ActiveSession.destroy_with_public_id(current_user, params[:id])
+
+    respond_to do |format|
+      format.html { redirect_to profile_active_sessions_url, status: :found }
+      format.js { head :ok }
+    end
+  end
 end

app/models/active_session.rb

@@ -6,9 +6,11 @@ class ActiveSession
   SESSION_BATCH_SIZE = 200
   ALLOWED_NUMBER_OF_ACTIVE_SESSIONS = 100
 
+  attr_writer :session_id
+
   attr_accessor :created_at, :updated_at,
-    :session_id, :ip_address,
-    :browser, :os, :device_name, :device_type,
+    :ip_address, :browser, :os,
+    :device_name, :device_type,
     :is_impersonated
 
   def current?(session)
@@ -21,6 +23,11 @@ class ActiveSession
     device_type&.titleize
   end
 
+  def public_id
+    encrypted_id = Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
+    CGI.escape(encrypted_id)
+  end
+
   def self.set(user, request)
     Gitlab::Redis::SharedState.with do |redis|
       session_id = request.session.id
@@ -70,6 +77,11 @@ class ActiveSession
     end
   end
 
+  def self.destroy_with_public_id(user, public_id)
+    session_id = decrypt_public_id(public_id)
+    destroy(user, session_id) unless session_id.nil?
+  end
+
   def self.destroy_sessions(redis, user, session_ids)
     key_names = session_ids.map {|session_id| key_name(user.id, session_id) }
     session_names = session_ids.map {|session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
@@ -146,9 +158,9 @@ class ActiveSession
     # remove sessions if there are more than ALLOWED_NUMBER_OF_ACTIVE_SESSIONS.
     sessions = active_session_entries(session_ids, user.id, redis)
     sessions.sort_by! {|session| session.updated_at }.reverse!
-    sessions = sessions.drop(ALLOWED_NUMBER_OF_ACTIVE_SESSIONS)
-    sessions = sessions.map { |session| session.session_id }
-    destroy_sessions(redis, user, sessions) if sessions.any?
+    destroyable_sessions = sessions.drop(ALLOWED_NUMBER_OF_ACTIVE_SESSIONS)
+    destroyable_session_ids = destroyable_sessions.map { |session| session.send :session_id } # rubocop:disable GitlabSecurity/PublicSend
+    destroy_sessions(redis, user, destroyable_session_ids) if destroyable_session_ids.any?
   end
 
   def self.cleaned_up_lookup_entries(redis, user)
@@ -167,4 +179,15 @@ class ActiveSession
 
     entries.compact
   end
+
+  private_class_method def self.decrypt_public_id(public_id)
+    decoded_id = CGI.unescape(public_id)
+    Gitlab::CryptoHelper.aes256_gcm_decrypt(decoded_id)
+  rescue
+    nil
+  end
+
+  private
+
+  attr_reader :session_id
 end

app/views/profiles/active_sessions/_active_session.html.haml

@@ -24,3 +24,9 @@
       %strong= _('Signed in')
       = s_('ProfileSession|on')
       = l(active_session.created_at, format: :short)
+
+  - unless is_current_session
+    .float-right
+      = link_to profile_active_session_path(active_session.public_id), data: { confirm: _('Are you sure? The device will be signed out of GitLab.') }, method: :delete, class: "btn btn-danger prepend-left-10" do
+        %span.sr-only= _('Revoke')
+        = _('Revoke')

changelogs/unreleased/27518-revoke-active-sessions.yml

+---
+title: Restores user's ability to revoke sessions from the active sessions
+  page.
+merge_request: 17462
+author: Jesse Hall @jessehall3
+type: changed

doc/user/profile/active_sessions.md

@@ -24,6 +24,11 @@ review the sessions, and revoke any you don't recognize.
 GitLab allows users to have up to 100 active sessions at once. If the number of active sessions
 exceeds 100, the oldest ones are deleted.
 
+## Revoking a session
+
+1. Use the previous steps to navigate to **Active Sessions**.
+1. Click on **Revoke** besides a session. The current session cannot be revoked, as this would sign you out of GitLab.
+
 <!-- ## Troubleshooting
 
 Include any troubleshooting steps that you can foresee. If you know beforehand what issues

locale/gitlab.pot

@@ -2152,6 +2152,9 @@ msgstr ""
 msgid "Are you sure? Removing this GPG key does not affect already signed commits."
 msgstr ""
 
+msgid "Are you sure? The device will be signed out of GitLab."
+msgstr ""
+
 msgid "Are you sure? This will invalidate your registered applications and U2F devices."
 msgstr ""
 

spec/features/profiles/active_sessions_spec.rb

@@ -84,4 +84,31 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do
       expect(page).not_to have_content('Chrome on Windows')
     end
   end
+
+  it 'User can revoke a session', :js, :redis_session_store do
+    Capybara::Session.new(:session1)
+    Capybara::Session.new(:session2)
+
+    # set an additional session in another browser
+    using_session :session2 do
+      gitlab_sign_in(user)
+    end
+
+    using_session :session1 do
+      gitlab_sign_in(user)
+      visit profile_active_sessions_path
+
+      expect(page).to have_link('Revoke', count: 1)
+
+      accept_confirm { click_on 'Revoke' }
+
+      expect(page).not_to have_link('Revoke')
+    end
+
+    using_session :session2 do
+      visit profile_active_sessions_path
+
+      expect(page).to have_content('You need to sign in or sign up before continuing.')
+    end
+  end
 end

spec/models/active_session_spec.rb

@@ -44,6 +44,19 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
+  describe '#public_id' do
+    it 'returns an encrypted, url-encoded session id' do
+      original_session_id = "!*'();:@&\n=+$,/?%abcd#123[4567]8"
+      active_session = ActiveSession.new(session_id: original_session_id)
+      encrypted_encoded_id = active_session.public_id
+
+      encrypted_id = CGI.unescape(encrypted_encoded_id)
+      derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id)
+
+      expect(original_session_id).to eq derived_session_id
+    end
+  end
+
   describe '.list' do
     it 'returns all sessions by user' do
       Gitlab::Redis::SharedState.with do |redis|
@@ -173,8 +186,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
           device_name: 'iPhone 6',
           device_type: 'smartphone',
           created_at: Time.zone.parse('2018-03-12 09:06'),
-          updated_at: Time.zone.parse('2018-03-12 09:06'),
-          session_id: '6919a6f1bb119dd7396fadc38fd18d0d'
+          updated_at: Time.zone.parse('2018-03-12 09:06')
         )
       end
     end
@@ -244,6 +256,40 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
+  describe '.destroy_with_public_id' do
+    it 'receives a user and public id and destroys the associated session' do
+      ActiveSession.set(user, request)
+      session = ActiveSession.list(user).first
+
+      ActiveSession.destroy_with_public_id(user, session.public_id)
+
+      total_sessions = ActiveSession.list(user).count
+      expect(total_sessions).to eq 0
+    end
+
+    it 'handles invalid input for public id' do
+      expect do
+        ActiveSession.destroy_with_public_id(user, nil)
+      end.not_to raise_error
+
+      expect do
+        ActiveSession.destroy_with_public_id(user, "")
+      end.not_to raise_error
+
+      expect do
+        ActiveSession.destroy_with_public_id(user, "aaaaaaaa")
+      end.not_to raise_error
+    end
+
+    it 'does not attempt to destroy session when given invalid input for public id' do
+      expect(ActiveSession).not_to receive(:destroy)
+
+      ActiveSession.destroy_with_public_id(user, nil)
+      ActiveSession.destroy_with_public_id(user, "")
+      ActiveSession.destroy_with_public_id(user, "aaaaaaaa")
+    end
+  end
+
   describe '.cleanup' do
     before do
       stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5)