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

Add granular permissions for course sections 

Note: we'll want to re-run the data fix-up when we're ready to turn
on the feature flag permanently; in hopes to capture any differences
made to course section permissions between now and then.

fixes FOO-131
flag = granular_permissions_course_sections

Test Plan:
  - Run the migration and make sure there are no errors
  - With the granular_permissions_course_sections FF turned off,
    course sections and REST API should work the same with this patch
    set checked out as it does in beta/production
  - Some things to check:
    * How it acts as a teacher, student, designer, and public user
      in the course with the various settings above toggled to
      different states
  - With the granular_permissions_course_sections feature flag turned on
    course sections and REST api should work as expected. The same list
    checked above should be done so again, but this time:
    * Should only be able to Add course sections if the
      Course Sections - add permission is enabled for the users type
    * Should only be able to edit course sections if the
      Course Sections - edit permission is enabled for the users type
    * Should only be able to delete course sections if the
      Course Sections - delete permission is enabled for the user type
    * That the REST API works as expected
    * That the UI works as expected

Change-Id: Ia79a020e9b02c9362f837d146910707f830d2f47
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/253107
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Charley Kline <ckline@instructure.com>
QA-Review: Charley Kline <ckline@instructure.com>
Product-Review: Charley Kline <ckline@instructure.com>
This commit is contained in:
August Thornton 2020-11-18 10:49:18 -07:00
parent 2b92bee1c8
commit 9df3026063
10 changed files with 465 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
app/jsx/permissions/templates.js
View File

@ -44,7 +44,8 @@ export const PERMISSION_DETAIL_SECTIONS = [
export const GROUP_PERMISSION_DESCRIPTIONS = {
manage_wiki: () => I18n.t('Create / Delete / Update Pages'),
manage_course_admin_users: () => I18n.t('TAs / Observers / Designers')
manage_course_admin_users: () => I18n.t('TAs / Observers / Designers'),
manage_sections: () => I18n.t('add / edit / delete Sections')
}
export const generateActionTemplates = (

24
app/models/course_section.rb
View File

@ -142,7 +142,29 @@ class CourseSection < ActiveRecord::Base
end
set_policy do
given { |user, session| self.course.grants_right?(user, session, :manage_sections) }
given do |user, session|
self.course.root_account.feature_enabled?(:granular_permissions_course_sections) &&
self.course.grants_right?(user, session, :manage_sections_add)
end
can :read and can :create
given do |user, session|
self.course.root_account.feature_enabled?(:granular_permissions_course_sections) &&
self.course.grants_right?(user, session, :manage_sections_edit)
end
can :read and can :update
given do |user, session|
self.course.root_account.feature_enabled?(:granular_permissions_course_sections) &&
self.course.grants_right?(user, session, :manage_sections_delete)
end
can :read and can :delete
# Bundled legacy role override for managing course sections
given do |user, session|
!self.course.root_account.feature_enabled?(:granular_permissions_course_sections) &&
self.course.grants_right?(user, session, :manage_sections)
end
can :read and can :create and can :update and can :delete
given { |user, session| self.course.grants_any_right?(user, session, :manage_students, :manage_admin_users) }

76
app/models/role_override.rb
View File

@ -773,11 +773,77 @@ class RoleOverride < ActiveRecord::Base
'AccountAdmin'
]
},
:manage_sections => {
:label => lambda { t('permissions.manage_sections', "Manage (create / edit / delete) course sections") },
:label_v2 => lambda { t("Course Sections - add / edit / delete") },
:true_for => %w(AccountAdmin TeacherEnrollment DesignerEnrollment),
:available_to => %w(AccountAdmin AccountMembership TeacherEnrollment TaEnrollment DesignerEnrollment),
manage_sections: {
label: lambda do
t(
# Legacy bundled role override for managing course sections
'permissions.manage_sections',
'Manage (create / edit / delete) course sections'
)
end,
label_v2: -> { t('Course Sections - add / edit / delete') },
available_to: %w[
AccountAdmin
AccountMembership
TeacherEnrollment
TaEnrollment
DesignerEnrollment
],
true_for: %w[AccountAdmin TeacherEnrollment DesignerEnrollment],
account_allows: lambda do |a|
!a.root_account.feature_enabled?(:granular_permissions_course_sections)
end
},
manage_sections_add: {
label: -> { t('permissions.manage_sections_add', 'Add course sections') },
label_v2: -> { t('Course Sections - add') },
group: "manage_sections",
group_label: lambda { t("Manage Course Sections") },
available_to: %w[
AccountAdmin
AccountMembership
TeacherEnrollment
TaEnrollment
DesignerEnrollment
],
true_for: %w[AccountAdmin TeacherEnrollment DesignerEnrollment],
account_allows: lambda do |a|
a.root_account.feature_enabled?(:granular_permissions_course_sections)
end
},
manage_sections_edit: {
label: -> { t('permissions.manage_sections_edit', 'Edit course sections') },
label_v2: -> { t('Course Sections - edit') },
group: "manage_sections",
group_label: lambda { t("Manage Course Sections") },
available_to: %w[
AccountAdmin
AccountMembership
TeacherEnrollment
TaEnrollment
DesignerEnrollment
],
true_for: %w[AccountAdmin TeacherEnrollment DesignerEnrollment],
account_allows: lambda do |a|
a.root_account.feature_enabled?(:granular_permissions_course_sections)
end
},
manage_sections_delete: {
label: -> { t('permissions.manage_sections_delete', 'Delete course sections') },
label_v2: -> { t('Course Sections - delete') },
group: "manage_sections",
group_label: lambda { t("Manage Course Sections") },
available_to: %w[
AccountAdmin
AccountMembership
TeacherEnrollment
TaEnrollment
DesignerEnrollment
],
true_for: %w[AccountAdmin TeacherEnrollment DesignerEnrollment],
account_allows: lambda do |a|
a.root_account.feature_enabled?(:granular_permissions_course_sections)
end
},
:manage_students => {
:label => lambda { t('permissions.manage_students', "Add/remove students for the course") },

4
app/views/sections/show.html.erb
View File

@ -22,7 +22,9 @@
<% if can_do(@section, @current_user, :update) %>
<a href="#" class="btn button-sidebar-wide edit_section_link"><i class="icon-edit"></i> <%= t('buttons.edit_section', 'Edit Section') %></a>
<% if @section.nonxlist_course_id %>
<% if can_do(@section.nonxlist_course, @current_user, :manage_sections) %>
<% if !@section.root_account.feature_enabled?(:granular_permissions_course_sections) && can_do(@section.nonxlist_course, @current_user, :manage_sections) %>
<a href="#" class="btn button-sidebar-wide uncrosslist_link"><i class="icon-off"></i> <%= t('buttons.uncrosslist_section', 'De-Cross-List this Section') %></a>
<% elsif @section.root_account.feature_enabled?(:granular_permissions_course_sections) && can_do(@section.nonxlist_course, @current_user, :manage_sections_edit) %>
<a href="#" class="btn button-sidebar-wide uncrosslist_link"><i class="icon-off"></i> <%= t('buttons.uncrosslist_section', 'De-Cross-List this Section') %></a>
<% end %>
<a href="#" class="btn button-sidebar-wide crosslist_link"><i class="icon-off"></i> <%= t('buttons.recrosslist_section', 'Re-Cross-List this Section') %></a>

7
config/feature_flags/app_fundamentals_release_flags.yml
View File

@ -8,4 +8,9 @@ granular_permissions_manage_admin_users:
state: hidden
applies_to: RootAccount
display_name: Granular permissions for Add/Remove admin roles from Course
description: Adds granular permissions for adding and removing each role, and a separate permission for other course admin actions
description: Adds granular permissions for adding and removing each role, and a separate permission for other course admin actions
granular_permissions_course_sections:
state: hidden
applies_to: RootAccount
display_name: Granular permissions for managing course sections
description: Add granularity to permissions around managing course sections

28
db/migrate/20201117163234_granular_course_sections_permissions.rb Normal file
View File

@ -0,0 +1,28 @@
# frozen_string_literal: true
#
# Copyright (C) 2020 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
class GranularCourseSectionsPermissions < ActiveRecord::Migration[5.2]
tag :postdeploy
def change
DataFixup::AddRoleOverridesForNewPermission.run(:manage_sections, :manage_sections_add)
DataFixup::AddRoleOverridesForNewPermission.run(:manage_sections, :manage_sections_edit)
DataFixup::AddRoleOverridesForNewPermission.run(:manage_sections, :manage_sections_delete)
end
end

252
spec/apis/v1/sections_api_spec.rb
View File

@ -26,6 +26,10 @@ describe SectionsController, type: :request do
before :once do
course_with_teacher(:active_all => true, :user => user_with_pseudonym(:name => 'UWP'))
# granular permissions disabled by default
@course.root_account.disable_feature!(:granular_permissions_course_sections)
@me = @user
@course1 = @course
course_with_student(:user => @user, :active_all => true)
@ -147,12 +151,38 @@ describe SectionsController, type: :request do
json = api_call(:get, endpoint, params, {})
expect(json.size).to eq @course2.course_sections.count
end
context 'with granular permissions enabled' do
before :once do
@course.root_account.enable_feature!(:granular_permissions_course_sections)
end
it "should return the list of sections for a course" do
user1 = @user
user2 = User.create!(:name => 'Zombo')
section1 = @course2.default_section
section2 = @course2.course_sections.create!(:name => 'Section B')
section2.update_attribute :sis_source_id, 'sis-section'
@course2.enroll_user(user2, 'StudentEnrollment', :section => section2).accept!
RoleOverride.create!(:context => Account.default, :permission => 'read_sis', :role => teacher_role, :enabled => false)
@user = @me
json = api_call(:get, "/api/v1/courses/#{@course2.id}/sections.json",
{ :controller => 'sections', :action => 'index', :course_id => @course2.to_param, :format => 'json' }, { :include => ['students'] })
expect(json.size).to eq 2
expect(json.find { |s| s['name'] == section1.name }['students']).to eq api_json_response([user1], :only => user_api_fields)
expect(json.find { |s| s['name'] == section2.name }['students']).to eq api_json_response([user2], :only => user_api_fields)
end
end
end
describe "#show" do
before :once do
course_with_teacher
@section = @course.default_section
# granular permissions disabled by default
@course.root_account.disable_feature!(:granular_permissions_course_sections)
end
context "scoped by course" do
@ -220,6 +250,29 @@ describe SectionsController, type: :request do
site_admin_user
api_call(:get, "#{@path_prefix}/#{@other_section.id}", @path_params.merge({ :id => @other_section.to_param }), {}, {}, :expected_status => 404)
end
context 'with granular permissions enabled' do
before :once do
@course.root_account.enable_feature!(:granular_permissions_course_sections)
end
it "should be accessible from the course" do
json = api_call(:get, "#{@path_prefix}/#{@section.id}", @path_params.merge({ :id => @section.to_param }))
expect(json).to eq({
'id' => @section.id,
'name' => @section.name,
'course_id' => @course.id,
'nonxlist_course_id' => nil,
'start_at' => nil,
'end_at' => nil,
'restrict_enrollments_to_section_dates' => nil,
'integration_id' => nil,
'sis_course_id' => nil,
'sis_section_id' => nil,
'created_at' => @section.created_at.iso8601
})
end
end
end
context "unscoped" do
@ -285,6 +338,29 @@ describe SectionsController, type: :request do
@course.destroy
json = api_call(:get, "#{@path_prefix}/#{@section.id}", @path_params.merge({ :id => @section.to_param }), {}, {}, :expected_status => 404)
end
context 'with granular permissions enabled' do
before :once do
@course.root_account.enable_feature!(:granular_permissions_course_sections)
end
it "should be accessible without a course context" do
json = api_call(:get, "#{@path_prefix}/#{@section.id}", @path_params.merge({ :id => @section.to_param }))
expect(json).to eq({
'id' => @section.id,
'name' => @section.name,
'course_id' => @course.id,
'nonxlist_course_id' => nil,
'start_at' => nil,
'end_at' => nil,
'restrict_enrollments_to_section_dates' => nil,
'integration_id' => nil,
'sis_course_id' => nil,
'sis_section_id' => nil,
'created_at' => @section.created_at.iso8601
})
end
end
end
context "as an admin" do
@ -320,10 +396,13 @@ describe SectionsController, type: :request do
course_factory
@path_prefix = "/api/v1/courses/#{@course.id}/sections"
@path_params = { :controller => 'sections', :action => 'create', :course_id => @course.to_param, :format => 'json' }
# granular permissions disabled by default
@course.root_account.disable_feature!(:granular_permissions_course_sections)
end
context "as teacher" do
before :once do
before do
course_with_teacher :course => @course
end
@ -440,6 +519,62 @@ describe SectionsController, type: :request do
expect(section.sis_source_id).to eq 'fail'
end
end
context 'with granular permissions enabled' do
before :once do
@course.root_account.enable_feature!(:granular_permissions_course_sections)
end
context "as teacher having manage_sections_add permission" do
before do
course_with_teacher :course => @course
end
it "should create a section with default parameters" do
json = api_call(:post, @path_prefix, @path_params)
@course.reload
expect(@course.active_course_sections.where(id: json['id'].to_i)).to be_exists
end
it "should create a section with custom parameters" do
json = api_call(:post, @path_prefix, @path_params, { :course_section =>
{ :name => 'Name', :start_at => '2011-01-01T01:00Z', :end_at => '2011-07-01T01:00Z' }})
@course.reload
section = @course.active_course_sections.find(json['id'].to_i)
expect(section.name).to eq 'Name'
expect(section.sis_source_id).to be_nil
expect(section.start_at).to eq Time.parse('2011-01-01T01:00Z')
expect(section.end_at).to eq Time.parse('2011-07-01T01:00Z')
end
end
context "as teacher without manage_sections_add permission" do
before do
course_with_teacher :course => @course
teacher_role = Role.get_built_in_role('TeacherEnrollment', root_account_id: @course.root_account.id)
RoleOverride.create!(
permission: 'manage_sections_add',
enabled: false,
role: teacher_role,
account: @course.root_account
)
end
it "should disallow creating a section" do
api_call(:post, @path_prefix, @path_params, {}, {}, :expected_status => 401)
end
end
context "as student" do
before do
course_with_student_logged_in(:course => @course)
end
it "should disallow creating a section" do
api_call(:post, @path_prefix, @path_params, {}, {}, :expected_status => 401)
end
end
end
end
describe "#update" do
@ -550,6 +685,70 @@ describe SectionsController, type: :request do
expect(@section.sis_source_id).to eq 'NEWSIS'
end
end
context 'with granular permissions enabled' do
before :once do
@course.root_account.enable_feature!(:granular_permissions_course_sections)
end
context "as teacher having manage_sections_edit permission" do
before do
course_with_teacher :course => @course
end
it "should modify section data by id" do
json = api_call(:put, "#{@path_prefix}/#{@section.id}", @path_params.merge(:id => @section.to_param), { :course_section =>
{ :name => 'New Name', :start_at => '2012-01-01T01:00Z', :end_at => '2012-07-01T01:00Z', :restrict_enrollments_to_section_dates => '1' }})
expect(json['id']).to eq @section.id
@section.reload
expect(@section.name).to eq 'New Name'
expect(@section.sis_source_id).to eq 'SISsy'
expect(@section.start_at).to eq Time.parse('2012-01-01T01:00Z')
expect(@section.end_at).to eq Time.parse('2012-07-01T01:00Z')
expect(@section.restrict_enrollments_to_section_dates).to be_truthy
end
it "should modify section data by sis id" do
json = api_call(:put, "#{@path_prefix}/sis_section_id:SISsy", @path_params.merge(:id => "sis_section_id:SISsy"), { :course_section =>
{ :name => 'New Name', :start_at => '2012-01-01T01:00Z', :end_at => '2012-07-01T01:00Z' }})
expect(json['id']).to eq @section.id
@section.reload
expect(@section.name).to eq 'New Name'
expect(@section.sis_source_id).to eq 'SISsy'
expect(@section.start_at).to eq Time.parse('2012-01-01T01:00Z')
expect(@section.end_at).to eq Time.parse('2012-07-01T01:00Z')
end
end
context "as teacher without manage_sections_edit permission" do
before do
course_with_teacher :course => @course
teacher_role = Role.get_built_in_role('TeacherEnrollment', root_account_id: @course.root_account.id)
RoleOverride.create!(
permission: 'manage_sections_edit',
enabled: false,
role: teacher_role,
account: @course.root_account
)
end
it "should disallow modifying a section" do
api_call(:put, "#{@path_prefix}/#{@section.id}", @path_params.merge(:id => @section.to_param),
{ :course_section => { :name => 'New Name' } }, {}, :expected_status => 401)
end
end
context "as student" do
before do
course_with_student_logged_in(:course => @course)
end
it "should disallow modifying a section" do
api_call(:put, "#{@path_prefix}/#{@section.id}", @path_params.merge(:id => @section.to_param),
{ :course_section => { :name => 'New Name' } }, {}, :expected_status => 401)
end
end
end
end
describe "#delete" do
@ -604,6 +803,57 @@ describe SectionsController, type: :request do
api_call(:delete, "#{@path_prefix}/#{@section.id}", @path_params.merge(:id => @section.to_param), {}, {}, :expected_status => 401)
end
end
context 'with granular permissions enabled' do
before :once do
@course.root_account.enable_feature!(:granular_permissions_course_sections)
end
context "as teacher having manage_sections_edit permission" do
before do
course_with_teacher :course => @course
end
it "should delete a section by id" do
json = api_call(:delete, "#{@path_prefix}/#{@section.id}", @path_params.merge(:id => @section.to_param))
expect(json['id']).to eq @section.id
expect(@section.reload).to be_deleted
end
it "should delete a section by sis id" do
json = api_call(:delete, "#{@path_prefix}/sis_section_id:SISsy", @path_params.merge(:id => "sis_section_id:SISsy"))
expect(json['id']).to eq @section.id
expect(@section.reload).to be_deleted
end
end
context "as teacher without manage_sections_delete permission" do
before do
course_with_teacher :course => @course
teacher_role = Role.get_built_in_role('TeacherEnrollment', root_account_id: @course.root_account.id)
RoleOverride.create!(
permission: 'manage_sections_delete',
enabled: false,
role: teacher_role,
account: @course.root_account
)
end
it "should disallow deleting a section" do
api_call(:delete, "#{@path_prefix}/#{@section.id}", @path_params.merge(:id => @section.to_param), {}, {}, :expected_status => 401)
end
end
context "as student" do
before do
course_with_student_logged_in(:course => @course)
end
it "should disallow deleting a section" do
api_call(:delete, "#{@path_prefix}/#{@section.id}", @path_params.merge(:id => @section.to_param), {}, {}, :expected_status => 401)
end
end
end
end
describe "#crosslist" do

22
spec/integration/security_spec.rb
View File

@ -1040,6 +1040,8 @@ describe "security" do
it 'manage_sections' do
course_with_teacher_logged_in(:active_all => 1)
# granular permissions disabled by default
@course.root_account.disable_feature!(:granular_permissions_course_sections)
remove_permission(:manage_sections, teacher_role)
get "/courses/#{@course.id}/settings"
@ -1056,6 +1058,26 @@ describe "security" do
assert_status(401)
end
it 'manage_sections with granular permissions enabled' do
course_with_teacher_logged_in(:active_all => 1)
@course.root_account.enable_feature!(:granular_permissions_course_sections)
remove_permission(:manage_sections_add, teacher_role)
remove_permission(:manage_sections_edit, teacher_role)
get "/courses/#{@course.id}/settings"
expect(response).to be_successful
expect(response.body).not_to match 'Add Section'
post "/courses/#{@course.id}/sections"
assert_status(401)
get "/courses/#{@course.id}/sections/#{@course.default_section.id}"
expect(response).to be_successful
put "/courses/#{@course.id}/sections/#{@course.default_section.id}"
assert_status(401)
end
it 'change_course_state' do
course_with_teacher_logged_in(:active_all => 1)
remove_permission(:change_course_state, teacher_role)

41
spec/models/course_section_spec.rb
View File

@ -484,6 +484,8 @@ describe CourseSection, "moving to new course" do
})
course_with_ta(:active_all => true)
@other_section = @course.course_sections.create!(:name => "Other Section")
# granular permissions disabled by default
@course.root_account.disable_feature!(:granular_permissions_course_sections)
end
it "should work with section_limited true" do
@ -505,6 +507,45 @@ describe CourseSection, "moving to new course" do
expect(@other_section.grants_right?(@ta, :read)).to be_truthy
end
end
context 'with granular permissions enabled' do
context ":read and section_visibilities" do
before do
RoleOverride.create!({
:context => Account.default,
:permission => 'manage_students',
:role => ta_role,
:enabled => false
})
course_with_ta(:active_all => true)
@other_section = @course.course_sections.create!(:name => "Other Section")
@course.root_account.enable_feature!(:granular_permissions_course_sections)
end
it "should work with section_limited true" do
@ta.enrollments.update_all(:limit_privileges_to_course_section => true)
@ta.reload
# make sure other ways to get :read are false
expect(@other_section.course.grants_right?(@ta, :manage_sections_add)).to be_falsey
expect(@other_section.course.grants_right?(@ta, :manage_sections_edit)).to be_falsey
expect(@other_section.course.grants_right?(@ta, :manage_sections_delete)).to be_falsey
expect(@other_section.course.grants_right?(@ta, :manage_students)).to be_falsey
expect(@other_section.grants_right?(@ta, :read)).to be_falsey
end
it "should work with section_limited false" do
# make sure other ways to get :read are false
expect(@other_section.course.grants_right?(@ta, :manage_sections_add)).to be_falsey
expect(@other_section.course.grants_right?(@ta, :manage_sections_edit)).to be_falsey
expect(@other_section.course.grants_right?(@ta, :manage_sections_delete)).to be_falsey
expect(@other_section.course.grants_right?(@ta, :manage_students)).to be_falsey
expect(@other_section.grants_right?(@ta, :read)).to be_truthy
end
end
end
end
context 'enrollment state invalidation' do

20
spec/selenium/course_sections_spec.rb
View File

@ -19,6 +19,9 @@
require File.expand_path(File.dirname(__FILE__) + '/common')
# We have the funky indenting here because we will remove this once the granular
# permission stuff is released, and I don't want to complicate the git history
RSpec.shared_examples "course_sections" do
describe "course sections" do
include_context "in-process server selenium tests"
@ -31,8 +34,9 @@ describe "course sections" do
ff('#enrollment_table tr')
end
before (:each) do
before(:each) do
course_with_teacher_logged_in
set_granular_permission
@section = @course.default_section
end
@ -129,7 +133,6 @@ describe "course sections" do
end
context "student tray" do
before(:each) do
@account = Account.default
@account.enable_feature!(:student_context_cards)
@ -143,3 +146,16 @@ describe "course sections" do
end
end
end
end # End shared_example block
RSpec.describe 'With granular permission on' do
it_behaves_like "course_sections" do
let(:set_granular_permission) { @course.root_account.enable_feature!(:granular_permissions_course_sections) }
end
end
RSpec.describe 'With granular permission off' do
it_behaves_like "course_sections" do
let(:set_granular_permission) { @course.root_account.disable_feature!(:granular_permissions_course_sections) }
end
end