From a9379e8183d67348e901790880a0969d2fe95627 Mon Sep 17 00:00:00 2001 From: mishaschwartz Date: Tue, 18 Oct 2022 20:28:14 -0400 Subject: [PATCH 1/3] release: allow released marks to be hidden behind a unique url/token (#6244) --- Changelog.md | 1 + .../Components/Modals/release_urls_modal.jsx | 175 +++++++++++ .../Modals/submit_view_token_modal.jsx | 60 ++++ .../Components/Result/left_pane.jsx | 1 - .../Components/Result/remark_panel.jsx | 6 +- .../Components/submission_table.jsx | 120 +++++-- app/assets/stylesheets/common/_icons.scss | 6 + app/assets/stylesheets/common/core.scss | 3 +- app/controllers/assignments_controller.rb | 3 +- app/controllers/results_controller.rb | 94 ++++-- app/controllers/submissions_controller.rb | 11 + app/helpers/application_helper.rb | 12 +- app/helpers/random_assign_helper.rb | 8 +- app/javascript/application_webpack.js | 2 + app/models/assignment.rb | 9 +- app/models/result.rb | 12 +- app/models/submission.rb | 8 + app/policies/result_policy.rb | 26 +- app/views/assignments/_form.html.erb | 4 + app/views/assignments/_row.html.erb | 25 +- .../layouts/menus/_student_sub_menu.html.erb | 29 +- .../results/student/no_remark_result.html.erb | 3 +- .../shared/_submit_view_token_modal.js.erb | 15 + app/views/submissions/browse.html.erb | 1 + config/locales/common/en.yml | 1 + config/locales/models/assignments/en.yml | 1 + config/locales/policies/en.yml | 1 + config/locales/views/results/en.yml | 2 + config/locales/views/submissions/en.yml | 25 ++ config/routes.rb | 16 +- ...lease_with_url_to_assignment_properties.rb | 17 + db/structure.sql | 17 +- lib/tasks/marks.rake | 3 +- .../assignments_controller_spec.rb | 1 + spec/controllers/results_controller_spec.rb | 295 ++++++++++++++++-- spec/models/assignment_spec.rb | 21 ++ spec/models/result_spec.rb | 19 ++ spec/models/submission_spec.rb | 32 ++ spec/policies/result_policy_spec.rb | 92 +++++- 39 files changed, 1063 insertions(+), 114 deletions(-) create mode 100644 app/assets/javascripts/Components/Modals/release_urls_modal.jsx create mode 100644 app/assets/javascripts/Components/Modals/submit_view_token_modal.jsx create mode 100644 app/views/shared/_submit_view_token_modal.js.erb create mode 100644 db/migrate/20220922131809_add_release_with_url_to_assignment_properties.rb diff --git a/Changelog.md b/Changelog.md index a895370fe1..a80aa1e8bf 100644 --- a/Changelog.md +++ b/Changelog.md @@ -14,6 +14,7 @@ - Fix bug where gzipped binary feedback files were not unzipped correctly (#6283) - Fix bug where remark request summary table wasn't being rendered correctly (#6284) - Fix bug where test results were being associated with the wrong test runs (#6287) +- Allow results to be made available only through unique tokens (#6244) ## [v2.1.4] - Fix bug where git hooks are not run server side when symlinked (#6276/#6277) diff --git a/app/assets/javascripts/Components/Modals/release_urls_modal.jsx b/app/assets/javascripts/Components/Modals/release_urls_modal.jsx new file mode 100644 index 0000000000..5329a8078b --- /dev/null +++ b/app/assets/javascripts/Components/Modals/release_urls_modal.jsx @@ -0,0 +1,175 @@ +import React from "react"; +import Modal from "react-modal"; +import ReactTable from "react-table"; +import Flatpickr from "react-flatpickr"; + +class ReleaseUrlsModal extends React.Component { + constructor() { + super(); + this.state = {loading: false}; + } + + componentDidMount() { + Modal.setAppElement("body"); + } + + refreshViewTokens = result_ids => { + if (!confirm(I18n.t("submissions.refresh_token_confirmation"))) { + return; + } + this.setState({loading: true}, () => { + $.ajax({ + type: "PUT", + url: Routes.refresh_view_tokens_course_assignment_results_url( + this.props.course_id, + this.props.assignment_id, + {result_ids: result_ids} + ), + }).then(res => this.props.refreshViewTokens(res, () => this.setState({loading: false}))); + }); + }; + + refreshViewTokenExpiry = (expiry_datetime, result_ids) => { + if ( + result_ids.length > 1 && + !confirm( + expiry_datetime + ? I18n.t("submissions.update_all_token_expiry_confirmation", {date: expiry_datetime}) + : I18n.t("submissions.clear_all_token_expiry_confirmation") + ) + ) { + return; + } + this.setState({loading: true}, () => { + $.ajax({ + type: "PUT", + url: Routes.update_view_token_expiry_course_assignment_results_url( + this.props.course_id, + this.props.assignment_id, + {result_ids: result_ids, expiry_datetime: expiry_datetime} + ), + }).then(res => this.props.refreshViewTokenExpiry(res, () => this.setState({loading: false}))); + }); + }; + + render() { + return ( + +
+ + { + this.refreshViewTokenExpiry( + selectedDates[0], + this.props.data.map(d => d["result_id"]) + ); + }} + options={{ + altInput: true, + altFormat: I18n.t("time.format_string.flatpickr"), + dateFormat: "Z", + }} + /> + d["result_id"])} + )} + > + {I18n.t("download")} + +
+ { + return ( +
+ this.refreshViewTokens([row.original.result_id])} + title={I18n.t("refresh")} + /> + {row.original.result_view_token} +
+ ); + }, + }, + { + Header: I18n.t("submissions.release_token_expires"), + id: "result_view_token_expiry", + filterable: false, + sortable: false, + minWidth: 200, + Cell: row => { + return ( + + this.refreshViewTokenExpiry(selectedDates[0], [row.original.result_id]) + } + options={{ + altInput: true, + altFormat: I18n.t("time.format_string.flatpickr"), + dateFormat: "Z", + }} + /> + ); + }, + }, + ]} + SubComponent={row => { + if (row.original.result_view_token) { + const url = Routes.view_marks_course_result_url( + this.props.course_id, + row.original.result_id, + {view_token: row.original.result_view_token} + ); + return ( +
+ {I18n.t("submissions.release_url_with_token", {url: url})} +
+ ); + } else { + return ""; + } + }} + /> + + ); + } +} + +export default ReleaseUrlsModal; diff --git a/app/assets/javascripts/Components/Modals/submit_view_token_modal.jsx b/app/assets/javascripts/Components/Modals/submit_view_token_modal.jsx new file mode 100644 index 0000000000..c417f13fe8 --- /dev/null +++ b/app/assets/javascripts/Components/Modals/submit_view_token_modal.jsx @@ -0,0 +1,60 @@ +import React from "react"; +import {render} from "react-dom"; +import Modal from "react-modal"; + +class SubmitViewTokenModal extends React.Component { + constructor() { + super(); + this.state = { + isOpen: false, + token: null, + }; + } + + componentDidMount() { + Modal.setAppElement("body"); + } + + onSubmit = () => { + $.ajax({ + url: Routes.view_token_check_course_result_path(this.props.course_id, this.props.result_id, { + view_token: this.state.token, + }), + }).then( + () => { + window.location = Routes.view_marks_course_result_path( + this.props.course_id, + this.props.result_id, + {view_token: this.state.token} + ); + }, + () => this.setState({isOpen: false, token: null}) + ); + }; + + render() { + return ( + this.setState({isOpen: false, token: null})} + > +

{I18n.t("results.view_token_submit")}

+
+
+
+ this.setState({token: e.target.value})} /> +
+
+ +
+
+
+ + ); + } +} + +export function makeSubmitViewTokenModal(elem, props) { + return render(, elem); +} diff --git a/app/assets/javascripts/Components/Result/left_pane.jsx b/app/assets/javascripts/Components/Result/left_pane.jsx index 1a8e95b730..e5302d549c 100644 --- a/app/assets/javascripts/Components/Result/left_pane.jsx +++ b/app/assets/javascripts/Components/Result/left_pane.jsx @@ -164,7 +164,6 @@ export class LeftPane extends React.Component {
{ + let members = ""; + if ( + !row.original.members || + (row.original.members.length === 1 && row.value === row.original.members[0]) + ) { + members = ""; + } else { + members = ` (${row.original.members.join(", ")})`; + } + return row.value + members; + }; + + groupNameFilter = (filter, row) => { + if (filter.value) { + // Check group name + if (row._original.group_name.includes(filter.value)) { + return true; + } + // Check member names + return ( + row._original.members && row._original.members.some(name => name.includes(filter.value)) + ); + } else { + return true; + } + }; + columns = () => [ { show: false, @@ -69,41 +99,19 @@ class RawSubmissionTable extends React.Component { accessor: "group_name", id: "group_name", Cell: row => { - let members = ""; - if ( - !row.original.members || - (row.original.members.length === 1 && row.value === row.original.members[0]) - ) { - members = ""; - } else { - members = ` (${row.original.members.join(", ")})`; - } + const group_name = this.groupNameWithMembers(row); if (row.original.result_id) { const result_url = Routes.edit_course_result_path( this.props.course_id, row.original.result_id ); - return {row.value + members}; + return {group_name}; } else { - return row.value + members; + return group_name; } }, minWidth: 170, - filterMethod: (filter, row) => { - if (filter.value) { - // Check group name - if (row._original.group_name.includes(filter.value)) { - return true; - } - - // Check member names - return ( - row._original.members && row._original.members.some(name => name.includes(filter.value)) - ); - } else { - return true; - } - }, + filterMethod: this.groupNameFilter, }, { Header: I18n.t("submissions.repo_browser.repository"), @@ -219,7 +227,7 @@ class RawSubmissionTable extends React.Component { // Submission table actions collectSubmissions = override => { - this.setState({showModal: false}); + this.setState({showCollectSubmissionsModal: false}); $.post({ url: Routes.collect_submissions_course_assignment_submissions_path( this.props.course_id, @@ -297,6 +305,28 @@ class RawSubmissionTable extends React.Component { }); }; + refreshViewTokens = (updated_tokens, after_function) => { + this.setState(prevState => { + prevState.groupings.forEach(row => { + if (updated_tokens[row.result_id]) { + row["result_view_token"] = updated_tokens[row.result_id]; + } + }); + return prevState; + }, after_function); + }; + + refreshViewTokenExpiry = (updated_tokens, after_function) => { + this.setState(prevState => { + prevState.groupings.forEach(row => { + if (updated_tokens[row.result_id] !== undefined) { + row["result_view_token_expiry"] = updated_tokens[row.result_id]; + } + }); + return prevState; + }, after_function); + }; + render() { const {loading} = this.state; @@ -309,9 +339,10 @@ class RawSubmissionTable extends React.Component { assignment_id={this.props.assignment_id} can_run_tests={this.props.can_run_tests} collectSubmissions={() => { - this.setState({showModal: true}); + this.setState({showCollectSubmissionsModal: true}); }} downloadGroupingFiles={this.prepareGroupingFiles} + showReleaseUrls={() => this.setState({showReleaseUrlsModal: true})} selection={this.props.selection} runTests={this.runTests} releaseMarks={() => this.toggleRelease(true)} @@ -319,6 +350,7 @@ class RawSubmissionTable extends React.Component { completeResults={() => this.setMarkingStates("complete")} incompleteResults={() => this.setMarkingStates("incomplete")} authenticity_token={this.props.authenticity_token} + release_with_urls={this.props.release_with_urls} /> (this.checkboxTable = r)} @@ -337,13 +369,28 @@ class RawSubmissionTable extends React.Component { {...this.props.getCheckboxProps()} /> { - this.setState({showModal: false}); + this.setState({showCollectSubmissionsModal: false}); }} onSubmit={this.collectSubmissions} /> + this.props.selection.includes(g._id) && !!g.result_view_token + )} + groupNameWithMembers={this.groupNameWithMembers} + groupNameFilter={this.groupNameFilter} + course_id={this.props.course_id} + assignment_id={this.props.assignment_id} + refreshViewTokens={this.refreshViewTokens} + refreshViewTokenExpiry={this.refreshViewTokenExpiry} + onRequestClose={() => { + this.setState({showReleaseUrlsModal: false}); + }} + />
); } @@ -370,7 +417,8 @@ class SubmissionsActionBox extends React.Component { collectButton, runTestsButton, releaseMarksButton, - unreleaseMarksButton; + unreleaseMarksButton, + showReleaseUrlsButton; completeButton = ( ); + if (this.props.release_with_urls) { + showReleaseUrlsButton = ( + + ); + } } if (this.props.can_run_tests) { runTestsButton = ( @@ -426,6 +481,7 @@ class SubmissionsActionBox extends React.Component { {runTestsButton} {releaseMarksButton} {unreleaseMarksButton} + {showReleaseUrlsButton} ); }; diff --git a/app/assets/stylesheets/common/_icons.scss b/app/assets/stylesheets/common/_icons.scss index 55b9902c35..12301250dc 100644 --- a/app/assets/stylesheets/common/_icons.scss +++ b/app/assets/stylesheets/common/_icons.scss @@ -174,3 +174,9 @@ .array-item-move-down::after { content: asset-url('icons/arrow_down.png'); } + +.refresh::after { + content: asset-url('icons/arrow_refresh.png'); + padding-right: 5px; + vertical-align: text-top; +} diff --git a/app/assets/stylesheets/common/core.scss b/app/assets/stylesheets/common/core.scss index b16191e816..ded33437a5 100644 --- a/app/assets/stylesheets/common/core.scss +++ b/app/assets/stylesheets/common/core.scss @@ -403,7 +403,8 @@ th.required::after { width: 100%; button, - .button { + .button, + .form-control { height: 3em; line-height: 2.75em; margin-left: 1em; diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index 96c92d23c4..9b32d64c9a 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -851,7 +851,8 @@ def assignment_params :section_due_dates_type, :scanned_exam, :is_timed, - :start_time + :start_time, + :release_with_urls ], assessment_section_properties_attributes: [ :_destroy, diff --git a/app/controllers/results_controller.rb b/app/controllers/results_controller.rb index c8cc889b3a..0016ec9703 100644 --- a/app/controllers/results_controller.rb +++ b/app/controllers/results_controller.rb @@ -3,6 +3,7 @@ class ResultsController < ApplicationController authorize :from_codeviewer, through: :from_codeviewer_param authorize :select_file, through: :select_file + authorize :view_token, through: :view_token_param content_security_policy only: [:edit, :view_marks] do |p| # required because heic2any uses libheif which calls @@ -535,11 +536,20 @@ def revert_to_automatic_deductions } end + # Check whether the token submitted as the view_token param is valid (matches the result and is not expired) + def view_token_check + token_is_good = flash_allowance(:error, allowance_to(:view?), flash.now, most_specific: true).value + token_is_good ? head(:ok) : head(:unauthorized) + end + def view_marks - result_from_id = record - @assignment = result_from_id.submission.grouping.assignment + # set a successful view token in the session so that it doesn't have to be re-entered for every request + (session['view_token'] ||= {})[record.id] = view_token_param + + @result = record + @assignment = @result.submission.grouping.assignment @assignment = @assignment.is_peer_review? ? @assignment.parent_assignment : @assignment - is_review = result_from_id.is_a_review? || result_from_id.is_review_for?(current_role, @assignment) + is_review = @result.is_a_review? || @result.is_review_for?(current_role, @assignment) if current_role.student? @grouping = current_role.accepted_grouping_for(@assignment.id) @@ -556,17 +566,11 @@ def view_marks render 'results/student/no_result' return end - if result_from_id.is_a_review? - @result = result_from_id - else - unless @submission - render 'results/student/no_result' - return - end - @result = @submission.get_original_result + if !@result.is_a_review? && !@submission + render 'results/student/no_result' + return end else - @result = result_from_id @submission = @result.submission @grouping = @submission.grouping end @@ -598,12 +602,10 @@ def view_marks @current_group_name = @current_pr_result.submission.grouping.group.group_name end - @old_result = nil if !is_review && @submission.remark_submitted? - @old_result = @result - @result = @submission.remark_result + remark_result = @submission.remark_result # Check if remark request has been submitted but not released yet - if !@result.remark_request_submitted_at.nil? && !@result.released_to_students + if !remark_result.remark_request_submitted_at.nil? && !remark_result.released_to_students render 'results/student/no_remark_result' return end @@ -653,7 +655,7 @@ def update_overall_comment end def update_remark_request - @submission = Submission.find(params[:submission_id]) + @submission = record.submission @assignment = @submission.grouping.assignment if @assignment.past_remark_due_date? head :bad_request @@ -680,7 +682,7 @@ def update_remark_request # Allows student to cancel a remark request. def cancel_remark_request - submission = Submission.find(params[:submission_id]) + submission = record.submission submission.remark_result.destroy submission.get_original_result.update(released_to_students: true) @@ -708,6 +710,53 @@ def get_test_runs_instructors_released render json: submission.grouping.test_runs_instructors_released(submission) end + # Regenerate the view tokens for the results whose ids are given + def refresh_view_tokens + updated = requested_results.map { |r| r.regenerate_view_token ? r.id : nil }.compact + render json: Result.where(id: updated).pluck(:id, :view_token).to_h + end + + # Update the view token expiry date for the results whose ids are given + def update_view_token_expiry + expiry = params[:expiry_datetime] + updated = requested_results.map { |r| r.update(view_token_expiry: expiry) ? r.id : nil }.compact + render json: Result.where(id: updated).pluck(:id, :view_token_expiry).to_h + end + + # Download a csv containing view token and grouping information for the results whose ids are given + def download_view_tokens + data = requested_results.left_outer_joins(grouping: [:group, { accepted_student_memberships: [role: :user] }]) + .order('groups.group_name') + .pluck('groups.group_name', + 'users.user_name', + 'users.first_name', + 'users.last_name', + 'users.email', + 'users.id_number', + 'results.view_token', + 'results.view_token_expiry', + 'results.id') + header = [[I18n.t('activerecord.models.group.one'), + I18n.t('activerecord.attributes.user.user_name'), + I18n.t('activerecord.attributes.user.first_name'), + I18n.t('activerecord.attributes.user.last_name'), + I18n.t('activerecord.attributes.user.email'), + I18n.t('activerecord.attributes.user.id_number'), + I18n.t('submissions.release_token'), + I18n.t('submissions.release_token_expires'), + I18n.t('submissions.release_token_url')]] + assignment = Assignment.find(params[:assignment_id]) + csv_string = MarkusCsv.generate(data, header) do |row| + view_token, view_token_expiry, result_id = row.pop(3) + view_token_expiry ||= I18n.t('submissions.release_token_expires_null') + url = view_marks_course_result_url(current_course.id, result_id, view_token: view_token) + [*row, view_token, view_token_expiry, url] + end + send_data csv_string, + disposition: 'attachment', + filename: "#{assignment.short_identifier}_release_view_tokens.csv" + end + private def from_codeviewer_param @@ -723,4 +772,13 @@ def extra_mark_params :description, :extra_mark) end + + def view_token_param + params[:view_token] || session['view_token']&.[](record&.id&.to_s) + end + + def requested_results + Result.joins(grouping: :assignment) + .where('results.id': params[:result_ids], 'assessments.id': params[:assignment_id]) + end end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index f2bc5affc3..a259259913 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -90,6 +90,17 @@ def revisions def file_manager @assignment = Assignment.find(params[:assignment_id]) + unless allowed_to?(:see_hidden?, @assignment) + render 'shared/http_status', + formats: [:html], + locals: { + code: '404', + message: HttpStatusHelper::ERROR_CODE['message']['404'] + }, + status: :not_found, + layout: false + return + end @grouping = current_role.accepted_grouping_for(@assignment.id) if @grouping.nil? head :bad_request diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 7d2e876df9..4b48fdd8aa 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -23,10 +23,14 @@ def flash_now(type, text = '', **kwargs) # A version of flash_message that accepts an ActionPolicy authorization result # instead of a message. The result is used to get failure messages and those - # messages are added to the flash hash. If the result is a success, this method - # does nothing. The result is then returned. - def flash_allowance(type, result, flash_type = flash) - message = result.reasons.full_messages.join("\n") + # messages are added to the flash hash. If +most_specific+ is true, then only + # the last error message is added to the flash hash. + # + # If the result is a success, this method does nothing. The result is then returned. + def flash_allowance(type, result, flash_type = flash, most_specific: false) + full_messages = result.reasons.full_messages + full_messages = [full_messages.last] if most_specific + message = full_messages.join("\n") message = result.message if message.blank? flash_message(type, message, flash_type) unless result.value result diff --git a/app/helpers/random_assign_helper.rb b/app/helpers/random_assign_helper.rb index 0b124d080b..b5612f152c 100644 --- a/app/helpers/random_assign_helper.rb +++ b/app/helpers/random_assign_helper.rb @@ -169,7 +169,7 @@ def save_peer_reviews(pr_assignment) end.to_h now = Time.current - results = Result.insert_all( + results = Result.create( @reviewees.map do |reviewee_id| { submission_id: submission_map[reviewee_id], marking_state: Result::MARKING_STATES[:incomplete], @@ -181,7 +181,7 @@ def save_peer_reviews(pr_assignment) Mark.insert_all( results.flat_map do |result| assignment_criteria.map do |criterion| - { result_id: result['id'], + { result_id: result.id, criterion_id: criterion.id, created_at: now, updated_at: now } @@ -193,13 +193,13 @@ def save_peer_reviews(pr_assignment) # Peer reviews require IDs to have been made, so they come last. pr_ids = PeerReview.insert_all( results.zip(@reviewers).map do |result, reviewer_id| - { reviewer_id: reviewer_id, result_id: result['id'], created_at: now, updated_at: now } + { reviewer_id: reviewer_id, result_id: result.id, created_at: now, updated_at: now } end ) Result.upsert_all( results.zip(pr_ids).map do |result, pr_id| - { id: result['id'], peer_review_id: pr_id['id'] } + { id: result.id, peer_review_id: pr_id['id'], view_token: result.view_token } end ) end diff --git a/app/javascript/application_webpack.js b/app/javascript/application_webpack.js index 9780d9f7d4..2a0a9f6334 100644 --- a/app/javascript/application_webpack.js +++ b/app/javascript/application_webpack.js @@ -130,3 +130,5 @@ import {makeAdminUsersList} from "javascripts/Components/admin_users_list"; window.makeAdminUsersList = makeAdminUsersList; import {makeCourseList} from "javascripts/Components/course_list"; window.makeCourseList = makeCourseList; +import {makeSubmitViewTokenModal} from "javascripts/Components/Modals/submit_view_token_modal"; +window.makeSubmitViewTokenModal = makeSubmitViewTokenModal; diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 630306805f..da6364ee3f 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -1086,7 +1086,9 @@ def current_submission_data(current_role) 'results.id', 'results.marking_state', 'results.total_mark', - 'results.released_to_students') + 'results.released_to_students', + 'results.view_token', + 'results.view_token_expiry') .group_by { |h| h['groupings.id'] } if current_role.ta? && anonymize_groups @@ -1157,6 +1159,11 @@ def current_submission_data(current_role) extra_mark = extra_marks_hash[result_info['results.id']] || 0 base[:result_id] = result_info['results.id'] base[:final_grade] = [0, (total_marks[result_info['results.id']] || 0.0) + extra_mark].max + if self.release_with_urls + base[:result_view_token] = result_info['results.view_token'] + token_expiry = result_info['results.view_token_expiry'] + base[:result_view_token_expiry] = token_expiry.nil? ? nil : I18n.l(token_expiry.in_time_zone) + end end base[:members] = member_info.map { |h| h['users.user_name'] } unless member_info.nil? diff --git a/app/models/result.rb b/app/models/result.rb index 56cd4e1e01..474bc359ee 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -13,6 +13,8 @@ class Result < ApplicationRecord has_one :course, through: :submission + has_secure_token :view_token + before_save :check_for_nil_marks after_create :create_marks validates :marking_state, presence: true @@ -32,8 +34,11 @@ def update_total_mark # Update the total_mark attributes for the results with id in +result_ids+ def self.update_total_marks(result_ids, user_visibility: :ta_visible) total_marks = Result.get_total_marks(result_ids, user_visibility: user_visibility) + view_tokens = Result.where(id: result_ids).pluck(:id, :view_token).to_h unless total_marks.empty? - Result.upsert_all(total_marks.map { |r_id, total_mark| { id: r_id, total_mark: total_mark } }) + Result.upsert_all( + total_marks.map { |r_id, total_mark| { id: r_id, total_mark: total_mark, view_token: view_tokens[r_id] } } + ) end end @@ -64,6 +69,7 @@ def self.set_release_on_results(grouping_ids, release) if release groupings.includes(:accepted_students).find_each do |grouping| + next if grouping.assignment.release_with_urls # don't email if release_with_urls is true grouping.accepted_students.each do |student| if student.receives_results_emails? NotificationMailer.with(user: student, grouping: grouping).release_email.deliver_later @@ -181,6 +187,10 @@ def mark_hash marks.pluck_to_hash(:criterion_id, :mark, :override).index_by { |x| x[:criterion_id] } end + def view_token_expired? + !self.view_token_expiry.nil? && Time.current >= self.view_token_expiry + end + private # Do not allow the marking state to be changed to incomplete if the result is released diff --git a/app/models/submission.rb b/app/models/submission.rb index e56ee1da25..ff3094de93 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -99,6 +99,14 @@ def get_latest_result end end + # Returns the latest result that has been released to students + # If no results are released (because a remark request has been submitted but not released) + # then return the original result instead since that one should be visible while the remark + # request is being processed + def get_visible_result + non_pr_results.order(id: :desc).where(released_to_students: true).first || get_original_result + end + # Sets marks when automated tests are run def set_autotest_marks test_run = test_runs.first diff --git a/app/policies/result_policy.rb b/app/policies/result_policy.rb index 7cd82e91f7..8ab17d7698 100644 --- a/app/policies/result_policy.rb +++ b/app/policies/result_policy.rb @@ -1,19 +1,32 @@ # Result policy class class ResultPolicy < ApplicationPolicy default_rule :manage? - alias_rule :update_remark_request?, :cancel_remark_request?, :get_test_runs_instructors_released?, to: :student? + alias_rule :update_remark_request?, :cancel_remark_request?, :get_test_runs_instructors_released?, to: :view_marks? alias_rule :create?, :add_extra_mark?, :remove_extra_mark?, :get_test_runs_instructors?, :add_tag?, :remove_tag?, :revert_to_automatic_deductions?, to: :grade? - alias_rule :show?, :view_marks?, :get_annotations?, :show?, to: :view? + alias_rule :show?, :get_annotations?, to: :view? alias_rule :download_zip?, to: :download? alias_rule :edit?, :update_mark?, :toggle_marking_state?, :update_overall_comment?, :next_grouping?, to: :review? + alias_rule :refresh_view_tokens?, :update_view_token_expiry?, to: :set_released_to_students? - authorize :from_codeviewer, :select_file, optional: true + authorize :from_codeviewer, :select_file, :view_token, optional: true def view? check?(:manage_submissions?, role) || check?(:assigned_grader?, record.grouping) || - check?(:member?, record.submission.grouping) + (!record.grouping.assignment.release_with_urls && check?(:member?, record.submission.grouping)) || + check?(:view_with_result_token?) + end + + def view_marks? + check?(:member?, record.submission.grouping) && + (!record.grouping.assignment.release_with_urls || check?(:view_with_result_token?)) + end + + def view_token_check? + check?(:member?, record.submission.grouping) && + record.grouping.assignment.release_with_urls && + !record.view_token_expired? end def run_tests? @@ -54,4 +67,9 @@ def download? ) ) end + + # This is a separate policy function because it reports a specific error message on failure + def view_with_result_token? + check?(:member?, record.submission.grouping) && record.view_token == view_token && !record.view_token_expired? + end end diff --git a/app/views/assignments/_form.html.erb b/app/views/assignments/_form.html.erb index a587cd8bf5..48721a34ba 100644 --- a/app/views/assignments/_form.html.erb +++ b/app/views/assignments/_form.html.erb @@ -218,6 +218,10 @@ <%= ap_f.label :display_grader_names_to_students, Assignment.human_attribute_name(:display_grader_names_to_students) %> <%= ap_f.check_box :display_grader_names_to_students %> + + <%= ap_f.label :release_with_urls, + Assignment.human_attribute_name(:release_with_urls) %> + <%= ap_f.check_box :release_with_urls %> diff --git a/app/views/assignments/_row.html.erb b/app/views/assignments/_row.html.erb index a2ebd805f3..33e530139f 100644 --- a/app/views/assignments/_row.html.erb +++ b/app/views/assignments/_row.html.erb @@ -13,10 +13,27 @@ <% result = @a_id_results[assignment.id] %> <% if !result.nil? %> <% if assignment.max_mark > 0 && !assignment.results_average.nil? %> -

- <%= link_to Result.model_name.human.pluralize, - view_marks_course_result_path(@current_course, result) %> -

+ <% if allowed_to?(:view?, result, context: {view_token: session["view_token"]&.[](result&.id&.to_s)}) %> + +

+ <%= link_to Result.model_name.human.pluralize, + view_marks_course_result_path(@current_course, result) %> +

+ <% elsif allowed_to? :view_token_check?, result %> +

+ <%= link_to Result.model_name.human.pluralize, '#', + id: opener_id = "submit-view-token-#{assignment.id}" %> +

+ <%= javascript_tag nonce: true do %> + <%= render partial: 'shared/submit_view_token_modal', + formats: :js, + locals: { result: result, + modal_id: "submit-view-token-modal-#{assignment.id}", + opener_id: opener_id } %> + <% end %> + <% else %> + <%= t('results.no_result') %> + <% end %> <% end %> <% elsif assignment.due_date < Time.current %> <%= t('results.no_result') %> diff --git a/app/views/layouts/menus/_student_sub_menu.html.erb b/app/views/layouts/menus/_student_sub_menu.html.erb index 327f042c06..3064bd6fda 100644 --- a/app/views/layouts/menus/_student_sub_menu.html.erb +++ b/app/views/layouts/menus/_student_sub_menu.html.erb @@ -29,7 +29,8 @@ <% end %> <% if !@assignment.nil? && - @current_role.has_accepted_grouping_for?(@assignment) %> + @current_role.has_accepted_grouping_for?(@assignment) && + allowed_to?(:see_hidden?, @assignment) %>
  • '> <%= link_to Assignment.model_name.human.pluralize, course_assignment_path(@current_course, @assignment) %> @@ -53,13 +54,27 @@ <% end %> <% @submission = @grouping.nil? ? nil : @grouping.current_submission_used %> - <% @result = @submission.nil? ? nil : @submission.get_original_result %> + <% @result = @submission.nil? ? nil : @submission.get_visible_result %> <% if !@grouping.nil? && !@submission.nil? && !@result.nil? %> -
  • '> - <%= link_to Result.model_name.human.pluralize, - view_marks_course_result_path(@current_course, @result) %> -
    • + <% active = controller.controller_name == 'results' && !@result.is_a_review? %> + <% if allowed_to?(:view?, @result, context: {view_token: session["view_token"]&.[](@result&.id&.to_s)}) %> + +
  • + <%= link_to Result.model_name.human.pluralize, + view_marks_course_result_path(@current_course, @result) %> +
    • + <% elsif allowed_to? :view_token_check?, @result %> +
  • + <%= link_to Result.model_name.human.pluralize, '#', id: "submit-view-token" %> +
    • + <%= javascript_tag nonce: true do %> + <%= render partial: 'shared/submit_view_token_modal', + formats: :js, + locals: { result: @result, + modal_id: "submit-view-token-modal", + opener_id: "submit-view-token" } %> + <% end %> + <% end %> <% end %> <% if @assignment.has_peer_review_assignment? && allowed_to?(:see_hidden?, @assignment.pr_assignment) %> diff --git a/app/views/results/student/no_remark_result.html.erb b/app/views/results/student/no_remark_result.html.erb index 2a37042a14..e820de14c8 100644 --- a/app/views/results/student/no_remark_result.html.erb +++ b/app/views/results/student/no_remark_result.html.erb @@ -36,8 +36,7 @@ <%= link_to t('results.remark.cancel'), { controller: 'results', action: 'cancel_remark_request', - id: @result.id, - submission_id: @submission.id }, + id: @result.id }, class: 'button', data: { confirm: t('results.remark.cancel_confirm') }, method: :delete %> diff --git a/app/views/shared/_submit_view_token_modal.js.erb b/app/views/shared/_submit_view_token_modal.js.erb new file mode 100644 index 0000000000..803ae1e2f6 --- /dev/null +++ b/app/views/shared/_submit_view_token_modal.js.erb @@ -0,0 +1,15 @@ +window.submitViewTokenModals ||= {}; + +const modal_div = document.createElement('div'); +modal_div.setAttribute('id', '<%= modal_id %>') +document.body.appendChild(modal_div) + +window.submitViewTokenModals['<%= modal_id %>'] = makeSubmitViewTokenModal( + document.getElementById('<%= modal_id %>'), { + course_id: <%= @current_course.id %>, + result_id: <%= result.id %> + } +); +document.getElementById('<%= opener_id %>').addEventListener('click', () => { + window.submitViewTokenModals['<%= modal_id %>'].setState({isOpen: true}) +}) diff --git a/app/views/submissions/browse.html.erb b/app/views/submissions/browse.html.erb index 74af4cb8e7..85387c38db 100644 --- a/app/views/submissions/browse.html.erb +++ b/app/views/submissions/browse.html.erb @@ -18,6 +18,7 @@ show_sections: <%= @current_course.sections.exists? %>, is_timed: <%= @assignment.is_timed %>, is_scanned_exam: <%= @assignment.scanned_exam? %>, + release_with_urls: <%= @assignment.release_with_urls %>, can_collect: <%= allowed_to? :manage? %>, can_run_tests: <%= allowed_to?(:view_test_options?, @assignment) %>, defaultFiltered: [{ id: '<%= params[:filter_by] %>', value: '<%= params[:filter_value] %>' }] diff --git a/config/locales/common/en.yml b/config/locales/common/en.yml index 392de34377..da2ca8cca6 100644 --- a/config/locales/common/en.yml +++ b/config/locales/common/en.yml @@ -30,6 +30,7 @@ en: other_info: Other Info please_wait: Please wait… preview: Preview + refresh: Refresh save: Save search: Search select_filename: Select Filename diff --git a/config/locales/models/assignments/en.yml b/config/locales/models/assignments/en.yml index c381490b18..ee11514173 100644 --- a/config/locales/models/assignments/en.yml +++ b/config/locales/models/assignments/en.yml @@ -24,6 +24,7 @@ en: non_regenerating_tokens: Tokens do not regenerate only_required_files: Only allow students to submit the required files (this also restricts the file extensions) parent_assignment: Source assignment + release_with_urls: Only allow students to view released marks via a unique URL remark_due_date: Remark Due Date remark_message: Remark Request Instructions repository_folder: Repository Folder diff --git a/config/locales/policies/en.yml b/config/locales/policies/en.yml index 6b4334ca59..b0892e9bf5 100644 --- a/config/locales/policies/en.yml +++ b/config/locales/policies/en.yml @@ -102,6 +102,7 @@ en: review?: You do not have permission to grade this result. run_tests?: You do not have permission to run tests. view?: You do not have permission to view results. + view_with_result_token?: The provided token is invalid or expired. section: manage?: You do not have permission to manage sections. starter_file_group: diff --git a/config/locales/views/results/en.yml b/config/locales/views/results/en.yml index 7fdcff8e83..b25468a3f8 100644 --- a/config/locales/views/results/en.yml +++ b/config/locales/views/results/en.yml @@ -70,10 +70,12 @@ en: set_to_complete: Set to Complete set_to_incomplete: Set to Incomplete submission_info: Submission Info + submit_token: Submit Token subtotal: Subtotal summary: Summary total_extra_marks: Total Extra Marks view_group_repo: View group repository + view_token_submit: Please enter the unique token provided by your instructor to view the results for this assignment. your_mark: Your Mark zoom_in_image: Zoom in zoom_out_image: Zoom out diff --git a/config/locales/views/submissions/en.yml b/config/locales/views/submissions/en.yml index 25b02bd726..e01db90031 100644 --- a/config/locales/views/submissions/en.yml +++ b/config/locales/views/submissions/en.yml @@ -3,6 +3,10 @@ en: submissions: api_submission_disabled: The instructor has disabled submission via the API cannot_display: Cannot display this file. Please download it to view. + clear_all_token_expiry_confirmation: |- + The release token expiry date for all students will cleared. + + Would you like to continue? collect: apply_late_penalty: Apply late penalty/Deduct grace credits (Leaving this box unchecked means the submission will not be penalized, regardless of submission time.) could_not_collect_some_due: Could not collect submissions for some groupings and assignment %{assignment_identifier} - the collection date has not been reached for these groupings. @@ -44,7 +48,22 @@ en: no_files_available: No files available no_url_name: The URL %{url} has not been given a name past_collection_time: Any changes you make to your submitted files will be recorded, but not graded. + refresh_all_tokens: Refresh All Release Tokens + refresh_token_confirmation: |- + New release tokens will be generated for these groups. + + Any old tokens or URLs will be automatically expired and you will have to provide the new tokens or URLs to the students. + + Would you like to continue? release_marks: Release Marks + release_token: Release Token + release_token_expires: Expiry Date + release_token_expires_null: Never + release_token_url: Release URL + release_url_with_token: |- + Students can access released marks only at the following URL: + + %{url} repo_browser: client_time: 'commit:' collected: collected @@ -57,6 +76,7 @@ en: repo_checkout_file: Repository Checkout File (%{type}) repo_list: Repository List run_tests: Run Tests + show_release_tokens: Show Release Tokens state: before_due_date: Before Due Date complete: Complete @@ -92,5 +112,10 @@ en: submission_summary: Submission Report successfully_changed: Successfully updated %{changed} results. unrelease_marks: Unrelease Marks + update_all_token_expiry: Update all expiry dates + update_all_token_expiry_confirmation: |- + The release token expiry date for all students will be updated to %{date}. + + Would you like to continue? url_preview_error: Unable to display a preview for this URL urls_disabled: Unable to submit URL. URL submissions are disabled for this assignment. diff --git a/config/routes.rb b/config/routes.rb index 9f6cb4de8a..86dbb5f7ff 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -153,12 +153,6 @@ get 'collect_and_begin_grading' get 'get_file' end - - resources :results, only: [:edit] do - collection do - patch 'update_remark_request' - end - end end resources :results, only: [:show, :edit] do @@ -186,6 +180,8 @@ get 'stop_test' get 'get_test_runs_instructors' get 'get_test_runs_instructors_released' + get 'view_token_check' + patch 'update_remark_request' end end @@ -360,6 +356,14 @@ get 'uncategorized_annotations' end end + + resources :results, only: [] do + collection do + put 'refresh_view_tokens' + put 'update_view_token_expiry' + get 'download_view_tokens' + end + end end resources :grade_entry_forms, except: [:index, :destroy] do diff --git a/db/migrate/20220922131809_add_release_with_url_to_assignment_properties.rb b/db/migrate/20220922131809_add_release_with_url_to_assignment_properties.rb new file mode 100644 index 0000000000..94d3cb5fe9 --- /dev/null +++ b/db/migrate/20220922131809_add_release_with_url_to_assignment_properties.rb @@ -0,0 +1,17 @@ +class AddReleaseWithUrlToAssignmentProperties < ActiveRecord::Migration[7.0] + def up + add_column :assignment_properties, :release_with_urls, :boolean, default: false, null: false + add_column :results, :view_token, :string + puts '-- Creating view tokens for existing results' + Result.where(view_token: nil).each { |result| result.regenerate_view_token } + change_column_null :results, :view_token, false + add_column :results, :view_token_expiry, :timestamp + add_index :results, :view_token, unique: true + end + def down + remove_column :assignment_properties, :release_with_urls + remove_index :results, :view_token + remove_column :results, :view_token + remove_column :results, :view_token_expiry + end +end diff --git a/db/structure.sql b/db/structure.sql index 145aa9d7bf..8ddd74fe07 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -395,7 +395,8 @@ CREATE TABLE public.assignment_properties ( starter_files_after_due boolean DEFAULT true NOT NULL, url_submit boolean DEFAULT false NOT NULL, autotest_settings json, - api_submit boolean DEFAULT false NOT NULL + api_submit boolean DEFAULT false NOT NULL, + release_with_urls boolean DEFAULT false NOT NULL ); @@ -1537,7 +1538,9 @@ CREATE TABLE public.results ( released_to_students boolean DEFAULT false NOT NULL, total_mark double precision DEFAULT 0.0, remark_request_submitted_at timestamp without time zone, - peer_review_id integer + peer_review_id integer, + view_token character varying NOT NULL, + view_token_expiry timestamp without time zone ); @@ -3531,6 +3534,13 @@ CREATE INDEX index_periods_on_submission_rule_id ON public.periods USING btree ( CREATE INDEX index_results_on_peer_review_id ON public.results USING btree (peer_review_id); +-- +-- Name: index_results_on_view_token; Type: INDEX; Schema: public; Owner: - +-- + +CREATE UNIQUE INDEX index_results_on_view_token ON public.results USING btree (view_token); + + -- -- Name: index_roles_on_course_id; Type: INDEX; Schema: public; Owner: - -- @@ -4577,4 +4587,5 @@ INSERT INTO "schema_migrations" (version) VALUES ('20220629225622'), ('20220707182822'), ('20220726142501'), -('20220726201403'); +('20220726201403'), +('20220922131809'); diff --git a/lib/tasks/marks.rake b/lib/tasks/marks.rake index fee2628812..91e2ec7fa3 100644 --- a/lib/tasks/marks.rake +++ b/lib/tasks/marks.rake @@ -190,7 +190,8 @@ namespace :db do id: result_id, total_mark: total_mark, marking_state: Result::MARKING_STATES[:complete], - released_to_students: true + released_to_students: true, + view_token: Result.generate_unique_secure_token } end Annotation.insert_all annotations diff --git a/spec/controllers/assignments_controller_spec.rb b/spec/controllers/assignments_controller_spec.rb index 4025265261..c4040286b4 100644 --- a/spec/controllers/assignments_controller_spec.rb +++ b/spec/controllers/assignments_controller_spec.rb @@ -642,6 +642,7 @@ it_behaves_like 'create assignment_properties', :allow_remarks, true it_behaves_like 'create assignment_properties', :group_min, 2 it_behaves_like 'create assignment_properties', :group_min, 3 + it_behaves_like 'create assignment_properties', :release_with_urls, true it_behaves_like 'create assignment', :description, 'BBB' it_behaves_like 'create assignment', :message, 'BBB' it_behaves_like 'create assignment', :due_date, 1.hour.ago.to_s diff --git a/spec/controllers/results_controller_spec.rb b/spec/controllers/results_controller_spec.rb index 9ccf31b684..efd46bd7ae 100644 --- a/spec/controllers/results_controller_spec.rb +++ b/spec/controllers/results_controller_spec.rb @@ -39,7 +39,9 @@ def self.test_no_flash def self.test_unauthorized(route_name) it "should not be authorized to access #{route_name}" do - method(ROUTES[route_name]).call(route_name, params: { course_id: course.id, id: incomplete_result.id }) + method(ROUTES[route_name]).call(route_name, params: { course_id: course.id, + id: incomplete_result.id, + assignment_id: assignment.id }) expect(response).to have_http_status(:forbidden) end end @@ -339,7 +341,7 @@ def self.test_unauthorized(route_name) get :view_marks, params: { course_id: course.id, id: incomplete_result.id }, xhr: true end - it { expect(response).to have_http_status(:success) } + it { expect(response).to have_http_status(:forbidden) } end context 'accessing add_extra_mark' do context 'and user can access the action' do @@ -603,7 +605,10 @@ def self.test_unauthorized(route_name) run_tests: :post, stop_test: :get, get_test_runs_instructors: :get, - get_test_runs_instructors_released: :get }.freeze + get_test_runs_instructors_released: :get, + refresh_view_tokens: :put, + update_view_token_expiry: :put, + download_view_tokens: :get }.freeze context 'A student' do before(:each) { sign_in student } @@ -614,7 +619,10 @@ def self.test_unauthorized(route_name) :update_overall_comment, :update_mark, :add_extra_mark, - :remove_extra_mark].each { |route_name| test_unauthorized(route_name) } + :remove_extra_mark, + :refresh_view_tokens, + :update_view_token_expiry, + :download_view_tokens].each { |route_name| test_unauthorized(route_name) } context 'downloading files' do shared_examples 'without permission' do before :each do @@ -662,6 +670,61 @@ def self.test_unauthorized(route_name) end include_examples 'download files' include_examples 'showing json data', true + context '#view_token_check' do + let(:role) { create(:student) } + let(:grouping) { create :grouping_with_inviter_and_submission, inviter: student } + let(:record) { grouping.current_result } + let(:assignment) { record.grouping.assignment } + let(:view_token) { nil } + let(:base_params) { { course_id: record.course.id, id: record.id } } + let(:params) { view_token ? { **base_params, view_token: view_token } : base_params } + subject { get :view_token_check, params: params } + context 'assignment.release_with_urls is false' do + before { assignment.update! release_with_urls: false } + it { is_expected.to have_http_status(:forbidden) } + it 'should not flash an error message' do + subject + expect(flash.now[:error]).to be_blank + end + end + context 'assignment.release_with_urls is true' do + before { assignment.update! release_with_urls: true } + context 'the view token does not match the record token' do + let(:view_token) { "#{record.view_token}abc123" } + it { is_expected.to have_http_status(:unauthorized) } + it 'should flash an error message' do + subject + expect(flash.now[:error]).not_to be_blank + end + end + context 'the view token matches the record token' do + let(:view_token) { record.view_token } + context 'the token does not have an expiry set' do + it { is_expected.to have_http_status(:success) } + it 'should not flash an error message' do + subject + expect(flash.now[:error]).to be_blank + end + end + context 'the record has a token expiry set in the future' do + before { record.update! view_token_expiry: 1.hour.from_now } + it { is_expected.to have_http_status(:success) } + it 'should not flash an error message' do + subject + expect(flash.now[:error]).to be_blank + end + end + context 'the record has a token expiry set in the past' do + before { record.update! view_token_expiry: 1.hour.ago } + it { is_expected.to have_http_status(:forbidden) } + it 'should not flash an error message' do + subject + expect(flash.now[:error]).to be_blank + end + end + end + end + end context 'viewing a file' do context 'for a grouping with no submission' do before :each do @@ -700,21 +763,78 @@ def self.test_unauthorized(route_name) test_assigns_not_nil :submission end context 'and the result is available for viewing' do - before :each do + before do allow_any_instance_of(Submission).to receive(:has_result?).and_return true allow_any_instance_of(Result).to receive(:released_to_students).and_return true - get :view_marks, params: { course_id: course.id, - id: complete_result.id } end - it { expect(response).to have_http_status(:success) } - it { expect(response).to render_template(:view_marks) } - test_assigns_not_nil :assignment - test_assigns_not_nil :grouping - test_assigns_not_nil :submission - test_assigns_not_nil :result - test_assigns_not_nil :annotation_categories - test_assigns_not_nil :group - test_assigns_not_nil :files + subject { get :view_marks, params: { course_id: course.id, id: complete_result.id } } + context 'assignment.release_with_urls is false' do + before { subject } + it { expect(response).to have_http_status(:success) } + it { expect(response).to render_template(:view_marks) } + test_assigns_not_nil :assignment + test_assigns_not_nil :grouping + test_assigns_not_nil :submission + test_assigns_not_nil :result + test_assigns_not_nil :annotation_categories + test_assigns_not_nil :group + test_assigns_not_nil :files + end + context 'assignment.release_with_urls is true' do + before { assignment.update! release_with_urls: true } + let(:view_token) { complete_result.view_token } + let(:session) { {} } + let(:params) { { course_id: course.id, id: complete_result.id, view_token: view_token } } + subject do + get :view_marks, + params: params, + session: session + end + context 'view token has expired' do + before { allow_any_instance_of(Result).to receive(:view_token_expired?).and_return(true) } + it 'should be forbidden when the tokens match' do + subject + expect(response).to have_http_status(:forbidden) + end + end + context 'view token has not expired' do + before { allow_any_instance_of(Result).to receive(:view_token_expired?).and_return(false) } + it 'should succeed when the tokens match' do + subject + expect(response).to have_http_status(:success) + end + context 'when the token does not match' do + let(:view_token) { "#{complete_result.view_token}abc123" } + it 'should be forbidden' do + subject + expect(response).to have_http_status(:forbidden) + end + end + context 'when the token is nil' do + let(:params) { { course_id: course.id, id: complete_result.id } } + it 'should be forbidden' do + subject + expect(response).to have_http_status(:forbidden) + end + context 'but the token is saved in the session for this result' do + context 'when the tokens match' do + let(:session) { { view_token: { complete_result.id.to_s => complete_result.view_token } } } + it 'should succeed' do + subject + expect(response).to have_http_status(:success) + end + end + context 'when the tokens do not match' do + let(:session) { { view_token: { complete_result.id.to_s => "#{complete_result.view_token}abc123" } } } + it 'should be forbidden' do + subject + expect(response).to have_http_status(:forbidden) + end + end + end + end + end + end end end @@ -727,13 +847,14 @@ def self.test_unauthorized(route_name) s.get_original_result.update!(released_to_students: true) s end + let(:result) { submission.get_original_result } context 'when saving a remark request message' do let(:subject) do patch_as student, :update_remark_request, params: { course_id: assignment.course_id, - submission_id: submission.id, + id: result.id, submission: { remark_request: 'Message' }, save: true } end @@ -754,7 +875,7 @@ def self.test_unauthorized(route_name) patch_as student, :update_remark_request, params: { course_id: assignment.course_id, - submission_id: submission.id, + id: result.id, submission: { remark_request: 'Message' }, submit: true } end @@ -794,8 +915,7 @@ def self.test_unauthorized(route_name) delete_as student, :cancel_remark_request, params: { course_id: assignment.course_id, - id: submission.remark_result.id, - submission_id: submission.id } + id: submission.remark_result.id } end before { subject } @@ -841,6 +961,141 @@ def self.test_unauthorized(route_name) expect(incomplete_result.overall_comment).to eq SAMPLE_COMMENT end end + describe '#refresh_view_tokens' do + let(:assignment) { create :assignment_with_criteria_and_results } + let(:results) { assignment.current_results } + let(:ids) { results.ids } + subject do + put :refresh_view_tokens, + params: { course_id: assignment.course.id, assignment_id: assignment.id, result_ids: ids } + end + it { is_expected.to have_http_status(:success) } + it 'should regenerate view tokens for all results' do + view_tokens = results.pluck(:id, :view_token) + subject + new_view_tokens = results.pluck(:id, :view_token) + expect((view_tokens | new_view_tokens).size).to eq 6 + end + it 'should return a json containing the new tokens' do + subject + expect(JSON.parse(response.body)).to eq results.pluck(:id, :view_token).to_h.transform_keys(&:to_s) + end + context 'some result ids are not associated with the assignment' do + let(:extra_result) { create :complete_result } + let(:ids) { results.ids + [extra_result.id] } + it { is_expected.to have_http_status(:success) } + it 'should regenerate view tokens for all results for the assignment' do + view_tokens = results.pluck(:id, :view_token) + subject + new_view_tokens = results.pluck(:id, :view_token) + expect((view_tokens | new_view_tokens).size).to eq 6 + end + it 'should not regenerate view tokens for the extra result' do + old_token = extra_result.view_token + subject + expect(old_token).to eq extra_result.reload.view_token + end + it 'should return a json containing the new tokens for the assignment (not the extra one)' do + subject + expect(JSON.parse(response.body)).to eq results.pluck(:id, :view_token).to_h.transform_keys(&:to_s) + end + end + end + describe '#update_view_token_expiry' do + let(:assignment) { create :assignment_with_criteria_and_results } + let(:results) { assignment.current_results } + let(:ids) { results.ids } + let(:expiry_datetime) { 1.hour.from_now } + before { results.update_all view_token_expiry: 1.day.ago } + subject do + put :update_view_token_expiry, + params: { course_id: assignment.course.id, assignment_id: assignment.id, + result_ids: ids, expiry_datetime: expiry_datetime } + end + it { is_expected.to have_http_status(:success) } + it 'should update the expiry for all results' do + subject + results.pluck(:view_token_expiry).each { |d| expect(d).to be_within(1.second).of(expiry_datetime) } + end + it 'should return a json containing the new dates' do + subject + data = JSON.parse(response.body) + results.pluck(:id, :view_token_expiry).each do |id, date| + expect(Time.zone.parse(data[id.to_s])).to be_within(1.second).of(date) + end + end + context 'when the expiry_datetime is nil' do + let(:expiry_datetime) { nil } + it 'should remove the expiry date' do + subject + expect(results.pluck(:view_token_expiry)).to eq([expiry_datetime] * results.count) + end + end + context 'some result ids are not associated with the assignment' do + let(:extra_result) { create :complete_result } + let(:ids) { results.ids + [extra_result.id] } + it { is_expected.to have_http_status(:success) } + it 'should set the expiry date for all results for the assignment' do + subject + results.pluck(:view_token_expiry).each { |d| expect(d).to be_within(1.second).of(expiry_datetime) } + end + it 'should not set the expiry date for the extra result' do + old_date = extra_result.view_token_expiry + subject + expect(old_date).to eq extra_result.reload.view_token_expiry + end + it 'should return a json containing the new tokens for the assignment (not the extra one)' do + subject + data = JSON.parse(response.body) + results.pluck(:id, :view_token_expiry).each do |id, date| + expect(Time.zone.parse(data[id.to_s])).to be_within(1.second).of(date) + end + end + end + end + describe '#download_view_tokens' do + let(:assignment) { create :assignment_with_criteria_and_results } + let(:results) { assignment.current_results } + let(:ids) { results.ids } + before { results.update_all view_token_expiry: 1.day.ago } + subject do + put :download_view_tokens, + params: { course_id: assignment.course.id, assignment_id: assignment.id, result_ids: ids } + end + it { is_expected.to have_http_status(:success) } + it 'should return a csv with a header and a row for all results' do + expect(subject.body.split("\n").count).to eq(1 + results.count) + end + shared_examples 'csv contains the right stuff' do + it 'should return the correct info for all results' do + data = subject.body.lines + data.shift # skip header + data.each do |row| + group_name, user_name, first_name, last_name, email, id_number, + view_token, view_token_expiry, url = row.chomp.split(',') + result = results.find_by(view_token: view_token) + expect(group_name).to eq result.grouping.group.group_name + expect(url).to eq view_marks_course_result_url(result.course.id, result.id, view_token: view_token) + expect(Time.zone.parse(view_token_expiry)).to be_within(1.second).of(result.view_token_expiry) + user_info = result.grouping.accepted_student_memberships.joins(role: :user).pluck('users.user_name', + 'users.first_name', + 'users.last_name', + 'users.email', + 'users.id_number') + user_info = user_info.map { |info| info.map { |a| a || '' } } + expect(user_info).to include([user_name, first_name, last_name, email, id_number]) + end + end + end + it_behaves_like 'csv contains the right stuff' + context 'some result ids are not associated with the assignment' do + let(:extra_result) { create :complete_result } + let(:ids) { results.ids + [extra_result.id] } + it { is_expected.to have_http_status(:success) } + it_behaves_like 'csv contains the right stuff' + end + end + describe '#delete_grace_period_deduction' do it 'deletes an existing grace period deduction' do expect(grouping.grace_period_deductions.exists?).to be false diff --git a/spec/models/assignment_spec.rb b/spec/models/assignment_spec.rb index c6c451d349..f95e949879 100644 --- a/spec/models/assignment_spec.rb +++ b/spec/models/assignment_spec.rb @@ -1867,6 +1867,27 @@ def grouping_count(groupings) expect(data.select { |h| h.key? :final_grade }.count).to eq 1 end + context 'release_with_urls is true' do + before { assignment.update! release_with_urls: true } + it 'should include the view_token if the result exists' do + token = submission.current_result.view_token + expect(data.pluck(:result_view_token)).to include(token) + end + it 'should include the view_token_expiry if the result exists' do + expiry = submission.current_result.view_token_expiry + expect(data.pluck(:result_view_token_expiry)).to include(expiry) + end + end + context 'release_with_urls is false' do + before { assignment.update! release_with_urls: true } + it 'should not include the view_token if the result exists' do + expect(data.pluck(:result_view_token).compact).to be_empty + end + it 'should include the view_token_expiry if the result exists' do + expect(data.pluck(:result_view_token_expiry).compact).to be_empty + end + end + context 'there is an extra mark' do let(:result) { create :complete_result, submission: submission } let!(:extra_mark) { create :extra_mark_points, result: result } diff --git a/spec/models/result_spec.rb b/spec/models/result_spec.rb index 87e25ae448..7263d41c85 100644 --- a/spec/models/result_spec.rb +++ b/spec/models/result_spec.rb @@ -166,6 +166,25 @@ end end + describe '#view_token_expired?' do + let(:result) { create :complete_result, view_token_expiry: expiry } + subject { result.view_token_expired? } + context 'no view token expiry exists for the result' do + let(:expiry) { nil } + it { is_expected.to be_falsy } + end + context 'a view token expiry exists' do + context 'and it is in the future' do + let(:expiry) { 1.day.from_now } + it { is_expected.to be_falsy } + end + context 'and it is in the past' do + let(:expiry) { 1.day.ago } + it { is_expected.to be_truthy } + end + end + end + describe '.set_release_on_results' do let(:assignment) { create(:assignment_with_criteria_and_results) } it 'should raise StandardError with message not_complete error if result has not been completed' do diff --git a/spec/models/submission_spec.rb b/spec/models/submission_spec.rb index d41285c18d..6e417aaa24 100644 --- a/spec/models/submission_spec.rb +++ b/spec/models/submission_spec.rb @@ -116,4 +116,36 @@ expect(s.remark_result.marking_state).to eq Result::MARKING_STATES[:incomplete] end end + + describe '#get_visible_result' do + let(:submission) { create :submission } + let(:released_result) { create :released_result, submission: submission } + let(:remark_result) { create :remark_result, submission: submission } + let(:released_remark_result) { create :remark_result, submission: submission, released_to_students: true } + context 'when the remark result is released' do + before do + released_remark_result + end + it 'should return the remark result' do + expect(submission.get_visible_result).to eq released_remark_result + end + end + context 'when the original result is released' do + before do + released_result + remark_result + end + it 'should return the original result' do + expect(submission.get_visible_result).to eq released_result + end + end + context 'when the original result is not released' do + before do + remark_result + end + it 'should return the original result' do + expect(submission.get_visible_result).to eq submission.get_original_result + end + end + end end diff --git a/spec/policies/result_policy_spec.rb b/spec/policies/result_policy_spec.rb index 9e6da27729..6708998154 100644 --- a/spec/policies/result_policy_spec.rb +++ b/spec/policies/result_policy_spec.rb @@ -24,11 +24,70 @@ let(:role) { create :student } let(:record) { create :complete_result } end - succeed 'role is a student who is part of the grouping' do + describe 'role is a student who is part of the grouping' do let(:role) { create :student } let(:grouping) { create :grouping_with_inviter_and_submission, inviter: role } let(:record) { grouping.current_result } let(:assignment) { record.grouping.assignment } + succeed 'assignment.release_with_urls is false' do + before { assignment.update! release_with_urls: false } + end + context 'assignment.release_with_urls is true' do + before { assignment.update! release_with_urls: true } + let(:context) { { role: role, real_user: role.user, view_token: view_token } } + failed 'the view token does not match the record token' do + let(:view_token) { "#{record.view_token}abc123" } + end + context 'the view token matches the record token' do + let(:view_token) { record.view_token } + succeed 'the token does not have an expiry set' + succeed 'the record has a token expiry set in the future' do + before { record.update! view_token_expiry: 1.hour.from_now } + end + failed 'the record has a token expiry set in the past' do + before { record.update! view_token_expiry: 1.hour.ago } + end + end + end + end + end + + describe_rule :view_marks? do + let(:record) { create :complete_result } + failed 'role is an instructor' do + let(:role) { create(:instructor) } + end + failed 'role is a ta' do + let(:role) { create(:ta) } + end + failed 'role is a student who is not part of the grouping' do + let(:role) { create :student } + end + describe 'role is a student who is part of the grouping' do + let(:role) { create :student } + let(:grouping) { create :grouping_with_inviter_and_submission, inviter: role } + let(:record) { grouping.current_result } + let(:assignment) { record.grouping.assignment } + succeed 'assignment.release_with_urls is false' do + before { assignment.update! release_with_urls: false } + end + context 'assignment.release_with_urls is true' do + before { assignment.update! release_with_urls: true } + let(:context) { { role: role, real_user: role.user, view_token: view_token } } + failed 'the view token does not match the record token' do + let(:view_token) { "#{record.view_token}abc123" } + end + context 'the view token matches the record token' do + let(:view_token) { record.view_token } + succeed 'the token does not have an expiry set' + succeed 'the record has a token expiry set in the future' do + before { record.update! view_token_expiry: 1.hour.from_now } + end + failed 'the record has a token expiry set in the past' do + before { record.update! view_token_expiry: 1.hour.ago } + end + end + end end end @@ -278,4 +337,35 @@ end end end + + describe_rule :view_token_check? do + let(:record) { create :complete_result } + failed 'role is an instructor' do + let(:role) { create :instructor } + end + failed 'role is a ta' do + let(:role) { create :ta } + end + failed 'role is a student who is not part of the grouping' do + let(:role) { create :student } + end + describe 'role is a student who is part of the grouping' do + let(:role) { create :student } + let(:grouping) { create :grouping_with_inviter_and_submission, inviter: role } + let(:record) { grouping.current_result } + let(:assignment) { record.grouping.assignment } + failed 'assignment.release_with_urls is false' do + before { assignment.update! release_with_urls: false } + end + context 'assignment.release_with_urls is true' do + before { assignment.update! release_with_urls: true } + failed 'view_token is expired' do + before { record.update! view_token_expiry: 1.minute.ago } + end + succeed 'view_token is not expired' do + before { record.update! view_token_expiry: 1.minute.from_now } + end + end + end + end end From 8f442c9a25b7b99348305bf7e58469745b0968ea Mon Sep 17 00:00:00 2001 From: mishaschwartz Date: Wed, 30 Nov 2022 16:19:06 -0500 Subject: [PATCH 2/3] version: bump to v2.1.7 --- Changelog.md | 4 +++- app/MARKUS_VERSION | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Changelog.md b/Changelog.md index a80aa1e8bf..624f7135a0 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,8 @@ # Changelog +## [v2.1.7] +- Allow results to be made available only through unique tokens (#6244) + ## [v2.1.6] - Fix bug where TAs were able to access urls for results they haven't been assigned to (#6321) - Fix bug where the marking state was left as "complete" after a new criterion is created (#6303) @@ -14,7 +17,6 @@ - Fix bug where gzipped binary feedback files were not unzipped correctly (#6283) - Fix bug where remark request summary table wasn't being rendered correctly (#6284) - Fix bug where test results were being associated with the wrong test runs (#6287) -- Allow results to be made available only through unique tokens (#6244) ## [v2.1.4] - Fix bug where git hooks are not run server side when symlinked (#6276/#6277) diff --git a/app/MARKUS_VERSION b/app/MARKUS_VERSION index 0195b80f53..b57a32026a 100644 --- a/app/MARKUS_VERSION +++ b/app/MARKUS_VERSION @@ -1 +1 @@ -VERSION=v2.1.6,PATCH_LEVEL=DEV +VERSION=v2.1.7,PATCH_LEVEL=DEV From ca4b82cc8ccd98fe872548e800f101e3dad60c65 Mon Sep 17 00:00:00 2001 From: Dhairya Khara <50108381+Dhairya-Khara@users.noreply.github.com> Date: Mon, 29 Aug 2022 08:03:12 -0400 Subject: [PATCH 3/3] forms: Switch from jquery-ui-timepicker-addon to flatpickr for datetime inputs (#6158) --- Changelog.md | 1 + app/assets/config/manifest.js | 1 + .../Components/autotest_manager.jsx | 21 +- .../javascripts/Components/date_picker.jsx | 92 - app/assets/javascripts/flatpickr_config.js | 24 + app/assets/stylesheets/application.css | 1 + app/controllers/assignments_controller.rb | 6 - app/controllers/automated_tests_controller.rb | 5 +- app/javascript/application_webpack.js | 5 + app/views/assignments/_boot.js.erb | 51 +- app/views/assignments/_form.html.erb | 20 +- .../assignments/_peer_review_form.html.erb | 8 +- app/views/automated_tests/_form.html.erb | 4 +- app/views/grade_entry_forms/_boot.js.erb | 10 +- app/views/grade_entry_forms/_form.html.erb | 3 +- app/views/groups/index.html.erb | 4 +- config/i18n-tasks.yml | 2 + .../initializers/content_security_policy.rb | 1 - config/locales/defaults/datetime/en.yml | 11 +- package.json | 2 + .../automated_tests_controller_spec.rb | 2 +- .../javascripts/jquery-ui-timepicker-addon.js | 2203 --- yarn.lock | 13624 +++++++--------- 23 files changed, 5757 insertions(+), 10344 deletions(-) delete mode 100644 app/assets/javascripts/Components/date_picker.jsx create mode 100644 app/assets/javascripts/flatpickr_config.js delete mode 100644 vendor/assets/javascripts/jquery-ui-timepicker-addon.js diff --git a/Changelog.md b/Changelog.md index 624f7135a0..c64555c409 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,6 +1,7 @@ # Changelog ## [v2.1.7] +- Switch from jquery-ui-timepicker-addon to flatpickr for datetime inputs (#6158) - Allow results to be made available only through unique tokens (#6244) ## [v2.1.6] diff --git a/app/assets/config/manifest.js b/app/assets/config/manifest.js index eb45bc4aad..14b1f52230 100644 --- a/app/assets/config/manifest.js +++ b/app/assets/config/manifest.js @@ -10,3 +10,4 @@ // //= link_tree ../images //= link_tree ../builds +//= link flatpickr/dist/flatpickr.css diff --git a/app/assets/javascripts/Components/autotest_manager.jsx b/app/assets/javascripts/Components/autotest_manager.jsx index 653ced5150..e543314a22 100644 --- a/app/assets/javascripts/Components/autotest_manager.jsx +++ b/app/assets/javascripts/Components/autotest_manager.jsx @@ -2,7 +2,8 @@ import React from "react"; import {render} from "react-dom"; import FileManager from "./markus_file_manager"; import Form from "@rjsf/core"; -import Datepicker from "./date_picker"; +import Flatpickr from "react-flatpickr"; +import labelPlugin from "flatpickr/dist/plugins/labelPlugin/labelPlugin"; import FileUploadModal from "./Modals/file_upload_modal"; import AutotestSpecsUploadModal from "./Modals/autotest_specs_upload_modal"; @@ -169,8 +170,9 @@ class AutotestManager extends React.Component { ); }; - handleTokenStartDateChange = date => { - this.setState({token_start_date: date}, () => this.toggleFormChanged(true)); + handleTokenStartDateChange = selectedDates => { + const newDate = selectedDates[0] || ""; + this.setState({token_start_date: newDate}, () => this.toggleFormChanged(true)); }; handleTokensPerPeriodChange = event => { @@ -298,12 +300,17 @@ class AutotestManager extends React.Component { -