From b5ec43c4b124e4b64449d41ea0f7cc6bd404196f Mon Sep 17 00:00:00 2001 From: bmccandless Date: Thu, 8 Oct 2020 08:44:09 -0700 Subject: [PATCH] Add a function to check the configuration for errors. (#1919) This can be used as a sanity check before a deployment: chanzuckerberg/single-cell#63 --- server/common/config/server_config.py | 2 +- server/eb/check_config.py | 39 ++++++++++++ server/test/__init__.py | 8 ++- server/test/unit/auth/test_auth.py | 60 ++++++++++--------- server/test/unit/auth/test_oauth.py | 2 + server/test/unit/common/config/__init__.py | 4 +- .../unit/common/config/test_app_config.py | 8 ++- .../unit/common/config/test_base_config.py | 2 + .../unit/common/config/test_dataset_config.py | 10 +++- .../unit/common/config/test_server_config.py | 3 + server/test/unit/eb/test_eb.py | 39 ++++++++++-- 11 files changed, 136 insertions(+), 41 deletions(-) create mode 100644 server/eb/check_config.py diff --git a/server/common/config/server_config.py b/server/common/config/server_config.py index 6c4b5570..14eac613 100644 --- a/server/common/config/server_config.py +++ b/server/common/config/server_config.py @@ -114,7 +114,7 @@ class ServerConfig(BaseConfig): self.validate_correct_type_of_configuration_attribute("app__port", (type(None), int)) self.validate_correct_type_of_configuration_attribute("app__open_browser", bool) self.validate_correct_type_of_configuration_attribute("app__force_https", bool) - self.validate_correct_type_of_configuration_attribute("app__flask_secret_key", (type(None), str)) + self.validate_correct_type_of_configuration_attribute("app__flask_secret_key", str) self.validate_correct_type_of_configuration_attribute("app__generate_cache_control_headers", bool) self.validate_correct_type_of_configuration_attribute("app__server_timing_headers", bool) self.validate_correct_type_of_configuration_attribute("app__csp_directives", (type(None), dict)) diff --git a/server/eb/check_config.py b/server/eb/check_config.py new file mode 100644 index 00000000..6886e101 --- /dev/null +++ b/server/eb/check_config.py @@ -0,0 +1,39 @@ +import sys +import argparse +import yaml + +from server.common.config.app_config import AppConfig + + +def main(): + parser = argparse.ArgumentParser("A script to check hosted configuration files") + parser.add_argument("config_file", help="the configuration file") + parser.add_argument( + "-s", + "--show", + default=False, + action="store_true", + help="print the configuration. NOTE: this may print secret values to stdout", + ) + + args = parser.parse_args() + + app_config = AppConfig() + app_config.update_from_config_file(args.config_file) + try: + app_config.complete_config() + except Exception as e: + print(f"Error: {str(e)}") + print("FAIL:", args.config_file) + sys.exit(1) + + if args.show: + yaml_config = app_config.config_to_dict() + yaml.dump(yaml_config, sys.stdout) + + print("PASS:", args.config_file) + sys.exit(0) + + +if __name__ == "__main__": + main() diff --git a/server/test/__init__.py b/server/test/__init__.py index 586aa798..1b96f7c1 100644 --- a/server/test/__init__.py +++ b/server/test/__init__.py @@ -34,7 +34,7 @@ def data_with_tmp_tiledb_annotations(ext: MatrixDataType): data_locator = DataLocator(fname) config = AppConfig() config.update_server_config( - multi_dataset__dataroot=data_locator.path, authentication__type="test", + app__flask_secret_key="secret", multi_dataset__dataroot=data_locator.path, authentication__type="test", ) config.update_default_dataset_config( embeddings__names=["umap"], @@ -64,7 +64,10 @@ def data_with_tmp_annotations(ext: MatrixDataType, annotations_fixture=False): data_locator = DataLocator(fname) config = AppConfig() config.update_server_config( - single_dataset__obs_names=None, single_dataset__var_names=None, single_dataset__datapath=data_locator.path + app__flask_secret_key="secret", + single_dataset__obs_names=None, + single_dataset__var_names=None, + single_dataset__datapath=data_locator.path, ) config.update_default_dataset_config( embeddings__names=["umap"], presentation__max_categories=100, diffexp__lfc_cutoff=0.01, @@ -97,6 +100,7 @@ def skip_if(condition, reason: str): def app_config(data_locator, backed=False, extra_server_config={}, extra_dataset_config={}): config = AppConfig() config.update_server_config( + app__flask_secret_key="secret", single_dataset__obs_names=None, single_dataset__var_names=None, adaptor__anndata_adaptor__backed=backed, diff --git a/server/test/unit/auth/test_auth.py b/server/test/unit/auth/test_auth.py index 0b8c0bf1..2caba6b1 100644 --- a/server/test/unit/auth/test_auth.py +++ b/server/test/unit/auth/test_auth.py @@ -11,13 +11,14 @@ class AuthTest(unittest.TestCase): self.dataset_dataroot = FIXTURES_ROOT def test_auth_none(self): - c = AppConfig() - c.update_server_config(authentication__type=None, multi_dataset__dataroot=self.dataset_dataroot) - c.update_default_dataset_config(user_annotations__enable=False) + app_config = AppConfig() + app_config.update_server_config(app__flask_secret_key="secret") + app_config.update_server_config(authentication__type=None, multi_dataset__dataroot=self.dataset_dataroot) + app_config.update_default_dataset_config(user_annotations__enable=False) - c.complete_config() + app_config.complete_config() - with test_server(app_config=c) as server: + with test_server(app_config=app_config) as server: session = requests.Session() config = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/config").json() userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json() @@ -25,12 +26,13 @@ class AuthTest(unittest.TestCase): self.assertIsNone(userinfo) def test_auth_session(self): - c = AppConfig() - c.update_server_config(authentication__type="session", multi_dataset__dataroot=self.dataset_dataroot) - c.update_default_dataset_config(user_annotations__enable=True) - c.complete_config() + app_config = AppConfig() + app_config.update_server_config(app__flask_secret_key="secret") + app_config.update_server_config(authentication__type="session", multi_dataset__dataroot=self.dataset_dataroot) + app_config.update_default_dataset_config(user_annotations__enable=True) + app_config.complete_config() - with test_server(app_config=c) as server: + with test_server(app_config=app_config) as server: session = requests.Session() config = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/config").json() userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json() @@ -40,9 +42,10 @@ class AuthTest(unittest.TestCase): self.assertEqual(userinfo["userinfo"]["username"], "anonymous") def test_auth_test(self): - c = AppConfig() - c.update_server_config(authentication__type="test") - c.update_server_config( + app_config = AppConfig() + app_config.update_server_config(app__flask_secret_key="secret") + app_config.update_server_config(authentication__type="test") + app_config.update_server_config( multi_dataset__dataroot=dict( a1=dict(dataroot=self.dataset_dataroot, base_url="auth"), a2=dict(dataroot=self.dataset_dataroot, base_url="no-auth"), @@ -50,12 +53,12 @@ class AuthTest(unittest.TestCase): ) # specialize the configs - c.add_dataroot_config("a1", app__authentication_enable=True, user_annotations__enable=True) - c.add_dataroot_config("a2", app__authentication_enable=False, user_annotations__enable=False) + app_config.add_dataroot_config("a1", app__authentication_enable=True, user_annotations__enable=True) + app_config.add_dataroot_config("a2", app__authentication_enable=False, user_annotations__enable=False) - c.complete_config() + app_config.complete_config() - with test_server(app_config=c) as server: + with test_server(app_config=app_config) as server: session = requests.Session() # auth datasets @@ -102,20 +105,21 @@ class AuthTest(unittest.TestCase): self.assertFalse(config["config"]["parameters"]["annotations"]) # login with a picture - r = session.get(f"{server}/{login_uri}&picture=myimage.png") + session.get(f"{server}/{login_uri}&picture=myimage.png") userinfo = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/userinfo").json() self.assertTrue(userinfo["userinfo"]["is_authenticated"]) self.assertEqual(userinfo["userinfo"]["picture"], "myimage.png") def test_auth_test_single(self): - c = AppConfig() - c.update_server_config( + app_config = AppConfig() + app_config.update_server_config(app__flask_secret_key="secret") + app_config.update_server_config( authentication__type="test", single_dataset__datapath=f"{self.dataset_dataroot}/pbmc3k.cxg" ) - c.complete_config() + app_config.complete_config() - with test_server(app_config=c) as server: + with test_server(app_config=app_config) as server: session = requests.Session() config = session.get(f"{server}/api/v0.2/config").json() userinfo = session.get(f"{server}/api/v0.2/userinfo").json() @@ -130,10 +134,10 @@ class AuthTest(unittest.TestCase): self.assertEqual(login_uri, "/login") self.assertEqual(logout_uri, "/logout") - r = session.get(f"{server}/{login_uri}") + response = session.get(f"{server}/{login_uri}") # check that the login redirect worked - self.assertEqual(r.history[0].status_code, 302) - self.assertEqual(r.url, f"{server}/") + self.assertEqual(response.history[0].status_code, 302) + self.assertEqual(response.url, f"{server}/") config = session.get(f"{server}/api/v0.2/config").json() userinfo = session.get(f"{server}/api/v0.2/userinfo").json() @@ -141,10 +145,10 @@ class AuthTest(unittest.TestCase): self.assertEqual(userinfo["userinfo"]["username"], "test_account") self.assertTrue(config["config"]["parameters"]["annotations"]) - r = session.get(f"{server}/{logout_uri}") + response = session.get(f"{server}/{logout_uri}") # check that the logout redirect worked - self.assertEqual(r.history[0].status_code, 302) - self.assertEqual(r.url, f"{server}/") + self.assertEqual(response.history[0].status_code, 302) + self.assertEqual(response.url, f"{server}/") config = session.get(f"{server}/api/v0.2/config").json() userinfo = session.get(f"{server}/api/v0.2/userinfo").json() self.assertFalse(userinfo["userinfo"]["is_authenticated"]) diff --git a/server/test/unit/auth/test_oauth.py b/server/test/unit/auth/test_oauth.py index 8b250ed6..f5a85cb5 100644 --- a/server/test/unit/auth/test_oauth.py +++ b/server/test/unit/auth/test_oauth.py @@ -160,12 +160,14 @@ class AuthTest(unittest.TestCase): def test_auth_oauth_session(self): # test with session cookies app_config = AppConfig() + app_config.update_server_config(app__flask_secret_key="secret") app_config.update_server_config(authentication__params_oauth__session_cookie=True,) self.auth_flow(app_config) def test_auth_oauth_cookie(self): # test with specified cookie app_config = AppConfig() + app_config.update_server_config(app__flask_secret_key="secret") app_config.update_server_config( authentication__params_oauth__session_cookie=False, authentication__params_oauth__cookie=dict(key="test_cxguser", httponly=True, max_age=60), diff --git a/server/test/unit/common/config/__init__.py b/server/test/unit/common/config/__init__.py index 7c06b311..8a48be5e 100644 --- a/server/test/unit/common/config/__init__.py +++ b/server/test/unit/common/config/__init__.py @@ -31,7 +31,7 @@ class ConfigTests(unittest.TestCase): port="null", open_browser="false", force_https="false", - flask_secret_key="null", + flask_secret_key="secret", generate_cache_control_headers="false", server_timing_headers="false", csp_directives="null", @@ -82,7 +82,7 @@ class ConfigTests(unittest.TestCase): port="null", open_browser="false", force_https="false", - flask_secret_key="null", + flask_secret_key="secret", generate_cache_control_headers="false", server_timing_headers="false", csp_directives="null", diff --git a/server/test/unit/common/config/test_app_config.py b/server/test/unit/common/config/test_app_config.py index 362ab330..ea8fe047 100644 --- a/server/test/unit/common/config/test_app_config.py +++ b/server/test/unit/common/config/test_app_config.py @@ -15,6 +15,7 @@ class AppConfigTest(ConfigTests): def setUp(self): self.config_file_name = f"{unittest.TestCase.id(self).split('.')[-1]}.yml" self.config = AppConfig() + self.config.update_server_config(app__flask_secret_key="secret") self.config.update_server_config(multi_dataset__dataroot=FIXTURES_ROOT) self.server_config = self.config.server_config self.config.complete_config() @@ -106,6 +107,8 @@ class AppConfigTest(ConfigTests): with open(configfile, "w") as fconfig: config = """ server: + app: + flask_secret_key: secret multi_dataset: dataroot: test_dataroot @@ -116,7 +119,10 @@ class AppConfigTest(ConfigTests): app_config.update_from_config_file(configfile) server_changes = app_config.server_config.changes_from_default() dataset_changes = app_config.default_dataset_config.changes_from_default() - self.assertEqual(server_changes, [("multi_dataset__dataroot", "test_dataroot", None)]) + self.assertEqual( + server_changes, + [("app__flask_secret_key", "secret", None), ("multi_dataset__dataroot", "test_dataroot", None)], + ) self.assertEqual(dataset_changes, []) def test_configfile_no_server_section(self): diff --git a/server/test/unit/common/config/test_base_config.py b/server/test/unit/common/config/test_base_config.py index d41082cf..d5d538cd 100644 --- a/server/test/unit/common/config/test_base_config.py +++ b/server/test/unit/common/config/test_base_config.py @@ -10,6 +10,7 @@ class BaseConfigTest(ConfigTests): def setUp(self): self.config_file_name = f"{unittest.TestCase.id(self).split('.')[-1]}.yml" self.config = AppConfig() + self.config.update_server_config(app__flask_secret_key="secret") self.config.update_server_config(multi_dataset__dataroot=FIXTURES_ROOT) self.server_config = self.config.server_config self.config.complete_config() @@ -47,6 +48,7 @@ class BaseConfigTest(ConfigTests): server_changes, [ ("app__verbose", True, False), + ("app__flask_secret_key", "secret", None), ("multi_dataset__dataroot", FIXTURES_ROOT, None), ("multi_dataset__matrix_cache__timelimit_s", 5, 30), ("data_locator__s3__region_name", "us-east-1", True), diff --git a/server/test/unit/common/config/test_dataset_config.py b/server/test/unit/common/config/test_dataset_config.py index 5ad66e44..b65c7d83 100644 --- a/server/test/unit/common/config/test_dataset_config.py +++ b/server/test/unit/common/config/test_dataset_config.py @@ -19,6 +19,7 @@ class TestDatasetConfig(ConfigTests): def setUp(self): self.config_file_name = f"{unittest.TestCase.id(self).split('.')[-1]}.yml" self.config = AppConfig() + self.config.update_server_config(app__flask_secret_key="secret") self.config.update_server_config(multi_dataset__dataroot=FIXTURES_ROOT) self.dataset_config = self.config.default_dataset_config self.config.complete_config() @@ -155,7 +156,8 @@ class TestDatasetConfig(ConfigTests): # test for illegal url_dataroots for illegal in ("../b", "!$*", "\\n", "", "(bad)"): config.update_server_config( - multi_dataset__dataroot={"tag": {"base_url": illegal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}} + app__flask_secret_key="secret", + multi_dataset__dataroot={"tag": {"base_url": illegal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}}, ) with self.assertRaises(ConfigurationError): config.complete_config() @@ -163,17 +165,19 @@ class TestDatasetConfig(ConfigTests): # test for legal url_dataroots for legal in ("d", "this.is-okay_", "a/b"): config.update_server_config( - multi_dataset__dataroot={"tag": {"base_url": legal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}} + app__flask_secret_key="secret", + multi_dataset__dataroot={"tag": {"base_url": legal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}}, ) config.complete_config() # test that multi dataroots work end to end config.update_server_config( + app__flask_secret_key="secret", multi_dataset__dataroot=dict( s1=dict(dataroot=f"{PROJECT_ROOT}/example-dataset", base_url="set1/1/2"), s2=dict(dataroot=f"{FIXTURES_ROOT}", base_url="set2"), s3=dict(dataroot=f"{FIXTURES_ROOT}", base_url="set3"), - ) + ), ) # Change this default to test if the dataroot overrides below work. diff --git a/server/test/unit/common/config/test_server_config.py b/server/test/unit/common/config/test_server_config.py index 65974523..f862a40f 100644 --- a/server/test/unit/common/config/test_server_config.py +++ b/server/test/unit/common/config/test_server_config.py @@ -23,6 +23,7 @@ class TestServerConfig(ConfigTests): def setUp(self): self.config_file_name = f"{unittest.TestCase.id(self).split('.')[-1]}.yml" self.config = AppConfig() + self.config.update_server_config(app__flask_secret_key="secret") self.config.update_server_config(multi_dataset__dataroot=FIXTURES_ROOT) self.server_config = self.config.server_config self.config.complete_config() @@ -103,6 +104,7 @@ class TestServerConfig(ConfigTests): # Note if the port is set in the config file it will NOT be overwritten by a different envvar os.environ["CXG_SERVER_PORT"] = "4008" self.config = AppConfig() + self.config.update_server_config(app__flask_secret_key="secret") self.config.server_config.handle_app(self.context) self.assertEqual(self.config.server_config.app__port, 4008) del os.environ["CXG_SERVER_PORT"] @@ -152,6 +154,7 @@ class TestServerConfig(ConfigTests): config = AppConfig() backend_port = find_available_port("localhost", 10000) config.update_server_config( + app__flask_secret_key="secret", app__api_base_url=f"http://localhost:{backend_port}/additional/path", multi_dataset__dataroot=f"{PROJECT_ROOT}/example-dataset", ) diff --git a/server/test/unit/eb/test_eb.py b/server/test/unit/eb/test_eb.py index c148f6bc..61cf1867 100644 --- a/server/test/unit/eb/test_eb.py +++ b/server/test/unit/eb/test_eb.py @@ -6,6 +6,7 @@ from server.test import PROJECT_ROOT, FIXTURES_ROOT from server.common.config.app_config import AppConfig from contextlib import contextmanager import time +import os @contextmanager @@ -34,12 +35,12 @@ class Elastic_Beanstalk_Test(unittest.TestCase): tempdir = tempfile.TemporaryDirectory(dir=f"{PROJECT_ROOT}/server") tempdirname = tempdir.name - c = AppConfig() + config = AppConfig() # test that eb works - c.update_server_config(multi_dataset__dataroot=f"{FIXTURES_ROOT}", app__flask_secret_key="open sesame") + config.update_server_config(multi_dataset__dataroot=f"{FIXTURES_ROOT}", app__flask_secret_key="open sesame") - c.complete_config() - c.write_config(f"{tempdirname}/config.yaml") + config.complete_config() + config.write_config(f"{tempdirname}/config.yaml") subprocess.check_call(f"git ls-files . | cpio -pdm {tempdirname}", cwd=f"{PROJECT_ROOT}/server/eb", shell=True) subprocess.check_call(["make", "build"], cwd=tempdirname) @@ -50,3 +51,33 @@ class Elastic_Beanstalk_Test(unittest.TestCase): r = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/config") data_config = r.json() assert data_config["config"]["displayNames"]["dataset"] == "pbmc3k" + + def test_config(self): + check_config_script = os.path.join(PROJECT_ROOT, "server", "eb", "check_config.py") + with tempfile.TemporaryDirectory() as tempdir: + configfile = os.path.join(tempdir, "config.yaml") + app_config = AppConfig() + app_config.update_server_config(multi_dataset__dataroot=f"{FIXTURES_ROOT}") + app_config.write_config(configfile) + + command = ["python", check_config_script, configfile] + + # test failure mode (flask_secret_key not set) + env = os.environ.copy() + env.pop("CXG_SECRET_KEY", None) + with self.assertRaises(subprocess.CalledProcessError) as exception_context: + subprocess.check_output(command, env=env) + output = str(exception_context.exception.stdout, "utf-8") + self.assertTrue( + output.startswith( + "Error: Invalid type for attribute: app__flask_secret_key, expected type str, got NoneType" + ) + ) + self.assertEqual(exception_context.exception.returncode, 1) + + # test passing case + env = os.environ.copy() + env["CXG_SECRET_KEY"] = "secret" + output = subprocess.check_output(command, env=env) + output = str(output, "utf-8") + self.assertTrue(output.startswith("PASS"))