Add functions to get temporary URLs for files #1780
Conversation
|
Cool. This will also be the main ingredient in fixing #1732, which is an issue in our upcoming milestone M6. (I haven't yet looked at this implementation at all.) |
lib/api/route/messages.dart
Outdated
| /// | ||
| /// This endpoint is documented in the OpenAPI description: | ||
| /// https://github.com/zulip/zulip/blob/main/zerver/openapi/zulip.yaml | ||
| /// under the name `get_file_temporary_url`. |
There was a problem hiding this comment.
| /// under the name `get_file_temporary_url`. | |
| /// under the name `get-file-temporary-url`. |
I couldn't find a match of get_file_temporary_url searching in zulip.yaml, but searching get-file-temporary-url has a match.
| @@ -327,6 +327,51 @@ class UploadFileResult { | |||
| Map<String, dynamic> toJson() => _$UploadFileResultToJson(this); | |||
| } | |||
|
|
|||
| /// Get a temporary, authless partial URL to a realm-uploaded file. | |||
There was a problem hiding this comment.
nit: commit summary line
- api [nfc]: Add functions to get temporary URLs for files
+ api: Add routes getFileTemporaryUrl and tryGetFileTemporaryUrlThe two functions in the commit introduce new behavior (new API calls and logic), so it's not an NFC commit. Also, good to mention the routes introduced in the summary line.
test/api/route/messages_test.dart
Outdated
| }); | ||
| }); | ||
|
|
||
| test('returns temporary URL for valid realm file', () { |
There was a problem hiding this comment.
This test and the remaining test blocks in the group require an indentation.
test/api/route/messages_test.dart
Outdated
| realmUrl: connection.realmUrl); | ||
|
|
||
| check(result).isNull(); | ||
| // Verify no API calls were made |
There was a problem hiding this comment.
| // Verify no API calls were made | |
| // Verify no API calls were made. |
or
| // Verify no API calls were made | |
| // verify no API calls were made |
Either complete a sentence that starts with a capital letter by adding a period at the end, or start it with a lowercase letter and omit the period. 🙂
(the same applies to the one below this)
test/api/route/messages_test.dart
Outdated
| test('returns null for non-realm URL', () { | ||
| return FakeApiConnection.with_((connection) async { | ||
| final result = await tryGetFileTemporaryUrl(connection, | ||
| url: Uri.parse('https://example.com/image.jpg'), |
There was a problem hiding this comment.
| url: Uri.parse('https://example.com/image.jpg'), | |
| url: Uri.parse('https://example.com/user_uploads/123/testfile.jpg'), |
The URL provided is both non-realm URL and a non-matching URL, but the test case is only about non-realm URL, so it's good to adjust it to only what the test case is about. That way, it's easy to validate it against what the test case claims.
There was a problem hiding this comment.
Thanks @chimnayajith for taking care of this. A few comments above!
|
@chimnayajith please post here when you've pushed a revision for Sayed's review; thank you :) |
Implement `getFileTemporaryUrl` and `tryGetFileTemporaryUrl` to access
user-uploaded files without requiring authentication. The temporary URLs
remain valid for 60 seconds and provide a secure way to share files
without exposing API keys.
This uses the GET `/user_uploads/{realm_id}/{filename}` endpoint that
returns a URL allowing immediate access without requiring authentication.
Requested in zulip#1144 (comment)
chimnayajith
force-pushed
the
temp-file-url-api
branch
from
3e388ca to
46594cc
Compare
|
@sm-sayedi Pushed a revision. PTAL. |
|
Thanks, LGTM. Marking for Chris's review. |
|
Thanks @chimnayajith for building this, and @sm-sayedi for the reviews! I found this in the API docs, by trying the string "get-file-temporary-url" that was helpfully included in a dartdoc :) Here's the page: https://zulip.com/api/get-file-temporary-url It doesn't have a link in the left sidebar, and I've started a topic for that in |
|
I'd like to organize the code slightly differently than here (and in zulip-mobile), to make the API binding for this endpoint as thin a wrapper as possible. In particular:
|
|
#1732 is another issue that would benefit from this logic; @chimnayajith please let us know if we can help unblock you on any of the work here. |
Implement
getFileTemporaryUrlandtryGetFileTemporaryUrlto access user-uploaded files without requiring authentication. The temporary URLs remain valid for 60 seconds and provide a secure way to share files without exposing API keys.This uses the GET
/user_uploads/{realm_id}/{filename}endpoint that returns a URL allowing immediate access without requiring authentication.Requested in #1144 (comment)
Will be needed for #42 and #43