Skip to content

Simplify multipart spec

David Fernandez requested to merge 10io-cleanup-multipart-uploaded-file-specs into master

🤔 What does this MR do?

Related to !33277 (comment 399666499)

https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/lib/gitlab/middleware/multipart_spec.rb is quite hard to get for new readers. This MR aims to refactor the file to have a better code organization and give a better clarity on what's going on.

Please note, that we introduced feature tests a few days ago, so this file has already a good coverage.

⚔ Design choices

  • The multipart.rb is responsible to read the upload params from Workhorse and transform them into an UploadedFile. There are 3 aspects that need to be tested:
    • The parameters can be sent in two "modes"
      • local: in this mode, the uploaded file is in the file system, a path parameter indicates its location
      • remote: in this mode, the uploaded file has been put in object storage by Workhorse and here we receive the remote_id, which is simply the object storage key.
    • The parameters can be at the root of the request or nested. Imagine that the file upload is a field of a form, the param name can be:
      • file. That's a "root" parameter
      • user[avatar]. That's a nested parameter
      • user[friend][avatar]. That's a deeply nested parameter.
      • The upload request can come with multiple files, we can even have a mix of parameters at different nested levels.
    • In local mode, the path that is sent is verified against a known list of allowed directories.
  • You can imagine that setup code for those tests is quite heavy, to help us with the de-clutter the multipart_spec.rb file, this MR uses 3 auxiliary files:
    • MultipartHelpers A spec helper that has a collection of functions to help us:
      • #post_env: builds a Rack::MockRequest that will be sent to the middleware
      • #upload_parameters_for: builds the parameters set for a given upload
      • #expect_uploaded_files: puts expectations on the double to verify the params -> UploadedFile transformation.
    • spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb A collection of shared contexts. Their main aim is to build a temporary file and keep care of its deletion at the end of the example run.
      • with one temporary file for multipart: this context creates one temporary file. See the comments in the code for more details
      • with two temporary files for multipart: creates two temporary files
      • with three temporary files for multipart: creates three temporary files
      • All the contexts setup some functions so that the test code can access the temporary files.
    • spec/support/shared_examples/lib/gitlab/middleware/multipart_shared_examples.rb provides the following:
      • handling all upload parameters conditions shared example giving a full set of examples with different parameters conditions (nested or not, single or multiple)
    • spec/lib/gitlab/middleware/multipart/handler_spec.rb for the following:
      • Check the allowed_paths
      • Check that under different conditions, the packages uploader path is included or not
  • We use let vars instead of let_it_be for two reasons:
    • We mainly use Strings and Hashes only. Using let_it_be will not bring the usual benefits
    • All these vars should be evaluated at "example run" time. If we keep the var state between example runs, we can run into issues.
  • To simplify things further, the Dir.tmpdir directory is now accepted directly by the multipart Handler instead of the UploadedFile.from_params function.
  • All the examples from the original spec have been re-used/re-implemented except this one where we stub all the calls on the file system. I don't fully understand what we're trying to do here, if we stub all the file system calls, what are we testing? 🤷
  • More examples have been added:
    • added an example where the JWT header would point to a wrong parameter name.

🗒 Notes to reviewers/maintainers

  • You might have an easier time reviewing the whole file than the diff.

Screenshots

n / a

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by David Fernandez

Merge request reports