Merge pull request doubtfire-lms#517 from coskun-kilinc/fix/numbas-username-route

Fix/numbas username route

Author: b0ink

app/api/scorm_api.rb

@@ -52,7 +52,9 @@ def stream_file_from_zip(zip_path, file_path)
   params do
     requires :task_def_id, type: Integer, desc: 'Task Definition ID to get SCORM test data for'
   end
-  get '/scorm/:task_def_id/:username/:auth_token/*file_path' do
+
+  # NOTE: allow '.' in username; without this Rails/Grape treats '.' as format.
+  get '/scorm/:task_def_id/:username/:auth_token/*file_path', requirements: { username: %r{[^/]+} } do
     task_def = TaskDefinition.find(params[:task_def_id])
 
     unless authorise? current_user, task_def.unit, :get_unit

test/api/scorm_api_test.rb

@@ -10,7 +10,10 @@ def app
   end
 
   def scorm_path(task_def, user, file, token_type = :scorm)
-    "/api/scorm/#{task_def.id}/#{user.username.gsub('.', '%2e')}/#{auth_token(user, token_type)}/#{file}"
+    # NOTE: Do not encode '.' to '%2e' here. That hid the bug because the request worked regardless
+    # of the route constraint. Keeping the raw username ensures tests fail without the fix and pass
+    # with it.
+    "/api/scorm/#{task_def.id}/#{user.username}/#{auth_token(user, token_type)}/#{file}"
   end
 
   def test_serve_scorm_content
@@ -86,4 +89,22 @@ def test_serve_scorm_content
     td.destroy!
     unit.destroy!
   end
+
+  def test_username_with_dot_fails_without_fix
+    unit = FactoryBot.create(:unit)
+    user = unit.projects.first.student
+    user.update!(username: "user.name")
+
+    td = FactoryBot.create(:task_definition,
+      unit: unit,
+      tutorial_stream: unit.tutorial_streams.first,
+      scorm_enabled: true
+    )
+    td.add_scorm_data(test_file_path("numbas.zip"), copy: true)
+    td.save!
+
+    # This should fail without the route fix (Rails interprets '.' as format separator)
+    get scorm_path(td, user, "index.html")
+    assert_equal 200, last_response.status
+  end
 end