CSC/ECE 517 Fall 2021 - E2122. Refactor impersonate controller.rb

This wiki page describes the changes made under E2122, in order to refactor impersonate_controller.rb.

Overview

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Issues for this project

• Issue 1: Throughout the file, the user is being initialized but never used. This is bad coding practice as it slows down runtime.
• Issue 2: Convert nested if to elif around line 36.
• Issue 3: Refactor very long lines of code to make it more readable.
• Issue 4: Test functions and increase coverage:
  • auto_complete_for_user_name
  • overwrite_session test line 47-51
  • test_

Project implementation

Submitted work and demonstration of project

• Link to deployed code on NCSU VCL
• Link to the beta branch of our Github repository

Files involved

• app/controllers/impersonate_controller.rb - the refactored source code.
• spec/controllers/impersonate_controller_spec.rb - the related tests toward our refactoring.

Code changes

This part shows the changes in ./app/controllers/impersonate_controller.rb

In function overwrite_session

Before

 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 = 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
    else
      # If some user is to be impersonated, their session details are overwritten onto the current to impersonate
      if !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 = user = 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
        user = User.anonymized_view?(session[:ip]) ? User.real_user_from_anonymized_name(params[:user][:name]) : user = User.find_by(name: params[:user][:name])
        AuthController.clear_user_info(session, nil)
        session[:user] = session[:super_user]
        user = session[:user]
        session[:super_user] = nil
      end
    end
  end

After

 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
      if User.anonymized_view?(session[:ip])
        user = User.real_user_from_anonymized_name(params[:user][:name])
      else
        user = User.find_by(name: params[:user][:name])
      end
      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
      # If some user is to be impersonated, their session details are overwritten onto the current to impersonate
    elsif !params[:impersonate][:name].empty?
      # E1991 : check whether instructor is currently in anonymized view
      if User.anonymized_view?(session[:ip])
        user = User.real_user_from_anonymized_name(params[:impersonate][:name])
      else
        user = User.find_by(name: params[:impersonate][:name])
      end
      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

In function check_if_special_char

Before

  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

After

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

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

In function check_if_user_impersonateable

Before

  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 = User.find_by(name: params[:user][:name])
      if !@original_user.can_impersonate? user
        @message = "You cannot impersonate '#{params[:user][:name]}'."
        temp
	@@ -85,7 +89,6 @@ def check_if_user_impersonateable
    else
      unless 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 = user = User.find_by(name: params[:impersonate][:name])
        overwrite_session
      end
    end
  end

After

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

In function display_error_msg

Before

 def display_error_msg
    if params[:user]
      @message = "No user exists with the name '#{params[:user][:name]}'."
    elsif params[:impersonate]
      @message = "No user exists with the name '#{params[:impersonate][:name]}'."
    else
      if params[:impersonate].nil?
        @message = "You cannot impersonate '#{params[:user][:name]}'."
      else
        if !params[:impersonate][:name].empty?
          @message = "You cannot impersonate '#{params[:impersonate][:name]}'."
        else
          @message = "No original account was found. Please close your browser and start a new session."
        end
      end
    end
  rescue Exception => e
    flash[:error] = @message
  end

After

  def display_error_msg
    if params[:user]
      @message = "No user exists with the name '#{params[:user][:name]}'."
    elsif params[:impersonate]
      @message = "No user exists with the name '#{params[:impersonate][:name]}'."
    elsif params[:impersonate].nil?
      @message = "You cannot impersonate '#{params[:user][:name]}'."
    elsif !params[:impersonate][:name].empty?
      @message = "You cannot impersonate '#{params[:impersonate][:name]}'."
    else
      @message = "No original account was found. Please close your browser and start a new session."
    end
  rescue Exception => e
    flash[:error] = @message
  end

In function impersonate

Before

  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 = user = 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

After

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
        if User.anonymized_view?(session[:ip])
          user = User.real_user_from_anonymized_name(params[:user][:name])
        else
          user = User.find_by(name: params[:user][:name])
        end
        do_main_operation(user)
      elsif !params[:impersonate][:name].empty?
        # Impersonate a new account
        #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
        if User.anonymized_view?(session[:ip])
          user = User.real_user_from_anonymized_name(params[:impersonate][:name])
        else
          user = User.find_by(name: params[:impersonate][:name])
        end
        do_main_operation(user)
        # Revert to original account when currently in the impersonated session
      elsif !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

Test Plan

Manual UI Testing

The following steps must be performed to test the project UI:

Step 1: Log in as a super administrator, with Username - "super_administrator2", Password - "password"



Step 2: Select the function from the navigation bar "Manage"->"Impersonate User".



Step 3: Type the username to impersonate (here we use "student11" for test).



This view below is generated after clicking the button "Impersonate":



Step 4: Change the user to impersonate from the textbox at the navigation bar (here we use "student13" for test).



This view below is generated after clicking the button "Revert":



Step 5: Return to super_administrator2 by clicking the button "Revert" without filling out the textbox.



PS: We found some unknown internal errors (response 500) as the result of impersonating some students (for example: "student7" to "student10"). The reason for this error might be some improper values were generated for these users.
"student11" and "student13" are manually tested, which are able to impersonate.

RSpec Testing

In the file ./spec/controllers/impersonate_controller_spec.rb
The following RSpec test is modified.
Before

        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

After

        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
            expect(session[:user]).to eq student1
            expect(session[:super_user]).to eq instructor
            # 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

The following tests are implemented and added.

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

        it 'when typing the user name, there should be all possible complete user names appearing ' do
            @params = { user:  {name: 'A' }}
            get :auto_complete_for_user_name,@params
            expect(response.body).to eq  "<%= auto_complete_result @users, 'Amanda' %>"

        end

PS: Some issues we found during the test are related to some other files. The detailed information is listed in our pull request (E2122. Refactor impersonate_controller.rb).

Team Information

Mentor: John Bumgardner (jwbumga2)

Robin Piao (lpiao)

Shengjie Guo (sguo25)

Haoze Du (hdu5)