Restrict what lesson params can be updated

Previously any params could be updated in lessons which was dangerous - we don't expect many of these params to be updated to changing them could create odd behaviour or allow other data to be accessed.

I've checked the frontend, and only the name and visibility can be updated.

I've done this now so that users can't turn their projects into Scratch projects unexpectedly.

We could add attributes back in in the future if we want to.

Author: zetter-rpf

app/controllers/api/lessons_controller.rb

@@ -30,7 +30,7 @@ def show
     end
 
     def create
-      result = Lesson::Create.call(lesson_params:)
+      result = Lesson::Create.call(lesson_params: create_params)
 
       if result.success?
         @lesson_with_user = result[:lesson].with_user
@@ -41,7 +41,7 @@ def create
     end
 
     def create_copy
-      result = Lesson::CreateCopy.call(lesson: @lesson, lesson_params:)
+      result = Lesson::CreateCopy.call(lesson: @lesson, lesson_params: create_params)
 
       if result.success?
         @lesson_with_user = result[:lesson].with_user
@@ -54,7 +54,7 @@ def create_copy
     def update
       # TODO: Consider removing user_id from the lesson_params for update so users can update other users' lessons without changing ownership
       # OR consider dropping user_id on lessons and using teacher id/ids on the class instead
-      result = Lesson::Update.call(lesson: @lesson, lesson_params:)
+      result = Lesson::Update.call(lesson: @lesson, lesson_params: update_params)
 
       if result.success?
         @lesson_with_user = result[:lesson].with_user
@@ -78,8 +78,8 @@ def filtered_lessons_scope
     end
 
     def verify_school_class_belongs_to_school
-      return if base_params[:school_class_id].blank?
-      return if school&.classes&.pluck(:id)&.include?(base_params[:school_class_id])
+      return if create_params[:school_class_id].blank?
+      return if school&.classes&.pluck(:id)&.include?(create_params[:school_class_id])
 
       raise ParameterError, 'school_class_id does not correspond to school_id'
     end
@@ -105,14 +105,20 @@ def user_remix(lesson)
     end
 
     def scratch_project?
-      base_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH
+      create_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH
     end
 
-    def lesson_params
-      base_params.merge(user_id: current_user.id)
+    def update_params
+      params.fetch(:lesson, {}).permit(
+        :name,
+        :visibility,
+        {
+          project_attributes: [:name]
+        }
+      )
     end
 
-    def base_params
+    def create_params
       params.fetch(:lesson, {}).permit(
         :school_id,
         :school_class_id,
@@ -129,15 +135,15 @@ def base_params
             { scratch_component: {} }
           ]
         }
-      )
+      ).merge(user_id: current_user.id)
     end
 
     def school_owner?
       school && current_user.school_owner?(school)
     end
 
     def school
-      @school ||= @lesson&.school || School.find_by(id: base_params[:school_id]) || SchoolClass.find_by(id: params[:school_class_id])&.school
+      @school ||= @lesson&.school || School.find_by(id: create_params[:school_id]) || SchoolClass.find_by(id: params[:school_class_id])&.school
     end
   end
 end

spec/features/lesson/creating_a_lesson_spec.rb

@@ -94,6 +94,7 @@
       new_params = { lesson: params[:lesson].merge(user_id: 'ignored') }
 
       post('/api/lessons', headers:, params: new_params)
+      expect(response).to have_http_status(:created)
       data = JSON.parse(response.body, symbolize_names: true)
 
       expect(data[:user_id]).to eq(teacher.id)

spec/features/lesson/updating_a_lesson_spec.rb

@@ -117,15 +117,17 @@
       expect(response).to have_http_status(:ok)
     end
 
-    it 'responds 422 Unprocessable Entity when trying to re-assign the lesson to a different class' do
+    it 'does not allow re-assigning the lesson to a different class' do
       school = create(:school, id: SecureRandom.uuid)
       teacher = create(:teacher, school:)
       school_class = create(:school_class, school:, teacher_ids: [teacher.id])
 
       new_params = { lesson: params[:lesson].merge(school_class_id: school_class.id) }
       put("/api/lessons/#{lesson.id}", headers:, params: new_params)
+      expect(response).to have_http_status(:ok)
 
-      expect(response).to have_http_status(:unprocessable_content)
+      lesson.reload
+      expect(lesson.school_class_id).not_to eq(school_class.id)
     end
   end
 end