diff --git a/app/controllers/crocodoc_sessions_controller.rb b/app/controllers/crocodoc_sessions_controller.rb index 085f09b7035..a37329cf155 100644 --- a/app/controllers/crocodoc_sessions_controller.rb +++ b/app/controllers/crocodoc_sessions_controller.rb @@ -33,7 +33,8 @@ class CrocodocSessionsController < ApplicationController crocodoc = attachment.crocodoc_document url = crocodoc.session_url(:user => @current_user, - :annotations => annotations) + :annotations => annotations, + :crocodoc_ids => blob["crocodoc_ids"]) redirect_to url else render :text => "Not found", :status => :not_found diff --git a/app/controllers/gradebooks_controller.rb b/app/controllers/gradebooks_controller.rb index 6f9452a418e..b693d7141a7 100644 --- a/app/controllers/gradebooks_controller.rb +++ b/app/controllers/gradebooks_controller.rb @@ -440,6 +440,7 @@ class GradebooksController < ApplicationController 'SpeedGrader is enabled only for published content.') return redirect_to polymorphic_url([@context, @assignment]) end + grading_role = if moderated_grading_enabled_and_no_grades_published if @context.grants_right?(@current_user, :moderate_grades) :moderator @@ -450,6 +451,9 @@ class GradebooksController < ApplicationController :grader end + # TODO: Handle for moderator when behavior implemented + crocodoc_ids = [:provisional_grader].include?(grading_role) && [@current_user.crocodoc_id!] + respond_to do |format| format.html do @headers = false @@ -474,7 +478,8 @@ class GradebooksController < ApplicationController format.json do render :json => @assignment.speed_grader_json(@current_user, avatars: service_enabled?(:avatars), - grading_role: grading_role) + grading_role: grading_role, + crocodoc_ids: crocodoc_ids) end end end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 0efda644dd1..bd28d210d29 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -112,7 +112,7 @@ class SubmissionsController < ApplicationController id = @current_user.try(:id) end @user = @context.all_students.find(params[:id]) rescue nil - if !@user + unless @user flash[:error] = t('errors.student_not_enrolled', "The specified user is not a student in this course") respond_to do |format| format.html { redirect_to named_context_url(@context, :context_assignment_url, @assignment.id) } @@ -127,6 +127,11 @@ class SubmissionsController < ApplicationController @submission = @assignment.submissions.where(user_id: @user).first @submission ||= @assignment.submissions.build(:user => @user) + if @context.feature_enabled?(:moderated_grading) && @assignment.moderated_grading? + student = User.where(id: @submission.user_id).first + grader = User.where(id: @submission.grader_id).first + @crocodoc_ids = [student, grader].compact.map(&:crocodoc_id!) + end @rubric_association = @assignment.rubric_association @rubric_association.assessing_user_id = @submission.user_id if @rubric_association # can't just check the permission, because peer reviewiers can never read the grade @@ -194,7 +199,7 @@ class SubmissionsController < ApplicationController @submission.limit_comments(@current_user, session) format.html end - if !json_handled + unless json_handled format.json { @submission.limit_comments(@current_user, session) excludes = @assignment.grants_right?(@current_user, session, :grade) ? [:grade, :score] : [] diff --git a/app/helpers/attachment_helper.rb b/app/helpers/attachment_helper.rb index 619060f8165..56aded83f86 100644 --- a/app/helpers/attachment_helper.rb +++ b/app/helpers/attachment_helper.rb @@ -21,7 +21,7 @@ module AttachmentHelper def doc_preview_attributes(attachment, attrs={}) if attachment.crocodoc_available? begin - attrs[:crocodoc_session_url] = attachment.crocodoc_url(@current_user) + attrs[:crocodoc_session_url] = attachment.crocodoc_url(@current_user, attrs[:crocodoc_ids]) rescue => e Canvas::Errors.capture_exception(:crocodoc, e) end diff --git a/app/models/assignment.rb b/app/models/assignment.rb index e5ef06bd2be..2ad2af227e7 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -1305,7 +1305,7 @@ class Assignment < ActiveRecord::Base !moderated_grading? || grades_published_at.present? end - def speed_grader_json(user, avatars: false, grading_role: :grader) + def speed_grader_json(user, avatars: false, grading_role: :grader, crocodoc_ids: nil) Attachment.skip_thumbnails = true submission_fields = [:user_id, :id, :submitted_at, :workflow_state, :grade, :grade_matches_current_submission, @@ -1408,7 +1408,7 @@ class Assignment < ActiveRecord::Base :methods => [:view_inline_ping_url] ).tap { |json| json[:attachment][:canvadoc_url] = a.canvadoc_url(user) - json[:attachment][:crocodoc_url] = a.crocodoc_url(user) + json[:attachment][:crocodoc_url] = a.crocodoc_url(user, crocodoc_ids) json[:attachment][:submitted_to_crocodoc] = a.crocodoc_document.present? } end diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 262a7747fd2..6528a4c230a 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -1441,20 +1441,21 @@ class Attachment < ActiveRecord::Base "/api/v1/canvadoc_session?#{preview_params(user, "canvadoc")}" end - def crocodoc_url(user) + def crocodoc_url(user, crocodoc_ids = nil) return unless crocodoc_available? - "/api/v1/crocodoc_session?#{preview_params(user, "crocodoc")}" + "/api/v1/crocodoc_session?#{preview_params(user, "crocodoc", crocodoc_ids)}" end def previewable_media? self.content_type && self.content_type.match(/\A(video|audio)/) end - def preview_params(user, type) + def preview_params(user, type, crocodoc_ids = nil) blob = { user_id: user.try(:global_id), attachment_id: id, type: type, + crocodoc_ids: crocodoc_ids }.to_json hmac = Canvas::Security.hmac_sha1(blob) "blob=#{URI.encode blob}&hmac=#{URI.encode hmac}" diff --git a/app/models/crocodoc_document.rb b/app/models/crocodoc_document.rb index aeab693def6..470da925b53 100644 --- a/app/models/crocodoc_document.rb +++ b/app/models/crocodoc_document.rb @@ -66,7 +66,7 @@ class CrocodocDocument < ActiveRecord::Base opts[:user] = user.crocodoc_user end - opts.merge! permissions_for_user(user) + opts.merge! permissions_for_user(user, opts[:crocodoc_ids]) unless annotations_on opts[:filter] = 'none' @@ -80,7 +80,7 @@ class CrocodocDocument < ActiveRecord::Base end end - def permissions_for_user(user) + def permissions_for_user(user, whitelist = nil) opts = { :filter => 'none', :admin => false, @@ -114,6 +114,18 @@ class CrocodocDocument < ActiveRecord::Base opts[:filter] = 'none' end + if whitelist + opts[:filter] = case opts[:filter] + when "all" + whitelist.join(",") + when "none" + "none" + else + ids = whitelist.map(&:to_i) & [opts[:filter].to_i] + ids.any? && ids.join(",") || "none" + end + end + opts end diff --git a/app/models/user.rb b/app/models/user.rb index 5cc5d75ee9a..e82dfd48a4f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2544,7 +2544,7 @@ class User < ActiveRecord::Base return cid if cid Setting.transaction do - s = Setting.lock.where(name: 'crocodoc_counter').first + s = Setting.lock.where(name: 'crocodoc_counter').first_or_create(value: 0) cid = s.value = s.value.to_i + 1 s.save! end diff --git a/app/views/submissions/show_preview.html.erb b/app/views/submissions/show_preview.html.erb index 0766f9435f6..cd2b121e100 100644 --- a/app/views/submissions/show_preview.html.erb +++ b/app/views/submissions/show_preview.html.erb @@ -125,7 +125,7 @@ %> <% end %> <% if attachment.crocodoc_available? || attachment.canvadocable? %> - data-tooltip title="<%= preview_document %>" class="modal_preview_link Button--link" role="button" data-attachment_id="<%= attachment.id %>" data-submission_id="<%= @submission.id %>" data-dialog-title="<%= attachment.display_name %>" <%= doc_preview_attributes(attachment) %>><%= button_text %> + data-tooltip title="<%= preview_document %>" class="modal_preview_link Button--link" role="button" data-attachment_id="<%= attachment.id %>" data-submission_id="<%= @submission.id %>" data-dialog-title="<%= attachment.display_name %>" <%= doc_preview_attributes(attachment, crocodoc_ids: @crocodoc_ids) %>><%= button_text %> <% end %> diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb index 9696fd911a5..f6c86185224 100644 --- a/spec/factories/user_factory.rb +++ b/spec/factories/user_factory.rb @@ -86,6 +86,11 @@ def student_in_course(opts={}) course_with_student(opts) end +def ta_in_course(opts={}) + opts[:course] = @course if @course && !opts[:course] + course_with_ta(opts) +end + def student_in_section(section, opts={}) student = opts.fetch(:user) { user } enrollment = section.course.enroll_user(student, 'StudentEnrollment', :section => section, :force_update => true) diff --git a/spec/models/attachment_spec.rb b/spec/models/attachment_spec.rb index 08ca27852ef..ecbfabd218d 100644 --- a/spec/models/attachment_spec.rb +++ b/spec/models/attachment_spec.rb @@ -70,6 +70,13 @@ describe Attachment do end context "crocodoc" do + include HmacHelper + let_once(:user) { user_model } + let_once(:course) { course_model } + let_once(:student) do + course.enroll_student(user_model).accept + @user + end before { configure_crocodoc } it "crocodocable?" do @@ -77,6 +84,20 @@ describe Attachment do expect(@attachment).to be_crocodocable end + it "should include a whitelist of crocodoc_ids in the url blob" do + crocodocable_attachment_model + crocodoc_ids = [user.crocodoc_id!, student.crocodoc_id!] + @attachment.submit_to_crocodoc + + url = Rack::Utils.parse_nested_query(@attachment.crocodoc_url(user, crocodoc_ids).sub(/^.*\?{1}/, "")) + blob = extract_blob(url["hmac"], url["blob"], + "user_id" => user.id, + "type" => "crocodoc") + + expect(blob["crocodoc_ids"]).to be_present + expect(blob["crocodoc_ids"]).to eq(crocodoc_ids & blob["crocodoc_ids"]) + end + it "should submit to crocodoc" do crocodocable_attachment_model expect(@attachment.crocodoc_available?).to be_falsey diff --git a/spec/models/crocodoc_document_spec.rb b/spec/models/crocodoc_document_spec.rb index e53afe298bb..6de133c5b61 100644 --- a/spec/models/crocodoc_document_spec.rb +++ b/spec/models/crocodoc_document_spec.rb @@ -33,6 +33,7 @@ describe 'CrocodocDocument' do before :once do teacher_in_course(:active_all => true) student_in_course + ta_in_course @submitter = @student student_in_course @other_student = @student @@ -54,6 +55,14 @@ describe 'CrocodocDocument' do }) end + it "should only include ids specified in the whitelist" do + expect(@crocodoc.permissions_for_user(@teacher, [@teacher.crocodoc_id!, @submitter.crocodoc_id!])).to eq({ + :filter => "#{@teacher.crocodoc_id!},#{@submitter.crocodoc_id!}", + :admin => true, + :editable => true, + }) + end + context "submitter permissions" do it "should see everything (unless the assignment is muted)" do expect(@crocodoc.permissions_for_user(@submitter)).to eq({