From c6822f2e30f5a01190bf50b57f47ac52e929a3e3 Mon Sep 17 00:00:00 2001 From: vsklencar Date: Thu, 10 Dec 2020 17:38:14 +0100 Subject: [PATCH 1/7] Check storage availability only for a project owned by a current user Ownership depends on matching username with namespace, not ownership permissions --- mergin/client_push.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/mergin/client_push.py b/mergin/client_push.py index 6b6e6cc3..85907855 100644 --- a/mergin/client_push.py +++ b/mergin/client_push.py @@ -101,11 +101,14 @@ def push_project_async(mc, directory): changes = mp.get_push_changes() mp.log.debug("push changes:\n" + pprint.pformat(changes)) - enough_free_space, freespace = mc.enough_storage_available(changes) - if not enough_free_space: - freespace = int(freespace/(1024*1024)) - mp.log.error(f"--- push {project_path} - not enough space") - raise ClientError("Storage limit has been reached. Only " + str(freespace) + "MB left") + + # currently proceed storage limit check only if a project is own by a current user. + if username == project_path.split("/")[0]: + enough_free_space, freespace = mc.enough_storage_available(changes) + if not enough_free_space: + freespace = int(freespace/(1024*1024)) + mp.log.error(f"--- push {project_path} - not enough space") + raise ClientError("Storage limit has been reached. Only " + str(freespace) + "MB left") if not sum(len(v) for v in changes.values()): mp.log.info(f"--- push {project_path} - nothing to do") From 73a9e4aade3fb1c359a24ccd4d90449b7224ba29 Mon Sep 17 00:00:00 2001 From: vsklencar Date: Fri, 11 Dec 2020 13:42:43 +0100 Subject: [PATCH 2/7] Added test --- .github/workflows/autotests.yml | 2 + README.md | 2 + mergin/client.py | 15 ++++++ mergin/client_push.py | 2 +- mergin/test/test_client.py | 93 ++++++++++++++++++++++++++++++++- 5 files changed, 111 insertions(+), 3 deletions(-) diff --git a/.github/workflows/autotests.yml b/.github/workflows/autotests.yml index 180ca8f6..ced36649 100644 --- a/.github/workflows/autotests.yml +++ b/.github/workflows/autotests.yml @@ -4,6 +4,8 @@ env: TEST_MERGIN_URL: https://test.dev.cloudmergin.com/ TEST_API_USERNAME: test_plugin TEST_API_PASSWORD: ${{ secrets.MERGINTEST_API_PASSWORD }} + TEST_API_USERNAME2: test_plugin2 + TEST_API_PASSWORD2: ${{ secrets.MERGINTEST_API_PASSWORD2 }} jobs: tests: diff --git a/README.md b/README.md index 041f67ec..5dd2c1d0 100644 --- a/README.md +++ b/README.md @@ -118,4 +118,6 @@ For running test do: export TEST_MERGIN_URL= # testing server export TEST_API_USERNAME= export TEST_API_PASSWORD= + export TEST_API_USERNAME2= + export TEST_API_PASSWORD2= pipenv run pytest --cov-report html --cov=mergin test/ diff --git a/mergin/client.py b/mergin/client.py index 09328907..b4cc676d 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -373,6 +373,21 @@ def user_info(self): resp = self.get('/v1/user/' + self.username()) return json.load(resp) + def set_project_access(self, project_path, access): + if not self._user_info: + raise Exception("Authentication required") + + params = {"access": access} + path = "/v1/project/%s" % project_path + url = urllib.parse.urljoin(self.url, urllib.parse.quote(path)) + json_headers = {'Content-Type': 'application/json'} + try: + request = urllib.request.Request(url, data=json.dumps(params).encode(), headers=json_headers, method="PUT") + self._do_request(request) + except Exception as e: + detail = f"Project path: {project_path}" + + def push_project(self, directory): """ Upload local changes to the repository. diff --git a/mergin/client_push.py b/mergin/client_push.py index 85907855..ec23a19a 100644 --- a/mergin/client_push.py +++ b/mergin/client_push.py @@ -102,7 +102,7 @@ def push_project_async(mc, directory): changes = mp.get_push_changes() mp.log.debug("push changes:\n" + pprint.pformat(changes)) - # currently proceed storage limit check only if a project is own by a current user. + # currently proceed storage limit check only if a project is own by a current user. if username == project_path.split("/")[0]: enough_free_space, freespace = mc.enough_storage_available(changes) if not enough_free_space: diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index a7681788..4127a434 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -11,6 +11,8 @@ SERVER_URL = os.environ.get('TEST_MERGIN_URL') API_USER = os.environ.get('TEST_API_USERNAME') USER_PWD = os.environ.get('TEST_API_PASSWORD') +API_USER2 = os.environ.get('TEST_API_USERNAME2') +USER_PWD2 = os.environ.get('TEST_API_PASSWORD2') TMP_DIR = tempfile.gettempdir() TEST_DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'test_data') CHANGED_SCHEMA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'modified_schema') @@ -22,8 +24,16 @@ def toggle_geodiff(enabled): @pytest.fixture(scope='function') def mc(): - assert SERVER_URL and SERVER_URL.rstrip('/') != 'https://public.cloudmergin.com' and API_USER and USER_PWD - return MerginClient(SERVER_URL, login=API_USER, password=USER_PWD) + return create_client(API_USER, USER_PWD) + +@pytest.fixture(scope='function') +def mc2(): + return create_client(API_USER2, USER_PWD2) + + +def create_client(user, pwd): + assert SERVER_URL and SERVER_URL.rstrip('/') != 'https://public.cloudmergin.com' and user and pwd + return MerginClient(SERVER_URL, login=user, password=pwd) def cleanup(mc, project, dirs): @@ -32,6 +42,11 @@ def cleanup(mc, project, dirs): mc.delete_project(project) except ClientError: pass + remove_folders(dirs) + + +def remove_folders(dirs): + # clean given directories for d in dirs: if os.path.exists(d): shutil.rmtree(d) @@ -483,3 +498,77 @@ def test_clone_project(mc): mc.clone_project(test_project_fullname, cloned_project_name, API_USER) projects = mc.projects_list(flag='created') assert any(p for p in projects if p['name'] == cloned_project_name and p['namespace'] == API_USER) + + +def test_available_storage_validation(mc, mc2): + """ + Testing of storage limit - should not be applied for user pushing changes into project with different namespace. + This test also tests giving read and write access to another user. Additionally tests also uploading of big file. + """ + test_project = 'test_available_storage_validation' + test_project_fullname = API_USER2 + '/' + test_project + + # cleanups + project_dir = os.path.join(TMP_DIR, test_project, API_USER) + cleanup(mc, test_project_fullname, [project_dir]) + cleanup(mc2, test_project_fullname, [project_dir]) + + try: + # create new (empty) project on server + mc2.create_project(test_project) + + # Add writer access to another client + project_info = get_project_info(mc2, API_USER2, test_project) + access = project_info['access'] + access['writersnames'].append(API_USER) + access['readersnames'].append(API_USER) + project_info['access'] = access + mc2.set_project_access(test_project_fullname, access) + + # check writers access + project_info = get_project_info(mc2, API_USER2, test_project) + access = project_info['access'] + assert API_USER in access['writersnames'] + + # download project + mc.download_project(test_project_fullname, project_dir) + mp = MerginProject(project_dir) + + # get user_info about storage capacity + user_info = mc.user_info() + storage_remaining = user_info['storage'] - user_info['disk_usage'] + + # generate dummy data (remaining storage + extra 1024b) + dummy_data_path = project_dir + "/data" + file_size = storage_remaining + 1024 + _generate_big_file(dummy_data_path, file_size) + + # try to upload + mc.push_project(project_dir) + + # Check project content + project_info = get_project_info(mc2, API_USER2, test_project) + assert project_info['meta']['files_count'] == 1 + assert project_info['meta']['size'] == file_size + + # clean project due to high storage requirement + finally: + cleanup(mc, test_project_fullname, [project_dir]) + cleanup(mc2, test_project_fullname, [project_dir]) + + +def get_project_info(mc, namespace, project_name): + projects = mc.projects_list(flag='created') + test_project_list = [p for p in projects if p['name'] == project_name and p['namespace'] == namespace] + assert any(test_project_list) + return test_project_list[0] + + +def _generate_big_file(filepath, size): + """ + generate big binary file with the specified size in bytes + :param filepath: full filepath + :param size: the size in bytes + """ + with open('%s'%filepath, 'wb') as fout: + fout.write(os.urandom(size)) From aaf25bc874c37819dc7915467dba709900c9d9b3 Mon Sep 17 00:00:00 2001 From: vsklencar Date: Fri, 11 Dec 2020 16:34:28 +0100 Subject: [PATCH 3/7] Storage limit check small improvements --- mergin/client.py | 6 +++ mergin/test/test_client.py | 85 ++++++++++++++++++++------------------ 2 files changed, 51 insertions(+), 40 deletions(-) diff --git a/mergin/client.py b/mergin/client.py index b4cc676d..f50364b1 100644 --- a/mergin/client.py +++ b/mergin/client.py @@ -374,6 +374,11 @@ def user_info(self): return json.load(resp) def set_project_access(self, project_path, access): + """ + Updates access for given project. + :param project_path: project full name (/) + :param access: dict -> list of str username we want to give access to + """ if not self._user_info: raise Exception("Authentication required") @@ -386,6 +391,7 @@ def set_project_access(self, project_path, access): self._do_request(request) except Exception as e: detail = f"Project path: {project_path}" + raise ClientError(str(e), detail) def push_project(self, directory): diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index 4127a434..41fa0903 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -513,54 +513,59 @@ def test_available_storage_validation(mc, mc2): cleanup(mc, test_project_fullname, [project_dir]) cleanup(mc2, test_project_fullname, [project_dir]) - try: - # create new (empty) project on server - mc2.create_project(test_project) - - # Add writer access to another client - project_info = get_project_info(mc2, API_USER2, test_project) - access = project_info['access'] - access['writersnames'].append(API_USER) - access['readersnames'].append(API_USER) - project_info['access'] = access - mc2.set_project_access(test_project_fullname, access) - - # check writers access - project_info = get_project_info(mc2, API_USER2, test_project) - access = project_info['access'] - assert API_USER in access['writersnames'] - - # download project - mc.download_project(test_project_fullname, project_dir) - mp = MerginProject(project_dir) + # create new (empty) project on server + mc2.create_project(test_project) + + # Add writer access to another client + project_info = get_project_info(mc2, API_USER2, test_project) + access = project_info['access'] + access['writersnames'].append(API_USER) + access['readersnames'].append(API_USER) + project_info['access'] = access + # TODO create separate test + mc2.set_project_access(test_project_fullname, access) + + # check writers access + project_info = get_project_info(mc2, API_USER2, test_project) + access = project_info['access'] + assert API_USER in access['writersnames'] + + # download project + mc.download_project(test_project_fullname, project_dir) + mp = MerginProject(project_dir) - # get user_info about storage capacity - user_info = mc.user_info() - storage_remaining = user_info['storage'] - user_info['disk_usage'] + # get user_info about storage capacity + user_info = mc.user_info() + storage_remaining = user_info['storage'] - user_info['disk_usage'] - # generate dummy data (remaining storage + extra 1024b) - dummy_data_path = project_dir + "/data" - file_size = storage_remaining + 1024 - _generate_big_file(dummy_data_path, file_size) + # generate dummy data (remaining storage + extra 1024b) + dummy_data_path = project_dir + "/data" + file_size = storage_remaining + 1024 + _generate_big_file(dummy_data_path, file_size) - # try to upload - mc.push_project(project_dir) + # try to upload + mc.push_project(project_dir) - # Check project content - project_info = get_project_info(mc2, API_USER2, test_project) - assert project_info['meta']['files_count'] == 1 - assert project_info['meta']['size'] == file_size + # Check project content + project_info = get_project_info(mc2, API_USER2, test_project) + assert project_info['meta']['files_count'] == 1 + assert project_info['meta']['size'] == file_size - # clean project due to high storage requirement - finally: - cleanup(mc, test_project_fullname, [project_dir]) - cleanup(mc2, test_project_fullname, [project_dir]) + # remove dummy big file from a disk + remove_folders([project_dir]) def get_project_info(mc, namespace, project_name): + """ + Returns first (and suppose to be just one) project info dict of project matching given namespace and name. + :param mc: MerginClient instance + :param namespace: project's namespace + :param project_name: project's name + :return: dict with project info + """ projects = mc.projects_list(flag='created') test_project_list = [p for p in projects if p['name'] == project_name and p['namespace'] == namespace] - assert any(test_project_list) + assert len(test_project_list) == 1 return test_project_list[0] @@ -570,5 +575,5 @@ def _generate_big_file(filepath, size): :param filepath: full filepath :param size: the size in bytes """ - with open('%s'%filepath, 'wb') as fout: - fout.write(os.urandom(size)) + with open(filepath, 'wb') as fout: + fout.write(b"\0" * size) From fda80942002ad245b7dbe4c80c97d4543b5d63ab Mon Sep 17 00:00:00 2001 From: vsklencar Date: Fri, 11 Dec 2020 17:26:20 +0100 Subject: [PATCH 4/7] Test for storage limit and permissions --- mergin/test/test_client.py | 75 +++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 9 deletions(-) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index 41fa0903..5bb70533 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -476,6 +476,7 @@ def test_empty_file_in_subdir(mc): mc.pull_project(project_dir_2) assert os.path.exists(os.path.join(project_dir_2, 'subdir2', 'empty2.txt')) + def test_clone_project(mc): test_project = 'test_clone_project' test_project_fullname = API_USER + '/' + test_project @@ -500,12 +501,76 @@ def test_clone_project(mc): assert any(p for p in projects if p['name'] == cloned_project_name and p['namespace'] == API_USER) -def test_available_storage_validation(mc, mc2): +def test_set_read_write_access(mc): + test_project = 'test_set_access' + test_project_fullname = API_USER + '/' + test_project + + # create new (empty) project on server + mc.create_project(test_project) + + # Add writer access to another client + project_info = get_project_info(mc, API_USER, test_project) + access = project_info['access'] + access['writersnames'].append(API_USER2) + access['readersnames'].append(API_USER2) + mc.set_project_access(test_project_fullname, access) + + # check access + project_info = get_project_info(mc, API_USER, test_project) + access = project_info['access'] + assert API_USER2 in access['writersnames'] + assert API_USER2 in access['readersnames'] + + +def test_available_storage_validation(mc): """ Testing of storage limit - should not be applied for user pushing changes into project with different namespace. This test also tests giving read and write access to another user. Additionally tests also uploading of big file. """ test_project = 'test_available_storage_validation' + test_project_fullname = API_USER + '/' + test_project + + # cleanups + project_dir = os.path.join(TMP_DIR, test_project, API_USER) + cleanup(mc, test_project_fullname, [project_dir]) + + # create new (empty) project on server + mc.create_project(test_project) + + # download project + mc.download_project(test_project_fullname, project_dir) + + # get user_info about storage capacity + user_info = mc.user_info() + storage_remaining = user_info['storage'] - user_info['disk_usage'] + + # generate dummy data (remaining storage + extra 1024b) + dummy_data_path = project_dir + "/data" + file_size = storage_remaining + 1024 + _generate_big_file(dummy_data_path, file_size) + + # try to upload + got_right_err = False + try: + mc.push_project(project_dir) + except ClientError as e: + # Expecting "Storage limit has been reached" error msg. + assert str(e).startswith("Storage limit has been reached") + got_right_err = True + assert got_right_err + + # Expecting empty project + project_info = get_project_info(mc, API_USER, test_project) + assert project_info['meta']['files_count'] == 0 + assert project_info['meta']['size'] == 0 + + +def test_available_storage_validation2(mc, mc2): + """ + Testing of storage limit - should not be applied for user pushing changes into project with different namespace. + This test also tests giving read and write access to another user. Additionally tests also uploading of big file. + """ + test_project = 'test_available_storage_validation2' test_project_fullname = API_USER2 + '/' + test_project # cleanups @@ -521,18 +586,10 @@ def test_available_storage_validation(mc, mc2): access = project_info['access'] access['writersnames'].append(API_USER) access['readersnames'].append(API_USER) - project_info['access'] = access - # TODO create separate test mc2.set_project_access(test_project_fullname, access) - # check writers access - project_info = get_project_info(mc2, API_USER2, test_project) - access = project_info['access'] - assert API_USER in access['writersnames'] - # download project mc.download_project(test_project_fullname, project_dir) - mp = MerginProject(project_dir) # get user_info about storage capacity user_info = mc.user_info() From 4a621622a7390d5e98033d0119f7989be3892817 Mon Sep 17 00:00:00 2001 From: vsklencar Date: Mon, 14 Dec 2020 11:29:18 +0100 Subject: [PATCH 5/7] Test for storage limit and permissions Add cleanup for test_set_access --- mergin/test/test_client.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index 5bb70533..0fe964cf 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -505,6 +505,10 @@ def test_set_read_write_access(mc): test_project = 'test_set_access' test_project_fullname = API_USER + '/' + test_project + # cleanups + project_dir = os.path.join(TMP_DIR, test_project, API_USER) + cleanup(mc, test_project_fullname, [project_dir]) + # create new (empty) project on server mc.create_project(test_project) @@ -556,7 +560,7 @@ def test_available_storage_validation(mc): except ClientError as e: # Expecting "Storage limit has been reached" error msg. assert str(e).startswith("Storage limit has been reached") - got_right_err = True + got_right_err = True assert got_right_err # Expecting empty project From aa01ef53176d1add0600d87ca8048709ed8552a0 Mon Sep 17 00:00:00 2001 From: vsklencar Date: Mon, 14 Dec 2020 11:45:00 +0100 Subject: [PATCH 6/7] Test for storage limit and permissions Fixed name --- mergin/test/test_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index 0fe964cf..4fa340a0 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -502,7 +502,7 @@ def test_clone_project(mc): def test_set_read_write_access(mc): - test_project = 'test_set_access' + test_project = 'test_set_read_write_access' test_project_fullname = API_USER + '/' + test_project # cleanups From f478794a245905fccb257843c437e0ec812d0a74 Mon Sep 17 00:00:00 2001 From: vsklencar Date: Mon, 14 Dec 2020 14:26:24 +0100 Subject: [PATCH 7/7] Test for storage limit and permissions Improved docs --- mergin/test/test_client.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py index 4fa340a0..6aa3bd27 100644 --- a/mergin/test/test_client.py +++ b/mergin/test/test_client.py @@ -528,7 +528,7 @@ def test_set_read_write_access(mc): def test_available_storage_validation(mc): """ - Testing of storage limit - should not be applied for user pushing changes into project with different namespace. + Testing of storage limit - applies to user pushing changes into own project (namespace matching username). This test also tests giving read and write access to another user. Additionally tests also uploading of big file. """ test_project = 'test_available_storage_validation' @@ -572,7 +572,13 @@ def test_available_storage_validation(mc): def test_available_storage_validation2(mc, mc2): """ Testing of storage limit - should not be applied for user pushing changes into project with different namespace. - This test also tests giving read and write access to another user. Additionally tests also uploading of big file. + This should cover the exception of mergin-py-client that a user can push changes to someone else's project regardless + the user's own storage limitation. Of course, other limitations are still applied (write access, owner of + a modified project has to have enough free storage). + + Therefore NOTE that there are following assumptions: + - API_USER2's free storage >= API_USER's free storage + 1024b (size of changes to be pushed) + - both accounts should ideally have a free plan """ test_project = 'test_available_storage_validation2' test_project_fullname = API_USER2 + '/' + test_project