E1915 Authorization Utilities: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
Line 175: Line 175:
| 0
| 0
| 0
| 0
| TBD
| TBD
| TBD
| TBD
| TBD
Line 184: Line 185:
| 0
| 0
| 0
| 0
| TBD
| TBD
| TBD
| TBD
| TBD
Line 193: Line 195:
| 0
| 0
| 0
| 0
| TBD
| TBD
| TBD
| TBD
| TBD
Line 202: Line 205:
| 0
| 0
| 0
| 0
| TBD
| TBD
| TBD
| TBD
| TBD
Line 211: Line 215:
| 0
| 0
| 0
| 0
| TBD
| TBD
| TBD
| TBD
| TBD
Line 220: Line 225:
| 0
| 0
| 1
| 1
| TBD
| TBD
| TBD
| TBD
| TBD
Line 229: Line 235:
| 0
| 0
| 0
| 0
| TBD
| TBD
| TBD
| TBD
| TBD
Line 234: Line 241:
|-
|-
| lottery_controller_spec.rb
| lottery_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 242: Line 251:
|-
|-
| notifications_controller_spec.rb
| notifications_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 250: Line 261:
|-
|-
| participants_controller_spec.rb
| participants_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 258: Line 271:
|-
|-
| password_retrieval_controller_spec.rb
| password_retrieval_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 266: Line 281:
|-
|-
| popup_controller_spec.rb
| popup_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 274: Line 291:
|-
|-
| questionnaires_controller_spec.rb
| questionnaires_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 282: Line 301:
|-
|-
| reports_controller_spec.rb
| reports_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 290: Line 311:
|-
|-
| response_controller_spec.rb
| response_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 298: Line 321:
|-
|-
| review_mapping_controller_spec.rb
| review_mapping_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 306: Line 331:
|-
|-
| sign_up_sheet_controller_spec.rb
| sign_up_sheet_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 314: Line 341:
|-
|-
| student_teams_controller_spec.rb
| student_teams_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 322: Line 351:
|-
|-
| teams_controller_spec.rb
| teams_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 330: Line 361:
|-
|-
| tree_display_controller_spec.rb
| tree_display_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD
Line 338: Line 371:
|-
|-
| users_controller_spec.rb
| users_controller_spec.rb
| TBD
| TBD
| TBD
| TBD
| TBD
| TBD

Revision as of 19:22, 19 March 2019

E1915. Authorization Utilities

This page provides a description of an Expertiza OSS project.



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.

Problem Statement

The following tasks were accomplished in this project:

  • Centralize user authentication logic to support the DRY principle
  • Improve user authentication logic in cases where it was clearly flawed
  • Support this work with RSpec unit tests

Current Implementation

Functionality
  • Most controllers contain an action_allowed? method which determines which users are allowed to perform which actions
  • This logic is in most cases correct, but is often repeated between controllers (un-DRY)
  • This logic is in some cases slightly incorrect
  • The Roles model provides a helpful method hasAllPrivilegesOf, which could be used to simplify authorization logic
Problems and Solutions

The problems listed below are examples of the four main classes of problems we encountered with Expertiza authorization. This is not an exhaustive list of problems, but is a good representation of the classes of problems addressed.

  • Problem 1: Much of the authorization logic is repeated (un-DRY). For example, multiple controllers contain the following exact code.
  ['Super-Administrator',
  'Administrator',
  'Instructor',
  'Teaching Assistant'].include? current_role_name
  • Solution 1: Use one of the helper methods from the new authorization_helper.rb to allow TAs *and above* (instructors, admins, super-admins) to perform this work.
  current_user_has_ta_privileges?
  • Problem 2: Some logic is slightly incorrect. For example, some places call for a specific user type, when users "above" this type should also be allowed to perform the work. In the following example (advertise_for_partner_controller.rb), only Students may advertise for partners. However per Dr. Gehringer, "There are no cases I am aware of where a particular type of user can do something that more-privileged users cannot do".
  current_user.role.name.eql?("Student")
  • Solution 2: Use one of the helper methods from the new authorization_helper.rb to allow Students *and above* (TAs, instructors, admins, super-admins) to perform this work.
  current_user_has_student_privileges?
  • Problem 3: Too much authorization logic is present in the controllers. This makes the controllers more difficult to read, and scatters authorization logic, when it would be easier to understand if it were all in one place.
  def action_allowed?
    if %w[edit update list_submissions].include? params[:action]
      assignment = Assignment.find(params[:id])
      (%w[Super-Administrator Administrator].include? current_role_name) ||
      (assignment.instructor_id == current_user.try(:id)) ||
      TaMapping.exists?(ta_id: current_user.try(:id), course_id: assignment.course_id) ||
      (assignment.course_id && Course.find(assignment.course_id).instructor_id == current_user.try(:id))
    else
      ['Super-Administrator',
       'Administrator',
       'Instructor',
       'Teaching Assistant'].include? current_role_name
    end
  end
  • Solution 3: Establish helper methods in the new authorization_helper.rb to centralize as much authorization logic as possible. In this way, a developer with questions about authorization knows just where to look to find answers - authorization_helper.rb.
  def action_allowed?
    if %w[edit update list_submissions].include? params[:action]
      current_user_has_admin_privileges? || current_user_teaching_staff_of_assignment?(params[:id])
    else
      current_user_has_ta_privileges?
    end
  end
  • Problem 4: Some action_allowed? methods are difficult to follow, and/or knowledge about how the action parameter should affect authorization is buried in another method.
  def action_allowed?
    current_user_has_student_privileges?and
    ((%w[edit].include? action_name) ? are_needed_authorizations_present?(params[:id], "reader", "reviewer") : true) and
    one_team_can_submit_work?
  end
  • Solution 4: Clean up action_allowed? methods and make the influence of the action parameter visible at this level.
  def action_allowed?
    case params[:action]
    when 'edit'
      current_user_has_student_privileges? &&
      are_needed_authorizations_present?(params[:id], "reader", "reviewer")
    when 'submit_file', 'submit_hyperlink'
      current_user_has_student_privileges? &&
      one_team_can_submit_work?
    else
      current_user_has_student_privileges?
    end
  end

New Implementation

  • We make use of the existing role.rb model method hasAllPrivileges of. This logic defines a hierarchy of users, allowing us to easily determine if the current user has a particular role "or above". We made one correction to this method, to change the logic from ">" to ">=", to ensure that for example a TA has all the privileges of a TA.
  def hasAllPrivilegesOf(target_role)

    privileges = {}
    privileges["Student"] = 1
    privileges["Teaching Assistant"] = 2
    privileges["Instructor"] = 3
    privileges["Administrator"] = 4
    privileges["Super-Administrator"] = 5

    privileges[self.name] >= privileges[target_role.name]

  end
  • We establish several methods in authorization_helper.rb to expose easy-to-read method names for use in controllers.
  def current_user_has_super_admin_privileges?
  ...
  def current_user_has_admin_privileges?
  ...
  def current_user_has_instructor_privileges?
  ...
  def current_user_has_ta_privileges?
  ...
  def current_user_has_student_privileges?
  ...
  • We establish a method in authorization_helper.rb to expose an easy-to-read method for determining if the current user "is a" [particular user role]. This is used in a minority of cases, because most logic cares if the current user "is a" [particular role] "or above".
  def current_user_is_a?(role_name)
  ...
  • We establish several methods in authorization_helper.rb to centralize more complex authorization logic so that it is not scattered among controllers, but rather is kept in the same helper file as other authorization logic. Only a few of these methods are shown below.
  def current_user_is_assignment_participant?(assignment_team_id)
  ...
  def current_user_teaching_staff_of_assignment?(assignment_id)
  ...
  def current_user_created_bookmark_id?(bookmark_id)
  ...

Automated Testing with RSPEC

Our strategy for gaining confidence that our code changes did not break anything was as follows:

  • Run all existing controller RSpec tests before and after our changes to ensure no changes in results. We use the RSpec "--seed" flag to run the test with the same seed in the "after" column as the randomly selected seed that was applied in the "before" column.
Controller Spec Seed (before) Examples (before) Failures (before) Pending (before) Seed (after) Examples (after) Failures (after) Pending (after)
airbrake_exception_errors_controller_tests_spec.rb 43064 8 0 0 TBD TBD TBD TBD
application_controller_spec.rb 5410 2 0 0 TBD TBD TBD TBD
assessment360_controller_spec.rb 23270 2 0 0 TBD TBD TBD TBD
assignments_controller_spec.rb 20807 22 0 0 TBD TBD TBD TBD
course_controller_spec.rb 47911 6 0 0 TBD TBD TBD TBD
grades_controller_spec.rb 48285 11 0 1 TBD TBD TBD TBD
invitations_controller_spec.rb 26436 8 0 0 TBD TBD TBD TBD
lottery_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
notifications_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
participants_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
password_retrieval_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
popup_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
questionnaires_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
reports_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
response_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
review_mapping_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
sign_up_sheet_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
student_teams_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
teams_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
tree_display_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
users_controller_spec.rb TBD TBD TBD TBD TBD TBD TBD TBD
  • Write new comprehensive RSpec tests (authorization_helper_spec.rb) for every public method in our new helper (authorization_helper.rb). The command and output below shows an example of running these new tests. Please note that the output you see may differ slightly, as more tests may be added.

e@ubuntu:~/Desktop/expertiza$ bundle exec rspec spec/helpers/authorization_helper_spec.rb 
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's 'rails' settings.

Randomized with seed 50616
......................................................................................

Finished in 1 minute 54.21 seconds (files took 23.18 seconds to load)
86 examples, 0 failures

Randomized with seed 50616

Coverage report generated for RSpec to /home/e/Desktop/expertiza/coverage. 1393 / 17339 LOC (8.03%) covered.
[Coveralls] Outside the CI environment, not sending data.

Manual Testing with Deployed Application

  • TODO: add link to video demonstrating manual test steps for a few key authorizations

References (Our Work)

  1. GitHub Project Repository Fork
  2. Deployed Application

References (General)

  1. Expertiza on GitHub
  2. The live Expertiza website
  3. Expertiza project documentation wiki
  4. Rspec Documentation