feat(ssid): migrate to rails credentials, ssid multipart upload api

.gitignore

@@ -14,6 +14,12 @@
 # Ignore user-specific VSCode project files
 .vscode
 
+# Ignore credentials files which may contain sensitive information
+/config/credentials/*.key
+/config/credentials/*.yml.enc
+!/config/credentials/test.key
+!/config/credentials/test.yml.enc
+
 # Ignore the default SQLite database.
 /db/*.sqlite3
 /db/*.sqlite3-journal

Gemfile

@@ -199,7 +199,7 @@ gem 'filename'
 # Required by CarrierWave, for image resizing
 gem 'mini_magick'
 # Library for reading and writing zip files
-gem 'rubyzip', require: 'zip'
+gem 'rubyzip', '~> 3.0', require: 'zip'
 # Manipulating XML files, needed for programming evaluation test report parsing.
 gem 'nokogiri', '>= 1.18.8'
 

Gemfile.lock

@@ -216,9 +216,9 @@ GEM
     docker-api (2.4.0)
       excon (>= 0.64.0)
       multi_json
-    docx (0.9.1)
+    docx (0.10.0)
       nokogiri (~> 1.13, >= 1.13.0)
-      rubyzip (~> 2.0)
+      rubyzip (>= 2.0, < 4)
     domain_name (0.6.20240107)
     dotenv (3.1.7)
     dotenv-rails (3.1.7)
@@ -608,16 +608,16 @@ GEM
     ruby-vips (2.2.2)
       ffi (~> 1.12)
       logger
-    rubyzip (2.4.1)
+    rubyzip (3.2.2)
     sanitize (7.0.0)
       crass (~> 1.0.2)
       nokogiri (>= 1.16.8)
     securerandom (0.4.1)
-    selenium-webdriver (4.22.0)
+    selenium-webdriver (4.43.0)
       base64 (~> 0.2)
       logger (~> 1.4)
       rexml (~> 3.2, >= 3.2.5)
-      rubyzip (>= 1.2.2, < 3.0)
+      rubyzip (>= 1.2.2, < 4.0)
       websocket (~> 1.0)
     should_not (1.1.0)
     shoulda-matchers (7.0.1)
@@ -779,7 +779,7 @@ DEPENDENCIES
   rubocop-rails
   ruby-oembed
   ruby-openai
-  rubyzip
+  rubyzip (~> 3.0)
   rwordnet!
   sanitize (>= 4.6.3)
   settings_on_rails!

app/services/course/assessment/submission/base_zip_download_service.rb

@@ -42,7 +42,7 @@ def zip_file_path
   #
   # @return [String] The path to the zip file.
   def zip_base_dir
-    Zip::File.open(zip_file_path, Zip::File::CREATE) do |zip_file|
+    Zip::File.open(zip_file_path, create: true) do |zip_file|
       Dir["#{@base_dir}/**/**"].each do |file|
         zip_file.add(file.sub(File.join("#{@base_dir}/"), ''), file)
       end

app/services/course/assessment/submission/ssid_plagiarism_service.rb

@@ -32,7 +32,7 @@ def fetch_plagiarism_result(limit, offset)
   end
 
   def download_submission_pair_result(submission_pair_id)
-    ssid_api_service = SsidAsyncApiService.new(
+    ssid_api_service = SsidApiService.new(
       "submission-pairs/#{submission_pair_id}/report", {}
     )
     response_status, response_body = ssid_api_service.get
@@ -52,7 +52,7 @@ def share_assessment_result
   end
 
   def fetch_plagiarism_check_result
-    ssid_api_service = SsidAsyncApiService.new("folders/#{@main_assessment.ssid_folder_id}/plagiarism-checks", {})
+    ssid_api_service = SsidApiService.new("folders/#{@main_assessment.ssid_folder_id}/plagiarism-checks", {})
     response_status, response_body = ssid_api_service.get
     raise SsidError, { status: response_status, body: response_body } unless response_status == 200
 
@@ -67,13 +67,28 @@ def create_ssid_folders
     end
   end
 
+  def prepare_ssid_presigned_request(assessment)
+    ssid_api_service = SsidApiService.new("folders/#{assessment.ssid_folder_id}/uploads", {})
+    response_status, response_body = ssid_api_service.post
+    raise SsidError, { status: response_status, body: response_body } unless response_status == 200
+
+    upload_url = response_body['payload']['data']['url']
+    unless SsidApiService.upload_whitelist.match?(upload_url)
+      raise SsidError, { status: 500, body: 'Invalid presigned URL' }
+    end
+
+    [upload_url, response_body['payload']['data']['fields']]
+  end
+
   def run_upload_answers
     @linked_assessments.each do |assessment|
+      upload_url, upload_fields = prepare_ssid_presigned_request(assessment)
       service = Course::Assessment::Submission::SsidZipDownloadService.new(assessment)
       zip_files = service.download_and_zip
-      ssid_api_service = SsidAsyncApiService.new("folders/#{assessment.ssid_folder_id}/submissions", {})
+      ssid_api_service = SsidApiService.new('', {}, upload_url)
       zip_files.each do |zip_file|
-        response_status, response_body = ssid_api_service.post_multipart(zip_file)
+        form_data = upload_fields.merge('file' => Faraday::Multipart::FilePart.new(zip_file, 'application/zip'))
+        response_status, response_body = ssid_api_service.post_multipart(form_data)
         raise SsidError, { status: response_status, body: response_body } unless response_status == 204
       end
     ensure
@@ -82,7 +97,7 @@ def run_upload_answers
   end
 
   def send_plagiarism_check_request
-    ssid_api_service = SsidAsyncApiService.new("folders/#{@main_assessment.ssid_folder_id}/plagiarism-checks", {
+    ssid_api_service = SsidApiService.new("folders/#{@main_assessment.ssid_folder_id}/plagiarism-checks", {
       comparedFolderIds: @linked_assessments.pluck(:ssid_folder_id)
     })
     response_status, response_body = ssid_api_service.post
@@ -94,7 +109,7 @@ def ssid_submission_to_submission_id(ssid_submission)
   end
 
   def fetch_ssid_submission_pair_data(limit, offset)
-    ssid_api_service = SsidAsyncApiService.new(
+    ssid_api_service = SsidApiService.new(
       "folders/#{@main_assessment.ssid_folder_id}/plagiarism-checks/latest/submission-pairs",
       { limit: limit, offset: offset }
     )
@@ -105,7 +120,7 @@ def fetch_ssid_submission_pair_data(limit, offset)
   end
 
   def create_ssid_shared_resource_link(resource_type, resource_id)
-    ssid_api_service = SsidAsyncApiService.new('shared-resources', {
+    ssid_api_service = SsidApiService.new('shared-resources', {
       resourceType: resource_type,
       resourceId: resource_id
     })

app/services/course/assessment/submission/ssid_zip_download_service.rb

@@ -137,7 +137,7 @@ def zip_base_dir
     answer_partitions = partition_answers_by_size(answer_size_hash)
     @zip_files = answer_partitions.map.with_index do |partition, index|
       output_file = "#{@base_dir}_#{index}.zip"
-      Zip::File.open(output_file, Zip::File::CREATE) do |zip_file|
+      Zip::File.open(output_file, create: true) do |zip_file|
         partition.each do |answer_dir|
           Dir["#{answer_dir}/**/**"].each do |file|
             zip_file.add(file.sub(File.join("#{@base_dir}/"), ''), file)

app/services/course/material/zip_download_service.rb

@@ -39,7 +39,7 @@ def download_to_base_dir
   #
   # @return [String] The path to the zip file.
   def zip_base_dir
-    Zip::File.open(zip_file_path, Zip::File::CREATE) do |zip_file|
+    Zip::File.open(zip_file_path, create: true) do |zip_file|
       Dir["#{@base_dir}/**/**"].each do |file|
         zip_file.add(file.sub(File.join("#{@base_dir}/"), ''), file)
       end

app/services/course/ssid_folder_service.rb

@@ -5,7 +5,7 @@ def initialize(folder_name, parent_folder_id = nil)
   end
 
   def run_create_ssid_folder_service
-    ssid_api_service = SsidAsyncApiService.new('folders', @folder_object)
+    ssid_api_service = SsidApiService.new('folders', @folder_object)
     response_status, response_body = ssid_api_service.post
 
     # If id is lost in our DB somehow, we can recover it if SSID returns a 409

app/services/ssid_async_api_service.rb → app/services/ssid_api_service.rb

@@ -1,13 +1,20 @@
 # frozen_string_literal: true
 
-class SsidAsyncApiService
-  def config
-    ENV.fetch('SSID_URL')
+class SsidApiService
+  def self.api_url
+    Rails.application.credentials.ssid.url
   end
 
-  def initialize(api_namespace, payload)
+  # Validate that the URL is in the whitelist before allowing uploads to it.
+  # This is to prevent leaking data in case the SSID response is intercepted.
+  def self.upload_whitelist
+    Regexp.new(Rails.application.credentials.ssid.upload_whitelist_pattern)
+  end
+
+  def initialize(api_namespace, payload, url = nil)
     @api_namespace = api_namespace
     @payload = payload
+    @url = url || self.class.api_url
   end
 
   def post
@@ -20,8 +27,7 @@ def post
     [500, nil]
   end
 
-  def post_multipart(file_path)
-    form_data = { 'file' => Faraday::Multipart::FilePart.new(file_path, 'application/zip') }
+  def post_multipart(form_data)
     response = connection.post(@api_namespace) do |req|
       req.body = form_data
     end
@@ -42,8 +48,10 @@ def get
   private
 
   def connection
-    @connection ||= Faraday.new(url: config) do |builder|
-      builder.request :authorization, 'Bearer', -> { ENV.fetch('SSID_API_KEY', nil) }
+    @connection ||= Faraday.new(url: @url) do |builder|
+      if @url == self.class.api_url
+        builder.request :authorization, 'Bearer', -> { Rails.application.credentials.ssid.api_key }
+      end
       builder.request :multipart
     end
   end

config/credentials/README.md

@@ -11,7 +11,7 @@ Each environment has two files:
 - `<environment>.key`, the decryption key that allows reading the `.yml.enc` file. **Keys from deployed environments (staging and production) must NEVER BE COMMITTED.** Once leaked, all secrets in the environment will be compromised.
 - `<environment>.yml.enc`, an encrypted YAML file containing sensitive environment data (e.g. API keys). While theoretically safe to commit because the data is unreadable without the decryption key, we currently do not commit files containing real sensitive data as an additional safety measure.
 
-The `development` and `test` key files are checked into this repository, so local development works out of the box. The sample credentials use the same structure as staging/production but with redacted values, so external API integrations (e.g. Codaveri, AWS) will not function.
+The `test` key files are checked into this repository. The sample credentials use the same structure as other environments but with redacted values, so external API integrations (e.g. Codaveri, AWS) will not function.
 
 If you are a Coursemology team member who needs working credentials, contact current staff for the appropriate credentials file.
 

config/credentials/test.yml.enc

@@ -1 +1 @@
-22UauTnIrniRSCH/x4uVoj8HBcZJHb1ET04fxlhsYVsXPZ5vXG5mr+V5D5+F5iKv4pHNpAA7Osg6b/jWiG6t5uKPvlCwRL5j4aBi03bMIc5AMBH/8s2gEfvpWSZUC0viSNT4zwfJgKdgwHs4QhBg3EfkYxLe6S3R7W/RwX/hgW8pQB5Hw3bZhpbOtDuJeP/4Of4xg51PWP2hEz4K4IF9kTMKtjiGpprIoUQ0WFhe7l8cDBWsXBZdusCH4azQHkKo0A6YtuABe+Yaq5gBb7J2GqnVss89IJRp3TILfjgpFy8CrjGx1DpiY6csHFlvqbiOxJUjk+B0oqp2qIFOrGabcNEaGCT7tvNUEJwKb9WPYLTwfNW2EW4VgXAO7hM=--bb5c6OI9HMr/h3Jd--0p6ur3vWAdKRI3SYxGArog==
+ONmuV4CWoclCzxsWAbR+B0P9g7iT9wGrBGZBzNLfxp0S6lQ+kiJJrw8wV66vKDYxiODYdTFA4W+vL//7Dd3HAdMy1s+3qFxtEu0GvbFbHJDbu46uCQd/4UiVEpjHa0KeBwd2n6SpIUAZDmBbrtHoGKzxz1lP3M6vyd1g9D2vZguiExuH4JVDbF4jMYd4DQdlqLZL2Poa3r1Gwp/70mt4u2J+rNI1JshK0LFov9P3sAqWIDDbodaQXRDmp92eSNtdui5AmaeGtQjYryygE8njKE4OWhWAGmoRAk20hpiOZvvIc8DMtFxQQemrXZWFRdOkq2g/Lyy2MOMMSSM9dhoAOcPEjCEcSNDmkXddrefJPkEfWp+sXWC6590pbyx7Lvterl+u/bHMALhnUGdVkkKxrNVWAJ9xt1TOfR4TM6ThBZOPF8/x+VEnd0arhh3zW1pe8FJ2mZqFxGbL8VuOA/VPRzAszEq3u3bW5WY36AMIg7xgBplyBqQXDsTW7BCbSFHpX7Y+VwpEOj1lSyhoVGd11s/oWXXHZD/W9C/QbBiWHOe5cYJ/Ugvryhkob/6gkR1B5u6DWLOsQrokF5yMN33h3MwrKbhFYUcaH/D9hECJweVcW+zKUKBpPbI7smlaigBCFP1kUn4D4XoV4Lmr1A7Na+Cn2c3p7aVarPC7GJic4/pA0ZQKDL3/CKsTv24z44j0G4mDpT7Rp6SSYuIkb2mqufp3lqSgC/e7MkDHJ1lx5g3BmCCga0SeNaXxXvRNs05eDCewP69m58rFTS+JubhA8XrXl5Az8xovn3W9QgYR5Zp60gC7cVWigzCZTqpNBGranw3HecQ1N12hxZ/41u3ZLLz9WSIgWPI6WBQ5wXhyuFUBAD261Q==--RTxTAhlW3voVXY8u--+DWAPJUB5NgmSl6FAyhUYg==

lib/autoload/course/assessment/programming_package.rb

@@ -50,16 +50,16 @@ class Course::Assessment::ProgrammingPackage
   #
   # @overload initialize(path)
   #   @param [String|Pathname] path The path to the package on disk.
-  # @overload initialize(stream)
-  #   @param [IO] stream The stream to the file.
-  def initialize(path_or_stream)
-    case path_or_stream
+  # @overload initialize(file)
+  #   @param [File] file The file object.
+  def initialize(path_or_file)
+    case path_or_file
     when String, Pathname
-      @path = path_or_stream
-    when IO
-      @stream = path_or_stream
+      @path = path_or_file
+    when File
+      @file_stream_obj = path_or_file
     else
-      raise ArgumentError, 'Invalid path or stream object'
+      raise ArgumentError, 'Invalid path or File object'
     end
   end
 
@@ -72,8 +72,8 @@ def path
       @file.name
     elsif @path
       @path.to_s
-    elsif @stream.is_a?(File)
-      @stream.path
+    elsif @file_stream_obj
+      @file_stream_obj.path
     end
   end
 
@@ -170,8 +170,8 @@ def unzip_file(destination)
     ensure_file_open!
     @file.each do |entry|
       entry_path = File.join(destination, entry.name)
-      FileUtils.mkdir_p(File.dirname(entry_path))
-      @file.extract(entry, entry_path) unless File.exist?(entry_path)
+      FileUtils.mkdir_p(destination)
+      @file.extract(entry, destination_directory: destination) unless File.exist?(entry_path)
     end
   end
 
@@ -219,10 +219,8 @@ def ensure_file_open!
 
     if @path
       @file = Zip::File.open(@path.to_s)
-    elsif @stream
-      @file = Zip::File.new(@stream&.path, true)
-      @file.read_from_stream(@stream)
-      @file.instance_variable_set(:@stored_entries, @file.instance_variable_get(:@entry_set).dup)
+    elsif @file_stream_obj
+      @file = Zip::File.open_buffer(@file_stream_obj)
     end
     raise IllegalStateError unless @file
   end

spec/libraries/course/assessment/programming_package_spec.rb

@@ -11,13 +11,9 @@
                                        'empty_programming_question_template.zip')
 
   def temp_package_path
-    temp_package_stream.tap(&:close).path
-  end
-
-  def temp_package_stream
     package_path = Rails.application.config.x.temp_folder.join('spec/packages')
     FileUtils.mkdir_p(package_path) unless Dir.exist?(package_path)
-    Tempfile.create('programming_package', package_path)
+    Tempfile.create('programming_package', package_path).tap(&:close).path
   end
 
   def open_package(path)
@@ -34,14 +30,8 @@ def open_package(path)
       end
     end
 
-    context 'when a file stream is specified' do
-      let(:package_stream) { temp_package_stream }
-      subject { open_package(package_stream) }
-      before do
-        IO.copy_stream(self.class::PACKAGE_PATH, package_stream)
-        package_stream.seek(0)
-      end
-
+    context 'when a File object is specified' do
+      let(:package_path) { File.new(self.class::PACKAGE_PATH, 'rb') }
       it 'opens the file' do
         expect(subject.submission_files).not_to be_empty
       end

spec/rails_helper.rb

@@ -13,7 +13,6 @@
 # We use a dummy value since leaving it as null raises an error, and potentially
 # in the future we can use it in stub logic to differentiate from other external APIs.
 ENV['CODAVERI_URL'] ||= 'http://localhost:53896'
-ENV['SSID_URL'] ||= 'http://localhost:53897'
 
 require 'spec_helper'
 require 'rspec/rails'