From af9f9b3e58161598f982bcf9b85bdbb890d2ff6e Mon Sep 17 00:00:00 2001 From: Johan Schreurs Date: Wed, 14 Dec 2022 21:16:52 +0100 Subject: [PATCH 1/7] Issue #198 Use oschmod on Windows to set/get permissions of PrivateJsonFile --- openeo/rest/auth/config.py | 24 ++++++++++++++++++------ setup.py | 2 ++ tests/rest/auth/test_config.py | 14 ++++++++++++-- tests/test_config.py | 9 ++++++++- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/openeo/rest/auth/config.py b/openeo/rest/auth/config.py index 5b11dff3d..f406a13fb 100644 --- a/openeo/rest/auth/config.py +++ b/openeo/rest/auth/config.py @@ -14,6 +14,8 @@ from pathlib import Path from typing import Union, Tuple, Dict +import oschmod + from openeo import __version__ from openeo.config import get_user_config_dir, get_user_data_dir from openeo.util import rfc3339, deep_get, deep_set @@ -25,15 +27,21 @@ def assert_private_file(path: Path): """Check that given file is only readable by user.""" - mode = path.stat().st_mode + + # use oschmod on Windows + if platform.system() == "Windows": + mode = oschmod.get_mode(str(path)) + else: + mode = path.stat().st_mode if (mode & stat.S_IRWXG) or (mode & stat.S_IRWXO): message = "File {p} could be readable by others: mode {a:o} (expected: {e:o}).".format( p=path, a=mode & (stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO), e=_PRIVATE_PERMS ) - if platform.system() == 'Windows': - log.info(message) - else: - raise PermissionError(message) + raise PermissionError(message) + # if platform.system() == 'Windows': + # log.info(message) + # else: + # raise PermissionError(message) def utcnow_rfc3339() -> str: @@ -86,7 +94,11 @@ def _write(self, data: dict): # TODO: add file locking to avoid race conditions? with self._path.open("w", encoding="utf8") as f: json.dump(data, f, indent=2) - self._path.chmod(mode=_PRIVATE_PERMS) + # on Windows us oschmod because Python chmod implementation doesn't work on Windows. + if platform.system() == "Windows": + oschmod.set_mode(str(self._path), mode=_PRIVATE_PERMS) + else: + self._path.chmod(mode=_PRIVATE_PERMS) assert_private_file(self._path) def get(self, *keys, default=None) -> Union[dict, str, int]: diff --git a/setup.py b/setup.py index 28178c3af..e89060d2c 100644 --- a/setup.py +++ b/setup.py @@ -47,6 +47,8 @@ 'xarray>=0.12.3', 'pandas>0.20.0', 'deprecated>=1.2.12', + 'oschmod; sys_platform == "win32"', + 'pywin32; sys_platform == "win32"', ], extras_require={ "dev": tests_require + [ diff --git a/tests/rest/auth/test_config.py b/tests/rest/auth/test_config.py index 6d22ce6bd..9cfaef242 100644 --- a/tests/rest/auth/test_config.py +++ b/tests/rest/auth/test_config.py @@ -1,6 +1,8 @@ import json from unittest import mock +import platform +import oschmod import pytest import openeo.rest.auth.config @@ -31,7 +33,10 @@ def test_permissions(self, tmp_path): assert not private.path.exists() private.set("foo", "bar", value=42) assert private.path.exists() - st_mode = private.path.stat().st_mode + if platform.system() == 'Windows': + st_mode = oschmod.get_mode(str(private.path)) + else: + st_mode = private.path.stat().st_mode assert st_mode & 0o777 == 0o600 def test_wrong_permissions(self, tmp_path): @@ -151,7 +156,12 @@ def test_public_file(self, tmp_path): def test_permissions(self, tmp_path): r = RefreshTokenStore(path=tmp_path) r.set_refresh_token("foo", "bar", "imd6$3cr3t") - st_mode = (tmp_path / RefreshTokenStore.DEFAULT_FILENAME).stat().st_mode + token_path = (tmp_path / RefreshTokenStore.DEFAULT_FILENAME) + if platform.system() == 'Windows': + st_mode = oschmod.get_mode(str(token_path)) + else: + token_path.stat.st_mode() + st_mode = (tmp_path / RefreshTokenStore.DEFAULT_FILENAME).stat().st_mode assert st_mode & 0o777 == 0o600 def test_get_set_refresh_token(self, tmp_path): diff --git a/tests/test_config.py b/tests/test_config.py index 868002ce3..c2da08e81 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -159,6 +159,13 @@ def test_get_config_verbose(tmp_path, caplog, capsys, verbose, force_interactive config = get_config() assert config.get("general.verbose") == verbose - regex = re.compile(f"Loaded.*config from.*{config_path}") + # TODO: on Windows the path in config_path makes the regex invalid. + # My guess is the \ from the path is interpreted as an escape sequence + import platform + if platform.system() == "Windows": + config_path_escaped = str(config_path).replace("\\", "\\\\") + else: + config_path_escaped = config_path + regex = re.compile(f"Loaded.*config from.*{config_path_escaped}") assert regex.search(caplog.text) assert bool(regex.search(capsys.readouterr().out)) == on_stdout From e64f576c4790817b5fea1d23653d1bd7f563132d Mon Sep 17 00:00:00 2001 From: Johan Schreurs Date: Wed, 14 Dec 2022 21:55:07 +0100 Subject: [PATCH 2/7] Issue #198 Fix bug introduced in previous commit: tests/rest/auth/test_config.py:163: AttributeError AttributeError: 'function' object has no attribute 'st_mode' --- openeo/rest/auth/config.py | 8 +++----- setup.py | 2 +- tests/rest/auth/test_config.py | 2 +- tests/test_config.py | 7 ++++--- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/openeo/rest/auth/config.py b/openeo/rest/auth/config.py index f406a13fb..b735e8af7 100644 --- a/openeo/rest/auth/config.py +++ b/openeo/rest/auth/config.py @@ -29,6 +29,7 @@ def assert_private_file(path: Path): """Check that given file is only readable by user.""" # use oschmod on Windows + # TODO: can we use oschmod for all operating systems, make the code simpler and consitent? if platform.system() == "Windows": mode = oschmod.get_mode(str(path)) else: @@ -38,10 +39,6 @@ def assert_private_file(path: Path): p=path, a=mode & (stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO), e=_PRIVATE_PERMS ) raise PermissionError(message) - # if platform.system() == 'Windows': - # log.info(message) - # else: - # raise PermissionError(message) def utcnow_rfc3339() -> str: @@ -94,7 +91,8 @@ def _write(self, data: dict): # TODO: add file locking to avoid race conditions? with self._path.open("w", encoding="utf8") as f: json.dump(data, f, indent=2) - # on Windows us oschmod because Python chmod implementation doesn't work on Windows. + # On Windows we use oschmod because Python chmod implementation doesn't work on Windows. + # TODO: can we use oschmod on all platforms? Seems like that is the intention of oschmod. if platform.system() == "Windows": oschmod.set_mode(str(self._path), mode=_PRIVATE_PERMS) else: diff --git a/setup.py b/setup.py index e89060d2c..df9fd6de5 100644 --- a/setup.py +++ b/setup.py @@ -47,7 +47,7 @@ 'xarray>=0.12.3', 'pandas>0.20.0', 'deprecated>=1.2.12', - 'oschmod; sys_platform == "win32"', + 'oschmod', 'pywin32; sys_platform == "win32"', ], extras_require={ diff --git a/tests/rest/auth/test_config.py b/tests/rest/auth/test_config.py index 9cfaef242..07b17eb75 100644 --- a/tests/rest/auth/test_config.py +++ b/tests/rest/auth/test_config.py @@ -160,7 +160,7 @@ def test_permissions(self, tmp_path): if platform.system() == 'Windows': st_mode = oschmod.get_mode(str(token_path)) else: - token_path.stat.st_mode() + token_path.stat().st_mode st_mode = (tmp_path / RefreshTokenStore.DEFAULT_FILENAME).stat().st_mode assert st_mode & 0o777 == 0o600 diff --git a/tests/test_config.py b/tests/test_config.py index c2da08e81..f058d31f9 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,6 +1,7 @@ import contextlib import logging import os +import platform import random import re import textwrap @@ -159,9 +160,9 @@ def test_get_config_verbose(tmp_path, caplog, capsys, verbose, force_interactive config = get_config() assert config.get("general.verbose") == verbose - # TODO: on Windows the path in config_path makes the regex invalid. - # My guess is the \ from the path is interpreted as an escape sequence - import platform + # On Windows the path in config_path would make the regex invalid because + # windows paths contain \ and a regex interprets that as an escape sequence. + # Therefore we have to escape the \ to \\ . if platform.system() == "Windows": config_path_escaped = str(config_path).replace("\\", "\\\\") else: From 50c00f28fdd1c7d052933e9a6b258edbecbea32e Mon Sep 17 00:00:00 2001 From: Johan Schreurs Date: Thu, 15 Dec 2022 10:19:50 +0100 Subject: [PATCH 3/7] Issue #198 Only install and use oschmod when it is a Windows system. --- openeo/rest/auth/config.py | 6 ++++-- setup.py | 2 +- tests/rest/auth/test_config.py | 8 +++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/openeo/rest/auth/config.py b/openeo/rest/auth/config.py index b735e8af7..e60f7414b 100644 --- a/openeo/rest/auth/config.py +++ b/openeo/rest/auth/config.py @@ -14,12 +14,14 @@ from pathlib import Path from typing import Union, Tuple, Dict -import oschmod - from openeo import __version__ from openeo.config import get_user_config_dir, get_user_data_dir from openeo.util import rfc3339, deep_get, deep_set +if platform.system() == 'Windows': + import oschmod + + _PRIVATE_PERMS = stat.S_IRUSR | stat.S_IWUSR log = logging.getLogger(__name__) diff --git a/setup.py b/setup.py index df9fd6de5..e89060d2c 100644 --- a/setup.py +++ b/setup.py @@ -47,7 +47,7 @@ 'xarray>=0.12.3', 'pandas>0.20.0', 'deprecated>=1.2.12', - 'oschmod', + 'oschmod; sys_platform == "win32"', 'pywin32; sys_platform == "win32"', ], extras_require={ diff --git a/tests/rest/auth/test_config.py b/tests/rest/auth/test_config.py index 07b17eb75..ccbf3ce3e 100644 --- a/tests/rest/auth/test_config.py +++ b/tests/rest/auth/test_config.py @@ -2,12 +2,18 @@ from unittest import mock import platform -import oschmod import pytest import openeo.rest.auth.config from openeo.rest.auth.config import RefreshTokenStore, AuthConfig, PrivateJsonFile +# TODO: We could simplify the code if we can verify oschmod works transparently on Linux/Mac as well. +# I think oschmod does work on Linux and Mac, but since it we use it for a +# security sensitive feature, let's make sure there are no problems first. +# For now, it is fine to have some Windows specific code. +if platform.system() == 'Windows': + import oschmod + class TestPrivateJsonFile: From de07c273eb2e01103a99d36f21889c6c5dd5da27 Mon Sep 17 00:00:00 2001 From: Johan Schreurs Date: Thu, 15 Dec 2022 17:45:00 +0100 Subject: [PATCH 4/7] Issue #198 Code review: simplify and encapsulate getting/setting file permissions. --- openeo/rest/auth/config.py | 40 +++++++++++++++++++++------------- setup.py | 3 +-- tests/rest/auth/test_config.py | 22 +++---------------- tests/test_config.py | 11 ++-------- 4 files changed, 31 insertions(+), 45 deletions(-) diff --git a/openeo/rest/auth/config.py b/openeo/rest/auth/config.py index e60f7414b..68302da40 100644 --- a/openeo/rest/auth/config.py +++ b/openeo/rest/auth/config.py @@ -8,7 +8,6 @@ import json import logging -import platform import stat from datetime import datetime from pathlib import Path @@ -18,8 +17,15 @@ from openeo.config import get_user_config_dir, get_user_data_dir from openeo.util import rfc3339, deep_get, deep_set -if platform.system() == 'Windows': +# Use oschmod if it is installed. +# Otherwise we fall back on Python's chmod implementation. +# On Windows we have to use oschmod because Python chmod implementation doesn't work on Windows. +# TODO: Can we use oschmod transparantly on all platforms? +# Seems like that is the intention of oschmod, but for now we will play it safe. +try: import oschmod +except ImportError: + oschmod = None _PRIVATE_PERMS = stat.S_IRUSR | stat.S_IWUSR @@ -27,15 +33,24 @@ log = logging.getLogger(__name__) -def assert_private_file(path: Path): - """Check that given file is only readable by user.""" +def get_file_mode(path: Path): + """Get the file permission bits in a way that works on both *nix and Windows platforms.""" + if oschmod: + return oschmod.get_mode(str(path)) + return path.stat().st_mode - # use oschmod on Windows - # TODO: can we use oschmod for all operating systems, make the code simpler and consitent? - if platform.system() == "Windows": - mode = oschmod.get_mode(str(path)) + +def set_file_mode(path: Path, mode): + """Set the file permission bits in a way that works on both *nix and Windows platforms.""" + if oschmod: + oschmod.set_mode(str(path), mode=mode) else: - mode = path.stat().st_mode + path.chmod(mode=mode) + + +def assert_private_file(path: Path): + """Check that given file is only readable by user.""" + mode = get_file_mode(path) if (mode & stat.S_IRWXG) or (mode & stat.S_IRWXO): message = "File {p} could be readable by others: mode {a:o} (expected: {e:o}).".format( p=path, a=mode & (stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO), e=_PRIVATE_PERMS @@ -93,12 +108,7 @@ def _write(self, data: dict): # TODO: add file locking to avoid race conditions? with self._path.open("w", encoding="utf8") as f: json.dump(data, f, indent=2) - # On Windows we use oschmod because Python chmod implementation doesn't work on Windows. - # TODO: can we use oschmod on all platforms? Seems like that is the intention of oschmod. - if platform.system() == "Windows": - oschmod.set_mode(str(self._path), mode=_PRIVATE_PERMS) - else: - self._path.chmod(mode=_PRIVATE_PERMS) + set_file_mode(self._path, mode=_PRIVATE_PERMS) assert_private_file(self._path) def get(self, *keys, default=None) -> Union[dict, str, int]: diff --git a/setup.py b/setup.py index e89060d2c..f0bd463d1 100644 --- a/setup.py +++ b/setup.py @@ -47,8 +47,7 @@ 'xarray>=0.12.3', 'pandas>0.20.0', 'deprecated>=1.2.12', - 'oschmod; sys_platform == "win32"', - 'pywin32; sys_platform == "win32"', + 'oschmod>=0.3.12', ], extras_require={ "dev": tests_require + [ diff --git a/tests/rest/auth/test_config.py b/tests/rest/auth/test_config.py index ccbf3ce3e..231d3e352 100644 --- a/tests/rest/auth/test_config.py +++ b/tests/rest/auth/test_config.py @@ -1,18 +1,10 @@ import json from unittest import mock -import platform import pytest import openeo.rest.auth.config -from openeo.rest.auth.config import RefreshTokenStore, AuthConfig, PrivateJsonFile - -# TODO: We could simplify the code if we can verify oschmod works transparently on Linux/Mac as well. -# I think oschmod does work on Linux and Mac, but since it we use it for a -# security sensitive feature, let's make sure there are no problems first. -# For now, it is fine to have some Windows specific code. -if platform.system() == 'Windows': - import oschmod +from openeo.rest.auth.config import RefreshTokenStore, AuthConfig, PrivateJsonFile, get_file_mode class TestPrivateJsonFile: @@ -39,10 +31,7 @@ def test_permissions(self, tmp_path): assert not private.path.exists() private.set("foo", "bar", value=42) assert private.path.exists() - if platform.system() == 'Windows': - st_mode = oschmod.get_mode(str(private.path)) - else: - st_mode = private.path.stat().st_mode + st_mode = get_file_mode(private.path) assert st_mode & 0o777 == 0o600 def test_wrong_permissions(self, tmp_path): @@ -162,12 +151,7 @@ def test_public_file(self, tmp_path): def test_permissions(self, tmp_path): r = RefreshTokenStore(path=tmp_path) r.set_refresh_token("foo", "bar", "imd6$3cr3t") - token_path = (tmp_path / RefreshTokenStore.DEFAULT_FILENAME) - if platform.system() == 'Windows': - st_mode = oschmod.get_mode(str(token_path)) - else: - token_path.stat().st_mode - st_mode = (tmp_path / RefreshTokenStore.DEFAULT_FILENAME).stat().st_mode + st_mode = get_file_mode(tmp_path / RefreshTokenStore.DEFAULT_FILENAME) assert st_mode & 0o777 == 0o600 def test_get_set_refresh_token(self, tmp_path): diff --git a/tests/test_config.py b/tests/test_config.py index f058d31f9..51e048624 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,7 +1,6 @@ import contextlib import logging import os -import platform import random import re import textwrap @@ -160,13 +159,7 @@ def test_get_config_verbose(tmp_path, caplog, capsys, verbose, force_interactive config = get_config() assert config.get("general.verbose") == verbose - # On Windows the path in config_path would make the regex invalid because - # windows paths contain \ and a regex interprets that as an escape sequence. - # Therefore we have to escape the \ to \\ . - if platform.system() == "Windows": - config_path_escaped = str(config_path).replace("\\", "\\\\") - else: - config_path_escaped = config_path - regex = re.compile(f"Loaded.*config from.*{config_path_escaped}") + # re.escape config_path because Windows paths contain "\" + regex = re.compile(f"Loaded.*config from.*{re.escape(config_path)}") assert regex.search(caplog.text) assert bool(regex.search(capsys.readouterr().out)) == on_stdout From 1a1170a4b5664d697e5e6d6433ac6166094bee91 Mon Sep 17 00:00:00 2001 From: Johan Schreurs Date: Thu, 15 Dec 2022 18:16:50 +0100 Subject: [PATCH 5/7] Issue #198 Correct regex in test_config.py::test_get_config_verbose --- tests/test_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_config.py b/tests/test_config.py index 51e048624..32b5be12e 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -160,6 +160,6 @@ def test_get_config_verbose(tmp_path, caplog, capsys, verbose, force_interactive assert config.get("general.verbose") == verbose # re.escape config_path because Windows paths contain "\" - regex = re.compile(f"Loaded.*config from.*{re.escape(config_path)}") + regex = re.compile(f"Loaded.*config from.*{re.escape(str(config_path))}") assert regex.search(caplog.text) assert bool(regex.search(capsys.readouterr().out)) == on_stdout From 98b978995ea08d8656a06e8e23809c57d91db3b8 Mon Sep 17 00:00:00 2001 From: Johan Schreurs Date: Thu, 15 Dec 2022 18:29:11 +0100 Subject: [PATCH 6/7] Issue #198 Code review: make dependency oschmod required on Windows but in extra_requires if you want it on another platform --- setup.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/setup.py b/setup.py index f0bd463d1..2e179b1dc 100644 --- a/setup.py +++ b/setup.py @@ -47,7 +47,7 @@ 'xarray>=0.12.3', 'pandas>0.20.0', 'deprecated>=1.2.12', - 'oschmod>=0.3.12', + 'oschmod>=0.3.12; sys_platform == "win32"', ], extras_require={ "dev": tests_require + [ @@ -56,6 +56,9 @@ "sphinx-autodoc-typehints", "flake8", "myst-parser", + ], + "oschmod": [ # install oschmod even when platform is not Windows, e.g. for testing in CI. + "oschmod>=0.3.12" ] }, entry_points={ From 14abd8891c247cbdc6f3a75a63b657f7248e6392 Mon Sep 17 00:00:00 2001 From: Johan Schreurs Date: Fri, 16 Dec 2022 11:53:07 +0100 Subject: [PATCH 7/7] Issue #198: Updated changelog --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad2eb72dd..980471c3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed/improved math operator/process support for `DataCube`s in "apply" mode (non-"band math"), allowing expressions like `10 * cube.log10()` and `~(cube == 0)` ([#123](https://github.com/Open-EO/openeo-python-client/issues/123)) - +- Fixed: PermissionError with PrivateJsonFile on Windows. It can now protect the file, using oschmod. + ([#198](https://github.com/Open-EO/openeo-python-client/issues/198)) +- Fixed: Some unit tests for configuration that were broken on Windows due to differences in paths. + ([#350](https://github.com/Open-EO/openeo-python-client/issues/350)) ## [0.13.0] - 2022-10-10 - "UDF UX" release