feat(gradebook): restrict gradebook access to owners and managers

app/controllers/course/gradebook_controller.rb

@@ -24,7 +24,7 @@ def index
 
   def update_weights
     authorize! :manage_gradebook_weights, current_course
-    updates = update_weights_params[:weights].map { |entry| parse_weight_entry(entry) }
+    updates = (update_weights_params[:weights] || []).map { |entry| parse_weight_entry(entry) }
     Course::Gradebook::Contribution.bulk_update(course: current_course, updates: updates)
     render json: { weights: serialize_weight_updates(updates) }
   rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotFound => e

app/models/components/course/gradebook_ability_component.rb

@@ -3,7 +3,7 @@ module Course::GradebookAbilityComponent
   include AbilityHost::Component
 
   def define_permissions
-    can :read_gradebook, Course, id: course.id if course_user&.staff?
+    can :read_gradebook, Course, id: course.id if course_user&.manager_or_owner?
     can :manage_gradebook_weights, Course, id: course.id if course_user&.manager_or_owner?
     can :manage_gradebook_settings, Course, id: course.id if course_user&.manager_or_owner?
     super

spec/controllers/course/gradebook_controller_spec.rb

@@ -7,17 +7,17 @@
   with_tenant(:instance) do
     let(:course) { create(:course) }
     let(:student) { create(:course_user, :student, course: course) }
-    let(:staff) { create(:course_user, :teaching_assistant, course: course) }
+    let(:manager) { create(:course_user, :manager, course: course) }
 
     describe '#index' do
       render_views
       subject { get :index, params: { course_id: course.id }, format: :json }
 
       context 'when the gradebook component is disabled' do
-        let(:ta) { create(:course_teaching_assistant, course: course) }
+        let(:owner) { create(:course_owner, course: course) }
 
         before do
-          controller_sign_in(controller, ta.user)
+          controller_sign_in(controller, owner.user)
           allow(controller).to receive_message_chain('current_component_host.[]').and_return(nil)
         end
 
@@ -26,6 +26,20 @@
         end
       end
 
+      context 'when a owner visits the page' do
+        let(:owner) { create(:course_owner, course: course) }
+        before { controller_sign_in(controller, owner.user) }
+
+        it { expect(subject).to be_successful }
+      end
+
+      context 'when a manager visits the page' do
+        let(:manager) { create(:course_manager, course: course) }
+        before { controller_sign_in(controller, manager.user) }
+
+        it { expect(subject).to be_successful }
+      end
+
       context 'when a student visits the page' do
         let(:student) { create(:course_student, course: course) }
         before { controller_sign_in(controller, student.user) }
@@ -37,33 +51,23 @@
         let(:ta) { create(:course_teaching_assistant, course: course) }
         before { controller_sign_in(controller, ta.user) }
 
-        it { expect(subject).to be_successful }
-
-        it 'returns all required top-level keys' do
-          subject
-          data = JSON.parse(response.body)
-          %w[categories tabs assessments students submissions].each do |key|
-            expect(data).to have_key(key), "expected response to have key '#{key}'"
-          end
+        it 'is denied' do
+          expect { get :index, params: { course_id: course.id }, format: :json }.
+            to raise_error(CanCan::AccessDenied)
         end
       end
 
-      context 'when a manager visits the page' do
-        let(:manager) { create(:course_manager, course: course) }
-        before { controller_sign_in(controller, manager.user) }
-
-        it { expect(subject).to be_successful }
-      end
-
       context 'when an observer visits the page' do
         let(:observer) { create(:course_observer, course: course) }
         before { controller_sign_in(controller, observer.user) }
 
-        it { expect(subject).to be_successful }
+        it 'is denied' do
+          expect { get :index, params: { course_id: course.id }, format: :json }.
+            to raise_error(CanCan::AccessDenied)
+        end
       end
 
       context 'with a published assessment and a graded submission' do
-        let(:ta) { create(:course_teaching_assistant, course: course) }
         let(:tab) { course.assessment_categories.first.tabs.first }
         let!(:assessment) do
           create(:course_assessment_assessment, :published_with_mcq_question,
@@ -77,7 +81,7 @@
 
         before do
           submission.answers.update_all(grade: 5.0, current_answer: true)
-          controller_sign_in(controller, ta.user)
+          controller_sign_in(controller, manager.user)
         end
 
         it 'includes the assessment in the assessments array' do
@@ -129,9 +133,8 @@
       end
 
       context 'when a student has an external ID' do
-        let(:ta) { create(:course_teaching_assistant, course: course) }
         let!(:student) { create(:course_student, course: course, external_id: 'EXT-123') }
-        before { controller_sign_in(controller, ta.user) }
+        before { controller_sign_in(controller, manager.user) }
 
         it 'returns the external ID in the students array' do
           subject
@@ -142,8 +145,50 @@
         end
       end
 
-      context 'with a graded submission where the answer grade is exactly 0' do
+      context 'when a phantom student is enrolled' do
+        let!(:real_student) { create(:course_student, course: course) }
+        let!(:phantom_student) { create(:course_student, :phantom, course: course) }
+        before { controller_sign_in(controller, manager.user) }
+
+        it 'omits phantom users from the students array' do
+          subject
+          data = JSON.parse(response.body)
+          ids = data['students'].map { |s| s['id'] }
+          expect(ids).to include(real_student.user_id)
+          expect(ids).not_to include(phantom_student.user_id)
+        end
+      end
+
+      context 'when the course has no published assessments or students' do
+        let(:ta) { create(:course_teaching_assistant, course: course) }
+        before { controller_sign_in(controller, manager.user) }
+
+        it 'returns empty assessments and students without error' do
+          subject
+          expect(response).to be_successful
+          data = JSON.parse(response.body)
+          expect(data['assessments']).to eq([])
+          expect(data['students']).to eq([])
+          expect(data['submissions']).to eq([])
+        end
+      end
+
+      context 'with an unpublished (draft) assessment' do
         let(:ta) { create(:course_teaching_assistant, course: course) }
+        let(:tab) { course.assessment_categories.first.tabs.first }
+        let!(:draft) do
+          create(:course_assessment_assessment, course: course, tab: tab, published: false)
+        end
+        before { controller_sign_in(controller, manager.user) }
+
+        it 'excludes the draft from the assessments array' do
+          subject
+          data = JSON.parse(response.body)
+          expect(data['assessments'].map { |a| a['id'] }).not_to include(draft.id)
+        end
+      end
+
+      context 'with a graded submission where the answer grade is exactly 0' do
         let(:tab) { course.assessment_categories.first.tabs.first }
         let!(:assessment) do
           create(:course_assessment_assessment, :published_with_mcq_question,
@@ -157,7 +202,7 @@
 
         before do
           submission.answers.update_all(grade: 0.0, current_answer: true)
-          controller_sign_in(controller, ta.user)
+          controller_sign_in(controller, manager.user)
         end
 
         it 'returns grade 0 (not null) in the submissions array' do
@@ -172,7 +217,6 @@
       end
 
       context 'with a graded submission where answer grades are null (blank)' do
-        let(:ta) { create(:course_teaching_assistant, course: course) }
         let(:tab) { course.assessment_categories.first.tabs.first }
         let!(:assessment) do
           create(:course_assessment_assessment, :published_with_mcq_question,
@@ -186,7 +230,7 @@
 
         before do
           submission.answers.update_all(grade: nil, current_answer: true)
-          controller_sign_in(controller, ta.user)
+          controller_sign_in(controller, manager.user)
         end
 
         it 'returns null grade (not 0) in the submissions array' do
@@ -202,7 +246,6 @@
 
       context 'when the course has an external assessment' do
         render_views
-        let(:ta) { create(:course_teaching_assistant, course: course) }
         let(:gb_student) { create(:course_student, course: course) }
         let!(:external) do
           create(:course_external_assessment, course: course, title: 'Midterm', maximum_grade: 50)
@@ -211,7 +254,7 @@
           create(:course_external_assessment_grade,
                  external_assessment: external, course_user: gb_student, grade: 41)
         end
-        before { controller_sign_in(controller, ta.user) }
+        before { controller_sign_in(controller, manager.user) }
 
         it 'merges the external into assessments with a negative id and external flag' do
           subject
@@ -232,30 +275,20 @@
           expect(sub['studentId']).to eq(gb_student.user_id)
           expect(sub['grade']).to eq(41.0)
         end
+      end
 
-        it 'emits a synthetic External Assessments category' do
-          subject
-          data = JSON.parse(response.body)
-          cat = data['categories'].find { |c| c['id'] == Course::ExternalAssessment::SYNTHETIC_CATEGORY_ID }
-          expect(cat).to be_present
-          expect(cat['title']).to eq('External Assessments')
+      context 'when an external assessment has no grades' do
+        render_views
+        let!(:external) do
+          create(:course_external_assessment, course: course, title: 'Ungraded', maximum_grade: 20)
         end
+        before { controller_sign_in(controller, manager.user) }
 
-        it 'emits a synthetic tab with negative id under the synthetic category' do
+        it 'still emits the external leaf but no submission row for it' do
           subject
           data = JSON.parse(response.body)
-          tab = data['tabs'].find { |t| t['id'] == external.synthetic_tab_id }
-          expect(tab).to be_present
-          expect(tab['categoryId']).to eq(Course::ExternalAssessment::SYNTHETIC_CATEGORY_ID)
-        end
-
-        it 'creates no real tab or category for the external' do
-          tab_count_before = Course::Assessment::Tab.count
-          cat_count_before = Course::Assessment::Category.count
-          subject
-          expect(Course::Assessment::Tab.count).to eq(tab_count_before)
-          expect(Course::Assessment::Category.count).to eq(cat_count_before)
-          expect(Course::Assessment::Category.where(title: 'External Assessments')).to be_empty
+          expect(data['assessments'].map { |a| a['id'] }).to include(-external.id)
+          expect(data['submissions'].select { |s| s['assessmentId'] == -external.id }).to eq([])
         end
       end
     end
@@ -267,13 +300,12 @@
         Course::ExternalAssessment.create_for_course!(course: course, title: 'Midterm',
                                                       maximum_grade: 50.0, weight: 40)
       end
-      let(:ta) { create(:course_teaching_assistant, course: course) }
 
       before do
         ctx = Struct.new(:current_course, :key).new(course, Course::GradebookComponent.key)
         Course::Settings::GradebookComponent.new(ctx).weighted_view_enabled = true
         course.save!
-        controller_sign_in(controller, ta.user)
+        controller_sign_in(controller, manager.user)
       end
 
       subject(:body) do
@@ -310,9 +342,7 @@
     end
 
     describe 'PATCH update_weights' do
-      let(:manager) { create(:course_manager, course: course) }
       let(:ta) { create(:course_teaching_assistant, course: course) }
-      let(:student) { create(:course_student, course: course) }
       let(:category) { create(:course_assessment_category, course: course) }
       let!(:tab1) { create(:course_assessment_tab, category: category) }
       let!(:tab2) { create(:course_assessment_tab, category: category) }
@@ -334,6 +364,21 @@ def weight_for(tab)
           expect(weight_for(tab2)).to eq(40)
         end
 
+        it 'accepts an empty weights array as a no-op' do
+          patch :update_weights, params: { course_id: course.id, weights: [] }, format: :json
+          expect(response).to have_http_status(:ok)
+          expect(JSON.parse(response.body)['weights']).to eq([])
+        end
+
+        it 'ignores keys outside the permitted weights schema' do
+          patch :update_weights,
+                params: { course_id: course.id,
+                          weights: [tabId: tab1.id, weight: 60, bogus: 'x'] },
+                format: :json
+          expect(response).to have_http_status(:ok)
+          expect(weight_for(tab1)).to eq(60)
+        end
+
         it 'accepts sum < 100' do
           patch :update_weights,
                 params: { course_id: course.id, weights: [tabId: tab1.id, weight: 30] },
@@ -462,13 +507,22 @@ def weight_for(tab)
           entry = JSON.parse(response.body)['weights'].first
           expect(entry['excludedAssessmentIds']).to eq([a1.id])
         end
+
+        it 'omits assessmentWeights from the echoed entry in equal mode' do
+          post :update_weights, as: :json, params: {
+            course_id: course.id,
+            weights: [tabId: tab.id, weight: '50', weightMode: 'equal']
+          }
+          expect(response).to have_http_status(:ok)
+          entry = JSON.parse(response.body)['weights'].first
+          expect(entry).not_to have_key('assessmentWeights')
+          expect(entry['weightMode']).to eq('equal')
+        end
       end
     end
 
     describe 'GET index — weighted view fields' do
       render_views
-      let(:manager) { create(:course_manager, course: course) }
-      let(:ta) { create(:course_teaching_assistant, course: course) }
       let(:category) { create(:course_assessment_category, course: course) }
       let!(:tab) { create(:course_assessment_tab, category: category) }
       let!(:contribution) { create(:course_gradebook_contribution, tab: tab, course: course, weight: 30) }
@@ -506,13 +560,6 @@ def weight_for(tab)
           expect(tab_json['gradebookWeight']).to eq(30)
         end
 
-        it 'returns canManageWeights false for TA' do
-          controller_sign_in(controller, ta.user)
-          get :index, params: { course_id: course.id }, format: :json
-          body = JSON.parse(response.body)
-          expect(body['canManageWeights']).to eq(false)
-        end
-
         it 'serializes weightMode on tabs and gradebookWeight on assessments when weighted view is enabled' do
           controller_sign_in(controller, manager.user)
           get :index, params: { course_id: course.id }, format: :json
@@ -534,7 +581,6 @@ def weight_for(tab)
 
     describe 'external assessment ordering' do
       render_views
-      let(:manager) { create(:course_manager, course: course) }
 
       it 'serializes externals in position order, not creation order' do
         first = create(:course_external_assessment, course: course, title: 'Zeta')

spec/models/course/gradebook_ability_spec.rb

@@ -13,6 +13,10 @@
       it { is_expected.to be_able_to(:read_gradebook, course) }
       it { is_expected.to be_able_to(:manage_gradebook_weights, course) }
       it { is_expected.to be_able_to(:manage_gradebook_settings, course) }
+      it 'cannot read the gradebook of a different course' do
+        other_course = create(:course)
+        expect(subject).not_to be_able_to(:read_gradebook, other_course)
+      end
     end
 
     context 'when the user is a Course Owner' do
@@ -21,12 +25,16 @@
       it { is_expected.to be_able_to(:read_gradebook, course) }
       it { is_expected.to be_able_to(:manage_gradebook_weights, course) }
       it { is_expected.to be_able_to(:manage_gradebook_settings, course) }
+      it 'cannot read the gradebook of a different course' do
+        other_course = create(:course)
+        expect(subject).not_to be_able_to(:read_gradebook, other_course)
+      end
     end
 
     context 'when the user is a Teaching Assistant' do
       let(:course_user) { create(:course_teaching_assistant, course: course) }
       let(:user) { course_user.user }
-      it { is_expected.to be_able_to(:read_gradebook, course) }
+      it { is_expected.not_to be_able_to(:read_gradebook, course) }
       it { is_expected.not_to be_able_to(:manage_gradebook_weights, course) }
       it { is_expected.not_to be_able_to(:manage_gradebook_settings, course) }
     end
@@ -38,5 +46,13 @@
       it { is_expected.not_to be_able_to(:manage_gradebook_weights, course) }
       it { is_expected.not_to be_able_to(:manage_gradebook_settings, course) }
     end
+
+    context 'when the user is a Course Observer' do
+      let(:course_user) { create(:course_observer, course: course) }
+      let(:user) { course_user.user }
+      it { is_expected.not_to be_able_to(:read_gradebook, course) }
+      it { is_expected.not_to be_able_to(:manage_gradebook_weights, course) }
+      it { is_expected.not_to be_able_to(:manage_gradebook_settings, course) }
+    end
   end
 end