fix tests for https://github.com/matrix-org/synapse/pull/2090

tests/51media/01unicode.pl

@@ -1,3 +1,4 @@
+use HTTP::Headers::Util qw( split_header_words );
 use URI::Escape qw( uri_escape );
 use SyTest::TCPProxy;
 
@@ -40,36 +41,115 @@
 );
 
 
-test "Can upload with Unicode file name",
-   requires => [ $main::API_CLIENTS[0], local_user_fixture(),
-                 qw( can_upload_media )],
+=head2 upload_test_content
 
-   proves => [qw( can_upload_media_unicode )],
+   my ( $content_id, $content_uri ) = upload_test_content(
+      $user, filename => "filename",
+   ) -> get;
 
-   do => sub {
-      my ( $http, $user ) = @_;
+Uploads some test content with the given filename.
 
-      $http->do_request(
-         method       => "POST",
-         full_uri     => "/_matrix/media/r0/upload",
-         content      => "Test media file",
-         content_type => "text/plain",
-
-         params => {
-            access_token => $user->access_token,
-            filename     => $FILENAME,
-         }
-      )->then( sub {
-         my ( $body ) = @_;
+Returns the content id of the uploaded content.
+
+=cut
+sub upload_test_content
+{
+   my ( $user, %params ) = @_;
+
+   $user->http->do_request(
+      method       => "POST",
+      full_uri     => "/_matrix/media/r0/upload",
+      content      => "Test media file",
+      content_type => "text/plain",
+
+      params => {
+         access_token => $user->access_token,
+         %params,
+      },
+   )->then( sub {
+      my ( $body ) = @_;
+
+      assert_json_keys( $body, qw( content_uri ));
+
+      my $content_uri = $body->{content_uri};
+
+      my $parsed_uri = URI->new( $body->{content_uri} );
+      my $server = $parsed_uri->authority;
+      my $path = $parsed_uri->path;
+
+      my $content_id = "$server$path";
+
+      Future->done( $content_id, $content_uri );
+   });
+}
+push our @EXPORT, qw( upload_test_content );
+
+
+=head2 get_media
+
+   my ( $content_disposition_params, $content ) = get_media( $http, $content_id ) -> get;
+
+Fetches a piece of media from the server.
+
+=cut
+sub get_media
+{
+   my ( $http, $content_id ) = @_;
+
+   $http->do_request(
+      method   => "GET",
+      full_uri => "/_matrix/media/r0/download/$content_id",
+   )->then( sub {
+      my ( $body, $response ) = @_;
+
+      my $disposition = $response->header( "Content-Disposition" );
+
+      my $cd_params;
+      if ( defined $disposition ) {
+         $cd_params = parse_content_disposition_params( $disposition );
+      }
+      Future->done( $cd_params, $body );
+   });
+}
+push @EXPORT, qw( get_media );
+
+sub parse_content_disposition_params {
+   my ( $disposition ) = @_;
+   my @parts = split_header_words( $disposition );
+
+   # should be only one list of words
+   assert_eq( scalar @parts, 1, "number of content-dispostion header lists" );
+   @parts = @{$parts[0]};
+
+   # the first part must be 'inline'
+   my $k = shift @parts;
+   my $v = shift @parts;
+   assert_eq( $k, "inline", "content-disposition" );
+   die "invalid CD" if defined $v;
+
+   my %params;
+   while (@parts) {
+      my $k = shift @parts;
+      my $v = shift @parts;
+      die "multiple $k params" if exists $params{$k};
+      die "unknown param $k" unless ( $k eq 'filename' || $k eq 'filename*' );
+      $params{$k} = $v;
+   }
+   return \%params;
+}
 
-         assert_json_keys( $body, qw( content_uri ));
 
-         my $content_uri = URI->new( $body->{content_uri} );
-         my $server = $content_uri->authority;
-         my $path = $content_uri->path;
+test "Can upload with Unicode file name",
+   requires => [ local_user_fixture(),
+                 qw( can_upload_media )],
+
+   proves => [qw( can_upload_media_unicode )],
 
-         $content_id = "$server$path";
+   do => sub {
+      my ( $user ) = @_;
 
+      upload_test_content( $user, filename=>$FILENAME )->then( sub {
+         ( $content_id ) = @_;
          Future->done(1)
       });
    };
@@ -85,16 +165,9 @@ sub test_using_client
        $content = $content_id;
    }
 
-   $client->do_request(
-      method   => "GET",
-      full_uri => "/_matrix/media/r0/download/$content",
-   )->then( sub {
-      my ( $body, $response ) = @_;
-
-      my $disposition = $response->header( "Content-Disposition" );
-      uc $disposition eq uc "inline; filename*=utf-8''$FILENAME_ENCODED" or
-         die "Expected a UTF-8 filename parameter";
-
+   get_media( $client, $content )->then( sub {
+      my ( $cd_params ) = @_;
+      assert_eq( $cd_params->{'filename*'}, "utf-8''$FILENAME_ENCODED", "filename*" );
       Future->done(1);
    });
 }

tests/51media/02nofilename.pl

@@ -1,31 +1,13 @@
 my $content_id;
 
 test "Can upload without a file name",
-   requires => [ $main::API_CLIENTS[0], local_user_fixture() ],
+   requires => [ local_user_fixture() ],
 
    do => sub {
-      my ( $http, $user ) = @_;
-
-      $http->do_request(
-         method       => "POST",
-         full_uri     => "/_matrix/media/r0/upload",
-         content      => "Test media file",
-         content_type => "text/plain",
-
-         params => {
-            access_token => $user->access_token,
-         }
-      )->then(sub {
-         my ( $body ) = @_;
-
-         assert_json_keys( $body, qw( content_uri ));
-
-         my $content_uri = URI->new( $body->{content_uri} );
-         my $server = $content_uri->authority;
-         my $path = $content_uri->path;
-
-         $content_id = "$server$path";
+      my ( $user ) = @_;
 
+      upload_test_content( $user, )->then( sub {
+         ( $content_id ) = @_;
          Future->done(1)
       });
    };
@@ -37,13 +19,9 @@ sub test_using_client
 {
    my ( $client ) = @_;
 
-   $client->do_request(
-      method   => "GET",
-      full_uri => "/_matrix/media/r0/download/$content_id",
-   )->then( sub {
-      my ( $body, $response ) = @_;
+   get_media( $client, $content_id )->then( sub {
+      my ( $disposition ) = @_;
 
-      my $disposition = $response->header( "Content-Disposition" );
       defined $disposition and
          die "Unexpected Content-Disposition header";
 

tests/51media/03ascii.pl

@@ -2,80 +2,67 @@
 my $content_uri;
 
 test "Can upload with ASCII file name",
-   requires => [ $main::API_CLIENTS[0], local_user_fixture() ],
+   requires => [ local_user_fixture() ],
 
    do => sub {
-      my ( $http, $user ) = @_;
-
-      $http->do_request(
-         method       => "POST",
-         full_uri     => "/_matrix/media/r0/upload",
-         content      => "Test media file",
-         content_type => "text/plain",
-
-         params => {
-            access_token => $user->access_token,
-            filename => "ascii",
-         }
-      )->then( sub {
-         my ( $body ) = @_;
-
-         assert_json_keys( $body, qw( content_uri ));
-
-         $content_uri = $body->{content_uri};
-
-         my $parsed_uri = URI->new( $body->{content_uri} );
-         my $server = $parsed_uri->authority;
-         my $path = $parsed_uri->path;
-
-         $content_id = "$server$path";
-
-         Future->done(1)
+      my ( $user ) = @_;
+      upload_test_content( $user, filename=>"ascii" )->then( sub {
+         ( $content_id, $content_uri ) = @_;
+         Future->done(1);
       });
    };
 
-# These next two tests do the same thing with two different HTTP clients, to
-# test locally and via federation
+# we only need one user for these tests
+my $user_fixture = local_user_fixture();
 
-sub test_using_client
-{
-   my ( $client ) = @_;
+sub assert_cd_params_match_filename {
+   my ( $filename, $cd_params ) = @_;
 
-   $client->do_request(
-      method   => "GET",
-      full_uri => "/_matrix/media/r0/download/$content_id",
-   )->then( sub {
-      my ( $body, $response ) = @_;
+   # either we need a valid "filename*" param
+   if ( $cd_params->{"filename*"} ) {
+      my $f = $cd_params->{"filename*"};
+      $f =~ s/%(..)/chr hex $1/eg;
+      assert_eq( $f, "utf-8''$filename", "filename*" );
 
-      my $disposition = $response->header( "Content-Disposition" );
-      $disposition eq "inline; filename=ascii" or
-         die "Expected an ASCII filename parameter";
+      # there might also be a 'filename', but it doesn't really matter what it
+      # is.
+      return;
+   }
 
-      Future->done(1);
-   });
+   # or we need a valid filename
+   my $f = $cd_params->{"filename"};
+   assert_eq( $f, $filename, "filename" );
 }
 
-test "Can download with ASCII file name locally",
-   requires => [ $main::API_CLIENTS[0] ],
-
-   check => sub {
-      my ( $http ) = @_;
-      test_using_client( $http )
-      ->then( sub {
-         test_using_client( $http )
-      });
-   };
-
-test "Can download with ASCII file name over federation",
-   requires => [ $main::API_CLIENTS[1] ],
+foreach my $filename ( "ascii", "name with spaces", "name;with;semicolons" ) {
+   test "Can download file '$filename'",
+      requires => [
+         $user_fixture, $main::API_CLIENTS[1]
+      ],
+
+      check => sub {
+         my ( $user, $federation_client ) = @_;
+
+         my $content_id;
+
+         # first upload the content with the given filename
+         upload_test_content( $user, filename=>$filename )->then( sub {
+            ( $content_id ) = @_;
+
+            # try and fetch it as a local user
+            get_media( $user->http, $content_id );
+         })->then( sub {
+            assert_cd_params_match_filename( $filename, $_[0] );
+
+            # do the same over federation
+            get_media( $federation_client, $content_id );
+         })->then( sub {
+            assert_cd_params_match_filename( $filename, $_[0] );
+            Future->done(1);
+         });
+      };
+}
 
-   check => sub {
-      my ( $http ) = @_;
-      test_using_client( $http )
-      ->then( sub {
-         test_using_client( $http )
-      });
-   };
 
 test "Can download specifying a different ASCII file name",
    requires => [ $main::API_CLIENTS[0] ],
@@ -102,25 +89,19 @@ sub test_using_client
 
    check => sub {
       my ( $http, $user, $room_id ) = @_;
-      test_using_client( $http )
-      ->then( sub {
-         matrix_send_room_message( $user, $room_id,
-            content => { msgtype => "m.file", body => "test.txt", url => $content_uri }
-         )
-      });
+      matrix_send_room_message( $user, $room_id,
+         content => { msgtype => "m.file", body => "test.txt", url => $content_uri }
+      );
    };
 
 test "Can fetch images in room",
    requires => [ $main::API_CLIENTS[0], local_user_and_room_fixtures() ],
 
    check => sub {
       my ( $http, $user, $room_id ) = @_;
-      test_using_client( $http )
-      ->then( sub {
-         matrix_send_room_message_synced( $user, $room_id,
-            content => { msgtype => "m.text", body => "test" }
-         )
-      })->then( sub {
+      matrix_send_room_message_synced( $user, $room_id,
+         content => { msgtype => "m.text", body => "test" }
+      )->then( sub {
          matrix_send_room_message_synced( $user, $room_id,
             content => { msgtype => "m.file", body => "test.txt", url => $content_uri }
          )