FriendsOfFlarum / upload

The file upload extension with insane intelligence for your Flarum forum.
https://discuss.flarum.org/d/4154
MIT License
175 stars 94 forks source link

Adaptor mapping for custom S3 endpoint is incorrect #328

Closed rongcuid closed 3 weeks ago

rongcuid commented 1 year ago

In setting up Upload for a self-hosted Minio S3 endpoint, I find the URL mapping is incorrect.

Bug Report

Current Behavior The adaptor mapping generates URL to amazon AWS, instead of my custom endpoint.

Steps to Reproduce

  1. Configure adaptor to Mime ^image\/.*, S3/Compatible, Just URL
  2. Set up S3 storage settings. I used a valid service key/secret from my Minio instance, bucket flarum, and region home (which is definitely not an AWS region)
  3. Set endpoint to my Minio instance, in my case localhost:9000. This is tested to work using curl.
  4. Try to upload an image

Expected Behavior

Generate a URL like localhost:9000/flarum/path/to/image

Actual Behavior

Generated a URL to AWS

Screenshots

Screenshot from 2022-09-18 15-01-12

Environment

$ php flarum info
Flarum core 1.5.0
PHP version: 8.1.2
MySQL version: 5.5.5-10.6.7-MariaDB-2ubuntu1.1
Loaded extensions: Core, date, libxml, openssl, pcre, zlib, filter, hash, json, pcntl, Reflection, SPL, session, standard, sodium, mysqlnd, PDO, xml, calendar, ctype, curl, dom, mbstring, FFI, fileinfo, ftp, gd, gettext, iconv, intl, exif, mysqli, pdo_mysql, pdo_pgsql, pgsql, Phar, posix, readline, shmop, SimpleXML, soap, sockets, sysvmsg, sysvsem, sysvshm, tokenizer, xmlreader, xmlrpc, xmlwriter, xsl, zip, Zend OPcache
+----------------------+---------+--------+
| Flarum Extensions    |         |        |
+----------------------+---------+--------+
| ID                   | Version | Commit |
+----------------------+---------+--------+
| flarum-flags         | v1.5.0  |        |
| fof-upload           | 1.2.3   |        |
| flarum-tags          | v1.5.0  |        |
| flarum-suspend       | v1.5.0  |        |
| flarum-subscriptions | v1.5.0  |        |
| flarum-sticky        | v1.5.0  |        |
| flarum-statistics    | v1.5.0  |        |
| flarum-mentions      | v1.5.0  |        |
| flarum-markdown      | v1.5.0  |        |
| flarum-lock          | v1.5.0  |        |
| flarum-likes         | v1.5.0  |        |
| flarum-lang-english  | v1.5.0  |        |
| flarum-emoji         | v1.5.0  |        |
| flarum-bbcode        | v1.5.0  |        |
| flarum-approval      | v1.5.0  |        |
+----------------------+---------+--------+
Base URL: https://home.local/flarum
Installation path: /var/www/flarum
Queue driver: sync
Mail driver: mail
Debug mode: ON

Don't forget to turn off debug mode! It should never be turned on in a production system.

Possible solution(s)

Use custom endpoint when generating the URL

Additional Context Add any other context about the problem here.

rongcuid commented 1 year ago

Relavent code under src/Adapters/AwsS3.php:

    protected function generateUrl(File $file)
    {
        /** @var SettingsRepositoryInterface $settings */
        $settings = resolve(SettingsRepositoryInterface::class);

        $cdnUrl = $settings->get('fof-upload.cdnUrl');

        if (!$cdnUrl) {
            $region = $this->adapter->getClient()->getRegion();
            $bucket = $this->adapter->getBucket();

            $cdnUrl = sprintf('https://%s.s3.%s.amazonaws.com', $bucket, $region ?: 'us-east-1');
        }

        $file->url = sprintf('%s/%s', $cdnUrl, Arr::get($this->meta, 'path', $file->path));
    }

It might be a bad idea to use fof-upload.cdnUrl, because I think this is the setting for Local adapter. In addition, path-style endpoint does not have an effect here.

Now, my Minio is behind a reverse proxy. So the backend accesses through localhost:9000, but the frontend accesses through home.local/minio. I would suggest adding a setting that allows something like $frontend_endpoint/$bucket/$path.

I also had problem when I omit regions (which might not be set up in Minio), but this may be a separate issue.

clarkwinkelmann commented 1 year ago

It might be a bad idea to use fof-upload.cdnUrl

Are you using both local and s3 at the same time?


Regarding the wrong URL issues I'm sure we will need to overhaul the S3 adapter at some point, it's not the first issue reported to us and I'm not sure yet how we can fix it to accommodate all "compatible" S3 services.

The workaround in the meantime would be to create a custom adapter, you could copy the S3 adapter code in it and hard-code the values.

luceos commented 1 year ago

The driver is accurately named AwsS3 not S3Compatible or similar. So I think the adapter does what it needs to do.

What is reported:

I think that we do need a s3 compatible adapter at some point. We also talked about allowing a CDN Url per adapter. As both of these aren't available right now the best solution would be to create your own Adapter in your Flarum install or as an extension by listening to the Collecting event or by extending the Adapter Manager. In this case we should also improve extensibility a bit in a future revision.

rongcuid commented 1 year ago

It might be a bad idea to use fof-upload.cdnUrl

Are you using both local and s3 at the same time?

Regarding the wrong URL issues I'm sure we will need to overhaul the S3 adapter at some point, it's not the first issue reported to us and I'm not sure yet how we can fix it to accommodate all "compatible" S3 services.

The workaround in the meantime would be to create a custom adapter, you could copy the S3 adapter code in it and hard-code the values.

No, but it is under local's setting, so I assumed it affects local adapter only until I saw the code. I am also unsure whether local and S3 can be used simultaneously, because I see that I can set adapter based on MIME type. Certainly there is a use case where different adapters use different CDN url, right...?

I am new to S3, so I have to figure out a lot of things myself, so I cannot say for sure what to change, except maybe set URL to $endpoint/bucket/path if $endpoint is set.

I am also new to Flarum and its ecosystem, so I have not looked into how to extending an extension yet.

jeff-h commented 1 year ago

I'm a little confused; the config UI contains fields described as such "The following settings are only required when using S3 compatible storage. If you use AWS, you can leave them blank."

This obviously implies S3-compatible storage does work, but the comments on this issue suggest otherwise. In my experience, it does indeed work, with the single exception of the bug relating to the URL generation.

jeff-h commented 3 months ago

I've had another crack at getting my install to work, and thanks to the code snippet posted above by @rongcuid I tried putting my Backblaze "S3 Url" into FoF Upload's "Local storage settings: Content Delivery URL (prefixes files)" config, and downloading works now! (I don't use local storage, as I'm with FreeFlarum.)

In Backblaze:

In the FoF Upload config: