Skip to content

Add functions to get temporary URLs for files #1780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions lib/api/route/messages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,51 @@ class UploadFileResult {
Map<String, dynamic> toJson() => _$UploadFileResultToJson(this);
}

/// Get a temporary, authless partial URL to a realm-uploaded file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: commit summary line

- api [nfc]: Add functions to get temporary URLs for files
+ api: Add routes getFileTemporaryUrl and tryGetFileTemporaryUrl

The 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.

///
/// The URL returned allows a file to be viewed without requiring authentication,
/// but it doesn't include secrets like the API key. This URL remains valid for
/// 60 seconds.
///
/// 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`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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.

Future<Uri> getFileTemporaryUrl(ApiConnection connection, {
required String filePath,
}) async {
final response = await connection.get('getFileTemporaryUrl',
(json) => json['url'],
filePath.substring(1), // remove leading slash to avoid duplicate
{},
);

return Uri.parse('${connection.realmUrl}$response');
}

/// A wrapper for [getFileTemporaryUrl] that returns null on failure.
///
/// Validates that the URL is a realm-uploaded file before proceeding.
Future<Uri?> tryGetFileTemporaryUrl(
ApiConnection connection, {
required Uri url,
required Uri realmUrl,
}) async {
if (url.origin != realmUrl.origin) {
return null;
}

final filePath = url.path;
if (!RegExp(r'^/user_uploads/[0-9]+/.+$').hasMatch(filePath)) {
return null;
}

try {
return await getFileTemporaryUrl(connection, filePath: filePath);
} catch (e) {
return null;
}
}

/// https://zulip.com/api/add-reaction
Future<void> addReaction(ApiConnection connection, {
required int messageId,
Expand Down
73 changes: 73 additions & 0 deletions test/api/route/messages_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,79 @@ void main() {
});
});

group('getFileTemporaryUrl', () {
test('constructs URL correctly from response', () {
return FakeApiConnection.with_((connection) async {
connection.prepare(json: {
'url': '/user_uploads/temporary/abc123',
'result': 'success',
'msg': '',
});

final result = await getFileTemporaryUrl(connection,
filePath: '/user_uploads/1/2/testfile.jpg');

check(result.toString()).equals('${connection.realmUrl}/user_uploads/temporary/abc123');
check(connection.lastRequest).isA<http.Request>()
..method.equals('GET')
..url.path.equals('/api/v1/user_uploads/1/2/testfile.jpg');
});
});

test('returns temporary URL for valid realm file', () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and the remaining test blocks in the group require an indentation.

return FakeApiConnection.with_((connection) async {
connection.prepare(json: {
'url': '/user_uploads/temporary/abc123',
'result': 'success',
'msg': '',
});

final result = await tryGetFileTemporaryUrl(connection,
url: Uri.parse('${connection.realmUrl}user_uploads/123/testfile.jpg'),
realmUrl: connection.realmUrl);

check(result).isNotNull();
check(result.toString()).equals('${connection.realmUrl}/user_uploads/temporary/abc123');
});
});

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'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

realmUrl: connection.realmUrl);

check(result).isNull();
// Verify no API calls were made
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Verify no API calls were made
// Verify no API calls were made.

or

Suggested change
// 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)

check(connection.lastRequest).isNull();
});
});

test('returns null for non-matching URL pattern', () {
return FakeApiConnection.with_((connection) async {
final result = await tryGetFileTemporaryUrl(connection,
url: Uri.parse('${connection.realmUrl}/invalid/path/file.jpg'),
realmUrl: connection.realmUrl);

check(result).isNull();
// Verify no API calls were made
check(connection.lastRequest).isNull();
});
});

test('returns null when API request fails', () {
return FakeApiConnection.with_((connection) async {
connection.prepare(
apiException: eg.apiBadRequest(message: 'Not found'));

final result = await tryGetFileTemporaryUrl(connection,
url: Uri.parse('${connection.realmUrl}/user_uploads/1/2/testfile.jpg'),
realmUrl: connection.realmUrl);

check(result).isNull();
});
});
});
group('addReaction', () {
Future<void> checkAddReaction(FakeApiConnection connection, {
required int messageId,
Expand Down