Implement MSC3916 #3397
Implement MSC3916 #3397
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3397 +/- ##
==========================================
- Coverage 68.24% 68.10% -0.15%
==========================================
Files 513 513
Lines 46865 47176 +311
==========================================
+ Hits 31983 32127 +144
- Misses 10891 11045 +154
- Partials 3991 4004 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Complement PR: matrix-org/complement#731 |
mediaapi/routing/download.go
Outdated
| // create request for remote file | ||
| resp, err := client.CreateMediaDownloadRequest(ctx, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) | ||
| // Attempt to download via authenticated media endpoint | ||
| isMultiPart := true |
There was a problem hiding this comment.
I find the name isMutiPart a bit confusing. After some digging in the spec I think I understand where it comes from, but a `isAuthed' or something would be easier to understand.
There was a problem hiding this comment.
Agreed. It's an implementation detail that the authed endpoint needs multipart, so prefer isAuthed right up until you're forced to process the response.
| if resp != nil && resp.StatusCode == http.StatusNotFound { | ||
| return "", false, fmt.Errorf("File with media ID %q does not exist on %s", r.MediaMetadata.MediaID, r.MediaMetadata.Origin) | ||
| isMultiPart = false | ||
| // try again on the unauthed endpoint |
There was a problem hiding this comment.
fallback to unauthed download should only be done when status code is 404 according to the spec. Here it is done on all errors. Status code can also be 429, 502 or 504, They should probably be handled in their own ways.
There was a problem hiding this comment.
True, but what if e.g. matrix.org (or CF in that matter), returns a 520? (or whatever the response was for "CF is upset").
Going to revisit this in another PR, though.
There was a problem hiding this comment.
A few things:
- restructure the download functions to make the fallback logic clearer,
- double check error handling / messages as numerous ones aren't logging the right thing,
- add tests, at the very least for the multipart stuff as it seems pretty complex and fiddly. There's examples in the MSC itself, so I suggest you use them for the tests.
federationapi/routing/routing.go
Outdated
| @@ -678,6 +679,53 @@ func MakeFedAPI( | |||
| return httputil.MakeExternalAPI(metricsName, h) | |||
| } | |||
|
|
|||
| // MakeFedAPI makes an http.Handler that checks matrix federation authentication. | |||
| func MakeFedAPIHTML( | |||
There was a problem hiding this comment.
While true, we also have MakeHTMLAPI, with basically the same function signature. That's why I named it that way, could be MakeFedHTMLAPI, though.
mediaapi/routing/download.go
Outdated
| @@ -61,6 +64,9 @@ type downloadRequest struct { | |||
| ThumbnailSize types.ThumbnailSize | |||
| Logger *log.Entry | |||
| DownloadFilename string | |||
| forFederation bool // whether we need to return a multipart/mixed response | |||
There was a problem hiding this comment.
Flag is unclear. Is this implying that the download is over fed? Or the response will be sent via fed?
There was a problem hiding this comment.
That it's the response for an incoming federation request.
mediaapi/routing/download.go
Outdated
| // create request for remote file | ||
| resp, err := client.CreateMediaDownloadRequest(ctx, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) | ||
| // Attempt to download via authenticated media endpoint | ||
| isMultiPart := true |
There was a problem hiding this comment.
Agreed. It's an implementation detail that the authed endpoint needs multipart, so prefer isAuthed right up until you're forced to process the response.
| @@ -767,19 +826,94 @@ func (r *downloadRequest) fetchRemoteFile( | |||
| ) (types.Path, bool, error) { | |||
| r.Logger.Debug("Fetching remote file") | |||
|
|
|||
| // create request for remote file | |||
| resp, err := client.CreateMediaDownloadRequest(ctx, r.MediaMetadata.Origin, string(r.MediaMetadata.MediaID)) | |||
| // Attempt to download via authenticated media endpoint | |||
There was a problem hiding this comment.
I would have structured this differently, and instead spun off from fetchRemoteFile into fetchRemoteFileUnauthenticated and fetchRemteFileAuthenticated. Then the fallback becomes a lot clearer to read. At present, it's all mixed up in the impl.
There was a problem hiding this comment.
That would be an exercise when refactoring/rewriting the entire MediaAPI, imho.
Needs matrix-org/gomatrixserverlib#437