CSC/ECE 517 Fall 2019 - E1957. Time travel Not Allowed..!!! Restrict TAs’ ability to change their own grade + limit file-size upload: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
 
(44 intermediate revisions by 3 users not shown)
Line 6: Line 6:


* A student can upload files with their submission. In some cases, students upload long videos that might not be necessary for the submission. As there is no restriction on the files being uploaded, this is a security issue in Expertiza. Uploaded file's size and type should be restricted since a student may also upload malware into the system affecting Expertiza.
* A student can upload files with their submission. In some cases, students upload long videos that might not be necessary for the submission. As there is no restriction on the files being uploaded, this is a security issue in Expertiza. Uploaded file's size and type should be restricted since a student may also upload malware into the system affecting Expertiza.
===Description===
* TA
Account: student7126
Password: 123456
Spring 2017  Course:517  Role: student
Fall 2017      Course:517  Role: TA
Must log in as student7126, impersonate user via instructor will not work since impersonate will not change current user id to student user-id!
* File Upload
You can impersonate any students of CSC 506-Fall 2017 to upload a file for the assignments.


==Design Pattern==
==Design Pattern==
design pattern
This project's goal is to add new features in existing method. Since we have to use methods and features depends on different input situations, we decided to use Chain-of-responsibility pattern as our design pattern. With the benefit that the condition–action blocks can be dynamically rearranged and reconfigured at runtime, we could handle different situations in different ways. Also the function of the code could be easily figure out by the condition.


==Restrict TAs’ ability to change their own grade==
==Restrict TAs’ ability to change their own grade==
Line 45: Line 34:
* grades_controller.rb
* grades_controller.rb
<pre>
<pre>
if TaMapping.where(ta_id:current_user.id,course_id:@assignment.course.id).empty?&&current_user.role.name !='Instructor'
    if tm.nil? && current_user.role.name != 'Instructor'
       flash[:error] = 'Unauthorized action!'
       flash[:error] = 'Unauthorized action'
      redirect_to controller: 'grades', action: 'view_team', id: participant.id
     else
     else
    participant = AssignmentParticipant.find_by(id: params[:participant_id])
      @team = @participant.team
    @team = participant.team
      @team.grade_for_submission = params[:grade_for_submission]
    @team.grade_for_submission = params[:grade_for_submission]
      @team.comment_for_submission = params[:comment_for_submission]
    @team.comment_for_submission = params[:comment_for_submission]
      begin
    begin
        @team.save
      @team.save
        flash[:success] = 'Grade and comment for submission successfully saved.'
      flash[:success] = 'Grade and comment for submission successfully saved.'
      rescue StandardError
    rescue StandardError
        flash[:error] = $ERROR_INFO
      flash[:error] = $ERROR_INFO
      end
     end
     end
    redirect_to controller: 'grades', action: 'view_team', id: participant.id
end
</pre>
</pre>
The function in GradesController that should be modified is ''save_grade_and_comment_for_submission''. A condition is added to see a user's role. The grade and submission can be saved only if the user is a TA of an instructor of such course. Otherwise, there should be an error and the page will be redirected to the view_team page.
The function in GradesController that should be modified is ''save_grade_and_comment_for_submission''. A condition is added to see a user's role. The grade and submission can be saved only if the user is a TA of an instructor of such course. Otherwise, there should be an error and the page will be redirected to the view_team page.


==TA's time travel not allowed==
===Implementation===
Student 7126 had taken 517 in spring 2017, and became a TA for 517 in fall 2017. He could not change his own score for spring 2017's 517 course now.
Student 7126 had taken 517 in spring 2017, and became a TA for 517 in fall 2017. He could not change his own score for spring 2017's 517 course now.
[[File:Student7126 View His Score.png]]
[[File:Student7126 View His Score.png]]
<br>
For course 517 fall 2017 in which student7126 plays a TA, he could edit grade for submission.


[[File:Student7126 Grade Score.png]]
<br><br><br>
==Limit file-size/type upload==
==Limit file-size/type upload==


[[File:file size.jpg]]
===Solution===
Implemented validation on both front end side and server side. Limit the type (PDF, PNG, JPEG, JPG, ZIP, TAR, 7z, ODT, DOCX) and size (5MB) of the file to be uploaded.


===Files modified===
*_submitted_files.html.erb
<pre>
  <input type="file" id="uploaded_file" name="uploaded_file" size=40 onchange = "ValidateSingleInput(this);"/>
</pre>
<pre>
if (oInput.type == "file") {
            var sFileName = oInput.value;
            if(filesize>maxfilesize){
                alert("The maximum upload file size is 5MB!");
                oInput.value = "";
                return false;
            }
            if (sFileName.length > 0) {
                var blnValid = false;
                for (var j = 0; j < _validFileExtensions.length; j++) {
                    var sCurExtension = _validFileExtensions[j];
                    if (sFileName.substr(sFileName.length - sCurExtension.length, sCurExtension.length).toLowerCase() == sCurExtension.toLowerCase()) {
                        blnValid = true;
                        break;
                    }
                }
                if (!blnValid) {
                    // alert(sFileName + " is invalid, allowed file types are: " + _validFileExtensions.join(", "));
                    alert("Allowed file types are: " + _validFileExtensions.join(", "));


===Solution===
                    oInput.value = "";
===Files modified===
                    return false;
                }
            }
        }
</pre>
Above code is for client side valiidation.
*submitted_content_controller.rb
<pre>
if(file.size>5*1024*1024)
      flash[:error] = 'File Size must smaller than 5MB!'
      redirect_to action: 'edit', id: participant.id
    end
    type=file.original_filename.split('.')
    if %w(pdf jpg jpeg tar zip png 7z odt docx).include? type[1].downcase==false
      flash[:error] = 'File type error!'
      redirect_to action: 'edit', id: participant.id
    end
</pre>
Above code is for server side validation.
 
===Implementation===
[[File:File Size Limitation.png]]
<br>
[[File:File Type Limitation.png]]
<br>
 
 
==Test Plan==
For automotive test:
First, we have adder factor TaMapping in factories.rb to return a tamapping connected TA and the course.
<pre>
  factory :tamapping, class: TaMapping do
    ta_id 6
    course_id 1
  end
</pre>
Second, we added and modified the existing Rspec Tests for submit grade controllers to fit current implementations.
The TaMapping returns a nil object which means TA is grading the course he is not teaching for and will be redirect to the view page directly.
<pre>
    context 'when TA grade the assignment for course he plays a student' do
      it 'saves grade and comment for submission and refreshes the grades#view_team page' do
        allow(AssignmentParticipant).to receive(:find_by).with(id: '1').and_return(participant)
        allow(participant).to receive(:team).and_return(build(:assignment_team, id: 2, parent_id: 8))
        allow(TaMapping).to receive(:where).with(ta_id:'6', course_id:"1").and_return(tamapping)
        params = {
          ta_id: 6,
          participant_id: 1,
          course_id:1,
          grade_for_submission: 100,
          comment_for_submission: 'comment'
        }
        post :save_grade_and_comment_for_submission, params
        # redirect to view page without change grade
        expect(response).to redirect_to('/grades/view_team?id=1')
      end
</pre>
For file upload, a valid type file should be accepted
<pre>
  it "is able to submit valid type of file" do
    signup_topic
    file_path = Rails.root + "spec/features/assignment_submission_txts/valid_assignment_file.txt"
    attach_file('uploaded_file', file_path)
    click_on 'Upload file'
    expect(page).to have_content "valid_assignment_file.txt"


==Testing using RSPEC==
    # check content of the uploaded file
    file_upload_path = Rails.root + "pg_data/instructor6/csc517/test/Assignment1684/0/valid_assignment_file.txt"
    expect(File).to exist(file_upload_path)
    expect(File.read(file_upload_path)).to have_content "valid_assignment_file: This is a .txt file to test assignment submission."
  end
</pre>
<br><br>
For UI test:
*To test TAs’ ability to change their own grade
You can log in by the following account and password: <br>
Account: student7126 <br>
Password: 123456<br>
Role of this account: 
Student (517, Spring 2017)    & 
TA (517, Fall 2017)      <br>
Must log in as student7126, impersonate user via instructor will not work since impersonate will not change current user id to student user id!
<br>
*To test file upload type/size restriction
Impersonate any students of CSC 506-Fall 2017 via the instructor account to upload a file for the assignments. <br>
Or, you can modify the deadline for any assignments of any course to test relevant functionality.

Latest revision as of 01:23, 7 November 2019

This wiki page is for the description of the Expertiza based OSS project - E1957

Introduction

Background

  • In Expertiza, If a user is listed as a TA in one course and as a student in another course, then if they navigate to the "Your scores" page of one of the assignments in which they are participating as a student, they can see a TA's view of that page. This would allow them to assign their own grades! However, TAs should not be able to change their grades from the course that they participated in as a student before.
  • A student can upload files with their submission. In some cases, students upload long videos that might not be necessary for the submission. As there is no restriction on the files being uploaded, this is a security issue in Expertiza. Uploaded file's size and type should be restricted since a student may also upload malware into the system affecting Expertiza.

Design Pattern

This project's goal is to add new features in existing method. Since we have to use methods and features depends on different input situations, we decided to use Chain-of-responsibility pattern as our design pattern. With the benefit that the condition–action blocks can be dynamically rearranged and reconfigured at runtime, we could handle different situations in different ways. Also the function of the code could be easily figure out by the condition.

Restrict TAs’ ability to change their own grade

Solution

When a TA is added to a certain course, a TaMapping is created to connect the TA's id to the course's id. Therefore, we can use TaMapping.where to find a TaMapping with user's id and course's id then use .empty? method to see if such user is a TA of the course. If he/she is a TA of this certain course, he/she should be able to change the score. Otherwise, the changing score area should be hidden from the front end and the permission to change scores should be restricted on the back end.

Files modified

  • view_team.html.erb
<%if TaMapping.where(ta_id:current_user.id, course_id:@assignment.course.id).empty? && current_user.role.name != 'Instructor' %>
  Grade: <%= label_tag 'grade_for_submission', @team.try(:grade_for_submission) %><br/>
  Comment: <%= label_tag 'comment_for_submission', @team.try(:comment_for_submission) %>
<% else %>
  <%= form_tag 'save_grade_and_comment_for_submission' do %>
    <%= hidden_field_tag :participant_id, params[:id] %>
    <%= number_field_tag 'grade_for_submission', @team.try(:grade_for_submission) ,min: 0, max: 100, maxlength: 3, size: 3, class: "form-control width-150", placeholder: 'Grade' %><br/>
    <%= text_area_tag 'comment_for_submission', @team.try(:comment_for_submission), size: '75x10', placeholder: 'Comment', class: "form-control width-500" %><br>
    <%= submit_tag 'Save' ,class: "btn btn-default" %>
  <% end %>
<% end %>

A condition is added to decide what the view should look like. If a user is a TA of this course or a instructor, he/she can change the score. Otherwise, he/she doesn't have the permission to change the score.

  • grades_controller.rb
    if tm.nil? && current_user.role.name != 'Instructor'
      flash[:error] = 'Unauthorized action'
    else
      @team = @participant.team
      @team.grade_for_submission = params[:grade_for_submission]
      @team.comment_for_submission = params[:comment_for_submission]
      begin
        @team.save
        flash[:success] = 'Grade and comment for submission successfully saved.'
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
    end

The function in GradesController that should be modified is save_grade_and_comment_for_submission. A condition is added to see a user's role. The grade and submission can be saved only if the user is a TA of an instructor of such course. Otherwise, there should be an error and the page will be redirected to the view_team page.

Implementation

Student 7126 had taken 517 in spring 2017, and became a TA for 517 in fall 2017. He could not change his own score for spring 2017's 517 course now.
For course 517 fall 2017 in which student7126 plays a TA, he could edit grade for submission.




Limit file-size/type upload

Solution

Implemented validation on both front end side and server side. Limit the type (PDF, PNG, JPEG, JPG, ZIP, TAR, 7z, ODT, DOCX) and size (5MB) of the file to be uploaded.

Files modified

  • _submitted_files.html.erb
  <input type="file" id="uploaded_file" name="uploaded_file" size=40 onchange = "ValidateSingleInput(this);"/>
if (oInput.type == "file") {
            var sFileName = oInput.value;
            if(filesize>maxfilesize){
                alert("The maximum upload file size is 5MB!");
                oInput.value = "";
                return false;
            }
            if (sFileName.length > 0) {
                var blnValid = false;
                for (var j = 0; j < _validFileExtensions.length; j++) {
                    var sCurExtension = _validFileExtensions[j];
                    if (sFileName.substr(sFileName.length - sCurExtension.length, sCurExtension.length).toLowerCase() == sCurExtension.toLowerCase()) {
                        blnValid = true;
                        break;
                    }
                }
                if (!blnValid) {
                    // alert(sFileName + " is invalid, allowed file types are: " + _validFileExtensions.join(", "));
                    alert("Allowed file types are: " + _validFileExtensions.join(", "));

                    oInput.value = "";
                    return false;
                }
            }
        }

Above code is for client side valiidation.

  • submitted_content_controller.rb
if(file.size>5*1024*1024)
      flash[:error] = 'File Size must smaller than 5MB!'
      redirect_to action: 'edit', id: participant.id
    end
    type=file.original_filename.split('.')
    if %w(pdf jpg jpeg tar zip png 7z odt docx).include? type[1].downcase==false
      flash[:error] = 'File type error!'
      redirect_to action: 'edit', id: participant.id
    end

Above code is for server side validation.

Implementation




Test Plan

For automotive test: First, we have adder factor TaMapping in factories.rb to return a tamapping connected TA and the course.

  factory :tamapping, class: TaMapping do
    ta_id 6
    course_id 1
  end

Second, we added and modified the existing Rspec Tests for submit grade controllers to fit current implementations. The TaMapping returns a nil object which means TA is grading the course he is not teaching for and will be redirect to the view page directly.

    context 'when TA grade the assignment for course he plays a student' do
      it 'saves grade and comment for submission and refreshes the grades#view_team page' do
        allow(AssignmentParticipant).to receive(:find_by).with(id: '1').and_return(participant)
        allow(participant).to receive(:team).and_return(build(:assignment_team, id: 2, parent_id: 8))
        allow(TaMapping).to receive(:where).with(ta_id:'6', course_id:"1").and_return(tamapping)
        params = {
          ta_id: 6,
          participant_id: 1,
          course_id:1,
          grade_for_submission: 100,
          comment_for_submission: 'comment'
        }
        post :save_grade_and_comment_for_submission, params
        # redirect to view page without change grade
        expect(response).to redirect_to('/grades/view_team?id=1')
      end

For file upload, a valid type file should be accepted

  it "is able to submit valid type of file" do
    signup_topic
    file_path = Rails.root + "spec/features/assignment_submission_txts/valid_assignment_file.txt"
    attach_file('uploaded_file', file_path)
    click_on 'Upload file'
    expect(page).to have_content "valid_assignment_file.txt"

    # check content of the uploaded file
    file_upload_path = Rails.root + "pg_data/instructor6/csc517/test/Assignment1684/0/valid_assignment_file.txt"
    expect(File).to exist(file_upload_path)
    expect(File.read(file_upload_path)).to have_content "valid_assignment_file: This is a .txt file to test assignment submission."
  end



For UI test:

  • To test TAs’ ability to change their own grade

You can log in by the following account and password:
Account: student7126
Password: 123456
Role of this account: Student (517, Spring 2017) & TA (517, Fall 2017)
Must log in as student7126, impersonate user via instructor will not work since impersonate will not change current user id to student user id!

  • To test file upload type/size restriction

Impersonate any students of CSC 506-Fall 2017 via the instructor account to upload a file for the assignments.
Or, you can modify the deadline for any assignments of any course to test relevant functionality.