From b7c5c68653b2e4589b0ee7d7efb84e4936725ddd Mon Sep 17 00:00:00 2001 From: dbeltran Date: Thu, 29 Aug 2024 10:01:44 +0200 Subject: [PATCH 1/6] Fixed an issue with permissions --- autosubmit/autosubmit.py | 17 +++++++++++------ test/unit/test_ownership.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 test/unit/test_ownership.py diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index 73dcdf46..5d42d4d5 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -857,7 +857,6 @@ class Autosubmit: expids = [x.strip() for x in expids] for expid in expids: as_conf = AutosubmitConfig(expid, BasicConfig, YAMLParserFactory()) - as_conf.set_last_as_command(args.command) as_conf.reload(force_load=True) if len(as_conf.experiment_data) == 0: @@ -871,11 +870,7 @@ class Autosubmit: if not os.path.exists(exp_path): raise AutosubmitCritical("Experiment does not exist", 7012) # delete is treated differently - if args.command not in ["monitor", "describe", "delete", "report", "stats", "dbfix"]: - owner, eadmin, currentOwner = Autosubmit._check_ownership(expid, raise_error=True) - else: - owner, eadmin, currentOwner = Autosubmit._check_ownership(expid, raise_error=False) - + owner, eadmin, currentOwner = Autosubmit._check_ownership_and_set_last_command(as_conf, expid, args.command) if not os.path.exists(tmp_path): os.mkdir(tmp_path) if not os.path.exists(aslogs_path): @@ -969,6 +964,16 @@ class Autosubmit: Log.info( "Autosubmit is running with {0}", Autosubmit.autosubmit_version) + @staticmethod + def _check_ownership_and_set_last_command(as_conf, expid, command): + if command not in ["monitor", "describe", "delete", "report", "stats", "dbfix"]: + owner, eadmin, currentOwner = Autosubmit._check_ownership(expid, raise_error=True) + else: + owner, eadmin, currentOwner = Autosubmit._check_ownership(expid, raise_error=False) + if owner: + as_conf.set_last_as_command(command) + return owner, eadmin, currentOwner + @staticmethod def _check_ownership(expid, raise_error=False): """ diff --git a/test/unit/test_ownership.py b/test/unit/test_ownership.py new file mode 100644 index 00000000..0a6c0261 --- /dev/null +++ b/test/unit/test_ownership.py @@ -0,0 +1,30 @@ +import pytest + +# Assuming the function is in a module named `autosubmit` +from autosubmit.autosubmit import Autosubmit + +@pytest.fixture +def mock_as_conf(mocker): + return mocker.MagicMock() + +@pytest.fixture +def mock_autosubmit(mocker): + return mocker.patch.object(Autosubmit, '_check_ownership', return_value=("owner", "eadmin", "currentOwner")) + +def test_check_ownership_and_set_last_command(mock_as_conf, mock_autosubmit): + expid = "test_expid" + commands = ["monitor", "describe", "delete", "report", "stats", "dbfix", "other"] + + for command in commands: + owner, eadmin, currentOwner = Autosubmit._check_ownership_and_set_last_command(mock_as_conf, expid, command) + + if command in ["monitor", "describe", "delete", "report", "stats", "dbfix"]: + mock_autosubmit.assert_called_with(expid, raise_error=False) + else: + mock_autosubmit.assert_called_with(expid, raise_error=True) + + if owner: + mock_as_conf.set_last_as_command.assert_called_with(command) + assert owner == "owner" + assert eadmin == "eadmin" + assert currentOwner == "currentOwner" \ No newline at end of file -- GitLab From 2e8452ab5504e7a79633b460063bc6f5f104c041 Mon Sep 17 00:00:00 2001 From: dbeltran Date: Thu, 29 Aug 2024 15:16:50 +0200 Subject: [PATCH 2/6] Rewritted the ownership and added test --- autosubmit/autosubmit.py | 69 +++++++++++++------------------------ test/unit/test_ownership.py | 58 +++++++++++++++++++++---------- 2 files changed, 63 insertions(+), 64 deletions(-) diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index 5d42d4d5..ce795b25 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -967,64 +967,41 @@ class Autosubmit: @staticmethod def _check_ownership_and_set_last_command(as_conf, expid, command): if command not in ["monitor", "describe", "delete", "report", "stats", "dbfix"]: - owner, eadmin, currentOwner = Autosubmit._check_ownership(expid, raise_error=True) + owner, eadmin, current_owner = Autosubmit._check_ownership(expid, raise_error=True) else: - owner, eadmin, currentOwner = Autosubmit._check_ownership(expid, raise_error=False) + owner, eadmin, current_owner = Autosubmit._check_ownership(expid, raise_error=False) if owner: as_conf.set_last_as_command(command) - return owner, eadmin, currentOwner + return owner, eadmin, current_owner @staticmethod def _check_ownership(expid, raise_error=False): """ - Check if user owns or if it is edamin - :return: owner,eadmin - :rtype: boolean,boolean + Check if the user owns and if it is edamin + :return: the owner, eadmin and current_owner + :rtype: boolean, boolean, str """ - # return HelperUtils.check_experiment_ownership(expid, BasicConfig, raise_error, Log) - # Read current login - # Read current user uid - my_user = os.getuid() - # Read eadmin user uid - owner = False + current_owner = "empty" + current_owner_id = -1 eadmin = False + owner = False + current_user_id = os.getuid() # execute a id -u eadmin and supress error message - FNULL = open(os.devnull, 'w') - id_eadmin = subprocess.call(['id', '-u', 'eadmin'], - stdout=FNULL, - stderr=subprocess.STDOUT) - - # id_eadmin = subprocess.Popen('id -u eadmin',stdout=fp) - ret = False - # Handling possible failure of retrieval of current owner data - currentOwner_id = 0 - currentOwner = "empty" + if current_user_id == subprocess.call(['id', '-u', 'eadmin'], + stdout=open(os.devnull, 'w'), + stderr=subprocess.STDOUT): + eadmin = True + current_owner_id = os.stat(os.path.join(BasicConfig.LOCAL_ROOT_DIR, expid)).st_uid try: - currentOwner_id = os.stat(os.path.join(BasicConfig.LOCAL_ROOT_DIR, expid)).st_uid - currentOwner = pwd.getpwuid(os.stat(os.path.join(BasicConfig.LOCAL_ROOT_DIR, expid)).st_uid).pw_name - except Exception as e: - pass - finally: - if currentOwner_id <= 0: - Log.info("Current owner '{0}' of experiment {1} does not exist anymore.", currentOwner, expid) - if currentOwner_id == my_user: + current_owner = pwd.getpwuid(current_owner_id).pw_name + except BaseException: + if current_owner_id < 0: + Log.warning(f"Current owner of experiment {expid} could not be retrieved. The owner is no longer in the system database.") + if current_owner_id == current_user_id: owner = True - if my_user == id_eadmin: - eadmin = True - if owner and raise_error: - try: - current_user_id = pwd.getpwuid(os.getuid())[0] - current_owner_id = pwd.getpwuid(os.stat(os.path.join( - BasicConfig.LOCAL_ROOT_DIR, expid)).st_uid).pw_name - if current_user_id != current_owner_id: - raise AutosubmitCritical( - "You don't own the experiment {0}.".format(expid), 7012) - except BaseException as e: - raise AutosubmitCritical( - "User or owner does not exists", 7012, str(e)) - except AutosubmitCritical as e: - raise - return owner, eadmin, currentOwner + elif raise_error: + raise AutosubmitCritical("You don't own the experiment {0}.".format(expid), 7012) + return owner, eadmin, current_owner @staticmethod def _delete_expid(expid_delete, force=False): diff --git a/test/unit/test_ownership.py b/test/unit/test_ownership.py index 0a6c0261..2d6718bc 100644 --- a/test/unit/test_ownership.py +++ b/test/unit/test_ownership.py @@ -1,30 +1,52 @@ import pytest - -# Assuming the function is in a module named `autosubmit` from autosubmit.autosubmit import Autosubmit +from log.log import AutosubmitCritical @pytest.fixture def mock_as_conf(mocker): return mocker.MagicMock() @pytest.fixture -def mock_autosubmit(mocker): - return mocker.patch.object(Autosubmit, '_check_ownership', return_value=("owner", "eadmin", "currentOwner")) +def mock_owner_user_same(mocker): + mock = mocker.patch('os.getuid') + mock.return_value = 1 # user + mock2 = mocker.patch('os.stat') + mock2.return_value.st_uid = 1 # owner + mock3 = mocker.patch('pwd.getpwuid') + mock3.return_value.pw_name = "test1" + return mock, mock2, mock3 + +@pytest.fixture +def mock_owner_user_diff(mocker): + mock = mocker.patch('os.getuid') + mock.return_value = 1 # user + mock2 = mocker.patch('os.stat') + mock2.return_value.st_uid = 2 # owner + mock3 = mocker.patch('pwd.getpwuid') + mock3.return_value.pw_name = "test1" + return mock, mock2, mock3 -def test_check_ownership_and_set_last_command(mock_as_conf, mock_autosubmit): - expid = "test_expid" - commands = ["monitor", "describe", "delete", "report", "stats", "dbfix", "other"] +def test_check_ownership_and_set_last_command_same_owner(mock_as_conf, mock_owner_user_same): + autosubmit = Autosubmit() + expid = "testexpid" + for command in ['monitor', 'other']: + owner, eadmin, current_owner = autosubmit._check_ownership_and_set_last_command(mock_as_conf, expid, command) + assert owner is True + assert eadmin is True + assert current_owner == "test1" + assert mock_as_conf.set_last_as_command.called - for command in commands: - owner, eadmin, currentOwner = Autosubmit._check_ownership_and_set_last_command(mock_as_conf, expid, command) - if command in ["monitor", "describe", "delete", "report", "stats", "dbfix"]: - mock_autosubmit.assert_called_with(expid, raise_error=False) - else: - mock_autosubmit.assert_called_with(expid, raise_error=True) - if owner: - mock_as_conf.set_last_as_command.assert_called_with(command) - assert owner == "owner" - assert eadmin == "eadmin" - assert currentOwner == "currentOwner" \ No newline at end of file +def test_check_ownership_and_set_last_command_diff_owner(mock_as_conf, mock_owner_user_diff): + autosubmit = Autosubmit() + expid = "testexpid" + command = "monitor" + owner, eadmin, current_owner = autosubmit._check_ownership_and_set_last_command(mock_as_conf, expid, command) + assert owner is False + assert eadmin is True + assert current_owner == "test1" + assert not mock_as_conf.set_last_as_command.called + command = "other" + with pytest.raises(AutosubmitCritical): + autosubmit._check_ownership_and_set_last_command(mock_as_conf, expid, command) -- GitLab From 9fbd04fea9da06029e9946a49d3120c0f3c57b8d Mon Sep 17 00:00:00 2001 From: dbeltran Date: Thu, 29 Aug 2024 15:51:23 +0200 Subject: [PATCH 3/6] Fixed indent error --- autosubmit/autosubmit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index ce795b25..d387f0e8 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -991,7 +991,7 @@ class Autosubmit: stdout=open(os.devnull, 'w'), stderr=subprocess.STDOUT): eadmin = True - current_owner_id = os.stat(os.path.join(BasicConfig.LOCAL_ROOT_DIR, expid)).st_uid + current_owner_id = os.stat(os.path.join(BasicConfig.LOCAL_ROOT_DIR, expid)).st_uid try: current_owner = pwd.getpwuid(current_owner_id).pw_name except BaseException: -- GitLab From 458a8feb5c9311ffbb205b3b6c3a86440d17f12c Mon Sep 17 00:00:00 2001 From: dbeltran Date: Mon, 2 Sep 2024 13:41:07 +0200 Subject: [PATCH 4/6] Feedback addressed --- autosubmit/autosubmit.py | 20 ++++++++++---------- test/unit/test_ownership.py | 21 ++++++++++++--------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index d387f0e8..d11b0c98 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -974,6 +974,7 @@ class Autosubmit: as_conf.set_last_as_command(command) return owner, eadmin, current_owner + @staticmethod def _check_ownership(expid, raise_error=False): """ @@ -982,21 +983,20 @@ class Autosubmit: :rtype: boolean, boolean, str """ current_owner = "empty" - current_owner_id = -1 eadmin = False owner = False current_user_id = os.getuid() - # execute a id -u eadmin and supress error message - if current_user_id == subprocess.call(['id', '-u', 'eadmin'], - stdout=open(os.devnull, 'w'), - stderr=subprocess.STDOUT): - eadmin = True - current_owner_id = os.stat(os.path.join(BasicConfig.LOCAL_ROOT_DIR, expid)).st_uid + admin_user = "eadmin" # to be improved in #944 + try: + if current_user_id == pwd.getpwnam(admin_user).pw_uid: + eadmin = True + except: + Log.info(f"Autosubmit admin user:{admin_user} is not set") + current_owner_id = Path(BasicConfig.LOCAL_ROOT_DIR, expid).stat().st_uid try: current_owner = pwd.getpwuid(current_owner_id).pw_name - except BaseException: - if current_owner_id < 0: - Log.warning(f"Current owner of experiment {expid} could not be retrieved. The owner is no longer in the system database.") + except (TypeError,KeyError): + Log.warning(f"Current owner of experiment {expid} could not be retrieved. The owner is no longer in the system database.") if current_owner_id == current_user_id: owner = True elif raise_error: diff --git a/test/unit/test_ownership.py b/test/unit/test_ownership.py index 2d6718bc..84a964f7 100644 --- a/test/unit/test_ownership.py +++ b/test/unit/test_ownership.py @@ -2,49 +2,52 @@ import pytest from autosubmit.autosubmit import Autosubmit from log.log import AutosubmitCritical + @pytest.fixture def mock_as_conf(mocker): return mocker.MagicMock() + @pytest.fixture def mock_owner_user_same(mocker): mock = mocker.patch('os.getuid') - mock.return_value = 1 # user - mock2 = mocker.patch('os.stat') - mock2.return_value.st_uid = 1 # owner + mock.return_value = 1 # user + mock2 = mocker.patch('pathlib.Path.stat') + mock2.return_value.st_uid = 1 # owner mock3 = mocker.patch('pwd.getpwuid') mock3.return_value.pw_name = "test1" return mock, mock2, mock3 + @pytest.fixture def mock_owner_user_diff(mocker): mock = mocker.patch('os.getuid') - mock.return_value = 1 # user - mock2 = mocker.patch('os.stat') - mock2.return_value.st_uid = 2 # owner + mock.return_value = 1 # user + mock2 = mocker.patch('pathlib.Path.stat') + mock2.return_value.st_uid = 2 # owner mock3 = mocker.patch('pwd.getpwuid') mock3.return_value.pw_name = "test1" return mock, mock2, mock3 + def test_check_ownership_and_set_last_command_same_owner(mock_as_conf, mock_owner_user_same): autosubmit = Autosubmit() expid = "testexpid" for command in ['monitor', 'other']: owner, eadmin, current_owner = autosubmit._check_ownership_and_set_last_command(mock_as_conf, expid, command) assert owner is True - assert eadmin is True + assert eadmin is False assert current_owner == "test1" assert mock_as_conf.set_last_as_command.called - def test_check_ownership_and_set_last_command_diff_owner(mock_as_conf, mock_owner_user_diff): autosubmit = Autosubmit() expid = "testexpid" command = "monitor" owner, eadmin, current_owner = autosubmit._check_ownership_and_set_last_command(mock_as_conf, expid, command) assert owner is False - assert eadmin is True + assert eadmin is False assert current_owner == "test1" assert not mock_as_conf.set_last_as_command.called command = "other" -- GitLab From 5261a2d912f419956811773565a2458f405afff2 Mon Sep 17 00:00:00 2001 From: dbeltran Date: Thu, 5 Sep 2024 09:40:35 +0200 Subject: [PATCH 5/6] Feedback from Bruno --- autosubmit/autosubmit.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index d11b0c98..718e79bf 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -870,7 +870,7 @@ class Autosubmit: if not os.path.exists(exp_path): raise AutosubmitCritical("Experiment does not exist", 7012) # delete is treated differently - owner, eadmin, currentOwner = Autosubmit._check_ownership_and_set_last_command(as_conf, expid, args.command) + owner, eadmin, current_owner = Autosubmit._check_ownership_and_set_last_command(as_conf, expid, args.command) if not os.path.exists(tmp_path): os.mkdir(tmp_path) if not os.path.exists(aslogs_path): @@ -982,14 +982,13 @@ class Autosubmit: :return: the owner, eadmin and current_owner :rtype: boolean, boolean, str """ - current_owner = "empty" + current_owner = None eadmin = False owner = False current_user_id = os.getuid() admin_user = "eadmin" # to be improved in #944 try: - if current_user_id == pwd.getpwnam(admin_user).pw_uid: - eadmin = True + eadmin = current_user_id == pwd.getpwnam(admin_user).pw_uid except: Log.info(f"Autosubmit admin user:{admin_user} is not set") current_owner_id = Path(BasicConfig.LOCAL_ROOT_DIR, expid).stat().st_uid @@ -1027,7 +1026,7 @@ class Autosubmit: "structure_{0}.db".format(expid_delete)) + "\n" message += os.path.join(BasicConfig.LOCAL_ROOT_DIR, BasicConfig.JOBDATA_DIR, "job_data_{0}.db".format(expid_delete)) + "\n" - owner, eadmin, currentOwner = Autosubmit._check_ownership(expid_delete) + owner, eadmin, current_owner = Autosubmit._check_ownership(expid_delete) if expid_delete == '' or expid_delete is None and not os.path.exists( os.path.join(str(BasicConfig.LOCAL_ROOT_DIR), str(expid_delete))): Log.printlog("Experiment directory does not exist.", Log.WARNING) @@ -1038,8 +1037,10 @@ class Autosubmit: try: if owner or (force and eadmin): if force and eadmin: - Log.info("Preparing deletion of experiment {0} from owner: {1}, as eadmin.", expid_delete, - currentOwner) + if current_owner: + Log.info(f"Preparing deletion of experiment {expid_delete} as eadmin. Current owner: {current_owner}") + else: + Log.info(f"Preparing deletion of experiment {expid_delete} as eadmin. Current owner: Unknown") try: Log.info("Deleting experiment from database...") try: -- GitLab From 1e098b79c689262d0e3187d4de4d89498cfa7413 Mon Sep 17 00:00:00 2001 From: dbeltran Date: Thu, 5 Sep 2024 14:16:02 +0200 Subject: [PATCH 6/6] Addressed feedback --- autosubmit/autosubmit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index 718e79bf..6bc39f86 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -990,7 +990,7 @@ class Autosubmit: try: eadmin = current_user_id == pwd.getpwnam(admin_user).pw_uid except: - Log.info(f"Autosubmit admin user:{admin_user} is not set") + Log.info(f"Autosubmit admin user: {admin_user} is not set") current_owner_id = Path(BasicConfig.LOCAL_ROOT_DIR, expid).stat().st_uid try: current_owner = pwd.getpwuid(current_owner_id).pw_name -- GitLab