Main Page/CSC/CSC 517 Spring 2020 Refactor impersonate controller
This wiki page describes the work done under E2002 OSS Program for Spring 2020, 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. Similarly, 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.
Problem Statement
The aim of the project is to refactor the impersonate controller. The pre-existing code had two major issues.
- All functions related to impersonate controller were present in a single method ( from figure 4a and 4b)
- Presence of repetitive code ( from figure 4a and 4b)
- 3 levels of block nesting (from figure 5)
- Too many return statements (from figure 6)
About Impersonate Controller
Expertiza allows the administrators, instructors or teaching assistants to impersonate another user (like a student) and access their account. For example, an instructor impersonating a student’s account can view their assignments, stage deadlines, peer reviews and anything else that the student can view. One thing to be noted is that most of these users con only impersonate users for whom they are a parent. For example, instructor6 is a parent of student3841 and not student3836; as a result, instructor6 can impersonate only 3841.
1. Instructor login: username -> instructor6, password -> password
when logged in as an instructor, under the manage option in the ribbon as in Figure 1, select impersonate user. Upon redirected to impersonate page, enter the account which needs to be impersonated. If possible it impersonates that user. Now a new button called revert appears on the ribbon as in figure 3, this can be used to revert the impersonation and return to the instructor profile.
Problem Solution
The above-mentioned issues have been tackled by refactoring the impersonate controller by splitting into many smaller methods which are later called by the main impersonate controller.
The following are the refactored new methods that help in tackling the issue1 apart from each being specifically for some issue rectification:
- check_if_user_impersonateable
- display_error_msg
- overwrite_session
- check_if_special_char
- do_main_operartion
check_if_user_impersonateable
This method plays the main role in tackling issue3 - 3 levels of block nesting apart from issue1.
Intial Code
if user
unless original_user.can_impersonate? user
flash[:error] = "You cannot impersonate #{params[:user][:name]}."
redirect_back
return
end
After recfactoring - Moved to separate method
def check_if_user_impersonateable
if params[:impersonate].nil?
user = 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
if !params[:impersonate][:name].empty?
user = User.find_by(name: params[:impersonate][:name])
overwrite_session
end
end
end
display_error_msg
This method is used to tackle issues1, 2 and 4. All the error message related code is moved to this method.
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
redirect_to :back
end
overwrite_session
This method reduces the number of return statements used in impersonate controller, apart from reducing the size of the controller.
Initial Code
if params[:impersonate].nil?
# check if special chars /\?<>|&$# are used to avoid html tags or system command
if warn_for_special_chars(params[:user][:name], "Username")
redirect_back
return
end
user = User.find_by(name: params[:user][:name])
if user
unless original_user.can_impersonate? user
flash[:error] = "You cannot impersonate #{params[:user][:name]}."
redirect_back
return
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
else
flash[:error] = message
redirect_back
return
end
else
# Impersonate a new account
if !params[:impersonate][:name].empty?
# check if special chars /\?<>|&$# are used to avoid html tags or system command
if warn_for_special_chars(params[:impersonate][:name], "Username")
redirect_back
return
end
user = User.find_by(name: params[:impersonate][:name])
if user
unless original_user.can_impersonate? user
flash[:error] = "You cannot impersonate #{params[:user][:name]}."
redirect_back
return
end
AuthController.clear_user_info(session, nil)
session[:user] = user
session[:impersonate] = true
session[:original_user] = original_user
else
flash[:error] = message
redirect_back
return
end
After Refactoring - Moved to a separate method and accessed through the adapter method do_main_operation
def overwrite_session
#if not impersonatable, then original user's session remains
if params[:impersonate].nil?
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?
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
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
check_if_special_char
This code is used to reduce one functionality performed under the impersonate controller. This method checks to see if the given username is acceptable.
def check_if_special_char
if warn_for_special_chars(params[:user][:name], "Username")
redirect_back
return
end
end
do_main_operation
This like an adapter method that is used to interface the impersonate method with display_error_msg and check_if_user_impersonatable. One main purpose to do this is to make the methods flexible for change apart from reducing the number of lines from the impersonate controller.
def do_main_operation(user)
if user
check_if_user_impersonateable
else
display_error_msg
end
end