Skip to content

Commit b3b7738

Browse files
committed
Merge branch '547-failed-to-download-module' into 'main'
Resolve "Failed to download module" Closes #547 See merge request pub/terrareg!433
2 parents 4345600 + 64a5652 commit b3b7738

File tree

5 files changed

+182
-27
lines changed

5 files changed

+182
-27
lines changed

terrareg/models.py

+15-18
Original file line numberDiff line numberDiff line change
@@ -3946,31 +3946,28 @@ def get_source_download_url(self, request_domain: str, direct_http_request: bool
39463946

39473947
# If a git URL is not present, revert to using built-in module hosting
39483948
if config.ALLOW_MODULE_HOSTING is not terrareg.config.ModuleHostingMode.DISALLOW:
3949-
url = '/v1/terrareg/modules/{0}/{1}'.format(self.id, self.archive_name_zip)
3949+
url = f'/v1/terrareg/modules/{self.id}'
3950+
3951+
# If authentication is required, generate pre-signed URL
3952+
if not config.ALLOW_UNAUTHENTICATED_ACCESS:
3953+
presign_key = TerraformSourcePresignedUrl.generate_presigned_key(url=url)
3954+
url = f'{url}/{presign_key}'
3955+
3956+
url += f'/{self.archive_name_zip}'
39503957

39513958
# If archive does not contain just the git_path,
39523959
# check if git_path has been set and prepend to path, if set.
39533960
if not self.archive_git_path:
39543961
path = os.path.join(self.git_path or '', path or '')
39553962

3956-
# Check if path is present for module
3957-
if path:
3958-
# Remove any trailing slashes from path
3959-
path = path.lstrip('/').rstrip('/')
3963+
# Check if path is present for module
3964+
if path:
3965+
# Remove any trailing slashes from path
3966+
path = path.lstrip('/').rstrip('/')
39603967

3961-
rendered_url = '{rendered_url}//{path}'.format(
3962-
rendered_url=rendered_url,
3963-
path=path)
3964-
if path:
3965-
url = '{url}//{path}'.format(
3966-
url=url,
3967-
path=path
3968-
)
3969-
3970-
# If authentication is required, generate pre-signed URL
3971-
if not config.ALLOW_UNAUTHENTICATED_ACCESS:
3972-
presign_key = TerraformSourcePresignedUrl.generate_presigned_key(url=url)
3973-
url = f'{url}?presign={presign_key}'
3968+
url = '{url}//{path}'.format(
3969+
url=url,
3970+
path=path)
39743971

39753972
# If request is a direct HTTP request,
39763973
# i.e. Terraform has been configured with a HTTP URL

terrareg/server/__init__.py

+2-1
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,8 @@ def _register_routes(self):
590590
)
591591
self._api.add_resource(
592592
ApiModuleVersionSourceDownload,
593-
'/v1/terrareg/modules/<string:namespace>/<string:name>/<string:provider>/<string:version>/source.zip'
593+
'/v1/terrareg/modules/<string:namespace>/<string:name>/<string:provider>/<string:version>/source.zip',
594+
'/v1/terrareg/modules/<string:namespace>/<string:name>/<string:provider>/<string:version>/<string:presign>/source.zip'
594595
)
595596
self._api.add_resource(
596597
ApiTerraregModuleVersionPublish,
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11

22

33
import os
4-
from flask import send_file, request
4+
import flask
55

66
from terrareg.errors import InvalidPresignedUrlKeyError
7-
from terrareg.presigned_url import TerraformSourcePresignedUrl
7+
import terrareg.presigned_url
88
from terrareg.server.error_catching_resource import ErrorCatchingResource
99
import terrareg.config
1010
import terrareg.file_storage
@@ -13,16 +13,28 @@
1313
class ApiModuleVersionSourceDownload(ErrorCatchingResource):
1414
"""Return source package of module version"""
1515

16-
def _get(self, namespace, name, provider, version):
16+
def _get(self, namespace, name, provider, version, presign=None):
1717
"""Return static file."""
1818
config = terrareg.config.Config()
1919
if config.ALLOW_MODULE_HOSTING is terrareg.config.ModuleHostingMode.DISALLOW:
2020
return {'message': 'Module hosting is disbaled'}, 500
2121

2222
# If authentication is required, check pre-signed URL
2323
if not config.ALLOW_UNAUTHENTICATED_ACCESS:
24+
presign = flask.request.args.get("presign", presign)
25+
26+
path = flask.request.path
27+
path_parts = path.split('/')
28+
# Remove last section of path (i.e. the file name)
29+
del path_parts[-1]
30+
31+
# If path ends with the pre-sign key, remove it
32+
if presign and path_parts[-1] == presign:
33+
del path_parts[-1]
34+
path = '/'.join(path_parts)
35+
2436
try:
25-
TerraformSourcePresignedUrl.validate_presigned_key(url=request.path, payload=request.args.get("presign"))
37+
terrareg.presigned_url.TerraformSourcePresignedUrl.validate_presigned_key(url=path, payload=presign)
2638
except InvalidPresignedUrlKeyError as exc:
2739
return {'message': str(exc)}, 403
2840

@@ -31,7 +43,8 @@ def _get(self, namespace, name, provider, version):
3143
return error
3244

3345
file_storage = terrareg.file_storage.FileStorageFactory().get_file_storage()
34-
return send_file(
46+
return flask.send_file(
3547
file_storage.read_file(os.path.join(module_version.base_directory, module_version.archive_name_zip), bytes_mode=True),
36-
download_name=module_version.archive_name_zip
48+
download_name=module_version.archive_name_zip,
49+
mimetype='application/zip'
3750
)

test/integration/terrareg/models/test_module_version.py

+2-2
Original file line numberDiff line numberDiff line change
@@ -1680,7 +1680,7 @@ def test_get_source_download_url_git_sha(self, git_sha, module_version_use_git_c
16801680
@pytest.mark.parametrize('module_name, module_version, git_path, expected_source_download_url, allow_unauthenticated_access, expect_presigned, should_prefix_domain', [
16811681
# Test no clone URL in any configuration, defaulting to source archive download
16821682
('no-git-provider', '1.0.0', None, '/v1/terrareg/modules/repo_url_tests/no-git-provider/test/1.0.0/source.zip', True, False, True),
1683-
('no-git-provider', '1.0.0', None, '/v1/terrareg/modules/repo_url_tests/no-git-provider/test/1.0.0/source.zip?presign=unittest-presign-key', False, True, True),
1683+
('no-git-provider', '1.0.0', None, '/v1/terrareg/modules/repo_url_tests/no-git-provider/test/1.0.0/unittest-presign-key/source.zip', False, True, True),
16841684
# Test clone URL only configured in module version, with public access allowed/disabled
16851685
('no-git-provider', '1.4.0', None, 'git::ssh://mv-clone-url.com/repo_url_tests/no-git-provider-test?ref=1.4.0', True, False, False),
16861686
('no-git-provider', '1.4.0', None, 'git::ssh://mv-clone-url.com/repo_url_tests/no-git-provider-test?ref=1.4.0', True, False, False),
@@ -1725,7 +1725,7 @@ def test_get_source_download_url_presigned(self, module_name, module_version, gi
17251725
) == expected_source_download_url
17261726

17271727
if expect_presigned:
1728-
mock_generate_presigned_key.assert_called_once_with(url='/v1/terrareg/modules/repo_url_tests/no-git-provider/test/1.0.0/source.zip')
1728+
mock_generate_presigned_key.assert_called_once_with(url='/v1/terrareg/modules/repo_url_tests/no-git-provider/test/1.0.0')
17291729
else:
17301730
mock_generate_presigned_key.assert_not_called()
17311731

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
2+
import unittest.mock
3+
4+
import pytest
5+
6+
from terrareg.analytics import AnalyticsEngine
7+
import terrareg.errors
8+
from test.unit.terrareg import (
9+
mock_models,
10+
setup_test_data, TerraregUnitTest
11+
)
12+
import terrareg.models
13+
import terrareg.config
14+
from test import client, mock_create_audit_event
15+
from . import mock_record_module_version_download
16+
17+
18+
class TestApiModuleVersionSourceDownload(TerraregUnitTest):
19+
"""Test ApiModuleVersionDownload resource."""
20+
21+
# '/v1/terrareg/modules/<string:namespace>/<string:name>/<string:provider>/<string:version>/source.zip',
22+
# '/v1/terrareg/modules/<string:namespace>/<string:name>/<string:provider>/<string:version>/<string:presign>/source.zip'
23+
24+
@setup_test_data()
25+
def test_disable_module_hosting(self, client, mock_models, mock_record_module_version_download):
26+
"""Test module version download with invalid analytics token"""
27+
with unittest.mock.patch('terrareg.config.Config.ALLOW_MODULE_HOSTING', terrareg.config.ModuleHostingMode.DISALLOW):
28+
res = client.get(f"/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip")
29+
assert res.status_code == 500
30+
assert res.json == {'message': 'Module hosting is disbaled'}
31+
32+
@setup_test_data()
33+
@pytest.mark.parametrize('namespace, module, provider, version, error', [
34+
# Non-existent version
35+
('testnamespace', 'testmodulename', 'testprovider', '2.4.2', 'Module version does not exist'),
36+
('testnamespace', 'testmodulename', 'nonexistent', '2.4.2', 'Module provider does not exist'),
37+
('testnamespace', 'nonexistent', 'nonexistent', '2.4.2', 'Module provider does not exist'),
38+
('nonexistent', 'nonexistent', 'nonexistent', '2.4.2', 'Namespace does not exist'),
39+
])
40+
def test_non_existent(self, namespace, module, provider, version, error, client, mock_models):
41+
"""Test module version download with invalid analytics token"""
42+
with unittest.mock.patch('terrareg.config.Config.ALLOW_MODULE_HOSTING', terrareg.config.ModuleHostingMode.ENFORCE):
43+
res = client.get(f"/v1/terrareg/modules/{namespace}/{module}/{provider}/{version}/source.zip")
44+
assert res.status_code == 400
45+
assert res.json == {'message': error}
46+
47+
@setup_test_data()
48+
@pytest.mark.parametrize('url, expected_presign_key', [
49+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip', None),
50+
51+
# Pre-sign key in URL
52+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/unittest-presign-key/source.zip', 'unittest-presign-key'),
53+
54+
# Presign key in params
55+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip?presign=unittest-presign-key', 'unittest-presign-key'),
56+
])
57+
def test_invalid_presign_key(self, url, expected_presign_key, client, mock_models):
58+
"""Ensure invalid pre-sign key throws an error"""
59+
def raise_exception(*args, **kwargs):
60+
raise terrareg.errors.InvalidPresignedUrlKeyError('Invalid pre-sign key')
61+
62+
mock_validate_presigned_key = unittest.mock.MagicMock(side_effect=raise_exception)
63+
with unittest.mock.patch('terrareg.config.Config.ALLOW_MODULE_HOSTING', terrareg.config.ModuleHostingMode.ALLOW), \
64+
unittest.mock.patch('terrareg.config.Config.ALLOW_UNAUTHENTICATED_ACCESS', False), \
65+
unittest.mock.patch('terrareg.presigned_url.TerraformSourcePresignedUrl.validate_presigned_key', mock_validate_presigned_key):
66+
res = client.get(url)
67+
assert res.status_code == 403
68+
assert res.json == {'message': 'Invalid pre-sign key'}
69+
70+
mock_validate_presigned_key.assert_called_once_with(url='/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1', payload=expected_presign_key)
71+
72+
@setup_test_data()
73+
@pytest.mark.parametrize('url, expected_presign_key', [
74+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip', None),
75+
76+
# Pre-sign key in URL
77+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/unittest-presign-key/source.zip', 'unittest-presign-key'),
78+
79+
# Presign key in params
80+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip?presign=unittest-presign-key', 'unittest-presign-key'),
81+
])
82+
def test_presign_validation_disabled(self, url, expected_presign_key, client, mock_models):
83+
"""Ensure pre-sign validation is not performed when unauthenticated access is allowed"""
84+
def raise_exception(*args, **kwargs):
85+
raise terrareg.errors.InvalidPresignedUrlKeyError('Invalid pre-sign key')
86+
87+
mock_get_file_storage = unittest.mock.MagicMock()
88+
mock_send_file = unittest.mock.MagicMock(return_value="UNIT TEST BINARY OUTPUT")
89+
90+
mock_validate_presigned_key = unittest.mock.MagicMock(side_effect=raise_exception)
91+
with unittest.mock.patch('terrareg.config.Config.ALLOW_MODULE_HOSTING', terrareg.config.ModuleHostingMode.ALLOW), \
92+
unittest.mock.patch('terrareg.config.Config.ALLOW_UNAUTHENTICATED_ACCESS', True), \
93+
unittest.mock.patch('terrareg.presigned_url.TerraformSourcePresignedUrl.validate_presigned_key', mock_validate_presigned_key), \
94+
unittest.mock.patch('flask.send_file', mock_send_file), \
95+
unittest.mock.patch('terrareg.file_storage.FileStorageFactory.get_file_storage', mock_get_file_storage):
96+
res = client.get(url)
97+
98+
assert res.status_code == 200
99+
assert res.json == "UNIT TEST BINARY OUTPUT"
100+
101+
mock_validate_presigned_key.assert_not_called()
102+
103+
@setup_test_data()
104+
@pytest.mark.parametrize('url, allow_unauthenticated_access', [
105+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip', True),
106+
107+
# Pre-sign key in URL
108+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/unittest-presign-key/source.zip', False),
109+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/unittest-presign-key/source.zip', True),
110+
111+
# Presign key in params
112+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip?presign=unittest-presign-key', False),
113+
('/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip?presign=unittest-presign-key', True),
114+
])
115+
def test_send_file(self, url, allow_unauthenticated_access, client, mock_models):
116+
"""Ensure send file and file storage is handled correctly"""
117+
mock_file_storage = unittest.mock.MagicMock()
118+
mock_get_file_storage = unittest.mock.MagicMock(return_value=mock_file_storage)
119+
mock_read_file_response = unittest.mock.MagicMock()
120+
mock_read_file = unittest.mock.MagicMock(return_value=mock_read_file_response)
121+
mock_file_storage.read_file = mock_read_file
122+
123+
mock_validate_presigned_key = unittest.mock.MagicMock()
124+
125+
mock_send_file = unittest.mock.MagicMock(return_value="UNIT TEST BINARY OUTPUT")
126+
127+
with unittest.mock.patch('terrareg.config.Config.ALLOW_MODULE_HOSTING', terrareg.config.ModuleHostingMode.ALLOW), \
128+
unittest.mock.patch('terrareg.config.Config.ALLOW_UNAUTHENTICATED_ACCESS', allow_unauthenticated_access), \
129+
unittest.mock.patch('flask.send_file', mock_send_file), \
130+
unittest.mock.patch('terrareg.presigned_url.TerraformSourcePresignedUrl.validate_presigned_key', mock_validate_presigned_key), \
131+
unittest.mock.patch('terrareg.file_storage.FileStorageFactory.get_file_storage', mock_get_file_storage):
132+
res = client.get(url)
133+
134+
assert res.status_code == 200
135+
assert res.json == "UNIT TEST BINARY OUTPUT"
136+
137+
mock_get_file_storage.assert_called_once()
138+
mock_read_file.assert_called_once_with('/modules/testnamespace/testmodulename/testprovider/2.4.1/source.zip', bytes_mode=True)
139+
mock_send_file.assert_called_once_with(mock_read_file_response, download_name='source.zip', mimetype='application/zip')
140+
141+
if not allow_unauthenticated_access:
142+
mock_validate_presigned_key.assert_called_once_with(url='/v1/terrareg/modules/testnamespace/testmodulename/testprovider/2.4.1', payload='unittest-presign-key')
143+
else:
144+
mock_validate_presigned_key.assert_not_called()

0 commit comments

Comments
 (0)