From 8abbce5de8bf77040f60c134b5c62db4e3969dae Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Wed, 30 Oct 2024 16:47:47 +0100 Subject: [PATCH 01/11] Add check for user x permissions to InstalledCode In `validate_filepath_executable` called for `verdi code test` --- src/aiida/orm/nodes/data/code/installed.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 5f9fb39749..30f3698c39 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -137,6 +137,10 @@ def validate_filepath_executable(self): with override_log_level(): # Temporarily suppress noisy logging with self.computer.get_transport() as transport: file_exists = transport.isfile(str(self.filepath_executable)) + mode = transport.get_mode(str(self.filepath_executable)) + user_permissions = mode >> 6 # Shift right to get the user permission bits + user_has_execute = (user_permissions & 1) != 0 # Check if the execute bit is set + except Exception as exception: raise exceptions.ValidationError( 'Could not connect to the configured computer to determine whether the specified executable exists.' @@ -147,6 +151,12 @@ def validate_filepath_executable(self): f'The provided remote absolute path `{self.filepath_executable}` does not exist on the computer.' ) + if not user_has_execute: + raise exceptions.ValidationError( + f'The executable at the remote absolute path `{self.filepath_executable}` is not executable.' + ) + + def can_run_on_computer(self, computer: Computer) -> bool: """Return whether the code can run on a given computer. From 9ba66143b1effa35d09984d8c41ae9c87043e159 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 30 Oct 2024 15:49:34 +0000 Subject: [PATCH 02/11] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/aiida/orm/nodes/data/code/installed.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 30f3698c39..c57fcedaf7 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -156,7 +156,6 @@ def validate_filepath_executable(self): f'The executable at the remote absolute path `{self.filepath_executable}` is not executable.' ) - def can_run_on_computer(self, computer: Computer) -> bool: """Return whether the code can run on a given computer. From e810dd9999d0a3e43a76b083b9cbf3bcd549d775 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Thu, 31 Oct 2024 18:11:46 +0100 Subject: [PATCH 03/11] Add test and make error message more vague --- src/aiida/orm/nodes/data/code/installed.py | 15 +++++++++------ tests/orm/data/code/test_installed.py | 13 +++++++++++-- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index c57fcedaf7..1c0b2d103e 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -137,23 +137,26 @@ def validate_filepath_executable(self): with override_log_level(): # Temporarily suppress noisy logging with self.computer.get_transport() as transport: file_exists = transport.isfile(str(self.filepath_executable)) - mode = transport.get_mode(str(self.filepath_executable)) - user_permissions = mode >> 6 # Shift right to get the user permission bits - user_has_execute = (user_permissions & 1) != 0 # Check if the execute bit is set + if file_exists: + mode = transport.get_mode(str(self.filepath_executable)) + # Shift right to get the user permission bits + user_permissions = mode >> 6 + # Check if the execute bit is set + user_has_execute = (user_permissions & 1) != 0 except Exception as exception: raise exceptions.ValidationError( - 'Could not connect to the configured computer to determine whether the specified executable exists.' + "Could not connect to the configured computer to determine whether the specified executable exists." ) from exception if not file_exists: raise exceptions.ValidationError( - f'The provided remote absolute path `{self.filepath_executable}` does not exist on the computer.' + f"The provided remote absolute path `{self.filepath_executable}` does not exist on the computer." ) if not user_has_execute: raise exceptions.ValidationError( - f'The executable at the remote absolute path `{self.filepath_executable}` is not executable.' + f"The executable at the remote absolute path `{self.filepath_executable}` exists, but might not actually be executable." ) def can_run_on_computer(self, computer: Computer) -> bool: diff --git a/tests/orm/data/code/test_installed.py b/tests/orm/data/code/test_installed.py index 928d718080..3bf0bb2670 100644 --- a/tests/orm/data/code/test_installed.py +++ b/tests/orm/data/code/test_installed.py @@ -13,7 +13,7 @@ import pytest from aiida.common.exceptions import ModificationNotAllowed, ValidationError from aiida.common.warnings import AiidaDeprecationWarning -from aiida.orm import Computer +from aiida.orm import Computer, User from aiida.orm.nodes.data.code.installed import InstalledCode @@ -101,10 +101,15 @@ def computer(request, aiida_computer_local, aiida_computer_ssh): @pytest.mark.usefixtures('aiida_profile_clean') @pytest.mark.parametrize('computer', ('core.local', 'core.ssh'), indirect=True) -def test_validate_filepath_executable(ssh_key, computer, bash_path): +def test_validate_filepath_executable(ssh_key, computer, bash_path, tmp_path): """Test the :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.validate_filepath_executable` method.""" + import os filepath_executable = '/usr/bin/not-existing' + dummy_executable = tmp_path / 'dummy.sh' + # Default Linux permissions are 664, so file is not executable + dummy_executable.touch() code = InstalledCode(computer=computer, filepath_executable=filepath_executable) + dummy_code = InstalledCode(computer=computer, filepath_executable=str(dummy_executable)) with pytest.raises(ValidationError, match=r'Could not connect to the configured computer.*'): code.validate_filepath_executable() @@ -117,6 +122,10 @@ def test_validate_filepath_executable(ssh_key, computer, bash_path): with pytest.raises(ValidationError, match=r'The provided remote absolute path .* does not exist on the computer\.'): code.validate_filepath_executable() + with pytest.raises(ValidationError, match=r'The executable at the remote absolute path .* exists, but might not actually be executable\.'): + # Didn't put this in the if-else and use transport.put, as we anyway only connect to localhost via SSH in this test + dummy_code.validate_filepath_executable() + code.filepath_executable = str(bash_path.absolute()) code.validate_filepath_executable() From efd2dd72385bfeb4319f8da3d4973f51c6175ca6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 31 Oct 2024 17:16:41 +0000 Subject: [PATCH 04/11] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/aiida/orm/nodes/data/code/installed.py | 6 +++--- tests/orm/data/code/test_installed.py | 9 ++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 1c0b2d103e..f418da0550 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -146,17 +146,17 @@ def validate_filepath_executable(self): except Exception as exception: raise exceptions.ValidationError( - "Could not connect to the configured computer to determine whether the specified executable exists." + 'Could not connect to the configured computer to determine whether the specified executable exists.' ) from exception if not file_exists: raise exceptions.ValidationError( - f"The provided remote absolute path `{self.filepath_executable}` does not exist on the computer." + f'The provided remote absolute path `{self.filepath_executable}` does not exist on the computer.' ) if not user_has_execute: raise exceptions.ValidationError( - f"The executable at the remote absolute path `{self.filepath_executable}` exists, but might not actually be executable." + f'The executable at the remote absolute path `{self.filepath_executable}` exists, but might not actually be executable.' ) def can_run_on_computer(self, computer: Computer) -> bool: diff --git a/tests/orm/data/code/test_installed.py b/tests/orm/data/code/test_installed.py index 3bf0bb2670..6140ee65cc 100644 --- a/tests/orm/data/code/test_installed.py +++ b/tests/orm/data/code/test_installed.py @@ -13,7 +13,7 @@ import pytest from aiida.common.exceptions import ModificationNotAllowed, ValidationError from aiida.common.warnings import AiidaDeprecationWarning -from aiida.orm import Computer, User +from aiida.orm import Computer from aiida.orm.nodes.data.code.installed import InstalledCode @@ -103,7 +103,7 @@ def computer(request, aiida_computer_local, aiida_computer_ssh): @pytest.mark.parametrize('computer', ('core.local', 'core.ssh'), indirect=True) def test_validate_filepath_executable(ssh_key, computer, bash_path, tmp_path): """Test the :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.validate_filepath_executable` method.""" - import os + filepath_executable = '/usr/bin/not-existing' dummy_executable = tmp_path / 'dummy.sh' # Default Linux permissions are 664, so file is not executable @@ -122,7 +122,10 @@ def test_validate_filepath_executable(ssh_key, computer, bash_path, tmp_path): with pytest.raises(ValidationError, match=r'The provided remote absolute path .* does not exist on the computer\.'): code.validate_filepath_executable() - with pytest.raises(ValidationError, match=r'The executable at the remote absolute path .* exists, but might not actually be executable\.'): + with pytest.raises( + ValidationError, + match=r'The executable at the remote absolute path .* exists, but might not actually be executable\.', + ): # Didn't put this in the if-else and use transport.put, as we anyway only connect to localhost via SSH in this test dummy_code.validate_filepath_executable() From 1e6752bad988d64196e1c5a477b6b9b521195ab2 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Thu, 31 Oct 2024 18:25:41 +0100 Subject: [PATCH 05/11] Line too long errors... --- src/aiida/orm/nodes/data/code/installed.py | 6 ++++-- tests/orm/data/code/test_installed.py | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index f418da0550..31191377f9 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -155,9 +155,11 @@ def validate_filepath_executable(self): ) if not user_has_execute: - raise exceptions.ValidationError( - f'The executable at the remote absolute path `{self.filepath_executable}` exists, but might not actually be executable.' + execute_msg = ( + f'The executable at the remote absolute path `{self.filepath_executable}` exists, ' + 'but might not actually be executable.' ) + raise exceptions.ValidationError(execute_msg) def can_run_on_computer(self, computer: Computer) -> bool: """Return whether the code can run on a given computer. diff --git a/tests/orm/data/code/test_installed.py b/tests/orm/data/code/test_installed.py index 6140ee65cc..275d7432ac 100644 --- a/tests/orm/data/code/test_installed.py +++ b/tests/orm/data/code/test_installed.py @@ -126,7 +126,8 @@ def test_validate_filepath_executable(ssh_key, computer, bash_path, tmp_path): ValidationError, match=r'The executable at the remote absolute path .* exists, but might not actually be executable\.', ): - # Didn't put this in the if-else and use transport.put, as we anyway only connect to localhost via SSH in this test + # Didn't put this in the if-else and use transport.put, as we anyway only connect to localhost via SSH in this + # test dummy_code.validate_filepath_executable() code.filepath_executable = str(bash_path.absolute()) From 80c5d0cd2ad1eebd7f3b71077bb86bc02008fe79 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Mon, 4 Nov 2024 13:56:53 +0100 Subject: [PATCH 06/11] Apply suggestions from code review Co-authored-by: Alexander Goscinski --- src/aiida/orm/nodes/data/code/installed.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 31191377f9..ad05fd45b0 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -139,10 +139,8 @@ def validate_filepath_executable(self): file_exists = transport.isfile(str(self.filepath_executable)) if file_exists: mode = transport.get_mode(str(self.filepath_executable)) - # Shift right to get the user permission bits - user_permissions = mode >> 6 - # Check if the execute bit is set - user_has_execute = (user_permissions & 1) != 0 + # check if execute but is set + user_has_execute = format(mode, "b")[6] == '1' except Exception as exception: raise exceptions.ValidationError( From 2e172d2434e8d03cbfb415182dfadc0d8600bbb6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 4 Nov 2024 12:57:09 +0000 Subject: [PATCH 07/11] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/aiida/orm/nodes/data/code/installed.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index ad05fd45b0..01009b2210 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -140,7 +140,7 @@ def validate_filepath_executable(self): if file_exists: mode = transport.get_mode(str(self.filepath_executable)) # check if execute but is set - user_has_execute = format(mode, "b")[6] == '1' + user_has_execute = format(mode, 'b')[6] == '1' except Exception as exception: raise exceptions.ValidationError( From 0f02d914b1d3a795bd6c91aeabf747c831eb48c4 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Tue, 5 Nov 2024 08:20:49 +0100 Subject: [PATCH 08/11] Fix user execute check --- src/aiida/orm/nodes/data/code/installed.py | 6 ++++-- tests/orm/data/code/test_installed.py | 3 +-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 01009b2210..b532cbcb71 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -139,8 +139,10 @@ def validate_filepath_executable(self): file_exists = transport.isfile(str(self.filepath_executable)) if file_exists: mode = transport.get_mode(str(self.filepath_executable)) - # check if execute but is set - user_has_execute = format(mode, 'b')[6] == '1' + # `format(mode, 'b')` with default permissions + # gives 110110100, representing rw-rw-r-- + # Check on index 2 if user has execute + user_has_execute = format(mode, 'b')[2] == '1' except Exception as exception: raise exceptions.ValidationError( diff --git a/tests/orm/data/code/test_installed.py b/tests/orm/data/code/test_installed.py index 275d7432ac..e5592d60d6 100644 --- a/tests/orm/data/code/test_installed.py +++ b/tests/orm/data/code/test_installed.py @@ -126,8 +126,7 @@ def test_validate_filepath_executable(ssh_key, computer, bash_path, tmp_path): ValidationError, match=r'The executable at the remote absolute path .* exists, but might not actually be executable\.', ): - # Didn't put this in the if-else and use transport.put, as we anyway only connect to localhost via SSH in this - # test + # Didn't put this in the if, using transport.put, as we anyway only connect to localhost via SSH in this test dummy_code.validate_filepath_executable() code.filepath_executable = str(bash_path.absolute()) From d74bcb813d2461dd527fb52d10dc3f79345f4bc6 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Tue, 5 Nov 2024 08:52:36 +0100 Subject: [PATCH 09/11] Apply suggestions from code review Co-authored-by: Ali Khosravi --- src/aiida/orm/nodes/data/code/installed.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index b532cbcb71..701588ecf9 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -156,7 +156,7 @@ def validate_filepath_executable(self): if not user_has_execute: execute_msg = ( - f'The executable at the remote absolute path `{self.filepath_executable}` exists, ' + f'File `{self.filepath_executable}` exists, ' 'but might not actually be executable.' ) raise exceptions.ValidationError(execute_msg) From a372ad6668199ea72844c0e50ec2ac3e8231f53f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 5 Nov 2024 07:53:26 +0000 Subject: [PATCH 10/11] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/aiida/orm/nodes/data/code/installed.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 701588ecf9..2a23375c9c 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -155,10 +155,7 @@ def validate_filepath_executable(self): ) if not user_has_execute: - execute_msg = ( - f'File `{self.filepath_executable}` exists, ' - 'but might not actually be executable.' - ) + execute_msg = f'File `{self.filepath_executable}` exists, ' 'but might not actually be executable.' raise exceptions.ValidationError(execute_msg) def can_run_on_computer(self, computer: Computer) -> bool: From 618f086824ad6539d8dd56c7e7262532dd60e50f Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Tue, 5 Nov 2024 10:52:50 +0100 Subject: [PATCH 11/11] Update test --- src/aiida/orm/nodes/data/code/installed.py | 5 ++++- tests/orm/data/code/test_installed.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 2a23375c9c..7546c2acb8 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -155,7 +155,10 @@ def validate_filepath_executable(self): ) if not user_has_execute: - execute_msg = f'File `{self.filepath_executable}` exists, ' 'but might not actually be executable.' + execute_msg = ( + f'The file at the remote absolute path `{self.filepath_executable}` exists, ' + 'but might not actually be executable. Check the permissions.' + ) raise exceptions.ValidationError(execute_msg) def can_run_on_computer(self, computer: Computer) -> bool: diff --git a/tests/orm/data/code/test_installed.py b/tests/orm/data/code/test_installed.py index e5592d60d6..15c297f43e 100644 --- a/tests/orm/data/code/test_installed.py +++ b/tests/orm/data/code/test_installed.py @@ -124,7 +124,7 @@ def test_validate_filepath_executable(ssh_key, computer, bash_path, tmp_path): with pytest.raises( ValidationError, - match=r'The executable at the remote absolute path .* exists, but might not actually be executable\.', + match=r'The file at the remote absolute path .* exists, but might not actually be executable\.', ): # Didn't put this in the if, using transport.put, as we anyway only connect to localhost via SSH in this test dummy_code.validate_filepath_executable()