From 84e2a0e23c4d9c013e839b331c649c0b875983dd Mon Sep 17 00:00:00 2001 From: andre Date: Sun, 12 Jul 2020 02:17:03 -0700 Subject: [PATCH 1/5] Added subsonic server config for salt auth logic Allows users to toggle between the plain password auth and the salted auth. Salted auth is turned off by default since its only supported after Subsonic API 1.13.0 --- sublime/adapters/subsonic/adapter.py | 34 +++++++++++++++- tests/adapter_tests/adapter_manager_tests.py | 1 + tests/adapter_tests/subsonic_adapter_tests.py | 39 ++++++++++++++++++- 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/sublime/adapters/subsonic/adapter.py b/sublime/adapters/subsonic/adapter.py index bfd5ed8..cdb1126 100644 --- a/sublime/adapters/subsonic/adapter.py +++ b/sublime/adapters/subsonic/adapter.py @@ -1,3 +1,4 @@ +import hashlib import json import logging import math @@ -5,6 +6,7 @@ import multiprocessing import os import pickle import random +import string import tempfile from datetime import datetime, timedelta from pathlib import Path @@ -109,6 +111,15 @@ class SubsonicAdapter(Adapter): helptext="If toggled, Sublime Music will periodically save the play " "queue state so that you can resume on other devices.", ), + "salt_auth": ConfigParamDescriptor( + bool, + "Use Salt Authentication", + default=False, + advanced=True, + helptext="If toggled, Sublime Music will use salted hash tokens " + "instead of the plain password in the request urls (only supported on " + "Subsonic API 1.13.0+)", + ), } if networkmanager_imported: @@ -210,6 +221,7 @@ class SubsonicAdapter(Adapter): self.username = config["username"] self.password = cast(str, config.get_secret("password")) self.verify_cert = config["verify_cert"] + self.use_salt_auth = config["salt_auth"] if "salt_auth" in config else False self.is_shutting_down = False @@ -333,14 +345,32 @@ class SubsonicAdapter(Adapter): Gets the parameters that are needed for all requests to the Subsonic API. See Subsonic API Introduction for details. """ - return { + params = { "u": self.username, - "p": self.password, "c": "Sublime Music", "f": "json", "v": "1.15.0", } + if self.use_salt_auth: + salt, token = self._generate_auth_token() + params["s"] = salt + params["t"] = token + else: + params["p"] = self.password + + return params + + def _generate_auth_token(self) -> Tuple[str, str]: + """ + Generates the necessary authentication data to call the Subsonic API See the + Authentication section of www.subsonic.org/pages/api.jsp for more information + """ + salt = "".join(random.choices(string.ascii_letters + string.digits, k=8)) + unhashed_token = "{}{}".format(self.password, salt) + hashed_token = hashlib.md5(str.encode(unhashed_token)).hexdigest() + return (salt, hashed_token) + def _make_url(self, endpoint: str) -> str: return f"{self.hostname}/rest/{endpoint}.view" diff --git a/tests/adapter_tests/adapter_manager_tests.py b/tests/adapter_tests/adapter_manager_tests.py index afc6f35..355124d 100644 --- a/tests/adapter_tests/adapter_manager_tests.py +++ b/tests/adapter_tests/adapter_manager_tests.py @@ -16,6 +16,7 @@ def adapter_manager(tmp_path: Path): server_address="https://subsonic.example.com", username="test", verify_cert=True, + salt_auth=False, ) subsonic_config_store.set_secret("password", "testpass") diff --git a/tests/adapter_tests/subsonic_adapter_tests.py b/tests/adapter_tests/subsonic_adapter_tests.py index 989e140..df48009 100644 --- a/tests/adapter_tests/subsonic_adapter_tests.py +++ b/tests/adapter_tests/subsonic_adapter_tests.py @@ -24,11 +24,31 @@ def adapter(tmp_path: Path): server_address="https://subsonic.example.com", username="test", verify_cert=True, + salt_auth=False, ) config.set_secret("password", "testpass") adapter = SubsonicAdapter(config, tmp_path) adapter._is_mock = True + + yield adapter + adapter.shutdown() + + +@pytest.fixture +def salt_auth_adapter(tmp_path: Path): + ConfigurationStore.MOCK = True + config = ConfigurationStore( + server_address="https://subsonic.example.com", + username="test", + verify_cert=True, + salt_auth=True, + ) + config.set_secret("password", "testpass") + + adapter = SubsonicAdapter(config, tmp_path) + adapter._is_mock = True + yield adapter adapter.shutdown() @@ -82,7 +102,7 @@ def test_config_form(): SubsonicAdapter.get_configuration_form(config_store) -def test_request_making_methods(adapter: SubsonicAdapter): +def test_plain_auth_logic(adapter: SubsonicAdapter): expected = { "u": "test", "p": "testpass", @@ -92,6 +112,23 @@ def test_request_making_methods(adapter: SubsonicAdapter): } assert sorted(expected.items()) == sorted(adapter._get_params().items()) + +def test_salt_auth_logic(salt_auth_adapter: SubsonicAdapter): + expected = { + "u": "test", + "c": "Sublime Music", + "f": "json", + "v": "1.15.0", + } + + params = salt_auth_adapter._get_params() + assert "p" not in params + assert "s" in params + assert "t" in params + assert all(key in params and params[key] == expected[key] for key in expected) + + +def test_make_url(adapter: SubsonicAdapter): assert adapter._make_url("foo") == "https://subsonic.example.com/rest/foo.view" From d37bb160f616e2f04a2cd0b9ea1055f9d999039c Mon Sep 17 00:00:00 2001 From: andre Date: Sun, 12 Jul 2020 12:22:56 -0700 Subject: [PATCH 2/5] Address merge request comments 1. change string.format to string interpolation when creating the auth token 2. implemented migrate_configuration logic for salt_auth config value 3. added more explicit check of the token in the unit tests --- sublime/adapters/subsonic/adapter.py | 10 +++---- tests/adapter_tests/subsonic_adapter_tests.py | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/sublime/adapters/subsonic/adapter.py b/sublime/adapters/subsonic/adapter.py index cdb1126..cfbeacb 100644 --- a/sublime/adapters/subsonic/adapter.py +++ b/sublime/adapters/subsonic/adapter.py @@ -182,7 +182,8 @@ class SubsonicAdapter(Adapter): @staticmethod def migrate_configuration(config_store: ConfigurationStore): - pass + if "salt_auth" not in config_store: + config_store["salt_auth"] = False def __init__(self, config: ConfigurationStore, data_directory: Path): self.data_directory = data_directory @@ -221,7 +222,7 @@ class SubsonicAdapter(Adapter): self.username = config["username"] self.password = cast(str, config.get_secret("password")) self.verify_cert = config["verify_cert"] - self.use_salt_auth = config["salt_auth"] if "salt_auth" in config else False + self.use_salt_auth = config["salt_auth"] self.is_shutting_down = False @@ -367,9 +368,8 @@ class SubsonicAdapter(Adapter): Authentication section of www.subsonic.org/pages/api.jsp for more information """ salt = "".join(random.choices(string.ascii_letters + string.digits, k=8)) - unhashed_token = "{}{}".format(self.password, salt) - hashed_token = hashlib.md5(str.encode(unhashed_token)).hexdigest() - return (salt, hashed_token) + token = hashlib.md5(f"{self.password}{salt}".encode()).hexdigest() + return (salt, token) def _make_url(self, endpoint: str) -> str: return f"{self.hostname}/rest/{endpoint}.view" diff --git a/tests/adapter_tests/subsonic_adapter_tests.py b/tests/adapter_tests/subsonic_adapter_tests.py index df48009..036adc8 100644 --- a/tests/adapter_tests/subsonic_adapter_tests.py +++ b/tests/adapter_tests/subsonic_adapter_tests.py @@ -1,3 +1,4 @@ +import hashlib import json import logging import re @@ -124,10 +125,35 @@ def test_salt_auth_logic(salt_auth_adapter: SubsonicAdapter): params = salt_auth_adapter._get_params() assert "p" not in params assert "s" in params + salt = params["s"] assert "t" in params + assert params["t"] == hashlib.md5(f"testpass{salt}".encode()).hexdigest() assert all(key in params and params[key] == expected[key] for key in expected) +def test_migrate_configuration_populate_salt_auth(): + config = ConfigurationStore( + server_address="https://subsonic.example.com", + username="test", + verify_cert=True, + ) + SubsonicAdapter.migrate_configuration(config) + assert "salt_auth" in config + assert not config["salt_auth"] + + +def test_migrate_configuration_salt_auth_present(): + config = ConfigurationStore( + server_address="https://subsonic.example.com", + username="test", + verify_cert=True, + salt_auth=True, + ) + SubsonicAdapter.migrate_configuration(config) + assert "salt_auth" in config + assert config["salt_auth"] + + def test_make_url(adapter: SubsonicAdapter): assert adapter._make_url("foo") == "https://subsonic.example.com/rest/foo.view" From 7c0adc120fe17ee7168ad68b35fcd9db1f7b6f1f Mon Sep 17 00:00:00 2001 From: andre Date: Mon, 20 Jul 2020 23:08:54 -0700 Subject: [PATCH 3/5] Added auto-detection for salt auth --- sublime/adapters/subsonic/adapter.py | 42 ++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/sublime/adapters/subsonic/adapter.py b/sublime/adapters/subsonic/adapter.py index cfbeacb..7abd9fa 100644 --- a/sublime/adapters/subsonic/adapter.py +++ b/sublime/adapters/subsonic/adapter.py @@ -114,7 +114,7 @@ class SubsonicAdapter(Adapter): "salt_auth": ConfigParamDescriptor( bool, "Use Salt Authentication", - default=False, + default=True, advanced=True, helptext="If toggled, Sublime Music will use salted hash tokens " "instead of the plain password in the request urls (only supported on " @@ -169,10 +169,40 @@ class SubsonicAdapter(Adapter): "Double check the server address." ) except ServerError as e: - errors["__ping__"] = ( - "Error connecting to the server.\n" - f"Error {e.status_code}: {str(e)}" - ) + if e.status_code == 41: + # as per subsonic api docs, description of status_code 41 is + # "Token authentication not supported for LDAP users." so fall + # back to password auth + config_store["salt_auth"] = False + logging.warn( + "Salted auth no supported for LDAP users, falling back to " + "regular password auth" + ) + elif e.status_code == 10 and config_store["salt_auth"]: + # if salt auth is not enabled, server will return error server + # error with status_code 10 since it'll interpret it as a + # missing (password) parameter + try: + config_store["salt_auth"] = False + tmp_adapter = SubsonicAdapter( + config_store, Path(tmp_dir_name) + ) + tmp_adapter._get_json( + tmp_adapter._make_url("ping"), + timeout=2, + is_exponential_backoff_ping=True, + ) + logging.warn( + "Salted auth not supported, falling back to regular " + "password auth" + ) + except ServerError: + config_store["salt_auth"] = True + else: + errors["__ping__"] = ( + "Error connecting to the server.\n" + f"Error {e.status_code}: {str(e)}" + ) except Exception as e: errors["__ping__"] = str(e) @@ -183,7 +213,7 @@ class SubsonicAdapter(Adapter): @staticmethod def migrate_configuration(config_store: ConfigurationStore): if "salt_auth" not in config_store: - config_store["salt_auth"] = False + config_store["salt_auth"] = True def __init__(self, config: ConfigurationStore, data_directory: Path): self.data_directory = data_directory From 7b3f839d3420a7711c6ebdb208f7c8ba3c5ebd19 Mon Sep 17 00:00:00 2001 From: andre Date: Mon, 20 Jul 2020 23:37:04 -0700 Subject: [PATCH 4/5] Fixed handling of status code 41 and surfacing errors on retry --- sublime/adapters/subsonic/adapter.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/sublime/adapters/subsonic/adapter.py b/sublime/adapters/subsonic/adapter.py index 7abd9fa..684be2d 100644 --- a/sublime/adapters/subsonic/adapter.py +++ b/sublime/adapters/subsonic/adapter.py @@ -169,19 +169,13 @@ class SubsonicAdapter(Adapter): "Double check the server address." ) except ServerError as e: - if e.status_code == 41: - # as per subsonic api docs, description of status_code 41 is - # "Token authentication not supported for LDAP users." so fall - # back to password auth - config_store["salt_auth"] = False - logging.warn( - "Salted auth no supported for LDAP users, falling back to " - "regular password auth" - ) - elif e.status_code == 10 and config_store["salt_auth"]: - # if salt auth is not enabled, server will return error server - # error with status_code 10 since it'll interpret it as a - # missing (password) parameter + if e.status_code in [10, 41] and config_store["salt_auth"]: + # status code 10: if salt auth is not enabled, server will + # return error server error with status_code 10 since it'll + # interpret it as a missing (password) parameter + # status code 41: as per subsonic api docs, description of + # status_code 41 is "Token authentication not supported for LDAP + # users." so fall back to password auth try: config_store["salt_auth"] = False tmp_adapter = SubsonicAdapter( @@ -196,8 +190,12 @@ class SubsonicAdapter(Adapter): "Salted auth not supported, falling back to regular " "password auth" ) - except ServerError: + except ServerError as retry_e: config_store["salt_auth"] = True + errors["__ping__"] = ( + "Error connecting to the server.\n" + f"Error {retry_e.status_code}: {str(retry_e)}" + ) else: errors["__ping__"] = ( "Error connecting to the server.\n" From 340b24e91012c86e3e61e1d3b67a23bdcabfcec9 Mon Sep 17 00:00:00 2001 From: andre Date: Mon, 20 Jul 2020 23:40:54 -0700 Subject: [PATCH 5/5] Fixed failing unit test --- tests/adapter_tests/subsonic_adapter_tests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/adapter_tests/subsonic_adapter_tests.py b/tests/adapter_tests/subsonic_adapter_tests.py index 036adc8..2cd3e87 100644 --- a/tests/adapter_tests/subsonic_adapter_tests.py +++ b/tests/adapter_tests/subsonic_adapter_tests.py @@ -139,7 +139,7 @@ def test_migrate_configuration_populate_salt_auth(): ) SubsonicAdapter.migrate_configuration(config) assert "salt_auth" in config - assert not config["salt_auth"] + assert config["salt_auth"] def test_migrate_configuration_salt_auth_present(): @@ -147,11 +147,11 @@ def test_migrate_configuration_salt_auth_present(): server_address="https://subsonic.example.com", username="test", verify_cert=True, - salt_auth=True, + salt_auth=False, ) SubsonicAdapter.migrate_configuration(config) assert "salt_auth" in config - assert config["salt_auth"] + assert not config["salt_auth"] def test_make_url(adapter: SubsonicAdapter):