From dcf8166d267a7cda41003602193573f5e96f608d Mon Sep 17 00:00:00 2001 From: Tyler Compton Date: Wed, 30 Sep 2020 12:42:07 -0700 Subject: [PATCH 1/7] Improve permission checks for Compose commands --- brainframe/cli/commands/compose.py | 15 ------- brainframe/cli/docker_compose.py | 48 ++++++++++++++++++++++ brainframe/cli/translations/compose.en.yml | 8 ---- brainframe/cli/translations/general.en.yml | 12 ++++++ 4 files changed, 60 insertions(+), 23 deletions(-) delete mode 100644 brainframe/cli/translations/compose.en.yml diff --git a/brainframe/cli/commands/compose.py b/brainframe/cli/commands/compose.py index 0d564f2..cdbe799 100644 --- a/brainframe/cli/commands/compose.py +++ b/brainframe/cli/commands/compose.py @@ -8,21 +8,6 @@ @command("compose") def compose(): - # Check first if the user has permission to interact with Docker - if not os_utils.is_root(): - if not os_utils.currently_in_group("docker"): - error_message = i18n.t("compose.docker-bad-permissions") + "\n" - - if os_utils.added_to_group("docker"): - # The user is in the group, they just need to restart - error_message += i18n.t("compose.restart-for-group-access") - else: - # The user is not in the group, so they need to either add - # themselves or use sudo - error_message += i18n.t("compose.retry-with-sudo-or-group") - - print_utils.fail(error_message) - install_path = env_vars.install_path.get() docker_compose.assert_installed(install_path) docker_compose.run(install_path, sys.argv[2:]) diff --git a/brainframe/cli/docker_compose.py b/brainframe/cli/docker_compose.py index bc3301f..d0d0204 100644 --- a/brainframe/cli/docker_compose.py +++ b/brainframe/cli/docker_compose.py @@ -2,6 +2,8 @@ from pathlib import Path from typing import List import yaml +import i18n +import os from . import os_utils, print_utils, env_vars @@ -25,6 +27,8 @@ def assert_installed(install_path: Path): def run(install_path: Path, commands: List[str]): + _assert_has_docker_permissions() + compose_path = install_path / "docker-compose.yml" full_command = ["docker-compose", "--file", str(compose_path)] @@ -43,6 +47,8 @@ def run(install_path: Path, commands: List[str]): def download(target: Path, version="latest"): + _assert_has_write_permissions(target) + subdomain, auth_flags, version = check_download_version(version=version) url = BRAINFRAME_DOCKER_COMPOSE_URL.format( @@ -98,3 +104,45 @@ def check_existing_version(install_path: Path): version = compose["services"]["core"]["image"].split(":")[-1] version = "v" + version return version + + +def _assert_has_docker_permissions(): + """Fails if the user does not have permissions to interact with Docker""" + if not os_utils.is_root(): + if not os_utils.currently_in_group("docker"): + error_message = ( + i18n.t("general.docker-bad-permissions") + + "\n" + + _group_recommendation_message("docker") + ) + + print_utils.fail(error_message) + + +def _assert_has_write_permissions(path: Path): + """Fails if the user does not have write access to the given path.""" + if os.access(path, os.W_OK): + return + + error_message = i18n.t("general.file-bad-write-permissions", path=path) + + print() + + if path.stat().st_gid == os_utils.BRAINFRAME_GROUP_ID: + error_message += _group_recommendation_message("brainframe") + else: + error_message += i18n.t( + "general.unexpected-group-for-file", path=path, group="brainframe" + ) + + print_utils.fail(error_message) + + +def _group_recommendation_message(group: str): + if os_utils.added_to_group("brainframe"): + # The user is in the group, they just need to restart + return i18n.t("general.restart-for-group-access", group=group) + else: + # The user is not in the group, so they need to either add + # themselves or use sudo + return i18n.t("general.retry-with-sudo-or-group", group=group) diff --git a/brainframe/cli/translations/compose.en.yml b/brainframe/cli/translations/compose.en.yml deleted file mode 100644 index 4657f34..0000000 --- a/brainframe/cli/translations/compose.en.yml +++ /dev/null @@ -1,8 +0,0 @@ -en: - docker-bad-permissions: "Your user does not have sufficient privileges to - use Docker." - restart-for-group-access: "Your user has been added to the \"docker\" group - but you must restart to apply this change. Alternatively, you can try again - with sudo." - retry-with-sudo-or-group: "Please try again with sudo or consider adding your - user to the \"docker\" group." diff --git a/brainframe/cli/translations/general.en.yml b/brainframe/cli/translations/general.en.yml index ae84377..946b379 100644 --- a/brainframe/cli/translations/general.en.yml +++ b/brainframe/cli/translations/general.en.yml @@ -16,3 +16,15 @@ en: staging-missing-credentials: "The %{username_env_var} and %{password_env_var} variables must be set to access staging." user-not-root: "This command must be run as root!" + docker-bad-permissions: "Your user does not have sufficient privileges to + use Docker." + restart-for-group-access: "Your user has been added to the \"%{group}\" + group but you must restart to apply this change. Alternatively, you can try + again with sudo." + retry-with-sudo-or-group: "Please try again with sudo or consider adding your + user to the \"%{group_name}\" group." + file-bad-write-permissions: "Your user does not have write access to + \"%{path}\"." + unexpected-group-for-file: "File \"%{path}\" is not part of the + \"%{group}\" group, but probably should be. Please add this file to the group + or try again with sudo." From 112e010e6f1e0f5e77f7f306867596e8ceb84ea8 Mon Sep 17 00:00:00 2001 From: Tyler Compton Date: Thu, 22 Oct 2020 23:13:41 -0700 Subject: [PATCH 2/7] Fix write permission check in download --- brainframe/cli/docker_compose.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/brainframe/cli/docker_compose.py b/brainframe/cli/docker_compose.py index d0d0204..1f62f9a 100644 --- a/brainframe/cli/docker_compose.py +++ b/brainframe/cli/docker_compose.py @@ -47,7 +47,7 @@ def run(install_path: Path, commands: List[str]): def download(target: Path, version="latest"): - _assert_has_write_permissions(target) + _assert_has_write_permissions(target.parent) subdomain, auth_flags, version = check_download_version(version=version) From 47213dc13d600b51e6fe9ddf3e50ed972d3d27ad Mon Sep 17 00:00:00 2001 From: Tyler Compton Date: Fri, 23 Oct 2020 00:02:19 -0700 Subject: [PATCH 3/7] Fix more minor issues --- brainframe/cli/docker_compose.py | 4 ++-- brainframe/cli/translations/general.en.yml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/brainframe/cli/docker_compose.py b/brainframe/cli/docker_compose.py index 1f62f9a..b268962 100644 --- a/brainframe/cli/docker_compose.py +++ b/brainframe/cli/docker_compose.py @@ -129,9 +129,9 @@ def _assert_has_write_permissions(path: Path): print() if path.stat().st_gid == os_utils.BRAINFRAME_GROUP_ID: - error_message += _group_recommendation_message("brainframe") + error_message += " " + _group_recommendation_message("brainframe") else: - error_message += i18n.t( + error_message += " " + i18n.t( "general.unexpected-group-for-file", path=path, group="brainframe" ) diff --git a/brainframe/cli/translations/general.en.yml b/brainframe/cli/translations/general.en.yml index 946b379..633c8c3 100644 --- a/brainframe/cli/translations/general.en.yml +++ b/brainframe/cli/translations/general.en.yml @@ -22,7 +22,7 @@ en: group but you must restart to apply this change. Alternatively, you can try again with sudo." retry-with-sudo-or-group: "Please try again with sudo or consider adding your - user to the \"%{group_name}\" group." + user to the \"%{group}\" group." file-bad-write-permissions: "Your user does not have write access to \"%{path}\"." unexpected-group-for-file: "File \"%{path}\" is not part of the From 643525ed24e1f6e8d9cf39b2832a842110012ee6 Mon Sep 17 00:00:00 2001 From: Tyler Compton Date: Fri, 23 Oct 2020 00:09:17 -0700 Subject: [PATCH 4/7] Clear up some boolean logic --- brainframe/cli/docker_compose.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/brainframe/cli/docker_compose.py b/brainframe/cli/docker_compose.py index b268962..2d6a26e 100644 --- a/brainframe/cli/docker_compose.py +++ b/brainframe/cli/docker_compose.py @@ -108,15 +108,14 @@ def check_existing_version(install_path: Path): def _assert_has_docker_permissions(): """Fails if the user does not have permissions to interact with Docker""" - if not os_utils.is_root(): - if not os_utils.currently_in_group("docker"): - error_message = ( - i18n.t("general.docker-bad-permissions") - + "\n" - + _group_recommendation_message("docker") - ) + if not (os_utils.is_root() or os_utils.currently_in_group("docker")): + error_message = ( + i18n.t("general.docker-bad-permissions") + + "\n" + + _group_recommendation_message("docker") + ) - print_utils.fail(error_message) + print_utils.fail(error_message) def _assert_has_write_permissions(path: Path): From c50a4547313116fd65d185b8b49e2325142ab91d Mon Sep 17 00:00:00 2001 From: Tyler Compton Date: Sat, 24 Oct 2020 12:05:13 -0700 Subject: [PATCH 5/7] Do code review fixes --- brainframe/cli/docker_compose.py | 24 +++++++++++----------- brainframe/cli/translations/general.en.yml | 10 ++++----- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/brainframe/cli/docker_compose.py b/brainframe/cli/docker_compose.py index 2d6a26e..2046664 100644 --- a/brainframe/cli/docker_compose.py +++ b/brainframe/cli/docker_compose.py @@ -1,6 +1,6 @@ import subprocess from pathlib import Path -from typing import List +from typing import List, Tuple import yaml import i18n import os @@ -16,7 +16,7 @@ ) -def assert_installed(install_path: Path): +def assert_installed(install_path: Path) -> None: compose_path = install_path / "docker-compose.yml" if not compose_path.is_file(): @@ -26,7 +26,7 @@ def assert_installed(install_path: Path): ) -def run(install_path: Path, commands: List[str]): +def run(install_path: Path, commands: List[str]) -> None: _assert_has_docker_permissions() compose_path = install_path / "docker-compose.yml" @@ -46,7 +46,7 @@ def run(install_path: Path, commands: List[str]): os_utils.run(full_command + commands) -def download(target: Path, version="latest"): +def download(target: Path, version: str = "latest") -> None: _assert_has_write_permissions(target.parent) subdomain, auth_flags, version = check_download_version(version=version) @@ -64,7 +64,8 @@ def download(target: Path, version="latest"): os_utils.give_brainframe_group_rw_access([target]) -def check_download_version(version="latest"): +def check_download_version(version: str = "latest") -> \ + Tuple[str, List[str], str]: subdomain = "" auth_flags = [] @@ -98,7 +99,7 @@ def check_download_version(version="latest"): return subdomain, auth_flags, version -def check_existing_version(install_path: Path): +def check_existing_version(install_path: Path) -> str: compose_path = install_path / "docker-compose.yml" compose = yaml.load(compose_path.read_text(), Loader=yaml.SafeLoader) version = compose["services"]["core"]["image"].split(":")[-1] @@ -106,7 +107,7 @@ def check_existing_version(install_path: Path): return version -def _assert_has_docker_permissions(): +def _assert_has_docker_permissions() -> None: """Fails if the user does not have permissions to interact with Docker""" if not (os_utils.is_root() or os_utils.currently_in_group("docker")): error_message = ( @@ -118,14 +119,13 @@ def _assert_has_docker_permissions(): print_utils.fail(error_message) -def _assert_has_write_permissions(path: Path): +def _assert_has_write_permissions(path: Path) -> None: """Fails if the user does not have write access to the given path.""" if os.access(path, os.W_OK): return error_message = i18n.t("general.file-bad-write-permissions", path=path) - - print() + error_message += "\n" if path.stat().st_gid == os_utils.BRAINFRAME_GROUP_ID: error_message += " " + _group_recommendation_message("brainframe") @@ -137,11 +137,11 @@ def _assert_has_write_permissions(path: Path): print_utils.fail(error_message) -def _group_recommendation_message(group: str): +def _group_recommendation_message(group: str) -> str: if os_utils.added_to_group("brainframe"): # The user is in the group, they just need to restart return i18n.t("general.restart-for-group-access", group=group) else: # The user is not in the group, so they need to either add # themselves or use sudo - return i18n.t("general.retry-with-sudo-or-group", group=group) + return i18n.t("general.retry-as-root-or-group", group=group) diff --git a/brainframe/cli/translations/general.en.yml b/brainframe/cli/translations/general.en.yml index 633c8c3..5ffe87a 100644 --- a/brainframe/cli/translations/general.en.yml +++ b/brainframe/cli/translations/general.en.yml @@ -12,7 +12,7 @@ en: to a custom location, ensure that the %{install_env_var} environment variable has been set." mkdir-permission-denied: "Permission denied while making directory - %{directory}, trying with sudo." + %{directory}, trying as root." staging-missing-credentials: "The %{username_env_var} and %{password_env_var} variables must be set to access staging." user-not-root: "This command must be run as root!" @@ -20,11 +20,11 @@ en: use Docker." restart-for-group-access: "Your user has been added to the \"%{group}\" group but you must restart to apply this change. Alternatively, you can try - again with sudo." - retry-with-sudo-or-group: "Please try again with sudo or consider adding your + again as root." + retry-as-root-or-group: "Please try again as root or consider adding your user to the \"%{group}\" group." file-bad-write-permissions: "Your user does not have write access to \"%{path}\"." - unexpected-group-for-file: "File \"%{path}\" is not part of the + unexpected-group-for-file: "File \"%{path}\" is not owned by the \"%{group}\" group, but probably should be. Please add this file to the group - or try again with sudo." + or try again as root." From 9e3f67e085cb41e59b18a8f386069fc29bfaf663 Mon Sep 17 00:00:00 2001 From: Tyler Compton Date: Sat, 24 Oct 2020 13:58:46 -0700 Subject: [PATCH 6/7] Run black --- brainframe/cli/docker_compose.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/brainframe/cli/docker_compose.py b/brainframe/cli/docker_compose.py index 2046664..8d008b9 100644 --- a/brainframe/cli/docker_compose.py +++ b/brainframe/cli/docker_compose.py @@ -64,8 +64,9 @@ def download(target: Path, version: str = "latest") -> None: os_utils.give_brainframe_group_rw_access([target]) -def check_download_version(version: str = "latest") -> \ - Tuple[str, List[str], str]: +def check_download_version( + version: str = "latest", +) -> Tuple[str, List[str], str]: subdomain = "" auth_flags = [] From a105147d790ac8eb5d86abb26ae46fc91af4ce2a Mon Sep 17 00:00:00 2001 From: Tyler Compton Date: Sat, 24 Oct 2020 14:04:48 -0700 Subject: [PATCH 7/7] Convince MyPy --- brainframe/cli/docker_compose.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/brainframe/cli/docker_compose.py b/brainframe/cli/docker_compose.py index 8d008b9..c5db614 100644 --- a/brainframe/cli/docker_compose.py +++ b/brainframe/cli/docker_compose.py @@ -1,6 +1,6 @@ import subprocess from pathlib import Path -from typing import List, Tuple +from typing import List, Tuple, cast, TextIO import yaml import i18n import os @@ -95,7 +95,10 @@ def check_download_version( stdout=subprocess.PIPE, encoding="utf-8", ) - version = result.stdout.readline().strip() + # stdout is a file-like object opened in text mode when the encoding + # argument is "utf-8" + stdout = cast(TextIO, result.stdout) + version = stdout.readline().strip() return subdomain, auth_flags, version