Commit 383490a3 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'security-2818_filter_impersonated_sessions' into 'master'

Filter impersonated sessions from active sessions and remove ability to revoke session

See merge request gitlab/gitlabhq!2968
parents 040e6e72 038d5305
...@@ -2,15 +2,6 @@ ...@@ -2,15 +2,6 @@
class Profiles::ActiveSessionsController < Profiles::ApplicationController class Profiles::ActiveSessionsController < Profiles::ApplicationController
def index def index
@sessions = ActiveSession.list(current_user) @sessions = ActiveSession.list(current_user).reject(&:is_impersonated)
end
def destroy
ActiveSession.destroy(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
end end
...@@ -5,7 +5,8 @@ class ActiveSession ...@@ -5,7 +5,8 @@ class ActiveSession
attr_accessor :created_at, :updated_at, attr_accessor :created_at, :updated_at,
:session_id, :ip_address, :session_id, :ip_address,
:browser, :os, :device_name, :device_type :browser, :os, :device_name, :device_type,
:is_impersonated
def current?(session) def current?(session)
return false if session_id.nil? || session.id.nil? return false if session_id.nil? || session.id.nil?
...@@ -31,7 +32,8 @@ class ActiveSession ...@@ -31,7 +32,8 @@ class ActiveSession
device_type: client.device_type, device_type: client.device_type,
created_at: user.current_sign_in_at || timestamp, created_at: user.current_sign_in_at || timestamp,
updated_at: timestamp, updated_at: timestamp,
session_id: session_id session_id: session_id,
is_impersonated: request.session[:impersonator_id].present?
) )
redis.pipelined do redis.pipelined do
......
...@@ -23,9 +23,3 @@ ...@@ -23,9 +23,3 @@
%strong Signed in %strong Signed in
on on
= l(active_session.created_at, format: :short) = l(active_session.created_at, format: :short)
- unless is_current_session
.float-right
= link_to profile_active_session_path(active_session.session_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
---
title: Do not display impersonated sessions under active sessions and remove ability
to revoke session
merge_request:
author:
type: security
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
> in GitLab 10.8. > in GitLab 10.8.
GitLab lists all devices that have logged into your account. This allows you to GitLab lists all devices that have logged into your account. This allows you to
review the sessions and revoke any of it that you don't recognize. review the sessions.
## Listing all active sessions ## Listing all active sessions
...@@ -12,9 +12,3 @@ review the sessions and revoke any of it that you don't recognize. ...@@ -12,9 +12,3 @@ review the sessions and revoke any of it that you don't recognize.
1. Navigate to the **Active Sessions** tab. 1. Navigate to the **Active Sessions** tab.
![Active sessions list](img/active_sessions_list.png) ![Active sessions list](img/active_sessions_list.png)
## Revoking a session
1. Navigate to your [profile's](#profile-settings) **Settings > Active Sessions**.
1. Click on **Revoke** besides a session. The current session cannot be
revoked, as this would sign you out of GitLab.
...@@ -7,6 +7,8 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do ...@@ -7,6 +7,8 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do
end end
end end
let(:admin) { create(:admin) }
around do |example| around do |example|
Timecop.freeze(Time.zone.parse('2018-03-12 09:06')) do Timecop.freeze(Time.zone.parse('2018-03-12 09:06')) do
example.run example.run
...@@ -16,6 +18,7 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do ...@@ -16,6 +18,7 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do
it 'User sees their active sessions' do it 'User sees their active sessions' do
Capybara::Session.new(:session1) Capybara::Session.new(:session1)
Capybara::Session.new(:session2) Capybara::Session.new(:session2)
Capybara::Session.new(:session3)
# note: headers can only be set on the non-js (aka. rack-test) driver # note: headers can only be set on the non-js (aka. rack-test) driver
using_session :session1 do using_session :session1 do
...@@ -37,9 +40,27 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do ...@@ -37,9 +40,27 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do
gitlab_sign_in(user) gitlab_sign_in(user)
end end
# set an admin session impersonating the user
using_session :session3 do
Capybara.page.driver.header(
'User-Agent',
'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36'
)
gitlab_sign_in(admin)
visit admin_user_path(user)
click_link 'Impersonate'
end
using_session :session1 do using_session :session1 do
visit profile_active_sessions_path visit profile_active_sessions_path
expect(page).to(
have_selector('ul.list-group li.list-group-item', { text: 'Signed in on',
count: 2 }))
expect(page).to have_content( expect(page).to have_content(
'127.0.0.1 ' \ '127.0.0.1 ' \
'This is your current session ' \ 'This is your current session ' \
...@@ -57,33 +78,8 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do ...@@ -57,33 +78,8 @@ describe 'Profile > Active Sessions', :clean_gitlab_redis_shared_state do
) )
expect(page).to have_selector '[title="Smartphone"]', count: 1 expect(page).to have_selector '[title="Smartphone"]', count: 1
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.') expect(page).not_to have_content('Chrome on Windows')
end end
end end
end end
...@@ -7,7 +7,10 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do ...@@ -7,7 +7,10 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
end end
end end
let(:session) { double(:session, id: '6919a6f1bb119dd7396fadc38fd18d0d') } let(:session) do
double(:session, { id: '6919a6f1bb119dd7396fadc38fd18d0d',
'[]': {} })
end
let(:request) do let(:request) do
double(:request, { double(:request, {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment