ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

cache 'false' values in adheres_to_policy 

should improve performance in requests when permissions
 are checked for users that don't have them

(e.g. student grade summary page)

refs #CNVS-21317

Change-Id: I349a02664bcc81da1808e76bbc023f5d743f90a4
Reviewed-on: https://gerrit.instructure.com/56846
Tested-by: Jenkins
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
Product-Review: James Williams  <jamesw@instructure.com>
QA-Review: James Williams  <jamesw@instructure.com>
This commit is contained in:
James Williams 2015-06-19 08:33:47 -06:00
parent cde03f7f41
commit 7f3eb3b828
11 changed files with 63 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/application_controller.rb
View File

@ -514,7 +514,7 @@ class ApplicationController < ActionController::Base
end
if @context && @context.respond_to?(:short_name)
crumb_url = named_context_url(@context, :context_url) if @context.grants_right?(@current_user, :read)
crumb_url = named_context_url(@context, :context_url) if @context.grants_right?(@current_user, session, :read)
add_crumb(@context.short_name, crumb_url)
end

3
gems/adheres_to_policy/lib/adheres_to_policy/cache.rb
View File

@ -37,7 +37,8 @@ module AdheresToPolicy
def self.fetch(key)
return yield unless key
unless value = self.read(key)
value = self.read(key)
if value.nil?
if block_given?
value = yield
self.write(key, value)

7
gems/adheres_to_policy/spec/adheres_to_policy/cache_spec.rb
View File

@ -36,6 +36,13 @@ describe AdheresToPolicy::Cache do
value = AdheresToPolicy::Cache.fetch(:key){ 'value' }
expect(value).to eq 'value'
end
it "does not write the key if the value is 'false'" do
AdheresToPolicy::Cache.write(:key, false)
expect(AdheresToPolicy::Cache).to_not receive(:write)
value = AdheresToPolicy::Cache.fetch(:key){ 'new_value' }
expect(value).to eq false
end
end
context "#write" do

1
spec/apis/v1/discussion_topics_api_spec.rb
View File

@ -68,6 +68,7 @@ describe Api::V1::DiscussionTopics do
expect(data[:permissions][:attach]).to eq false # students can't attach by default
@topic.context.update_attribute(:allow_student_forum_attachments, true)
AdheresToPolicy::Cache.clear
data = @test_api.discussion_topic_api_json(@topic, @topic.context, @student, nil)
expect(data[:permissions][:attach]).to eq true
end

1
spec/integration/enrollment_date_restrictions_spec.rb
View File

@ -126,6 +126,7 @@ describe "enrollment_date_restrictions" do
expect(html.css('.course').length).to eq 2
Account.default.account_users.create!(user: @user)
@user.reload
get "/users/#{@user.id}"
expect(response.body).to match /Inactive/
expect(response.body).to match /Completed/

2
spec/models/account_spec.rb
View File

@ -473,6 +473,7 @@ describe Account do
v[:account] = Account.find(account)
end
RoleOverride.clear_cached_contexts
AdheresToPolicy::Cache.clear
hash.each do |k, v|
account = v[:account]
expect(account.check_policy(hash[:site_admin][:admin])).to match_array full_access + (k == :site_admin ? [:read_global_outcomes] : [])
@ -826,6 +827,7 @@ describe Account do
expect(Account.default.grants_right?(@user, :read_sis)).to be_falsey
@course = Account.default.courses.create!
@course.enroll_teacher(@user).accept!
AdheresToPolicy::Cache.clear
expect(Account.default.grants_right?(@user, :read_sis)).to be_truthy
end

34
spec/models/attachment_spec.rb
View File

@ -589,31 +589,47 @@ describe Attachment do
a = attachment
expect(a.grants_right?(nil, :read)).to eql(false)
expect(a.grants_right?(nil, :download)).to eql(false)
expect(a.grants_right?(nil, {'file_access_user_id' => student.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :read)).to eql(true)
expect(a.grants_right?(nil, {'file_access_user_id' => student.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :download)).to eql(true)
mock_session = {
'file_access_user_id' => student.id,
'file_access_expiration' => 1.hour.from_now.to_i,
'permissions_key' => SecureRandom.uuid
}.with_indifferent_access
expect(a.grants_right?(nil, mock_session, :read)).to eql(true)
expect(a.grants_right?(nil, mock_session, :download)).to eql(true)
end
it "should correctly deny user access based on 'file_access_user_id'" do
a = attachment_model(context: user)
other_user = user_model
expect(a.grants_right?(nil, {'file_access_user_id' => other_user.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :read)).to eql(false)
expect(a.grants_right?(nil, {'file_access_user_id' => other_user.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :download)).to eql(false)
mock_session = {
'file_access_user_id' => other_user.id,
'file_access_expiration' => 1.hour.from_now.to_i,
'permissions_key' => SecureRandom.uuid
}.with_indifferent_access
expect(a.grants_right?(nil, mock_session, :read)).to eql(false)
expect(a.grants_right?(nil, mock_session, :download)).to eql(false)
end
it "should allow user access to anyone if the course is public to auth users (with 'file_access_user_id' and 'file_access_expiration' in the session)" do
a = attachment_model(context: course)
mock_session = {
'file_access_user_id' => user.id,
'file_access_expiration' => 1.hour.from_now.to_i,
'permissions_key' => SecureRandom.uuid
}.with_indifferent_access
expect(a.grants_right?(nil, {'file_access_user_id' => user.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :read)).to eql(false)
expect(a.grants_right?(nil, {'file_access_user_id' => user.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :download)).to eql(false)
a = attachment_model(context: course)
expect(a.grants_right?(nil, mock_session, :read)).to eql(false)
expect(a.grants_right?(nil, mock_session, :download)).to eql(false)
course.is_public_to_auth_users = true
course.save!
a.reload
AdheresToPolicy::Cache.clear
expect(a.grants_right?(nil, :read)).to eql(false)
expect(a.grants_right?(nil, :download)).to eql(false)
expect(a.grants_right?(nil, {'file_access_user_id' => user.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :read)).to eql(true)
expect(a.grants_right?(nil, {'file_access_user_id' => user.id, 'file_access_expiration' => 1.hour.from_now.to_i}, :download)).to eql(true)
expect(a.grants_right?(nil, mock_session, :read)).to eql(true)
expect(a.grants_right?(nil, mock_session, :download)).to eql(true)
end
it "should not allow user access based on incorrect 'file_access_user_id' in the session" do

12
spec/models/attachments/verification_spec.rb
View File

@ -136,8 +136,10 @@ describe Attachments::Verification do
token = v2.verifier_for_user(other_user, context: eportfolio.asset_string, permission_map_id: :r_rd)
expect(v2.valid_verifier_for_permission?(token, :read)).to eq(false)
expect(v2.valid_verifier_for_permission?(token, :download)).to eq(false)
expect(v2.valid_verifier_for_permission?(token, :read, {eportfolio_ids: [eportfolio.id]})).to eq(true)
expect(v2.valid_verifier_for_permission?(token, :download, {eportfolio_ids: [eportfolio.id]})).to eq(true)
mock_session = {eportfolio_ids: [eportfolio.id], permissions_key: SecureRandom.uuid}
expect(v2.valid_verifier_for_permission?(token, :read, mock_session)).to eq(true)
expect(v2.valid_verifier_for_permission?(token, :download, mock_session)).to eq(true)
end
it "should support custom permissions checks on nil (public) user" do
@ -145,10 +147,12 @@ describe Attachments::Verification do
eportfolio = student.eportfolios.create! public: false
v2 = Attachments::Verification.new(att2)
token = v2.verifier_for_user(nil, context: eportfolio.asset_string, permission_map_id: :r_rd)
expect(v2.valid_verifier_for_permission?(token, :read)).to eq(false)
expect(v2.valid_verifier_for_permission?(token, :download)).to eq(false)
expect(v2.valid_verifier_for_permission?(token, :read, {eportfolio_ids: [eportfolio.id]})).to eq(true)
expect(v2.valid_verifier_for_permission?(token, :download, {eportfolio_ids: [eportfolio.id]})).to eq(true)
mock_session = {eportfolio_ids: [eportfolio.id], permissions_key: SecureRandom.uuid}
expect(v2.valid_verifier_for_permission?(token, :read, mock_session)).to eq(true)
expect(v2.valid_verifier_for_permission?(token, :download, mock_session)).to eq(true)
end
end
end

22
spec/models/calendar_event_spec.rb
View File

@ -203,7 +203,7 @@ describe CalendarEvent do
HTML
ev = @event.to_ics(false)
expect(ev.description).to match_ignoring_whitespace("This assignment is due December 16th. Please do the reading.
[link!](www.example.com)")
expect(ev.x_alt_desc).to eq @event.description
@ -219,6 +219,7 @@ describe CalendarEvent do
@attachment.file_state = 'public'
@attachment.save!
AdheresToPolicy::Cache.clear
ev = @event.to_ics(false)
expect(ev.description).to include("verifier")
@ -228,6 +229,7 @@ describe CalendarEvent do
@course.is_public = true
@course.save!
AdheresToPolicy::Cache.clear
ev = @event.to_ics(false)
expect(ev.description).to include("verifier")
end
@ -827,7 +829,7 @@ describe CalendarEvent do
expect(child.start_at).to be_nil
expect(child.title).to eql @event.title
end
it "should update cascaded attributes on the child events whenever the parent is updated" do
calendar_event_model(:start_at => "Sep 3 2008", :title => "some event")
child = @event.child_events.build
@ -835,14 +837,14 @@ describe CalendarEvent do
child.save!
child.reload
orig_start_at = child.start_at
@event.title = 'asdf'
@event.start_at = Time.now.utc
@event.save!
expect(child.reload.title).to eql 'asdf'
expect(child.start_at).to eql orig_start_at
end
it "should disregard attempted changes to cascaded attributes" do
calendar_event_model(:start_at => "Sep 3 2008", :title => "some event")
child = @event.child_events.build
@ -869,31 +871,31 @@ describe CalendarEvent do
expect(child.start_at).to eql @event.start_at
expect(child.title).to eql @event.title
end
it "should update locked child events whenever the parent is updated" do
calendar_event_model(:start_at => "Sep 3 2008", :title => "some event")
child = @event.child_events.build
child.context = user
child.workflow_state = :locked
child.save!
@event.title = 'asdf'
@event.save!
expect(child.reload.title).to eql 'asdf'
end
it "should disregard attempted changes to locked attributes" do
calendar_event_model(:start_at => "Sep 3 2008", :title => "some event")
child = @event.child_events.build
child.context = user
child.workflow_state = :locked
child.save!
child.title = 'asdf'
child.save!
expect(child.title).to eql 'some event'
end
it "should unlock events when the last child is deleted" do
calendar_event_model(:start_at => "Sep 3 2008", :title => "some event")
@event.workflow_state = :locked
@ -902,7 +904,7 @@ describe CalendarEvent do
child.context = user
child.workflow_state = :locked
child.save!
child.destroy
expect(@event.reload).to be_active
expect(child.reload).to be_deleted

1
spec/models/enrollment_spec.rb
View File

@ -300,6 +300,7 @@ describe Enrollment do
expect(@enrollment.grants_right?(@new_user, :read_grades)).to be_falsey
@course.enroll_teacher(@new_user)
@enrollment.reload
AdheresToPolicy::Cache.clear
expect(@enrollment.grants_right?(@user, :read_grades)).to be_truthy
end

3
spec/models/submission_spec.rb
View File

@ -602,6 +602,7 @@ describe Submission do
@submission.score = 1
@submission.grade_it!
AdheresToPolicy::Cache.clear
expect(@submission).to be_grants_right(@user, nil, :view_turnitin_report)
expect(@submission.turnitin_report_url("submission_#{@submission.id}", @user)).not_to be_nil
@ -618,6 +619,7 @@ describe Submission do
@assignment.turnitin_settings[:originality_report_visibility] = 'immediate'
@assignment.save
@submission.reload
AdheresToPolicy::Cache.clear
expect(@submission).to be_grants_right(@user, nil, :view_turnitin_report)
expect(@submission.turnitin_report_url("submission_#{@submission.id}", @user)).not_to be_nil
@ -635,6 +637,7 @@ describe Submission do
@assignment.due_at = Time.now - 1.day
@assignment.save
@submission.reload
AdheresToPolicy::Cache.clear
expect(@submission).to be_grants_right(@user, nil, :view_turnitin_report)
expect(@submission.turnitin_report_url("submission_#{@submission.id}", @user)).not_to be_nil