S3 pre-signed URL: error with unicode filename

[PhucNguyen0311]

Hi, I got an issue relative to S3 storage with Unicode filename.

Currently, the CloudFront pre-signed method (generate_presigned_url_cloudfront) uses a function to get a nice filename but S3 isn't. So, I think it would be good if S3 pre-signed function (generate_presigned_url_s3) has the same method.

[marxjohnson]

@PhucNguyen0311 This looks like the right approach to me, however it introduces some duplicated logic. As you say, the cloudfront and s3 signing methods should do the same thing, so it would be good if they both did this by calling get_nice_filename(). Perhaps you could refactor that function slightly so that it returns the data in a format that can be used by both functions? This may require a small change to the cloudfront method too. For example, get_nice_filename() could return an array of content-disposition, filename and content-type, then generate_presigned_url_cloudfront() could use that to construct its query string while generate_presigned_url_s3() could use to it to construct its parameters.

[PhucNguyen0311]

@marxjohnson Thank you for your advice. I updated the code for this fix. Please help me to review and let me know if you have any concerns.

[marxjohnson]

@PhucNguyen0311 This fix looks good to me, @brendanheywood what do you think?

[almadog]

Bumping this up - have been experiencing the same issue since we enabled s3 pre-signed urls.

[brendanheywood]

@PhucNguyen0311 can you please rebase your patch

[marxjohnson]

@brendanheywood I tried to contact @PhucNguyen0311 but didn't get a response, so I have done the rebase and created a new PR: #493

[tuanngocnguyen]

Hi @marxjohnson

FYI, the commit "Set utf-8 encoding in signed URL filename header" is causing 403 error, so I create MRs to revert the commit.

[tuanngocnguyen]

Hi @marxjohnson ,

I assume you are working on this, please let me know if it is not the case. Thanks

[marxjohnson]

Hi @tuanngocnguyen, Thanks for addressing that, I have been on holiday so not had time to look at this further. That commit was just an attempt to tidy things up, if it was causing problems I believe the original issue should still be resolved without it. I'm not sure that the utf8_encode() that was originally there is correct (we are starting with a UTF-8 string, so why encode it as UTF-8 again?) but in my testing it seemed to work as expected whether the filename was ISO-8859-1 only or UTF-8.

If we are happy that the original issue is resolved by Phuc's commit, then I would suggest we leave it there for now, if necessary we could open a separate issue for tidying up the Content-Disposition header and test it more thoroughly.

[tuanngocnguyen]

Thanks @marxjohnson, let's wait for Brendan to review the issue.

[marxjohnson]

We did also find an issue with MySQL compatability, so I have raised another PR to fix that: https://github.com/catalyst/moodle-tool_objectfs/pull/503