[feature] better param error handling in rest api

5 files changed, 98 insertions, 25 deletions

diff --git a/docs/core/index.rst b/docs/core/index.rst
index 5b33d82..b17a432 100644
--- a/docs/core/index.rst
+++ b/docs/core/index.rst
@@ -12,7 +12,7 @@ The bitmask core daemon can be launched like this::
 
   bitmaskd
 
-The command-line program, ``bitmaskctl``, and the GUI, will launch the
+The command-line program ``bitmaskctl`` and the GUI, will launch the
 daemon when needed.
 
 Starting the API server
@@ -59,7 +59,7 @@ means that it does not need an authentication header.
 +------------------------------------+---------------------------------+
 | ``POST`` :ref:`cmd_user_create` *  | Create a new user               |
 +------------------------------------+---------------------------------+
-| ``POST`` :ref:`cmd_user_update`    | Update an user                  |
+| ``POST`` :ref:`cmd_user_update`    | Change the user password        |
 +------------------------------------+---------------------------------+
 | ``POST`` :ref:`cmd_user_auth` *    | Authenticate an user            |
 +------------------------------------+---------------------------------+
@@ -93,6 +93,7 @@ JSON-encoded data to the POST.
   **Example request**::
 
         curl -X POST localhost:7070/API/core/version 
+
  
   **Example response**::
 
@@ -155,7 +156,8 @@ JSON-encoded data to the POST.
 
   **Example request**::
 
-  curl -X POST localhost:7070/API/bonafide/provider/read -d '["dev.bitmask.net"]' 
+  
+        curl -X POST localhost:7070/API/bonafide/provider/read -d '["dev.bitmask.net"]'
 
  
   **Example response**::
@@ -224,8 +226,6 @@ JSON-encoded data to the POST.
   List all the users known to the local backend. 
 
   **Form parameters**:
-        * ``foo`` *(required)* - foo bar.
-        * ``bar`` *(optional)* - foo bar.
 
   **Status codes**:
         * ``200`` - no error
@@ -247,7 +247,15 @@ JSON-encoded data to the POST.
   Create a new user.
 
   **Form parameters**:
-        * ``foo`` *(required)* - foo bar.
+        * ``username`` *(required)* - in the form user@provider.
+        * ``pass`` *(required)* - the username passphrase
+        * ``invitecode`` *(optional)* - an optional invitecode, to be used if
+          the provider requires it for creating a new account.
+        * ``autoconf`` *(optional)* - whether to autoconfigure the provider, if
+          we don't have seen it before.
+
+  **Status codes**:
+        * ``200`` - no error
 
 .. _cmd_user_update:
 
@@ -255,7 +263,15 @@ JSON-encoded data to the POST.
 ---------------------
 **POST /bonafide/user/update**
 
-  Update a given user.
+  Change the user password.
+
+  **Form parameters**:
+        * ``username`` *(required)* - in the form user@provider
+        * ``oldpass`` *(required)* - current password
+        * ``newpass`` *(required)* - new password
+
+  **Status codes**:
+        * ``200`` - no error
 
 .. _cmd_user_auth:
 
@@ -265,6 +281,16 @@ JSON-encoded data to the POST.
 
   Authenticate an user.
 
+  **Form parameters**:
+
+        * ``username`` *(required)* - in the form user@provider
+        * ``pass`` *(required)* - passphrase
+        * ``autoconf`` *(optional)* - whether to autoconfigure the provider, if
+          we don't have seen it before.
+
+  **Status codes**:
+        * ``200`` - no error
+
 .. _cmd_user_logout:
 
 /bonafide/user/logout
diff --git a/src/leap/bitmask/core/dispatcher.py b/src/leap/bitmask/core/dispatcher.py
index 3a5b5f1..f96bd45 100644
--- a/src/leap/bitmask/core/dispatcher.py
+++ b/src/leap/bitmask/core/dispatcher.py
@@ -108,7 +108,12 @@ class UserCmd(SubCommand):
     @register_method("{'srp_token': unicode, 'uuid': unicode "
                      "'lcl_token': unicode}")
     def do_AUTHENTICATE(self, bonafide, *parts):
-        user, password = parts[2], parts[3]
+        try:
+            user, password = parts[2], parts[3]
+        except IndexError:
+            raise DispatchError(
+                'wrong number of arguments: expected 2 or 3, got %s' % (
+                    len(parts[2:])))
         autoconf = False
         if len(parts) > 4:
             if parts[4] == 'true':
@@ -117,17 +122,21 @@ class UserCmd(SubCommand):
         # FIXME We still SHOULD pass a local token
         # even if the SRP authentication times out!!!
         def add_local_token(result):
-            result['lcl_token'] = bonafide.local_tokens[user]
+            result['lcl_token'] = bonafide.local_tokens.get(user)
             return result
 
-        d = bonafide.do_authenticate(user, password, autoconf)
+        d = defer.maybeDeferred(
+            bonafide.do_authenticate, user, password, autoconf)
         d.addCallback(add_local_token)
         return d
 
     @register_method("{'signup': 'ok', 'user': str}")
     def do_CREATE(self, bonafide, *parts):
         if len(parts) < 5:
-            raise DispatchError('Not enough parameters passed')
+            raise DispatchError(
+                'wrong number of arguments: expected min 3, got %s' % (
+                    len(parts[2:])))
+
         # params are: [user, create, full_id, password, invite, autoconf]
         user, password, invite = parts[2], parts[3], parts[4]
 
@@ -142,7 +151,12 @@ class UserCmd(SubCommand):
 
     @register_method("{'logout': 'ok'}")
     def do_LOGOUT(self, bonafide, *parts):
-        user = parts[2]
+        try:
+            user = parts[2]
+        except IndexError:
+            raise DispatchError(
+                'wrong number of arguments: expected 1, got %s' % (
+                    len(parts[2:])))
         return bonafide.do_logout(user)
 
     @register_method("[{'userid': str, 'authenticated': bool}]")
@@ -151,7 +165,12 @@ class UserCmd(SubCommand):
 
     @register_method("{'update': 'ok'}")
     def do_UPDATE(self, bonafide, *parts):
-        user, current_password, new_password = parts[2], parts[3], parts[4]
+        try:
+            user, current_password, new_password = parts[2], parts[3], parts[4]
+        except IndexError:
+            raise DispatchError(
+                'wrong number of arguments: expected 3, got %s' % (
+                    len(parts[2:])))
         return bonafide.do_change_password(
             user, current_password, new_password)
 
diff --git a/src/leap/bitmask/core/dummy.py b/src/leap/bitmask/core/dummy.py
index 2037b81..51eec1a 100644
--- a/src/leap/bitmask/core/dummy.py
+++ b/src/leap/bitmask/core/dummy.py
@@ -37,6 +37,7 @@ class CannedData:
 
     class bonafide:
         auth = {
+            u'lcl_token': u'deadbeef',
             u'srp_token': u'deadbeef123456789012345678901234567890123',
             u'uuid': u'01234567890abcde01234567890abcde'}
         signup = {
@@ -48,6 +49,8 @@ class CannedData:
         logout = {
             'logout': 'ok'}
         get_active_user = 'dummyuser@provider.example.org'
+        change_password = {
+            'update': 'ok'}
 
 
 class BackendCommands(object):
@@ -90,10 +93,10 @@ class BonafideService(HookableService):
     def __init__(self, basedir):
         self.canned = CannedData
 
-    def do_authenticate(self, user, password):
+    def do_authenticate(self, user, password, autoconf):
         return self.canned.bonafide.auth
 
-    def do_signup(self, user, password):
+    def do_signup(self, user, password, invite, autoconf):
         return self.canned.bonafide.signup
 
     def do_list_users(self):
@@ -104,3 +107,6 @@ class BonafideService(HookableService):
 
     def do_get_active_user(self):
         return self.canned.bonafide.get_active_user
+
+    def do_change_password(self, username, old, new):
+        return self.canned.bonafide.change_password
diff --git a/src/leap/bitmask/core/web/api.py b/src/leap/bitmask/core/web/api.py
index e8bd21e..6c386b5 100644
--- a/src/leap/bitmask/core/web/api.py
+++ b/src/leap/bitmask/core/web/api.py
@@ -2,6 +2,9 @@ import json
 from twisted.web.server import NOT_DONE_YET
 
 from twisted.web.resource import Resource
+from twisted.logger import Logger
+
+log = Logger()
 
 
 class Api(Resource):
@@ -16,6 +19,8 @@ class Api(Resource):
         command = request.uri.split('/')[2:]
         params = request.content.getvalue()
         if params:
+            # TODO sanitize this
+
             # json.loads returns unicode strings and the rest of the code
             # expects strings. This 'str(param)' conversion can be removed
             # if we move to python3
@@ -24,6 +29,7 @@ class Api(Resource):
 
         d = self.dispatcher.dispatch(command)
         d.addCallback(self._write_response, request)
+        d.addErrback(log.error)
         return NOT_DONE_YET
 
     def _write_response(self, response, request):
diff --git a/tests/unit/core/test_web_api.py b/tests/unit/core/test_web_api.py
index 02708f6..6d61686 100644
--- a/tests/unit/core/test_web_api.py
+++ b/tests/unit/core/test_web_api.py
@@ -132,7 +132,9 @@ class RESTApiTests(unittest.TestCase):
     def setUp(self):
         dispatcher = dummyDispatcherFactory()
         api = web.api.Api(dispatcher)
-        plainSite = Site(api)
+        root = resource.Resource()
+        root.putChild(b"API", api)
+        plainSite = Site(root)
         self.plainPort = reactor.listenTCP(0, plainSite, interface="127.0.0.1")
         self.plainPortno = self.plainPort.getHost().port
         self.canned = CannedData
@@ -161,17 +163,23 @@ class RESTApiTests(unittest.TestCase):
 
     @defer.inlineCallbacks
     def test_bonafide_user_create(self):
-        call = yield self.makeAPICall('bonafide/user/create')
-        self.assertCall(call, self.canned.bonafide.auth)
+        params = ['user@provider', 'pass', 'invitecode']
+        call = yield self.makeAPICall('bonafide/user/create',
+                                      params=params)
+        self.assertCall(call, self.canned.bonafide.signup)
 
     @defer.inlineCallbacks
     def test_bonafide_user_update(self):
-        call = yield self.makeAPICall('bonafide/user/update')
-        self.assertCall(call, self.canned.bonafide.update)
+        params = ['user@provider', 'oldpass', 'newpass']
+        call = yield self.makeAPICall('bonafide/user/update',
+                                      params=params)
+        self.assertCall(call, self.canned.bonafide.change_password)
 
     @defer.inlineCallbacks
     def test_bonafide_user_authenticate(self):
-        call = yield self.makeAPICall('bonafide/user/authenticate')
+        params = ['user@provider', 'pass', 'false']
+        call = yield self.makeAPICall('bonafide/user/authenticate',
+                                      params=params)
         self.assertCall(call, self.canned.bonafide.auth)
 
     @defer.inlineCallbacks
@@ -181,13 +189,20 @@ class RESTApiTests(unittest.TestCase):
 
     @defer.inlineCallbacks
     def test_bonafide_user_logout(self):
-        call = yield self.makeAPICall('bonafide/user/logout')
+        params = ['user@provider']
+        call = yield self.makeAPICall('bonafide/user/logout',
+                                      params=params)
         self.assertCall(call, self.canned.bonafide.logout)
 
-    def makeAPICall(self, path, method="POST"):
-        uri = networkString("http://127.0.0.1:%d/%s" % (
+    def makeAPICall(self, path, method="POST", params=None):
+        if params:
+            postdata = json.dumps(params)
+        else:
+            postdata = None
+        uri = networkString("http://127.0.0.1:%d/API/%s" % (
             self.plainPortno, path))
-        return client.getPage(uri, method=method, timeout=1)
+        return client.getPage(
+            uri, method=method, timeout=1, postdata=postdata)
 
     def assertCall(self, returned, expected):
         data = json.loads(returned)
@@ -219,6 +234,7 @@ class DummyCore(service.MultiService):
         self.init('mail', mail)
 
         self.core_cmds = BackendCommands(self)
+        self.tokens = {}
 
     def init(self, label, service, *args, **kw):
         s = service(*args, **kw)