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

Revert "restrict tool launches based on visibility" 

This reverts commit 9a469e6642.

Reason for revert: basically, for an account-level tool – hiding the course visibility settings in a single course, sets ‘admin only’ for the account level tool – affecting all courses using the tool
so you can hide an account-level tool in course 1
but bc this commit updates the visibility setting of the tool at the account level; students in all courses are prevented from launching the tool from course nav

Change-Id: I95ce3c27096b59af080a5a330627acb56339ec32
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/268387
Reviewed-by: Rob Orton <rob@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Xander Moffatt <xmoffatt@instructure.com>
Product-Review: Xander Moffatt <xmoffatt@instructure.com>
This commit is contained in:
Xander Moffatt 2021-07-06 19:11:33 +00:00
parent 7911c2f3ee
commit 2610015fb8
6 changed files with 17 additions and 376 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
app/controllers/external_tools_controller.rb
View File

@ -154,12 +154,6 @@ class ExternalToolsController < ApplicationController
raise InvalidSettingsError, t("#application.errors.invalid_external_tool", "Couldn't find valid settings for this link")
end
placement = placement_from_params
unless @tool.visible?(placement, @current_user, @context, session)
render_unauthorized_action
return
end
add_crumb(@context.name, named_context_url(@context, :context_url))
@lti_launch = lti_launch(
tool: @tool,
@ -386,11 +380,6 @@ class ExternalToolsController < ApplicationController
placement = placement_from_params
return unless find_tool(params[:id], placement)
unless @tool.visible?(placement, @current_user, @context, session)
render_unauthorized_action
return
end
add_crumb(@context.name, named_context_url(@context, :context_url))
@return_url = named_context_url(@context, :context_external_content_success_url, 'external_tool_redirect', {include_host: true})

51
app/models/context_external_tool.rb
View File

@ -966,34 +966,8 @@ end
end
end
def override_visibility(visibility, placement = nil)
to_override = placement.present? ? self.settings[placement.to_sym] : self.settings
to_override[:visibility_override] = visibility
end
def clear_visibility_override(placement = nil)
to_override = placement.present? ? self.settings[placement.to_sym] : self.settings
to_override.delete(:visibility_override)
end
def visible?(placement, user, context, session=nil)
return self.globally_visible?(user, context, session) if placement&.to_sym == :global_navigation
visibility = self.extension_setting(placement, :visibility_override) || self.extension_setting(placement, :visibility)
self.class.visible?(visibility, user, context, session)
end
def globally_visible?(user, context, session=nil)
granted_permissions =
self.class.global_navigation_granted_permissions(
root_account: context.root_account, user: user, context: context, session: session
)
self.class.global_tool_visible?(self, granted_permissions)
end
def visible_with_permission_check?(launch_type, user, context, session=nil)
return false unless self.visible?(launch_type, user, context, session)
return false unless self.class.visible?(self.extension_setting(launch_type, 'visibility'), user, context, session)
permission_given?(launch_type, user, context, session)
end
@ -1157,18 +1131,21 @@ end
def self.filtered_global_navigation_tools(root_account, granted_permissions)
tools = self.all_global_navigation_tools(root_account)
tools.select { |tool| self.global_tool_visible?(tool, granted_permissions) }
end
def self.global_tool_visible?(tool, granted_permissions)
# reject the admin only tools
return false if granted_permissions[:original_visibility] != 'admins' && tool.extension_setting(:global_navigation, :visibility) == 'admins'
if granted_permissions[:original_visibility] != 'admins'
# reject the admin only tools
tools.reject!{|tool| tool.global_navigation[:visibility] == 'admins'}
end
# check against permissions if needed
required_permissions_str = tool.extension_setting(:global_navigation, 'required_permissions')
return true unless required_permissions_str
required_permissions_str.split(",").map(&:to_sym).all? { |p| granted_permissions[p] }
tools.select! do |tool|
required_permissions_str = tool.extension_setting(:global_navigation, 'required_permissions')
if required_permissions_str
required_permissions_str.split(",").map(&:to_sym).all?{|p| granted_permissions[p]}
else
true
end
end
tools
end
def self.key_for_granted_permissions(granted_permissions)

29
app/models/course.rb
View File

@ -256,7 +256,6 @@ class Course < ActiveRecord::Base
before_update :handle_syllabus_changes_for_master_migration
before_save :touch_root_folder_if_necessary
before_save :hide_external_tool_tabs_if_necessary
before_validation :verify_unique_ids
validate :validate_course_dates
validate :validate_course_image
@ -2982,34 +2981,12 @@ class Course < ActiveRecord::Base
end
def external_tool_tabs(opts, user)
tools = external_tools_for_tabs.select { |t| t.permission_given?(:course_navigation, user, self) && t.feature_flag_enabled?(self) }
tools = self.context_external_tools.active.having_setting('course_navigation')
tools += ContextExternalTool.active.having_setting('course_navigation').where(context_type: 'Account', context_id: account_chain_ids).to_a
tools = tools.select { |t| t.permission_given?(:course_navigation, user, self) && t.feature_flag_enabled?(self) }
Lti::ExternalToolTab.new(self, :course_navigation, tools, opts[:language]).tabs
end
def external_tools_for_tabs
self.context_external_tools.active.having_setting('course_navigation') +
ContextExternalTool.active.having_setting('course_navigation').where(
context_type: 'Account', context_id: account_chain_ids
).to_a
end
def hide_external_tool_tabs_if_necessary
return true unless tab_configuration_changed?
tabs_by_id = tab_configuration.each_with_object({}) { |tab,memo| memo[tab['id']] = tab}
external_tools_for_tabs.each do |tool|
next unless tabs_by_id[tool.asset_string]
if tabs_by_id[tool.asset_string]['hidden']
tool.override_visibility('admins', :course_navigation)
else
tool.clear_visibility_override(:course_navigation)
end
tool.save!
end
true
end
def tabs_available(user=nil, opts={})
opts.reverse_merge!(:include_external => true, include_hidden_unused: true)
cache_key = [user, self, opts].cache_key

10
spec/controllers/external_tools_controller_spec.rb
View File

@ -866,16 +866,6 @@ describe ExternalToolsController do
assert_unauthorized
end
it 'should restrict students from launching tools with limited visibility' do
user_session(@student)
tool = @course.context_external_tools.new(name: "bob", consumer_key: "bob", shared_secret: "bob")
tool.url = "http://www.example.com/basic_lti"
tool.course_navigation = { visibility: 'admins'}
tool.save!
get 'retrieve', params: {course_id: @course.id, url: "http://www.example.com/basic_lti", placement: 'course_navigation'}
assert_unauthorized
end
it "should find tools matching by exact url" do
user_session(@teacher)
tool = @course.context_external_tools.new(:name => "bob", :consumer_key => "bob", :shared_secret => "bob")

226
spec/models/context_external_tool_spec.rb
View File

@ -2363,230 +2363,4 @@ describe ContextExternalTool do
end
end
end
describe 'override_visibility' do
let(:tool) { external_tool_model }
context 'without placement provided' do
it 'changes global visibility setting' do
tool.override_visibility('members')
expect(tool.settings[:visibility_override]).to eq 'members'
end
it 'does not change original visibility' do
tool.settings[:visibility] = 'admins'
tool.override_visibility('members')
expect(tool.settings[:visibility]).to eq 'admins'
end
end
context 'with placement provided' do
let(:placement) { :course_navigation }
let(:tool) do
t = external_tool_model
t.course_navigation = { enabled: true }
t.save!
t
end
it 'changes placement-specific visibility setting' do
tool.override_visibility('members', placement)
expect(tool.settings[placement][:visibility_override]).to eq 'members'
end
it 'ignores other placement visibility settings' do
tool.editor_button = { visibility: 'admins' }
tool.override_visibility('members', placement)
expect(tool.settings[:editor_button][:visibility]).to eq 'admins'
end
it 'does not change original visibility' do
tool.settings[placement][:visibility] = 'admins'
tool.override_visibility('members', placement)
expect(tool.settings[placement][:visibility]).to eq 'admins'
end
end
end
describe 'clear_visibility_override' do
let(:tool) { external_tool_model }
context 'without placement provided' do
it 'clears global override' do
tool.override_visibility('members')
tool.clear_visibility_override
expect(tool.settings[:visibility_override]).to eq nil
end
it 'does not change original visibility' do
tool.settings[:visibility] = 'admins'
tool.override_visibility('members')
tool.clear_visibility_override
expect(tool.settings[:visibility]).to eq 'admins'
end
end
context 'with placement provided' do
let(:placement) { :course_navigation }
let(:tool) do
t = external_tool_model
t.course_navigation = { enabled: true }
t.save!
t
end
it 'clears placement-specific override' do
tool.override_visibility('members', placement)
tool.clear_visibility_override(placement)
expect(tool.settings[placement][:visibility_override]).to eq nil
end
it 'does not change original visibility' do
tool.settings[placement][:visibility] = 'admins'
tool.override_visibility('members', placement)
tool.clear_visibility_override(placement)
expect(tool.settings[placement][:visibility]).to eq 'admins'
end
end
end
describe 'visible?' do
let(:c) {course_factory(active_course:true)}
let(:student) do
user_model.tap do |u|
e = c.enroll_student(u)
e.invite
e.accept
end
end
let(:tool) { external_tool_model context: c }
let(:p) { :course_navigation }
it 'uses global visibility setting if no placement provided' do
tool.settings[:visibility] = 'admins'
expect(tool.visible?(nil, student, c)).to be false
end
it 'returns true if placement has no visibility setting' do
expect(tool.visible?(p, student, c)).to be true
end
it 'uses placement visibility setting' do
tool.settings[p] = {visibility: 'admins'}
expect(tool.visible?(p, student, c)).to be false
end
it 'uses placement visibility override if present' do
tool.settings[p] = {visibility: 'admins', visibility_override: 'members'}
expect(tool.visible?(p, student, c)).to be true
end
it 'uses globally_visible for global_navigation placement' do
tool.settings[:global_navigation] = {visibility: 'admins'} # this should mean it returns false for a student
allow(tool).to receive(:globally_visible?).and_return(true) # but this will always override
expect(tool.visible?(:global_navigation, student, c)).to be true
end
end
describe 'globally_visible?' do
subject { tool.globally_visible?(user, account) }
let(:account) {account_model}
let(:c) {course_factory(active_course:true, account: account)}
let(:student) do
user_model.tap do |u|
e = c.enroll_student(u)
e.invite
e.accept
end
end
let(:teacher) do
user_model.tap do |u|
e = c.enroll_teacher(u)
e.invite
e.accept
end
end
let(:other_teacher) do
other_course = course_factory(active_course:true, account: account)
user_model.tap do |u|
e = other_course.enroll_teacher(u)
e.invite
e.accept
end
end
shared_examples_for 'a student can view' do
let(:user) { student }
it { is_expected.to eq true }
end
shared_examples_for 'a teacher can view' do
let(:user) { teacher }
it { is_expected.to eq true }
end
shared_examples_for 'an admin can view' do
let(:user) { account_admin_user(account: account, active_all: true) }
it { is_expected.to eq true }
end
shared_examples_for 'an unauthenticated user can view' do
let(:user) { nil }
it { is_expected.to eq true }
end
context 'with a global_navigation tool visible to the public' do
let(:tool) do
external_tool_model(context: c).tap do |t|
t.settings[:global_navigation] = {visibility: 'public', url: 'https://global_navigation.com'}
end
end
it_behaves_like 'a student can view'
it_behaves_like 'a teacher can view'
it_behaves_like 'an admin can view'
it_behaves_like 'an unauthenticated user can view'
end
context 'with a global_navigation tool visible to members' do
let(:tool) do
external_tool_model(context: c).tap do |t|
t.settings[:global_navigation] = {visibility: 'members', url: 'https://global_navigation.com'}
end
end
it_behaves_like 'a student can view'
it_behaves_like 'a teacher can view'
it_behaves_like 'an admin can view'
it_behaves_like 'an unauthenticated user can view'
end
context 'with a global_navigation tool visible to only admins' do
let(:tool) do
external_tool_model(context: c).tap do |t|
t.settings[:global_navigation] = {visibility: 'admins', url: 'https://global_navigation.com'}
end
end
it_behaves_like 'a teacher can view'
it_behaves_like 'an admin can view'
context 'with a student' do
let(:user) { student }
it { is_expected.to eq false }
end
context 'with no user' do
let(:user) { nil }
it { is_expected.to eq false }
end
end
end
end

66
spec/models/course_spec.rb
View File

@ -4543,72 +4543,6 @@ describe Course, 'tabs_available' do
end
end
describe Course, '#hide_external_tool_tabs_if_necessary' do
before :once do
course_model
end
def hide_tool(tool)
@course.tab_configuration = [{id: tool.asset_string, hidden: true}]
@course.save!
end
def unhide_tool(tool)
@course.tab_configuration = [{id: tool.asset_string}]
@course.save!
end
context 'when tool does not have visibility defined' do
def new_tool
tool = @course.context_external_tools.new(name: "bob", consumer_key: "bob", shared_secret: "bob", domain: "example.com")
tool.course_navigation = {url: "http://www.example.com", default: "active" }
tool.save!
tool
end
it 'restricts tool visibility to teachers when tool tab is hidden from navigation' do
tool = new_tool
hide_tool(tool)
expect(tool.reload.extension_setting(:course_navigation, :visibility_override)).to eq('admins')
end
it 'removes restrictions on tool visibility when tool tab is unhidden' do
tool = new_tool
hide_tool(tool)
unhide_tool(tool)
expect(tool.reload.extension_setting(:course_navigation, :visibility_override)).to be_nil
end
end
context 'when tool has visibility defined' do
def new_tool
tool = @course.context_external_tools.new(name: "bob", consumer_key: "bob", shared_secret: "bob", domain: "example.com")
tool.course_navigation = {url: "http://www.example.com", visibility: 'members', default: "active" }
tool.save!
tool
end
it 'keeps originally-defined visibility for later reuse' do
tool = new_tool
hide_tool(tool)
tool.reload
expect(tool.extension_setting(:course_navigation, :visibility_override)).to eq('admins')
expect(tool.extension_setting(:course_navigation, :visibility)).to eq('members')
end
it 'restores originally-defined visibility when tool is un-hidden' do
tool = new_tool
hide_tool(tool)
unhide_tool(tool)
tool.reload
expect(tool.extension_setting(:course_navigation, :visibility_override)).to be_nil
expect(tool.extension_setting(:course_navigation, :visibility)).to eq('members')
end
end
end
describe Course, 'tab_hidden?' do
before :once do
course_model