Added working support for private storage by nickovs · Pull Request #16903 · home-assistant/core

nickovs · 2018-09-26T19:02:26Z

Description:

This is a fixed version of #16878. It adds support for marking Store objects objects as private to prevent data exposure.

Compared to the original PR it:

fixes the corruption bug in the original PR caused by not properly truncating the storage file when writing JSON files using utils.json.save_json()
Implements atomic file replacement in the helpers.storage.Store class to prevent file corruption if the system crashes while writing a storage file.
Ensures that the file permissions of existing storage files get updated if they are now marked as private and the contents is updated.
Adds test cases to test/helpers/test_storage.py to test that updating an existing storage file works correctly.

Related issue (if applicable): fixes #16349

Pull request in home-assistant.io with documentation (if applicable): N/A

Example entry for `configuration.yaml` (if applicable):

N/A

Checklist:

The code change is tested and works locally.
Local tests pass with tox.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.io N/A

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example). N/A
New dependencies are only imported inside functions that use them (example). N/A
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py. N/A
New files were added to .coveragerc. N/A

If the code does not interact with devices:

Tests have been added to verify that the new code works. N/A

amelchio · 2018-09-26T22:25:47Z

Sorry, I removed some of your changes in #16909 to get rid of CI failures.

Please add them back in this PR :)

nickovs · 2018-09-26T23:51:07Z

@amelchio I fixed the merge conflict but it seems that you also reverted a fix to a broken test file (tests/components/emulated_hue/test_init.py) and I don't know how to put it back without pushing a whole new pull request. Any suggestions?

nickovs · 2018-09-27T00:16:06Z

OK, I just added the file back at my end and pushed it again. It seems to work now.

pvizeli · 2018-09-27T07:38:50Z

homeassistant/helpers/storage.py


        _LOGGER.debug('Writing data for %s', self.key)
-        json.save_json(path, data, self._private)
+        tmp_path = path + "__TEMP__"


Mark file as not visible with ., and we should use a context manager to cleanup the temporary file after this.

No need to clean up since we're using os.replace

amelchio · 2018-09-27T08:49:18Z

@nickovs Yes, fine to handle the revert like that 👍. You can push additional commits until the PR is merged, that will also work to address review comments.

balloob · 2018-09-27T09:51:15Z

tests/helpers/test_storage.py

    assert data == MOCK_DATA


+async def test_overwriting(hass, store):


This actually doesn't do anything because all writes for the storage helper are mocked in tests

Aha. Does that mean that the current storage implementation does not get tested down the filesystem level with the existing unit tests? If that's the case, is it fine to leave this out altogether?

Correct, we try to avoid I/O as much as possible in unit tests as it's slow.

We should still test this functionality but it should test util.save_json directly, as that one is not mocked out.

houndci-bot · 2018-09-28T12:51:17Z

tests/util/test_json.py

+        with open(fname, "w") as fh:
+            fh.write(TEST_BAD_SERIALIED)
+        with self.assertRaises(HomeAssistantError):
+            data = load_json(fname)


local variable 'data' is assigned to but never used

houndci-bot · 2018-09-28T12:51:17Z

tests/util/test_json.py

+        save_json(fname, TEST_JSON_B)
+        data = load_json(fname)
+        self.assertEqual(data, TEST_JSON_B)
+


blank line contains whitespace

houndci-bot · 2018-09-28T12:51:18Z

tests/util/test_json.py

+        for fname in os.listdir(self.tmp_dir):
+            os.remove(os.path.join(self.tmp_dir, fname))
+        os.rmdir(self.tmp_dir)
+


blank line contains whitespace

houndci-bot · 2018-09-28T12:51:18Z

tests/util/test_json.py

+# Test data that can not be loaded as JSON
+TEST_BAD_SERIALIED = "THIS IS NOT JSON\n"
+
+class TestJSON(unittest.TestCase):


expected 2 blank lines, found 1

houndci-bot · 2018-09-28T12:51:18Z

tests/util/test_json.py

+TEST_JSON_A = {"a":1, "B":"two"}
+TEST_JSON_B = {"a":"one", "B":2}
+# Test data that can not be saved as JSON (keys must be strings)
+TEST_BAD_OBJECT = {("A",):1}


missing whitespace after ':'

houndci-bot · 2018-09-28T12:51:18Z

tests/util/test_json.py

+
+# Test data that can be saved as JSON
+TEST_JSON_A = {"a":1, "B":"two"}
+TEST_JSON_B = {"a":"one", "B":2}


missing whitespace after ':'

houndci-bot · 2018-09-28T12:51:18Z

tests/util/test_json.py

+from homeassistant.exceptions import HomeAssistantError
+
+# Test data that can be saved as JSON
+TEST_JSON_A = {"a":1, "B":"two"}


missing whitespace after ':'

houndci-bot · 2018-09-28T12:51:18Z

tests/util/test_json.py

+import sys
+from tempfile import mkdtemp
+
+from homeassistant.util.json import (SerializationError, WriteError,


'homeassistant.util.json.WriteError' imported but unused

houndci-bot · 2018-09-28T12:58:04Z

tests/util/test_json.py

+TEST_JSON_A = {"a" : 1, "B" : "two"}
+TEST_JSON_B = {"a" : "one", "B" : 2}
+# Test data that can not be saved as JSON (keys must be strings)
+TEST_BAD_OBJECT = {("A",) : 1}


whitespace before ':'

houndci-bot · 2018-09-28T12:58:05Z

tests/util/test_json.py

+
+# Test data that can be saved as JSON
+TEST_JSON_A = {"a" : 1, "B" : "two"}
+TEST_JSON_B = {"a" : "one", "B" : 2}


whitespace before ':'

houndci-bot · 2018-09-28T12:58:05Z

tests/util/test_json.py

+from homeassistant.exceptions import HomeAssistantError
+
+# Test data that can be saved as JSON
+TEST_JSON_A = {"a" : 1, "B" : "two"}


whitespace before ':'

nickovs · 2018-09-28T17:20:15Z

OK. I wrote a test suite for the underlying util/json.py code, which was largely untested before. These tests of necessity do some I/O, since they are testing file system operations, but they take very little time to do so and it's good to have this code tested. There are still some pathways between the upper layers of the storage system and the low-level file representation but, as discussed above in the code comments, this would require some rather different test suites to be written.

balloob · 2018-10-01T10:41:22Z

homeassistant/helpers/storage.py

-        json.save_json(path, data, self._private)
+        tmp_path = path + "__TEMP__"
+        json.save_json(tmp_path, data, self._private)
+        os.replace(tmp_path, path)


I think that we should move this functionality to save_json so that all calls can benefit.

I also think that we should consider cleaning up the temp path in case of an exception replacing the other file

Fair points. I'll make it so.

Added extra clean-up code to ensure that temporary files are removed in case of errors. Updated emulated_hue unit tests to avoid errors.

balloob · 2018-10-02T08:16:55Z

Alright let's try again, great work! 👍

…ous writes Reworked tests/components/emulated_hue/test_init.py to not be dependent on the specific internal implementation of util/jsonn.py

…7636) Reworked tests/components/emulated_hue/test_init.py to not be dependent on the specific internal implementation of util/jsonn.py

Fixed file corruption bugs in private storage code.

fe5ff38

nickovs requested a review from a team as a code owner September 26, 2018 19:02

homeassistant added core small-pr PRs with less than 30 lines. cla-signed labels Sep 26, 2018

ghost added the in progress label Sep 26, 2018

nickovs mentioned this pull request Sep 26, 2018

Added support for private storage. #16878

Merged

7 tasks

Merge branch 'dev' into private-storage-2

e5eb061

homeassistant added the cla-signed label Sep 26, 2018

Restoring fixed test case.

7df69ed

homeassistant added the cla-signed label Sep 26, 2018

pvizeli reviewed Sep 27, 2018

View reviewed changes

balloob reviewed Sep 27, 2018

View reviewed changes

nickovs added 2 commits September 27, 2018 20:58

Implemented test suite for utils/json.py

e5cb184

Added new unit test cases for util/json.py

86303e3

houndci-bot reviewed Sep 28, 2018

View reviewed changes

Dixed formatting nags

11d03d0

houndci-bot reviewed Sep 28, 2018

View reviewed changes

nickovs added 3 commits September 28, 2018 06:59

Fixed more nags from the Hound

10646fa

Added doc strings to some very short functions

83a1913

Fixing lint's complains about my choice of parts of speach. Sigh.

070ac97

balloob reviewed Oct 1, 2018

View reviewed changes

nickovs added 2 commits October 1, 2018 12:39

Moved atomic save operations down into util/json.py so that all benefit.

52dfcc1

Added extra clean-up code to ensure that temporary files are removed in case of errors. Updated emulated_hue unit tests to avoid errors.

Apparently 'e' is not allows as a variable name for an exception...

f80a92e

balloob added the Hacktoberfest label Oct 2, 2018

balloob approved these changes Oct 2, 2018

View reviewed changes

balloob merged commit b0b3620 into home-assistant:dev Oct 2, 2018

ghost removed the in progress label Oct 2, 2018

balloob mentioned this pull request Oct 12, 2018

0.80.0 #17361

Merged

wolph mentioned this pull request Oct 18, 2018

#16903 (private storage) breaks multiple simultaneous json writes #17587

Closed

nickovs mentioned this pull request Oct 20, 2018

Fixed issue #17587 exception with multiple simultanious writes #17636

Merged

8 tasks

balloob pushed a commit that referenced this pull request Oct 23, 2018

Fixed issue #16903 re exception with multiple simultanious writes (#1…

50f0eac

…7636) Reworked tests/components/emulated_hue/test_init.py to not be dependent on the specific internal implementation of util/jsonn.py

balloob pushed a commit that referenced this pull request Oct 23, 2018

Fixed issue #16903 re exception with multiple simultanious writes (#1…

fe8dec2

…7636) Reworked tests/components/emulated_hue/test_init.py to not be dependent on the specific internal implementation of util/jsonn.py

balloob mentioned this pull request Oct 26, 2018

0.81 #17809

Merged

home-assistant locked and limited conversation to collaborators Feb 5, 2019

		assert data == MOCK_DATA


		async def test_overwriting(hass, store):

Uh oh!

Conversation

nickovs commented Sep 26, 2018

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

amelchio commented Sep 26, 2018

Uh oh!

nickovs commented Sep 26, 2018

Uh oh!

nickovs commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amelchio commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickovs commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Example entry for `configuration.yaml` (if applicable):