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.
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
New Code
- This function checks if the logged in user is a student or not. If it is a student, do not allow the impersonate mode.
- If the logged in user has the role or anything other than the student, we allow that user to use the impersonate mode.
before_action :check_if_input_is_valid
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]}'." 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 Plan
- 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
- instructor should not be able to impersonate an admin user with their real name
This tests for the case when instructor tries to impersonate admin in which case parameter 'impersonate' will be nil in session parameters
it 'instructor should not be able to impersonate an admin user with their real name' do allow(User).to receive(:find_by).with(name: admin.name).and_return(admin) allow(instructor).to receive(:can_impersonate?).with(admin).and_return(false) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: admin.name } } @session = { user: instructor } post :impersonate, @params, @session expect(session[:impersonate]).to be nil end
- instructor should be able to impersonate a teaching assistant user with their real name
This tests for the case when instructor tries to impersonate a teaching assistant in which case parameter 'impersonate' will be true in session parameters
it 'instructor should be able to impersonate a teaching assistant user with their real name' do allow(User).to receive(:find_by).with(name: teaching_assistant.name).and_return(teaching_assistant) allow(instructor).to receive(:can_impersonate?).with(teaching_assistant).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: teaching_assistant.name } } @session = { user: instructor } post :impersonate, @params, @session expect(session[:super_user]).to eq instructor expect(session[:user]).to eq teaching_assistant expect(session[:original_user]).to eq instructor expect(session[:impersonate]).to be true end
- teaching assistant should be able to impersonate a student with their real name
This tests for the case when teaching assistant tries to impersonate a student in which case parameter 'impersonate' will be true in session parameters
it 'teaching assistant should be able to impersonate a student with their real name' do stub_current_user(teaching_assistant, teaching_assistant.role.name, teaching_assistant.role) allow(User).to receive(:find_by).with(name: student1.name).and_return(student1) allow(teaching_assistant).to receive(:can_impersonate?).with(student1).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: student1.name } } @session = { user: teaching_assistant } post :impersonate, @params, @session expect(session[:super_user]).to eq teaching_assistant expect(session[:user]).to eq student1 expect(session[:original_user]).to eq teaching_assistant expect(session[:impersonate]).to be true end
- teaching assistant should not be able to impersonate an instructor with their real name
This tests for the case when teaching assistant tries to impersonate an instructor in which case parameter 'impersonate' will be nil in session parameters
it 'teaching assistant should not be able to impersonate an instructor with their real name' do stub_current_user(teaching_assistant, teaching_assistant.role.name, teaching_assistant.role) allow(User).to receive(:find_by).with(name: instructor.name).and_return(instructor) allow(teaching_assistant).to receive(:can_impersonate?).with(instructor).and_return(false) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: instructor.name } } @session = { user: teaching_assistant } post :impersonate, @params, @session expect(session[:impersonate]).to be nil end end
- teaching assistant should not be able to impersonate an admin with their real name
This tests for the case when teaching assistant tries to impersonate an admin in which case parameter 'impersonate' will be nil in session parameters
it 'teaching assistant should not be able to impersonate an admin with their real name' do stub_current_user(teaching_assistant, teaching_assistant.role.name, teaching_assistant.role) allow(User).to receive(:find_by).with(name: admin.name).and_return(admin) allow(teaching_assistant).to receive(:can_impersonate?).with(admin).and_return(false) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: admin.name } } @session = { user: teaching_assistant } post :impersonate, @params, @session expect(session[:impersonate]).to be nil end
- teaching assistant should not be able to impersonate a super admin with their real name
This tests for the case when teaching assistant tries to impersonate a super admin in which case parameter 'impersonate' will be nil in session parameters
it 'teaching assistant should not be able to impersonate an super admin with their real name' do stub_current_user(teaching_assistant, teaching_assistant.role.name, teaching_assistant.role) allow(User).to receive(:find_by).with(name: super_admin.name).and_return(super_admin) allow(teaching_assistant).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: teaching_assistant } post :impersonate, @params, @session expect(session[:impersonate]).to be nil end
- admin should be able to impersonate a student with their real name
This tests for the case when admin tries to impersonate a student in which case parameter 'impersonate' will be true in session parameters
it 'admin should be able to impersonate a student with their real name' do stub_current_user(admin, admin.role.name, admin.role) allow(User).to receive(:find_by).with(name: student1.name).and_return(student1) allow(admin).to receive(:can_impersonate?).with(student1).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: student1.name } } @session = { user: admin } post :impersonate, @params, @session expect(session[:super_user]).to eq admin expect(session[:user]).to eq student1 expect(session[:original_user]).to eq admin expect(session[:impersonate]).to be true end
- admin should be able to impersonate a teaching assistant with their real name
This tests for the case when admin tries to impersonate a teaching assistant in which case parameter 'impersonate' will be true in session parameters
it 'admin should be able to impersonate a teaching assistant with their real name' do stub_current_user(admin, admin.role.name, admin.role) allow(User).to receive(:find_by).with(name: teaching_assistant.name).and_return(teaching_assistant) allow(admin).to receive(:can_impersonate?).with(teaching_assistant).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: teaching_assistant.name } } @session = { user: admin } post :impersonate, @params, @session expect(session[:super_user]).to eq admin expect(session[:user]).to eq teaching_assistant expect(session[:original_user]).to eq admin expect(session[:impersonate]).to be true end
- admin should be able to impersonate an instructor with their real name
This tests for the case when admin tries to impersonate an instructor in which case parameter 'impersonate' will be true in session parameters
it 'admin should be able to impersonate an instructor with their real name' do stub_current_user(admin, admin.role.name, admin.role) allow(User).to receive(:find_by).with(name: instructor.name).and_return(instructor) allow(admin).to receive(:can_impersonate?).with(instructor).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: instructor.name } } @session = { user: admin } post :impersonate, @params, @session expect(session[:super_user]).to eq admin expect(session[:user]).to eq instructor expect(session[:original_user]).to eq admin expect(session[:impersonate]).to be true end
- admin should not be able to impersonate a super admin with their real name
This tests for the case when admin tries to impersonate a super admin in which case parameter 'impersonate' will be nil in session parameters
it 'admin should not be able to impersonate a super admin with their real name' do stub_current_user(admin, admin.role.name, admin.role) allow(User).to receive(:find_by).with(name: super_admin.name).and_return(super_admin) allow(admin).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: admin } post :impersonate, @params, @session expect(session[:impersonate]).to be nil end
- super admin should be able to impersonate a student with their real name
This tests for the case when super admin tries to impersonate a student in which case parameter 'impersonate' will be true in session parameters
it 'super admin should be able to impersonate a student with their real name' do stub_current_user(super_admin, super_admin.role.name, super_admin.role) allow(User).to receive(:find_by).with(name: student1.name).and_return(student1) allow(super_admin).to receive(:can_impersonate?).with(student1).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: student1.name } } @session = { user: super_admin } post :impersonate, @params, @session expect(session[:super_user]).to eq super_admin expect(session[:user]).to eq student1 expect(session[:original_user]).to eq super_admin expect(session[:impersonate]).to be true end
- super admin should be able to impersonate a teaching assistant with their real name
This tests for the case when super admin tries to impersonate a teaching assistant in which case parameter 'impersonate' will be true in session parameters
it 'super admin should be able to impersonate a teaching assistant with their real name' do stub_current_user(super_admin, super_admin.role.name, super_admin.role) allow(User).to receive(:find_by).with(name: teaching_assistant.name).and_return(teaching_assistant) allow(super_admin).to receive(:can_impersonate?).with(teaching_assistant).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: teaching_assistant.name } } @session = { user: super_admin } post :impersonate, @params, @session expect(session[:super_user]).to eq super_admin expect(session[:user]).to eq teaching_assistant expect(session[:original_user]).to eq super_admin expect(session[:impersonate]).to be true end
- super admin should be able to impersonate an instructor with their real name
This tests for the case when super admin tries to impersonate an instructor in which case parameter 'impersonate' will be true in session parameters
it 'super admin should be able to impersonate an instructor with their real name' do stub_current_user(super_admin, super_admin.role.name, super_admin.role) allow(User).to receive(:find_by).with(name: instructor.name).and_return(instructor) allow(super_admin).to receive(:can_impersonate?).with(instructor).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: instructor.name } } @session = { user: super_admin } post :impersonate, @params, @session expect(session[:super_user]).to eq super_admin expect(session[:user]).to eq instructor expect(session[:original_user]).to eq super_admin expect(session[:impersonate]).to be true end
- super admin should be able to impersonate an admin with their real name
This tests for the case when super admin tries to impersonate an admin in which case parameter 'impersonate' will be true in session parameters
it 'super admin should be able to impersonate an admin with their real name' do stub_current_user(super_admin, super_admin.role.name, super_admin.role) allow(User).to receive(:find_by).with(name: admin.name).and_return(admin) allow(super_admin).to receive(:can_impersonate?).with(admin).and_return(true) request.env['HTTP_REFERER'] = 'http://www.example.com' @params = { user: { name: admin.name } } @session = { user: super_admin } post :impersonate, @params, @session expect(session[:super_user]).to eq super_admin expect(session[:user]).to eq admin expect(session[:original_user]).to eq super_admin expect(session[:impersonate]).to be true end
Useful Links
The following are the links to the useful pages in understanding this project(as of Spring 2022)
- [1] - General wiki describing the Expertiza project.
- [2] - GitHub repo that maintains code for Expertiza.
- [3] - GitHub repo for the work done in Spring 2022 for the refactoring of impersonate controller.
- [4] - Spring 2021's wiki for the refactoring of impersonate controller.
- [5] - Pull request for the work done in Spring 2022.
- [6]- Issue that has been raised for the undiscovered bug that was breaking the application.
- [7] - Application Link