diff --git a/docs/settings.rst b/docs/settings.rst index 532f52cf..9adbe76b 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -95,6 +95,13 @@ As a secure default, Mopidy only accepts connections from ``localhost``. If you want to open it for connections from other machines on your network, see the documentation for :attr:`mopidy.settings.MPD_SERVER_HOSTNAME`. +If you open up Mopidy for your local network, you should consider turning on +MPD password authentication by setting +:attr:`mopidy.settings.MPD_SERVER_PASSWORD` to the password you want to use. +If the password is set, Mopidy will require MPD clients to provide the password +before they can do anything else. Mopidy only supports a single password, and +do not support different permission schemes like the original MPD server. + Scrobbling tracks to Last.fm ============================ diff --git a/mopidy/frontends/mpd/__init__.py b/mopidy/frontends/mpd/__init__.py index ce9abc6d..2f87088c 100644 --- a/mopidy/frontends/mpd/__init__.py +++ b/mopidy/frontends/mpd/__init__.py @@ -14,6 +14,7 @@ class MpdFrontend(BaseFrontend): **Settings:** - :attr:`mopidy.settings.MPD_SERVER_HOSTNAME` + - :attr:`mopidy.settings.MPD_SERVER_PASSWORD` - :attr:`mopidy.settings.MPD_SERVER_PORT` """ diff --git a/mopidy/frontends/mpd/exceptions.py b/mopidy/frontends/mpd/exceptions.py index 2a18b2f3..faf4ce2f 100644 --- a/mopidy/frontends/mpd/exceptions.py +++ b/mopidy/frontends/mpd/exceptions.py @@ -1,22 +1,20 @@ from mopidy import MopidyException class MpdAckError(MopidyException): - """ - Available MPD error codes:: + """See fields on this class for available MPD error codes""" - ACK_ERROR_NOT_LIST = 1 - ACK_ERROR_ARG = 2 - ACK_ERROR_PASSWORD = 3 - ACK_ERROR_PERMISSION = 4 - ACK_ERROR_UNKNOWN = 5 - ACK_ERROR_NO_EXIST = 50 - ACK_ERROR_PLAYLIST_MAX = 51 - ACK_ERROR_SYSTEM = 52 - ACK_ERROR_PLAYLIST_LOAD = 53 - ACK_ERROR_UPDATE_ALREADY = 54 - ACK_ERROR_PLAYER_SYNC = 55 - ACK_ERROR_EXIST = 56 - """ + ACK_ERROR_NOT_LIST = 1 + ACK_ERROR_ARG = 2 + ACK_ERROR_PASSWORD = 3 + ACK_ERROR_PERMISSION = 4 + ACK_ERROR_UNKNOWN = 5 + ACK_ERROR_NO_EXIST = 50 + ACK_ERROR_PLAYLIST_MAX = 51 + ACK_ERROR_SYSTEM = 52 + ACK_ERROR_PLAYLIST_LOAD = 53 + ACK_ERROR_UPDATE_ALREADY = 54 + ACK_ERROR_PLAYER_SYNC = 55 + ACK_ERROR_EXIST = 56 def __init__(self, message=u'', error_code=0, index=0, command=u''): super(MpdAckError, self).__init__(message, error_code, index, command) @@ -37,19 +35,24 @@ class MpdAckError(MopidyException): class MpdArgError(MpdAckError): def __init__(self, *args, **kwargs): super(MpdArgError, self).__init__(*args, **kwargs) - self.error_code = 2 # ACK_ERROR_ARG + self.error_code = MpdAckError.ACK_ERROR_ARG + +class MpdPasswordError(MpdAckError): + def __init__(self, *args, **kwargs): + super(MpdPasswordError, self).__init__(*args, **kwargs) + self.error_code = MpdAckError.ACK_ERROR_PASSWORD class MpdUnknownCommand(MpdAckError): def __init__(self, *args, **kwargs): super(MpdUnknownCommand, self).__init__(*args, **kwargs) self.message = u'unknown command "%s"' % self.command self.command = u'' - self.error_code = 5 # ACK_ERROR_UNKNOWN + self.error_code = MpdAckError.ACK_ERROR_UNKNOWN class MpdNoExistError(MpdAckError): def __init__(self, *args, **kwargs): super(MpdNoExistError, self).__init__(*args, **kwargs) - self.error_code = 50 # ACK_ERROR_NO_EXIST + self.error_code = MpdAckError.ACK_ERROR_NO_EXIST class MpdNotImplemented(MpdAckError): def __init__(self, *args, **kwargs): diff --git a/mopidy/frontends/mpd/protocol/connection.py b/mopidy/frontends/mpd/protocol/connection.py index 0ce3ef51..65811d09 100644 --- a/mopidy/frontends/mpd/protocol/connection.py +++ b/mopidy/frontends/mpd/protocol/connection.py @@ -1,5 +1,6 @@ +from mopidy import settings from mopidy.frontends.mpd.protocol import handle_pattern -from mopidy.frontends.mpd.exceptions import MpdNotImplemented +from mopidy.frontends.mpd.exceptions import MpdPasswordError @handle_pattern(r'^close$') def close(frontend): @@ -33,7 +34,11 @@ def password_(frontend, password): This is used for authentication with the server. ``PASSWORD`` is simply the plaintext password. """ - raise MpdNotImplemented # TODO + # You will not get to this code without being authenticated. This is for + # when you are already authenticated, and are sending additional 'password' + # requests. + if settings.MPD_SERVER_PASSWORD != password: + raise MpdPasswordError(u'incorrect password', command=u'password') @handle_pattern(r'^ping$') def ping(frontend): diff --git a/mopidy/frontends/mpd/protocol/reflection.py b/mopidy/frontends/mpd/protocol/reflection.py index d2c9c599..83efdd94 100644 --- a/mopidy/frontends/mpd/protocol/reflection.py +++ b/mopidy/frontends/mpd/protocol/reflection.py @@ -9,9 +9,12 @@ def commands(frontend): ``commands`` Shows which commands the current user has access to. - - As permissions is not implemented, any user has access to all commands. """ + # FIXME When password auth is turned on and the client is not + # authenticated, 'commands' should list only the commands the client does + # have access to. To implement this we need access to the session object to + # check if the client is authenticated or not. + sorted_commands = sorted(list(mpd_commands)) # Not shown by MPD in its command list @@ -51,9 +54,11 @@ def notcommands(frontend): ``notcommands`` Shows which commands the current user does not have access to. - - As permissions is not implemented, any user has access to all commands. """ + # FIXME When password auth is turned on and the client is not + # authenticated, 'notcommands' should list all the commands the client does + # not have access to. To implement this we need access to the session + # object to check if the client is authenticated or not. pass @handle_pattern(r'^tagtypes$') diff --git a/mopidy/frontends/mpd/session.py b/mopidy/frontends/mpd/session.py index 580b5905..e8e3291d 100644 --- a/mopidy/frontends/mpd/session.py +++ b/mopidy/frontends/mpd/session.py @@ -2,6 +2,7 @@ import asynchat import logging import multiprocessing +from mopidy import settings from mopidy.frontends.mpd.protocol import ENCODING, LINE_TERMINATOR, VERSION from mopidy.utils.log import indent from mopidy.utils.process import pickle_connection @@ -22,6 +23,7 @@ class MpdSession(asynchat.async_chat): self.client_port = client_socket_address[1] self.core_queue = core_queue self.input_buffer = [] + self.authenticated = False self.set_terminator(LINE_TERMINATOR.encode(ENCODING)) def start(self): @@ -46,6 +48,11 @@ class MpdSession(asynchat.async_chat): def handle_request(self, request): """Handle request by sending it to the MPD frontend.""" + if not self.authenticated: + (self.authenticated, response) = self.check_password(request) + if response is not None: + self.send_response(response) + return my_end, other_end = multiprocessing.Pipe() self.core_queue.put({ 'to': 'frontend', @@ -69,3 +76,26 @@ class MpdSession(asynchat.async_chat): output = u'%s%s' % (output, LINE_TERMINATOR) data = output.encode(ENCODING) self.push(data) + + def check_password(self, request): + """ + Takes any request and tries to authenticate the client using it. + + :rtype: a two-tuple containing (is_authenticated, response_message). If + the response_message is :class:`None`, normal processing should + continue, even though the client may not be authenticated. + """ + if settings.MPD_SERVER_PASSWORD is None: + return (True, None) + command = request.split(' ')[0] + if command == 'password': + if request == 'password "%s"' % settings.MPD_SERVER_PASSWORD: + return (True, u'OK') + else: + return (False, u'ACK [3@0] {password} incorrect password') + if command in ('close', 'commands', 'notcommands', 'ping'): + return (False, None) + else: + return (False, + u'ACK [4@0] {%(c)s} you don\'t have permission for "%(c)s"' % + {'c': command}) diff --git a/mopidy/settings.py b/mopidy/settings.py index 23aa7cb6..6e33ffaa 100644 --- a/mopidy/settings.py +++ b/mopidy/settings.py @@ -164,6 +164,11 @@ OUTPUT = u'mopidy.outputs.gstreamer.GStreamerOutput' #: Listens on all interfaces, both IPv4 and IPv6. MPD_SERVER_HOSTNAME = u'127.0.0.1' +#: The password required for connecting to the MPD server. +#: +#: Default: :class:`None`, which means no password required. +MPD_SERVER_PASSWORD = None + #: Which TCP port Mopidy's MPD server should listen to. #: #: Default: 6600 diff --git a/mopidy/utils/settings.py b/mopidy/utils/settings.py index 2ec0f716..7715721e 100644 --- a/mopidy/utils/settings.py +++ b/mopidy/utils/settings.py @@ -49,7 +49,7 @@ class SettingsProxy(object): if attr not in self.current: raise SettingsError(u'Setting "%s" is not set.' % attr) value = self.current[attr] - if type(value) != bool and not value: + if isinstance(value, basestring) and len(value) == 0: raise SettingsError(u'Setting "%s" is empty.' % attr) if attr.endswith('_PATH') or attr.endswith('_FILE'): value = os.path.expanduser(value) diff --git a/tests/frontends/mpd/connection_test.py b/tests/frontends/mpd/connection_test.py index 21753054..44ce78ca 100644 --- a/tests/frontends/mpd/connection_test.py +++ b/tests/frontends/mpd/connection_test.py @@ -1,5 +1,6 @@ import unittest +from mopidy import settings from mopidy.backends.dummy import DummyBackend from mopidy.frontends.mpd import dispatcher from mopidy.mixers.dummy import DummyMixer @@ -9,6 +10,9 @@ class ConnectionHandlerTest(unittest.TestCase): self.b = DummyBackend(mixer_class=DummyMixer) self.h = dispatcher.MpdDispatcher(backend=self.b) + def tearDown(self): + settings.runtime.clear() + def test_close(self): result = self.h.handle_request(u'close') self.assert_(u'OK' in result) @@ -21,9 +25,20 @@ class ConnectionHandlerTest(unittest.TestCase): result = self.h.handle_request(u'kill') self.assert_(u'OK' in result) - def test_password(self): + def test_valid_password_is_accepted(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + result = self.h.handle_request(u'password "topsecret"') + self.assert_(u'OK' in result) + + def test_invalid_password_is_not_accepted(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' result = self.h.handle_request(u'password "secret"') - self.assert_(u'ACK [0@0] {} Not implemented' in result) + self.assert_(u'ACK [3@0] {password} incorrect password' in result) + + def test_any_password_is_not_accepted_when_password_check_turned_off(self): + settings.MPD_SERVER_PASSWORD = None + result = self.h.handle_request(u'password "secret"') + self.assert_(u'ACK [3@0] {password} incorrect password' in result) def test_ping(self): result = self.h.handle_request(u'ping') diff --git a/tests/frontends/mpd/server_test.py b/tests/frontends/mpd/server_test.py index 9d006eb3..48c7e790 100644 --- a/tests/frontends/mpd/server_test.py +++ b/tests/frontends/mpd/server_test.py @@ -1,5 +1,6 @@ import unittest +from mopidy import settings from mopidy.frontends.mpd import server class MpdServerTest(unittest.TestCase): @@ -21,8 +22,60 @@ class MpdSessionTest(unittest.TestCase): def setUp(self): self.session = server.MpdSession(None, None, (None, None), None) + def tearDown(self): + settings.runtime.clear() + def test_found_terminator_catches_decode_error(self): # Pressing Ctrl+C in a telnet session sends a 0xff byte to the server. self.session.input_buffer = ['\xff'] self.session.found_terminator() self.assertEqual(len(self.session.input_buffer), 0) + + def test_authentication_with_valid_password_is_accepted(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + authed, response = self.session.check_password(u'password "topsecret"') + self.assertTrue(authed) + self.assertEqual(u'OK', response) + + def test_authentication_with_invalid_password_is_not_accepted(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + authed, response = self.session.check_password(u'password "secret"') + self.assertFalse(authed) + self.assertEqual(u'ACK [3@0] {password} incorrect password', response) + + def test_authentication_with_anything_when_password_check_turned_off(self): + settings.MPD_SERVER_PASSWORD = None + authed, response = self.session.check_password(u'any request at all') + self.assertTrue(authed) + self.assertEqual(None, response) + + def test_anything_when_not_authenticated_should_fail(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + authed, response = self.session.check_password(u'any request at all') + self.assertFalse(authed) + self.assertEqual( + u'ACK [4@0] {any} you don\'t have permission for "any"', response) + + def test_close_is_allowed_without_authentication(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + authed, response = self.session.check_password(u'close') + self.assertFalse(authed) + self.assertEqual(None, response) + + def test_commands_is_allowed_without_authentication(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + authed, response = self.session.check_password(u'commands') + self.assertFalse(authed) + self.assertEqual(None, response) + + def test_notcommands_is_allowed_without_authentication(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + authed, response = self.session.check_password(u'notcommands') + self.assertFalse(authed) + self.assertEqual(None, response) + + def test_ping_is_allowed_without_authentication(self): + settings.MPD_SERVER_PASSWORD = u'topsecret' + authed, response = self.session.check_password(u'ping') + self.assertFalse(authed) + self.assertEqual(None, response) diff --git a/tests/utils/settings_test.py b/tests/utils/settings_test.py index cef0069d..8e2575b9 100644 --- a/tests/utils/settings_test.py +++ b/tests/utils/settings_test.py @@ -1,7 +1,7 @@ import os import unittest -from mopidy import settings as default_settings_module +from mopidy import settings as default_settings_module, SettingsError from mopidy.utils.settings import validate_settings, SettingsProxy class ValidateSettingsTest(unittest.TestCase): @@ -55,6 +55,33 @@ class SettingsProxyTest(unittest.TestCase): self.settings.TEST = 'test' self.assertEqual(self.settings.TEST, 'test') + def test_getattr_raises_error_on_missing_setting(self): + try: + test = self.settings.TEST + self.fail(u'Should raise exception') + except SettingsError as e: + self.assertEqual(u'Setting "TEST" is not set.', e.message) + + def test_getattr_raises_error_on_empty_setting(self): + self.settings.TEST = u'' + try: + test = self.settings.TEST + self.fail(u'Should raise exception') + except SettingsError as e: + self.assertEqual(u'Setting "TEST" is empty.', e.message) + + def test_getattr_does_not_raise_error_if_setting_is_false(self): + self.settings.TEST = False + self.assertEqual(False, self.settings.TEST) + + def test_getattr_does_not_raise_error_if_setting_is_none(self): + self.settings.TEST = None + self.assertEqual(None, self.settings.TEST) + + def test_getattr_does_not_raise_error_if_setting_is_zero(self): + self.settings.TEST = 0 + self.assertEqual(0, self.settings.TEST) + def test_setattr_updates_runtime_settings(self): self.settings.TEST = 'test' self.assert_('TEST' in self.settings.runtime) @@ -69,34 +96,34 @@ class SettingsProxyTest(unittest.TestCase): def test_value_ending_in_path_is_expanded(self): self.settings.TEST_PATH = '~/test' - acctual = self.settings.TEST_PATH + actual = self.settings.TEST_PATH expected = os.path.expanduser('~/test') - self.assertEqual(acctual, expected) + self.assertEqual(actual, expected) def test_value_ending_in_path_is_absolute(self): self.settings.TEST_PATH = './test' - acctual = self.settings.TEST_PATH + actual = self.settings.TEST_PATH expected = os.path.abspath('./test') - self.assertEqual(acctual, expected) + self.assertEqual(actual, expected) def test_value_ending_in_file_is_expanded(self): self.settings.TEST_FILE = '~/test' - acctual = self.settings.TEST_FILE + actual = self.settings.TEST_FILE expected = os.path.expanduser('~/test') - self.assertEqual(acctual, expected) + self.assertEqual(actual, expected) def test_value_ending_in_file_is_absolute(self): self.settings.TEST_FILE = './test' - acctual = self.settings.TEST_FILE + actual = self.settings.TEST_FILE expected = os.path.abspath('./test') - self.assertEqual(acctual, expected) + self.assertEqual(actual, expected) def test_value_not_ending_in_path_or_file_is_not_expanded(self): self.settings.TEST = '~/test' - acctual = self.settings.TEST - self.assertEqual(acctual, '~/test') + actual = self.settings.TEST + self.assertEqual(actual, '~/test') def test_value_not_ending_in_path_or_file_is_not_absolute(self): self.settings.TEST = './test' - acctual = self.settings.TEST - self.assertEqual(acctual, './test') + actual = self.settings.TEST + self.assertEqual(actual, './test')