CSC/ECE 517 Spring 2022 - S2222: Refactor impersonate controller

This wiki page describes the work done under E2222 OSS Program for Spring 2022, in the CSC/ECE 517 course.

About Expertiza

Expertiza is an open-source project based on Ruby on Rails framework. Expertiza is a complete instructor-student usage website where the instructor can assign assignments, deadlines, grades, etc that is required for the course(s) under him and the students can use this website to perform the tasks required as part of the course like project or assignments submission, forming groups and collaborating with them, as well as reviewing projects and teammates.

This project focuses on a specific feature of expertiza which allows Administrators, Instructors or Teaching Assistants to impersonate other and access their account. The demonstration for the feature is as shown below.

Figure 1

Figure 2

Figure 3

What does this controller do?

Login: We can only login as an instructor(only instructor login details are available) username -> instructor6, password -> password

• When logged in as an instructor, under the manage option as shown in Figure 1, select impersonate user.
• Upon redirected to impersonate page, enter the account which needs to be impersonated.
• It impersonates that user provided that user can be impersonated.
• As shown in figure 3, this can be used to revert the impersonation and return to the instructor profile.

Problem Statement

Expertiza allows administrators, instructors and Teaching Assistants to impersonate other users like a student. This allows the impersonator to view assignments, deadlines and submissions of other students. The rules to impersonating a user is, the impersonator has to be an ancestor of the impersonate. The hierarchy of impersonation is as follows:

super administrator -> Administrator -> Instructor -> Teaching Assistant -> Student

Note: impersonation cannot happen within the same level of hierarchy. For Example, a Super Administrator can impersonate any user apart from other Super Administrators, an Administrator can impersonate Instructors, TA, Students and not other Admins and so on. The aim of this project is to refactor the impersonate controller.

Major issues in the previous version of code

• Long single lines of code throughout the controller.
• Repeated code written multiple times in different methods throughout the controller
• Method names used were not informative and readable.
• Undiscovered bug that was breaking the application(Has been reported in Expertiza).
• Unused method discovered.

What needs to be done

• Introduce Single Responsibility Principle - A method/function should only have one purpose to fulfill (Read more about it here). Read more about the ideal length of a function here.
• You’re supposed to figure out different things a method is doing, break that method into smaller methods such that each resulting method is doing only one thing.
• Throughout the file, the user is being initialized but never used. This is bad coding practice as it slows down runtime.
• Code comments are needed for each and every function. Figure out what the function is doing and write at-least 50 word descriptions for each function.
• Your code comments should also include the information about what the parameters are for, and what the method returns (not counted for 50 word description).
• Read more about how to write meaningful code comments here.
• Rename the methods to a more informative name. For example, There can be better names for check_if_user_impersonateable method, do_main_operation, check_if_special_char methods
• Refactor very long lines of code to make it more readable
• There are many unnecessary nesting of if statements. Convert them to elif. For example, Line 36, Line 139.
• Use a guard clause instead of wrapping the code inside a conditional expression. Read more about guard clauses here.
• A generalized exception is being rescued everywhere, which is against the coding principles that are supposed to be followed. Find what exceptions could a piece of code generate and rescue a more specialized exception.
• Test Functions and increase test code coverage.

Solution

Original Code

• Method to overwrite the session details that are corresponding to the user or one being impersonated

  def overwrite_session
    # If not impersonatable, then original user's session remains
    if params[:impersonate].nil?
      # E1991 : check whether instructor is currently in anonymized view
      user = User.anonymized_view?(session[:ip]) ? User.real_user_from_anonymized_name(params[:user][:name]) : User.find_by(name: params[:user][:name])
      session[:super_user] = session[:user] if session[:super_user].nil?
      AuthController.clear_user_info(session, nil)
      session[:original_user] = @original_user
      session[:impersonate] = true
      session[:user] = user
    elsif !params[:impersonate][:name].empty?
      # E1991 : check whether instructor is currently in anonymized view
      user = User.anonymized_view?(session[:ip]) ? User.real_user_from_anonymized_name(params[:impersonate][:name]) : User.find_by(name: params[:impersonate][:name])
      AuthController.clear_user_info(session, nil)
      session[:user] = user
      session[:impersonate] = true
      session[:original_user] = @original_user
    else
      # E1991 : check whether instructor is currently in anonymized view
      AuthController.clear_user_info(session, nil)
      session[:user] = session[:super_user]
      session[:super_user] = nil
    end
  end

  def check_if_special_char
    if params[:user]
      if warn_for_special_chars(params[:user][:name], 'Username')
        redirect_back
        return
      end
    end

    if params[:impersonate]
      if warn_for_special_chars(params[:impersonate][:name], 'Username')
        redirect_back
        return
      end
    end
  end

• Checking if the username provided can be impersonated or not

  def check_if_user_impersonateable
    if params[:impersonate].nil?
      # E1991 : check whether instructor is currently in anonymized view
      user = User.anonymized_view?(session[:ip]) ? User.real_user_from_anonymized_name(params[:user][:name]) : User.find_by(name: params[:user][:name])
      if !@original_user.can_impersonate? user
        @message = "You cannot impersonate '#{params[:user][:name]}'."
        temp
        AuthController.clear_user_info(session, nil)
      else
        overwrite_session
      end
    else
      unless params[:impersonate][:name].empty?
        # E1991 : check whether instructor is currently in anonymized view
        overwrite_session
      end
    end
  end

• Main operation

  def do_main_operation(user)
    if user
      check_if_user_impersonateable
    else
      display_error_msg
    end
  end

• Main operation, method used to break the functions in impersonate controller and bring out 2 functionalities at same level,
• checking if user impersonate-able, if not throw corresponding error message

def impersonate
    # Initial check to see if the username exists
    display_error_msg
    begin
      @original_user = session[:super_user] || session[:user]
      # Impersonate using form on /impersonate/start, based on the username provided, this method looks to see if that's possible by calling the do_main_operation method
      if params[:impersonate].nil?
        # Check if special chars /\?<>|&$# are used to avoid html tags or system command
        check_if_special_char
        # E1991 : check whether instructor is currently in anonymized view
        user = User.anonymized_view?(session[:ip]) ? User.real_user_from_anonymized_name(params[:user][:name]) : user = User.find_by(name: params[:user][:name])
        do_main_operation(user)
      else
        # Impersonate a new account
        if !params[:impersonate][:name].empty?
          # check if special chars /\?<>|&$# are used to avoid html tags or system command
          check_if_special_char
          # E1991 : check whether instructor is currently in anonymized view
          user = User.anonymized_view?(session[:ip]) ? User.real_user_from_anonymized_name(params[:impersonate][:name]) : User.find_by(name: params[:impersonate][:name])
          do_main_operation(user)
          # Revert to original account when currently in the impersonated session
        else
          if !session[:super_user].nil?
            AuthController.clear_user_info(session, nil)
            session[:user] = session[:super_user]
            user = session[:user]
            session[:super_user] = nil
          else
            display_error_msg
          end
        end
      end
      # Navigate to user's home location as the default landing page after impersonating or reverting
      AuthController.set_current_role(user.role_id, session)
      redirect_to action: AuthHelper.get_home_action(session[:user]),
                  controller: AuthHelper.get_home_controller(session[:user])
    rescue StandardError
      flash[:error] = @message
      redirect_to :back
    end
  end

Refactored Code

• Method to Generate Overwrite Session when the user tries to Impersonate another User
• Function created to generate session whenever needed.

  def generate_session(user)
    AuthController.clear_user_info(session, nil)
    session[:original_user] = @original_user
    session[:impersonate] = true
    session[:user] = user
  end

• Method to overwrite the session details that are corresponding to the user or one being impersonated
• The first 'if' statement is executed if the logged in user tried to access the impersonate feature from his account.
• The 'elsif' statement is executed if the user is impersonating someone and then tried to impersonate another person.
• Function Refactored to call the generate session method to reduce the length of the code and reduced long lines of code

  def overwrite_session
    if params[:impersonate].nil?
      user = get_real_user(params[:user][:name]) 
      session[:super_user] = session[:user] if session[:super_user].nil?
      generate_session(user)
    elsif !params[:impersonate][:name].empty?
      user = get_real_user(params[:impersonate][:name])
      generate_session(user)
    else
      session[:user] = session[:super_user]
      session[:super_user] = nil
    end
  end

• Checks if special characters are present in the username provided, only alphanumeric should be used
• warn_for_special_chars is a method in SecurityHelper class.SecurityHelper class has methods to handle this.
• special_chars method-Initialises string with special characters /\?<>|&$# .
• contains_special_chars method-converts it to regex and compares with the string
• warn_for_special_chars takes the output from above method and flashes an error if there are any special characters(/\?<>|&$#) in the string

• Function refactored to use guard clauses instead of if else nesting.

  def check_if_special_char
    redirect_back if params[:user] && warn_for_special_chars(params[:user][:name], 'Username')
    redirect_back if params[:impersonate] && warn_for_special_chars(params[:impersonate][:name], 'Username')
  end

• Function refactored to reduce long lines of code.

  def check_if_user_impersonateable
    if params[:impersonate].nil?
      user = get_real_user(params[:user][:name]) 
      if !@original_user.can_impersonate? user
        @message = "You cannot impersonate '#{params[:user][:name]}'."
        temp
        AuthController.clear_user_info(session, nil)
      else
        overwrite_session
      end
    else
      unless params[:impersonate][:name].empty?
        overwrite_session
      end
    end
  end

• Function name refactored to a better name

  def do_impersonate_operation(user)
    check_if_user_impersonateable if user
    display_error_msg
  end

• Function refactored to reduce long lines of code

  def impersonate
    # Initial check to see if the username exists
    display_error_msg
    begin
      @original_user = session[:super_user] || session[:user]
      # Impersonate using form on /impersonate/start, based on the username provided, this method looks to see if that's possible by calling the do_main_operation method
      if params[:impersonate].nil?
        # Check if special chars /\?<>|&$# are used to avoid html tags or system command
        check_if_special_char
        user = get_real_user(params[:user][:name]) 
        do_impersonate_operation(user)
      else
        # Impersonate a new account
        if !params[:impersonate][:name].empty?
          # check if special chars /\?<>|&$# are used to avoid html tags or system command
          check_if_special_char
          user = get_real_user(params[:impersonate][:name])
          do_impersonate_operation(user)
          # Revert to original account when currently in the impersonated session
        else
          if !session[:super_user].nil?
            AuthController.clear_user_info(session, nil)
            session[:user] = session[:super_user]
            user = session[:user]
            session[:super_user] = nil
          else
            display_error_msg
          end
        end
      end
      # Navigate to user's home location as the default landing page after impersonating or reverting
      AuthController.set_current_role(user.role_id, session)
      redirect_to action: AuthHelper.get_home_action(session[:user]),
                  controller: AuthHelper.get_home_controller(session[:user])
    rescue StandardError
      flash[:error] = @message
      redirect_to :back
    end
  end

• Function created to reduce long lines of code used throughout the code

  def get_real_user(name)
    if User.anonymized_view?(session[:ip])
      user = User.real_user_from_anonymized_name(name)
    else
      user = User.find_by(name: name)
    end
    return user
  end
end

Testing

• instructor should not be able to impersonate a super admin user with their real name

This tests for the case when instructor tries to impersonate super_admin in which case parameter 'impersonate' will be nil in session parameters

    it 'instructor should not be able to impersonate a super admin user with their real name' do
      allow(User).to receive(:find_by).with(name: super_admin.name).and_return(super_admin)
      allow(instructor).to receive(:can_impersonate?).with(super_admin).and_return(false)
      request.env['HTTP_REFERER'] = 'http://www.example.com'
      @params = { user: { name: super_admin.name } }
      @session = { user: instructor }
      post :impersonate, @params, @session
      expect(session[:impersonate]).to be nil
    end
    

• instructor should be able to impersonate a user while already impersonating a user but from nav bar

This test is to ascertain the functionality of the user being able to impersonate another user(obeying hierarchy) through the navigation bar on the top right hand corner.

  it 'instructor should be able to impersonate a user while already impersonating a user but from nav bar' do
    allow(User).to receive(:find_by).with(name: student1.name).and_return(student1)
    allow(User).to receive(:find_by).with(name: student2.name).and_return(student2)
    allow(instructor).to receive(:can_impersonate?).with(student1).and_return(true)
    allow(instructor).to receive(:can_impersonate?).with(student2).and_return(true)
    request.env["HTTP_REFERER"] = "http://www.example.com"
    @params = { user: { name: student1.name } }
    @session = { user: instructor }
    post :impersonate, @params, @session
    # nav bar uses the :impersonate as the param name, so let make sure it always works from there too.
    @params = { impersonate: { name: student2.name } }
    post :impersonate, @params, @session
    expect(session[:super_user]).to eq instructor
    expect(session[:user]).to eq student2
    expect(session[:original_user]).to eq instructor
    expect(session[:impersonate]).to be true
end

• instructor should be able to impersonate a user with their anonymized name

This test verifies the functionality that is available with from the Anonymized view. It checks for the impersonation from both the ends - Through the Manage tab as well as the Navigation Bar.

it ‘instructor should be able to impersonate a user with their anonymized name’ do
    allow(User).to receive(:find_by).with(name: student1.name).and_return(student1)
    allow(instructor).to receive(:can_impersonate?).with(student1).and_return(true)
    allow(User).to receive(:anonymized_view?).and_return(true)
    allow(User).to receive(:real_user_from_anonymized_name).with(“Student30”).and_return(student1)
    request.env[“HTTP_REFERER”] = “http://www.example.com”
    @params = { user: { name: “Student30" } }
    @session = { user: instructor }
    post :impersonate, @params, @session
    expect(session[:super_user]).to eq instructor
    expect(session[:user]).to eq student1
    expect(session[:original_user]).to eq instructor
    expect(session[:impersonate]).to be true
end