From 72b78b9a58b7eef6e93954c6b5e8fbbebea09cce Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Fri, 8 Sep 2017 11:25:46 +0100 Subject: [PATCH 01/13] Add 2 basic tests for cleanse (admin only and admin pass with output) --- .../test/integration/clitest/test_cleanse.py | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 components/tools/OmeroPy/test/integration/clitest/test_cleanse.py diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py new file mode 100644 index 00000000000..f363fab3045 --- /dev/null +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +# +# Copyright (C) 2014-2017 University of Dundee & Open Microscopy Environment. +# All rights reserved. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + + +from test.integration.clitest.cli import CLITest, RootCLITest +from omero.cli import NonZeroReturnCode + +import omero.plugins.admin +import pytest +import omero + + +class TestCleanse(CLITest): + + def setup_method(self, method): + super(TestCleanse, self).setup_method(method) + self.cli.register("admin", omero.plugins.admin.AdminControl, "TEST") + self.args += ["admin", "cleanse"] + + def testCleanseAdminOnly(self, capsys): + """Test cleanse is admin-only""" + data_dir = self.root.sf.getConfigService().getConfigValue("omero.data.dir") + self.args += [data_dir] + with pytest.raises(NonZeroReturnCode): + self.cli.invoke(self.args, strict=True) + out, err = capsys.readouterr() + assert err.endswith("SecurityViolation: Admins only!\n") + +class TestCleanseRoot(RootCLITest): + + def setup_method(self, method): + super(TestCleanseRoot, self).setup_method(method) + self.cli.register("admin", omero.plugins.admin.AdminControl, "TEST") + self.args += ["admin", "cleanse"] + + def testCleanseAdminOnly(self, capsys): + """Test cleanse works for root with expected output""" + data_dir = self.root.sf.getConfigService().getConfigValue("omero.data.dir") + self.args += [data_dir] + self.cli.invoke(self.args, strict=True) + out, err = capsys.readouterr() + output_string = "Removing empty directories from...\n " + data_dir + "ManagedRepository\n" + assert output_string in out From d90d2f401110d68bc6d5485d43d4fd26f785b1e8 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Fri, 8 Sep 2017 12:31:26 +0100 Subject: [PATCH 02/13] flake8 fix --- .../OmeroPy/test/integration/clitest/test_cleanse.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index f363fab3045..a9d987ccbb9 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -37,13 +37,15 @@ def setup_method(self, method): def testCleanseAdminOnly(self, capsys): """Test cleanse is admin-only""" - data_dir = self.root.sf.getConfigService().getConfigValue("omero.data.dir") + config_service = self.root.sf.getConfigService() + data_dir = config_service.getConfigValue("omero.data.dir") self.args += [data_dir] with pytest.raises(NonZeroReturnCode): self.cli.invoke(self.args, strict=True) out, err = capsys.readouterr() assert err.endswith("SecurityViolation: Admins only!\n") + class TestCleanseRoot(RootCLITest): def setup_method(self, method): @@ -53,9 +55,11 @@ def setup_method(self, method): def testCleanseAdminOnly(self, capsys): """Test cleanse works for root with expected output""" - data_dir = self.root.sf.getConfigService().getConfigValue("omero.data.dir") + config_service = self.root.sf.getConfigService() + data_dir = config_service.getConfigValue("omero.data.dir") self.args += [data_dir] self.cli.invoke(self.args, strict=True) out, err = capsys.readouterr() - output_string = "Removing empty directories from...\n " + data_dir + "ManagedRepository\n" + output_string_start = "Removing empty directories from...\n " + output_string = output_string_start + data_dir + "ManagedRepository\n" assert output_string in out From e9e4a37937e0ab3e5ceb42a6855fc5c874a3b1f3 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Tue, 12 Sep 2017 12:47:15 +0100 Subject: [PATCH 03/13] Replace double slashes, use omero.managed.dir to compare --- .../tools/OmeroPy/test/integration/clitest/test_cleanse.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index a9d987ccbb9..01fc887d869 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -61,5 +61,6 @@ def testCleanseAdminOnly(self, capsys): self.cli.invoke(self.args, strict=True) out, err = capsys.readouterr() output_string_start = "Removing empty directories from...\n " - output_string = output_string_start + data_dir + "ManagedRepository\n" + mrepo_dir = config_service.getConfigValue("omero.managed.dir") + output_string = mrepo_dir.replace("//", "/") assert output_string in out From e30ab4c5cb6166a8c0c156e80b199fe8141e30a5 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Tue, 12 Sep 2017 12:53:18 +0100 Subject: [PATCH 04/13] Compare the whole string, also the start --- .../tools/OmeroPy/test/integration/clitest/test_cleanse.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index 01fc887d869..88a851934ea 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -62,5 +62,5 @@ def testCleanseAdminOnly(self, capsys): out, err = capsys.readouterr() output_string_start = "Removing empty directories from...\n " mrepo_dir = config_service.getConfigValue("omero.managed.dir") - output_string = mrepo_dir.replace("//", "/") + output_string = output_string_start + mrepo_dir.replace("//", "/") assert output_string in out From 4d69e5db14a3de08c5b4a7ee70035ef678336e39 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Thu, 14 Sep 2017 19:03:52 +0100 Subject: [PATCH 05/13] Add test for renaming the original file and subsequent run of cleanse --- .../test/integration/clitest/test_cleanse.py | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index 88a851934ea..0cfd113ff1c 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -26,6 +26,10 @@ import omero.plugins.admin import pytest import omero +import omero.gateway +import os.path + +all_grps = {'omero.group': '-1'} class TestCleanse(CLITest): @@ -51,9 +55,10 @@ class TestCleanseRoot(RootCLITest): def setup_method(self, method): super(TestCleanseRoot, self).setup_method(method) self.cli.register("admin", omero.plugins.admin.AdminControl, "TEST") + self.import_args = self.args self.args += ["admin", "cleanse"] - def testCleanseAdminOnly(self, capsys): + def testCleanseBasic(self, capsys): """Test cleanse works for root with expected output""" config_service = self.root.sf.getConfigService() data_dir = config_service.getConfigValue("omero.data.dir") @@ -64,3 +69,52 @@ def testCleanseAdminOnly(self, capsys): mrepo_dir = config_service.getConfigValue("omero.managed.dir") output_string = output_string_start + mrepo_dir.replace("//", "/") assert output_string in out + + def testCleanseNonsenseName(self, tmpdir, capsys): + """Test cleanse removes a file when originalFile""" + """has nonsensical name""" + name = "nonsensical" + image = self.import_fake_file() + fileset = self.get_fileset(image) + fileset_id = fileset.getId() + params = omero.sys.ParametersI() + params.addId(fileset_id) + query = ("select details.group.id, originalFile.id, " + "originalFile.path from FilesetEntry where fileset.id = :id") + queryService = self.root.sf.getQueryService() + result = queryService.projection(query, params, all_grps) + group_id = result[0][0].getValue() + group_ctx = {'omero.group': str(group_id)} + orig_file_id = result[0][1].getValue() + path_in_mrepo = result[0][2].getValue() + query = 'from OriginalFile o where o.id = :id' + params_file = omero.sys.ParametersI() + params_file.addId(orig_file_id) + orig_file = queryService.findByQuery(query, params_file, group_ctx) + orig_file_name = orig_file.getName().getValue() + config_service = self.root.sf.getConfigService() + mrepo_dir = config_service.getConfigValue("omero.managed.dir") + path = mrepo_dir + '/' + path_in_mrepo + path = path.replace("//", "/") + assert os.path.exists(path) + orig_file_path = path + '/' + orig_file_name + orig_file_path = orig_file_path.replace("//", "/") + assert os.path.isfile(orig_file_path) + orig_file.setName(omero.rtypes.rstring(name)) + update_service = self.root.sf.getUpdateService() + update_service.saveAndReturnObject(orig_file, group_ctx) + + data_dir = config_service.getConfigValue("omero.data.dir") + self.args += [data_dir] + self.cli.invoke(self.args, strict=True) + out, err = capsys.readouterr() + assert os.path.exists(path) + assert os.path.isfile(orig_file_path) + + query = ("select o from FilesetJobLink l " + "join l.parent as fs join l.child as j " + "join j.originalFileLinks l2 join l2.child as o " + "where fs.id = :id and " + "o.mimetype = 'application/omero-log-file'") + logfile = queryService.findByQuery(query, params, group_ctx) + logfile.getName().getValue() From 22a5fa564086c6baf4ff7539015c0f4808232d99 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Fri, 15 Sep 2017 11:54:34 +0100 Subject: [PATCH 06/13] Delete the original file after renaming, then run cleanse and check --- .../test/integration/clitest/test_cleanse.py | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index 0cfd113ff1c..b56d7b7c18a 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -22,6 +22,7 @@ from test.integration.clitest.cli import CLITest, RootCLITest from omero.cli import NonZeroReturnCode +from omero.cmd import Delete2 import omero.plugins.admin import pytest @@ -74,8 +75,9 @@ def testCleanseNonsenseName(self, tmpdir, capsys): """Test cleanse removes a file when originalFile""" """has nonsensical name""" name = "nonsensical" - image = self.import_fake_file() - fileset = self.get_fileset(image) + images = self.import_fake_file() + ids = [image.id.val for image in images] + fileset = self.get_fileset(images) fileset_id = fileset.getId() params = omero.sys.ParametersI() params.addId(fileset_id) @@ -103,14 +105,6 @@ def testCleanseNonsenseName(self, tmpdir, capsys): orig_file.setName(omero.rtypes.rstring(name)) update_service = self.root.sf.getUpdateService() update_service.saveAndReturnObject(orig_file, group_ctx) - - data_dir = config_service.getConfigValue("omero.data.dir") - self.args += [data_dir] - self.cli.invoke(self.args, strict=True) - out, err = capsys.readouterr() - assert os.path.exists(path) - assert os.path.isfile(orig_file_path) - query = ("select o from FilesetJobLink l " "join l.parent as fs join l.child as j " "join j.originalFileLinks l2 join l2.child as o " @@ -118,3 +112,16 @@ def testCleanseNonsenseName(self, tmpdir, capsys): "o.mimetype = 'application/omero-log-file'") logfile = queryService.findByQuery(query, params, group_ctx) logfile.getName().getValue() + + command = Delete2(targetObjects={"Image": ids}) + handle = self.client.sf.submit(command) + self.wait_on_cmd(self.client, handle) + assert os.path.exists(path) + assert os.path.isfile(orig_file_path) + + data_dir = config_service.getConfigValue("omero.data.dir") + self.args += [data_dir] + self.cli.invoke(self.args, strict=True) + out, err = capsys.readouterr() + assert not os.path.isfile(orig_file_path) + assert not os.path.exists(path) From e401afe9ebc9d18f8d6af24fbdf80a8098e47a59 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Mon, 18 Sep 2017 18:18:42 +0100 Subject: [PATCH 07/13] Add checks for logfile, improve readability, clean code --- .../test/integration/clitest/test_cleanse.py | 117 +++++++++++------- 1 file changed, 71 insertions(+), 46 deletions(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index b56d7b7c18a..b241ee2d86c 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -26,12 +26,8 @@ import omero.plugins.admin import pytest -import omero -import omero.gateway import os.path -all_grps = {'omero.group': '-1'} - class TestCleanse(CLITest): @@ -58,6 +54,7 @@ def setup_method(self, method): self.cli.register("admin", omero.plugins.admin.AdminControl, "TEST") self.import_args = self.args self.args += ["admin", "cleanse"] + self.group_ctx = {'omero.group': str(self.group.id.val)} def testCleanseBasic(self, capsys): """Test cleanse works for root with expected output""" @@ -71,57 +68,85 @@ def testCleanseBasic(self, capsys): output_string = output_string_start + mrepo_dir.replace("//", "/") assert output_string in out - def testCleanseNonsenseName(self, tmpdir, capsys): - """Test cleanse removes a file when originalFile""" - """has nonsensical name""" - name = "nonsensical" - images = self.import_fake_file() - ids = [image.id.val for image in images] - fileset = self.get_fileset(images) - fileset_id = fileset.getId() + def testCleanseNonsenseName(self, capsys): + """ + Test cleanse removes file on disk after OriginalFile + name was changed to nonsense and its Image was deleted + """ + # import image and retrieve the OriginalFile + # (orig_file), its name and path + image = self.import_fake_file()[0] + fileset = self.get_fileset([image]) params = omero.sys.ParametersI() - params.addId(fileset_id) - query = ("select details.group.id, originalFile.id, " - "originalFile.path from FilesetEntry where fileset.id = :id") + params.addId(fileset.getId()) + q = ("select originalFile.id, originalFile.path " + "from FilesetEntry where fileset.id = :id") queryService = self.root.sf.getQueryService() - result = queryService.projection(query, params, all_grps) - group_id = result[0][0].getValue() - group_ctx = {'omero.group': str(group_id)} - orig_file_id = result[0][1].getValue() - path_in_mrepo = result[0][2].getValue() - query = 'from OriginalFile o where o.id = :id' - params_file = omero.sys.ParametersI() - params_file.addId(orig_file_id) - orig_file = queryService.findByQuery(query, params_file, group_ctx) + result = queryService.projection(q, params, self.group_ctx) + orig_file_id = result[0][0].getValue() + path_in_mrepo = result[0][1].getValue() + orig_file = self.query.get("OriginalFile", orig_file_id) orig_file_name = orig_file.getName().getValue() config_service = self.root.sf.getConfigService() mrepo_dir = config_service.getConfigValue("omero.managed.dir") - path = mrepo_dir + '/' + path_in_mrepo - path = path.replace("//", "/") - assert os.path.exists(path) - orig_file_path = path + '/' + orig_file_name + orig_file_path = mrepo_dir + '/' + path_in_mrepo orig_file_path = orig_file_path.replace("//", "/") - assert os.path.isfile(orig_file_path) - orig_file.setName(omero.rtypes.rstring(name)) + assert os.path.exists(orig_file_path) + orig_file_path_and_name = orig_file_path + '/' + orig_file_name + orig_file_path_and_name = orig_file_path_and_name.replace("//", "/") + assert os.path.isfile(orig_file_path_and_name) + + # retrieve the logfile, its name and path + q = ("select o from FilesetJobLink l " + "join l.parent as fs join l.child as j " + "join j.originalFileLinks l2 join l2.child as o " + "where fs.id = :id and " + "o.mimetype = 'application/omero-log-file'") + logfile = queryService.findByQuery(q, params, self.group_ctx) + logfile_name = logfile.getName().getValue() + logfile_path_in_mrepo = logfile.getPath().getValue() + logfile_path = mrepo_dir + '/' + logfile_path_in_mrepo + logfile_path = logfile_path.replace("//", "/") + assert os.path.exists(logfile_path) + logfile_path_and_name = logfile_path + '/' + logfile_name + logfile_path_and_name = logfile_path_and_name.replace("//", "/") + assert os.path.isfile(logfile_path_and_name) + + # change the names of original_file and logfile to nonsense + name = "nonsensical" update_service = self.root.sf.getUpdateService() - update_service.saveAndReturnObject(orig_file, group_ctx) - query = ("select o from FilesetJobLink l " - "join l.parent as fs join l.child as j " - "join j.originalFileLinks l2 join l2.child as o " - "where fs.id = :id and " - "o.mimetype = 'application/omero-log-file'") - logfile = queryService.findByQuery(query, params, group_ctx) - logfile.getName().getValue() - - command = Delete2(targetObjects={"Image": ids}) - handle = self.client.sf.submit(command) - self.wait_on_cmd(self.client, handle) - assert os.path.exists(path) - assert os.path.isfile(orig_file_path) + orig_file.setName(omero.rtypes.rstring(name)) + update_service.saveAndReturnObject(orig_file, self.group_ctx) + logfile.setName(omero.rtypes.rstring(name)) + update_service.saveAndReturnObject(logfile, self.group_ctx) + # run the cleanse command, which will not delete + # the files on disk data_dir = config_service.getConfigValue("omero.data.dir") self.args += [data_dir] self.cli.invoke(self.args, strict=True) + assert os.path.exists(orig_file_path) + assert os.path.isfile(orig_file_path_and_name) + assert os.path.exists(logfile_path) + assert os.path.isfile(logfile_path_and_name) + + # delete the image, which will not delete + # the files on disk because of the nonsensical name + # of orig_file and logfile + command = Delete2(targetObjects={"Image": [image.id.val]}) + handle = self.client.sf.submit(command) + self.wait_on_cmd(self.client, handle) + assert os.path.exists(orig_file_path) + assert os.path.isfile(orig_file_path_and_name) + assert os.path.exists(logfile_path) + assert os.path.isfile(logfile_path_and_name) + + # run cleanse command again, which will now delete the + # files on disk, the original file, the logfile + # and their directories + self.cli.invoke(self.args, strict=True) out, err = capsys.readouterr() - assert not os.path.isfile(orig_file_path) - assert not os.path.exists(path) + assert not os.path.exists(orig_file_path) + assert not os.path.isfile(orig_file_path_and_name) + assert not os.path.exists(logfile_path) + assert not os.path.isfile(logfile_path_and_name) From 7aaa2b04d2400dc83809bf7b6616692bc4b18903 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Mon, 18 Sep 2017 19:25:40 +0100 Subject: [PATCH 08/13] Add helper method make_path, define data_dir and mrepo_dir in setup --- .../test/integration/clitest/test_cleanse.py | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index b241ee2d86c..faf70da4b33 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -55,17 +55,24 @@ def setup_method(self, method): self.import_args = self.args self.args += ["admin", "cleanse"] self.group_ctx = {'omero.group': str(self.group.id.val)} + config_service = self.root.sf.getConfigService() + self.mrepo_dir = config_service.getConfigValue("omero.managed.dir") + self.data_dir = config_service.getConfigValue("omero.managed.dir") + + def make_path(self, path_in_mrepo, filename=None): + if filename is None: + path = self.mrepo_dir + '/' + path_in_mrepo + else: + path = self.mrepo_dir + '/' + path_in_mrepo + '/' + filename + return path.replace("//", "/") def testCleanseBasic(self, capsys): """Test cleanse works for root with expected output""" - config_service = self.root.sf.getConfigService() - data_dir = config_service.getConfigValue("omero.data.dir") - self.args += [data_dir] + self.args += [self.data_dir] self.cli.invoke(self.args, strict=True) out, err = capsys.readouterr() - output_string_start = "Removing empty directories from...\n " - mrepo_dir = config_service.getConfigValue("omero.managed.dir") - output_string = output_string_start + mrepo_dir.replace("//", "/") + output_start = "Removing empty directories from...\n " + output_string = output_start + self.mrepo_dir.replace("//", "/") assert output_string in out def testCleanseNonsenseName(self, capsys): @@ -79,21 +86,17 @@ def testCleanseNonsenseName(self, capsys): fileset = self.get_fileset([image]) params = omero.sys.ParametersI() params.addId(fileset.getId()) - q = ("select originalFile.id, originalFile.path " + q = ("select originalFile.path, originalFile.id " "from FilesetEntry where fileset.id = :id") queryService = self.root.sf.getQueryService() result = queryService.projection(q, params, self.group_ctx) - orig_file_id = result[0][0].getValue() - path_in_mrepo = result[0][1].getValue() + path_in_mrepo = result[0][0].getValue() + orig_file_path = self.make_path(path_in_mrepo) + assert os.path.exists(orig_file_path) + orig_file_id = result[0][1].getValue() orig_file = self.query.get("OriginalFile", orig_file_id) orig_file_name = orig_file.getName().getValue() - config_service = self.root.sf.getConfigService() - mrepo_dir = config_service.getConfigValue("omero.managed.dir") - orig_file_path = mrepo_dir + '/' + path_in_mrepo - orig_file_path = orig_file_path.replace("//", "/") - assert os.path.exists(orig_file_path) - orig_file_path_and_name = orig_file_path + '/' + orig_file_name - orig_file_path_and_name = orig_file_path_and_name.replace("//", "/") + orig_file_path_and_name = self.make_path(path_in_mrepo, orig_file_name) assert os.path.isfile(orig_file_path_and_name) # retrieve the logfile, its name and path @@ -104,12 +107,10 @@ def testCleanseNonsenseName(self, capsys): "o.mimetype = 'application/omero-log-file'") logfile = queryService.findByQuery(q, params, self.group_ctx) logfile_name = logfile.getName().getValue() - logfile_path_in_mrepo = logfile.getPath().getValue() - logfile_path = mrepo_dir + '/' + logfile_path_in_mrepo - logfile_path = logfile_path.replace("//", "/") + path_in_mrepo = logfile.getPath().getValue() + logfile_path = self.make_path(path_in_mrepo) assert os.path.exists(logfile_path) - logfile_path_and_name = logfile_path + '/' + logfile_name - logfile_path_and_name = logfile_path_and_name.replace("//", "/") + logfile_path_and_name = self.make_path(path_in_mrepo, logfile_name) assert os.path.isfile(logfile_path_and_name) # change the names of original_file and logfile to nonsense @@ -122,8 +123,7 @@ def testCleanseNonsenseName(self, capsys): # run the cleanse command, which will not delete # the files on disk - data_dir = config_service.getConfigValue("omero.data.dir") - self.args += [data_dir] + self.args += [self.data_dir] self.cli.invoke(self.args, strict=True) assert os.path.exists(orig_file_path) assert os.path.isfile(orig_file_path_and_name) From 9f79786c42a1d3a741c4683926212f81397e198c Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Tue, 19 Sep 2017 20:10:40 +0100 Subject: [PATCH 09/13] Add Restricted Admins test for cleanse --- .../OmeroPy/src/omero/testlib/__init__.py | 15 ++++++++++- .../test/integration/clitest/test_cleanse.py | 26 ++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/components/tools/OmeroPy/src/omero/testlib/__init__.py b/components/tools/OmeroPy/src/omero/testlib/__init__.py index cffd718ec16..efd3eb82471 100644 --- a/components/tools/OmeroPy/src/omero/testlib/__init__.py +++ b/components/tools/OmeroPy/src/omero/testlib/__init__.py @@ -93,6 +93,16 @@ class ITest(object): # If the new user created in setup_class should own their group # Can be overriden by test instances DEFAULT_GROUP_OWNER = False + # If the new user created in setup_class should + # be member of system group. This is to be + # set to True if the new user should be Restricted Admin + # Can be overriden by test instances + DEFAULT_SYSTEM = False + # If the new user created in setup_class is system group member + # default value None makes the new user to Full Admin + # For Restricted Admin with no privileges, set the value to [] + # Can be overriden by test instances + DEFAULT_PRIVILEGES = None @classmethod def setup_class(cls): @@ -115,7 +125,10 @@ def setup_class(cls): raise Exception("Could not initiate a root connection") cls.group = cls.new_group(perms=cls.DEFAULT_PERMS) - cls.user = cls.new_user(group=cls.group, owner=cls.DEFAULT_GROUP_OWNER) + cls.user = cls.new_user(group=cls.group, + owner=cls.DEFAULT_GROUP_OWNER, + system=cls.DEFAULT_SYSTEM, + privileges=cls.DEFAULT_PRIVILEGES) cls.client = omero.client() # ok because adds self cls.__clients.add(cls.client) cls.client.setAgent("OMERO.py.test") diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index faf70da4b33..95d36bc978c 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -52,7 +52,6 @@ class TestCleanseRoot(RootCLITest): def setup_method(self, method): super(TestCleanseRoot, self).setup_method(method) self.cli.register("admin", omero.plugins.admin.AdminControl, "TEST") - self.import_args = self.args self.args += ["admin", "cleanse"] self.group_ctx = {'omero.group': str(self.group.id.val)} config_service = self.root.sf.getConfigService() @@ -150,3 +149,28 @@ def testCleanseNonsenseName(self, capsys): assert not os.path.isfile(orig_file_path_and_name) assert not os.path.exists(logfile_path) assert not os.path.isfile(logfile_path_and_name) + + +class TestCleanseRestrictedAdmin(CLITest): + + # make the user in this test a member of system group + DEFAULT_SYSTEM = True + # make the new member of system group to a Restricted + # Admin with no privileges + DEFAULT_PRIVILEGES = [] + + def setup_method(self, method): + super(TestCleanseRestrictedAdmin, self).setup_method(method) + self.cli.register("admin", omero.plugins.admin.AdminControl, "TEST") + self.args += ["admin", "cleanse"] + + def testCleanseRestrictedAdmin(self, capsys): + """Test cleanse cannot be run by Restricted Admin""" + config_service = self.root.sf.getConfigService() + data_dir = config_service.getConfigValue("omero.data.dir") + self.args += [data_dir] + with pytest.raises(NonZeroReturnCode): + self.cli.invoke(self.args, strict=True) + out, err = capsys.readouterr() + output_end = "SecurityViolation: Admin restrictions: ReadSession\n" + assert err.endswith(output_end) From af4269d4006e5602f4b5891b31d2fa2964311f34 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Wed, 20 Sep 2017 17:13:56 +0100 Subject: [PATCH 10/13] Add comments to privileges parameter of new_user method --- components/tools/OmeroPy/src/omero/testlib/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/components/tools/OmeroPy/src/omero/testlib/__init__.py b/components/tools/OmeroPy/src/omero/testlib/__init__.py index efd3eb82471..9176d98bd1c 100644 --- a/components/tools/OmeroPy/src/omero/testlib/__init__.py +++ b/components/tools/OmeroPy/src/omero/testlib/__init__.py @@ -535,7 +535,10 @@ def new_user(cls, group=None, perms=None, email=None, privileges=None): """ :owner: If user is to be an owner of the created group - :system: If user is to be a system admin + :system: If user is to be a system group member + :privileges: If system group member is to have privileges + privileges=None gives all privileges (full admin) + privileges=[] gives no privileges """ if not cls.root: From b0ca085c3c4a578a569d2a5ef171ccf445232420 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Fri, 22 Sep 2017 15:12:10 +0100 Subject: [PATCH 11/13] Make changes cf. Josh's comments --- .../tools/OmeroPy/src/omero/testlib/__init__.py | 2 +- .../OmeroPy/test/integration/clitest/test_cleanse.py | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/components/tools/OmeroPy/src/omero/testlib/__init__.py b/components/tools/OmeroPy/src/omero/testlib/__init__.py index 9176d98bd1c..efcda8ce12f 100644 --- a/components/tools/OmeroPy/src/omero/testlib/__init__.py +++ b/components/tools/OmeroPy/src/omero/testlib/__init__.py @@ -100,7 +100,7 @@ class ITest(object): DEFAULT_SYSTEM = False # If the new user created in setup_class is system group member # default value None makes the new user to Full Admin - # For Restricted Admin with no privileges, set the value to [] + # For Restricted Admin with no privileges, set the value to () # Can be overriden by test instances DEFAULT_PRIVILEGES = None diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index 95d36bc978c..05d1e0e0d66 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -47,10 +47,15 @@ def testCleanseAdminOnly(self, capsys): assert err.endswith("SecurityViolation: Admins only!\n") -class TestCleanseRoot(RootCLITest): +class TestCleanseFullAdmin(RootCLITest): + + # make the user in this test a member of system group + DEFAULT_SYSTEM = True + # make the new member of system group to a Full Admin + DEFAULT_PRIVILEGES = None def setup_method(self, method): - super(TestCleanseRoot, self).setup_method(method) + super(TestCleanseFullAdmin, self).setup_method(method) self.cli.register("admin", omero.plugins.admin.AdminControl, "TEST") self.args += ["admin", "cleanse"] self.group_ctx = {'omero.group': str(self.group.id.val)} @@ -157,7 +162,7 @@ class TestCleanseRestrictedAdmin(CLITest): DEFAULT_SYSTEM = True # make the new member of system group to a Restricted # Admin with no privileges - DEFAULT_PRIVILEGES = [] + DEFAULT_PRIVILEGES = () def setup_method(self, method): super(TestCleanseRestrictedAdmin, self).setup_method(method) From 08afcb51fe3cae8c5d5c6baefbf7d67524819f41 Mon Sep 17 00:00:00 2001 From: Petr Walczysko Date: Sat, 23 Sep 2017 14:17:45 +0100 Subject: [PATCH 12/13] Extend CLITest class to run the test as Full Admin, not root --- .../tools/OmeroPy/test/integration/clitest/test_cleanse.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py index 05d1e0e0d66..25f6091049b 100644 --- a/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py +++ b/components/tools/OmeroPy/test/integration/clitest/test_cleanse.py @@ -20,7 +20,7 @@ # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -from test.integration.clitest.cli import CLITest, RootCLITest +from test.integration.clitest.cli import CLITest from omero.cli import NonZeroReturnCode from omero.cmd import Delete2 @@ -47,7 +47,7 @@ def testCleanseAdminOnly(self, capsys): assert err.endswith("SecurityViolation: Admins only!\n") -class TestCleanseFullAdmin(RootCLITest): +class TestCleanseFullAdmin(CLITest): # make the user in this test a member of system group DEFAULT_SYSTEM = True @@ -71,7 +71,7 @@ def make_path(self, path_in_mrepo, filename=None): return path.replace("//", "/") def testCleanseBasic(self, capsys): - """Test cleanse works for root with expected output""" + """Test cleanse works for Full Admin with expected output""" self.args += [self.data_dir] self.cli.invoke(self.args, strict=True) out, err = capsys.readouterr() From a0dfa5373dd0a2477bddb7be3dd936d57f181ac1 Mon Sep 17 00:00:00 2001 From: Josh Moore Date: Mon, 25 Sep 2017 14:57:32 +0200 Subject: [PATCH 13/13] Update doc string to use () rather than [] --- components/tools/OmeroPy/src/omero/testlib/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/tools/OmeroPy/src/omero/testlib/__init__.py b/components/tools/OmeroPy/src/omero/testlib/__init__.py index efcda8ce12f..1283984f37e 100644 --- a/components/tools/OmeroPy/src/omero/testlib/__init__.py +++ b/components/tools/OmeroPy/src/omero/testlib/__init__.py @@ -538,7 +538,7 @@ def new_user(cls, group=None, perms=None, :system: If user is to be a system group member :privileges: If system group member is to have privileges privileges=None gives all privileges (full admin) - privileges=[] gives no privileges + privileges=() gives no privileges """ if not cls.root: