Is this 'Sign In As' method secure?

Ruby on Rails
1

My ‘school lesson’ app has several kinds of user: admin, teacher, family. I need admins to be able to ‘sign in as’ other users in order to troubleshoot issues. Teachers create lessons for families, families view their lessons and admins observe all activity.

There are no privacy issues since all information stored in the database is hierarchical by design so admin has the right to see all data created by teachers and families don’t generate any data at all apart from viewing stats etc. (i.e. indirectly)

I’m using Devise, CanCan and default-scope- based multi-tenancy.

I read the following articles and mashed together my own solution.

http://pivotallabs.com/fast-user-switching-with-devise/

Interface for admin to access this feature:

# app/views/users/index.html.erb

<% @users.each do |user| %>
  <tr>
    <td><%= user.name %></td>
    <td><%= link_to 'Sign In As', become_user_path(user) %></td>
    ...
  </tr>
<% end %>

Controller action to perform the user switch, including authorizing:

# app/controllers/users_controller.rb

class UsersController < ApplicationController
  before_filter :authenticate_user! # Devise authentication
  before_filter :authorize_admin, only: :become # my custom method
  load_and_authorize_resource # CanCan authorization

  ...

  def become
    if impersonating?
      session[:admin_id] = nil
    else
      session[:admin_id] = current_user.id
    end
    user = User.find(params[:id])
    sign_in(:user, user)
    redirect_to root_url, notice: "Now signed in as #{user.full_name}"
  end

  ...

  protected
  def authorize_admin
    redirect_to '/' and return unless current_user.has_admin? || impersonating?
  end
end

Ability to sign back in as admin comes from:

# app/views/layouts/application.html.erb

<header>
  ...
  <% if impersonating? %>
    <%= link_to 'Back to Admin', become_user_path(impersonating_admin_id) %>
  <% end %>
  ...
</header>

impersonating? and impersonating_admin_id methods:

# app/controllers/application_controller.rb

def impersonating?
  session[:admin_id].present?
end
helper_method :impersonating?

def impersonating_admin_id
  session[:admin_id]
end
helper_method :impersonating_admin_id

and finally the CanCan ability:

# app/models/ability.rb

class Ability
  include CanCan::Ability

  def initialize(user)
    user ||= User.new

    can :become, User, school_id: user.school_id
   
    if user.type == 'Admin'
      ...
    end

    if user.type == 'Teacher'
      ...
    end

    if user.type == 'Family'
      ...
    end
  end
end

It’s the CanCan ability that worries me. I have to include the :become ability for all users because an admin signed in as a teacher bascially is a teacher as far as the app is concerned so they need to be able to sign back in as admin.

I’m working on the premise that only an admin is able to cause session[:admin_id] to be set so impersonating? is a safe method to rely on.

Does this all look secure or am I missing something terribly important?

2

My tests are all passing but they’re pretty basic:

# spec/features/sign_in_as_other_user_spec.rb

require 'spec_helper'

feature 'Sign in as other user' do
  before(:each) do
    @family  = create(:family)
    @teacher = create(:teacher)
    @admin   = create(:admin)
  end

  scenario 'Admin signs in as a family' do
    sign_in_user(@admin)
    click_on 'Families'
    find(:xpath, "//tr[contains(.,#{@family.name})]/td/a", text: 'Sign In As').click
    expect(page).to have_content "Welcome, #{@family.name}."
    expect(page).to have_content 'Back to Admin'
    click_on 'Back to Admin'
    expect(page).to have_content 'Admin Dashboard'
  end

  scenario 'Admin signs in as teacher' do
    sign_in_user(@admin)
    click_on 'Teachers'
    find(:xpath, "//tr[contains(.,#{@teacher.name})]/td/a", text: 'Sign In As').click
    expect(page).to have_content "Hey, #{@teacher.name}."
    expect(page).to have_content 'Back to Admin'
    click_on 'Back to Admin'
    expect(page).to have_content 'Admin Dashboard'
  end

  scenario 'Admin visits URL directly' do
    sign_in_user(@admin)
    visit "/users/#{@teacher.id}/become"
    expect(page).to have_content "Hey, #{@teacher.name}."
    expect(page).to have_content 'Back to Admin'
    click_on 'Back to Admin'
    expect(page).to have_content 'Admin Dashboard'
  end

  scenario 'Teacher cannot become another user' do
    sign_in_user(@teacher)
    visit "/users/#{@family.id}/become"
    expect(page).to_not have_content @family.name
    expect(page).to_not have_content 'Back to Admin'
    expect(page).to have_content "Hey, #{@teacher.name}"
  end

  scenario 'Family cannot become another user' do
    sign_in_user(@family)
    visit "/users/#{@teacher.id}/become"
    expect(page).to_not have_content @teacher.name
    expect(page).to_not have_content 'Back to Admin'
    expect(page).to have_content "Welcome, #{@family.name}."
  end

  scenario 'Anonymous user cannot become another user' do
    visit "/users/#{@teacher.id}/become"
    expect(page).to_not have_content "Hey, #{@teacher.name}."
    expect(page).to_not have_content 'Back to Admin'
    expect(page).to have_content 'Please sign in to continue'
  end
end

I’m not covering any tricky security stuff that could go on here and I’m also not covering any edge-cases like session[:admin_id] being left hanging around, if that’s possible?

3

Can anyone weigh in here? Is there any other code I can post that might help?

4

Could you instead just manually sign out and sign back in as an admin?

5

I think so. I’d make sure to use an encrypted (not just signed) session, though: ActionDispatch::Session::CookieStore

6

@benorenstein yes that would seem to be good way to avoid potential security issues but it would also lose some of the ‘magic’ of the feature.

Thanks for the tip on encrypted sessions, I’ll definitely do that.