From 1aeca369bd4c1e567d48a7ac2c506b718c362177 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Laur=C3=A9line=20Gu=C3=A9rin?= <zebuline@entrouvert.com>
Date: Mon, 3 Feb 2020 15:56:26 +0100
Subject: [PATCH] cmis: add a jsonschema for uploadfile endpoint (#39193)

---
 passerelle/apps/cmis/models.py                | 90 ++++++++++---------
 .../templates/cmis/cmisconnector_detail.html  | 29 ------
 tests/test_cmis.py                            | 60 +++++++------
 3 files changed, 78 insertions(+), 101 deletions(-)
 delete mode 100644 passerelle/apps/cmis/templates/cmis/cmisconnector_detail.html

diff --git a/passerelle/apps/cmis/models.py b/passerelle/apps/cmis/models.py
index d6eb78c6..0139969e 100644
--- a/passerelle/apps/cmis/models.py
+++ b/passerelle/apps/cmis/models.py
@@ -27,20 +27,45 @@ from cmislib.exceptions import PermissionDeniedException
 from cmislib.exceptions import UpdateConflictException
 from django.db import models
 from django.utils.translation import ugettext_lazy as _
-from django.utils.six import StringIO, text_type
+from django.utils.six import StringIO
 from django.utils.six.moves.urllib import error as urllib2
 
 from passerelle.base.models import BaseResource
-from passerelle.compat import json_loads
 from passerelle.utils.api import endpoint
 from passerelle.utils.jsonresponse import APIError
 
 
 SPECIAL_CHARS = '!#$%&+-^_`~;[]{}+=~'
-FILE_PATH_PATTERN = '^(/|(/[\w%s]+)+)$' % re.escape(SPECIAL_CHARS)
-FILE_NAME_PATTERN = '[\w%s\.]+' % re.escape(SPECIAL_CHARS)
-RE_FILE_PATH = re.compile(FILE_PATH_PATTERN)
-RE_FILE_NAME = re.compile(FILE_NAME_PATTERN)
+FILE_PATH_PATTERN = r'^(/|(/[\w%s]+)+)$' % re.escape(SPECIAL_CHARS)
+FILE_NAME_PATTERN = r'[\w%s\.]+$' % re.escape(SPECIAL_CHARS)
+
+
+UPLOAD_SCHEMA = {
+    'type': 'object',
+    'properties': {
+        'file': {
+            'type': 'object',
+            'properties': {
+                'filename': {
+                    'type': 'string',
+                    'pattern': FILE_NAME_PATTERN,
+                },
+                'content': {'type': 'string'},
+                'content_type': {'type': 'string'},
+            },
+            'required': ['content']
+        },
+        'filename': {
+            'type': 'string',
+            'pattern': FILE_NAME_PATTERN,
+        },
+        'path': {
+            'type': 'string',
+            'pattern': FILE_PATH_PATTERN,
+        },
+    },
+    'required': ['file', 'path']
+}
 
 
 class CmisConnector(BaseResource):
@@ -58,9 +83,17 @@ class CmisConnector(BaseResource):
         fields = super(CmisConnector, self).get_description_fields()
         return [(x[0], x[1]) for x in fields if x[0].name != 'password']
 
-    @endpoint(methods=['post'], perm='can_access')
-    def uploadfile(self, request):
-        error, error_msg, data = self._validate_inputs(request.body)
+    @endpoint(
+        perm='can_access',
+        post={
+            'request_body': {
+                'schema': {
+                    'application/json': UPLOAD_SCHEMA,
+                }
+            }
+        })
+    def uploadfile(self, request, post_data):
+        error, error_msg, data = self._validate_inputs(post_data)
         if error:
             self.logger.debug("received invalid data: %s" % error_msg)
             raise APIError(error_msg, http_status=400)
@@ -73,43 +106,14 @@ class CmisConnector(BaseResource):
             data['file_byte_content'])
         return {'data': {'properties': doc.properties}}
 
-    def _validate_inputs(self, body):
-        """ process JSON body
+    def _validate_inputs(self, data):
+        """ process dict
         return a tuple (error, error_msg, data)
         """
-        try:
-            data = json_loads(body)
-        except ValueError as e:
-            return True, "could not decode body to json: %s" % e, None
-        if 'file' not in data:
-            return True, '"file" is required', None
-        if 'path' not in data:
-            return True, '"path" is required', None
-        if not isinstance(data['file'], dict):
-            return True, '"file" must be a dict', None
-        if not isinstance(data['path'], text_type):
-            return True, '"path" must be string', None
-        if not RE_FILE_PATH.match(data['path']):
-            return True, '"path" must be valid path', None
-
         file_ = data['file']
-        if 'filename' in data:
-            if not isinstance(data['filename'], text_type):
-                return True, '"filename" must be string', None
-            if not RE_FILE_NAME.match(data['filename']):
-                return True, '"filename" must be valid file name', None
-        else:
-            if 'filename' not in file_:
-                return True, '"file[\'filename\']" is required', None
-            if not isinstance(file_['filename'], text_type):
-                return True, '"file[\'filename\']" must be string', None
-            if not RE_FILE_NAME.match(file_['filename']):
-                return True, '"file[\'filename\']" must be valid file name', None
-
-        if 'content' not in file_:
-            return True, '"file[\'content\']" is required', None
-        if not isinstance(file_['content'], text_type):
-            return True, '"file[\'content\']" must be string', None
+        if 'filename' not in file_ and 'filename' not in data:
+            return True, '"filename" or "file[\'filename\']" is required', None
+
         try:
             data['file_byte_content'] = base64.b64decode(file_['content'])
         except (TypeError, binascii.Error):
diff --git a/passerelle/apps/cmis/templates/cmis/cmisconnector_detail.html b/passerelle/apps/cmis/templates/cmis/cmisconnector_detail.html
deleted file mode 100644
index 577f9fa2..00000000
--- a/passerelle/apps/cmis/templates/cmis/cmisconnector_detail.html
+++ /dev/null
@@ -1,29 +0,0 @@
-{% extends "passerelle/manage/service_view.html" %}
-{% load i18n passerelle %}
-
-{% block endpoints %}
-<ul>
-  <li>
-    <h4>{% trans 'Upload file' %}</h4>
-    {% url "generic-endpoint" connector="cmis" slug=object.slug endpoint="uploadfile" as uploadfile %}
-    <p> <strong>POST</strong> <a href="{{uploadfile}}">{{uploadfile}}</a></p>
-    <pre>
-      data_send = {
-      'path': '/a/path',
-      'file': {
-        'filename': 'test.txt',
-        'content': 'ZmlsZSBjb250ZW50',
-        'content_type': 'image/jpeg'
-       }
-    }
-    </pre>
-  </li>
-</ul>
-{% endblock %}
-
-{% block security %}
-<p>
-{% trans 'Upload is limited to the following API users:' %}
-</p>
-{% access_rights_table resource=object permission='can_access' %}
-{% endblock %}
diff --git a/tests/test_cmis.py b/tests/test_cmis.py
index a5b709e9..0afbcac3 100644
--- a/tests/test_cmis.py
+++ b/tests/test_cmis.py
@@ -1,5 +1,6 @@
 import base64
 import httplib2
+import re
 
 from cmislib import CmisClient
 from cmislib.exceptions import CmisException
@@ -60,7 +61,7 @@ def test_uploadfile(app, setup, tmpdir, monkeypatch):
     assert result_file.exists()
     with result_file.open('rb'):
         assert result_file.read() == file_content
-    json_result = response.json_body
+    json_result = response.json
     assert json_result['err'] == 0
     assert json_result['data']['properties'] == {"toto": "tata"}
 
@@ -76,7 +77,7 @@ def test_uploadfile(app, setup, tmpdir, monkeypatch):
     assert result_file.exists()
     with result_file.open('rb'):
         assert result_file.read() == file_content
-    json_result = response.json_body
+    json_result = response.json
     assert json_result['err'] == 0
     assert json_result['data']['properties'] == {"toto": "tata"}
 
@@ -89,8 +90,8 @@ def test_uploadfile_error_if_no_file_name(app, setup):
                          "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"file[\'filename\']" is required')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'].startswith('"filename" or "file[\'filename\']" is required')
 
 
 def test_uploadfile_error_if_non_string_file_name(app, setup):
@@ -101,8 +102,8 @@ def test_uploadfile_error_if_non_string_file_name(app, setup):
                          "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"file[\'filename\']" must be string')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'] == "1 is not of type 'string'"
 
     response = app.post_json(
         '/cmis/slug-cmis/uploadfile',
@@ -112,8 +113,8 @@ def test_uploadfile_error_if_non_string_file_name(app, setup):
                 "filename": 1},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"filename" must be string')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'] == "1 is not of type 'string'"
 
 
 def test_uploadfile_error_if_non_valid_file_name(app, setup):
@@ -124,8 +125,8 @@ def test_uploadfile_error_if_non_valid_file_name(app, setup):
                          "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"file[\'filename\']" must be valid file name')
+    assert response.json['err'] == 1
+    assert "',.,' does not match " in response.json['err_desc']
 
     response = app.post_json(
         '/cmis/slug-cmis/uploadfile',
@@ -135,8 +136,8 @@ def test_uploadfile_error_if_non_valid_file_name(app, setup):
                 "filename": ",.,"},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"filename" must be valid file name')
+    assert response.json['err'] == 1
+    assert "',.,' does not match " in response.json['err_desc']
 
 
 def test_uploadfile_error_if_no_path(app, setup):
@@ -146,8 +147,8 @@ def test_uploadfile_error_if_no_path(app, setup):
                          "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"path" is required')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'] == "'path' is a required property"
 
 
 def test_uploadfile_error_if_non_string_path(app, setup):
@@ -158,8 +159,8 @@ def test_uploadfile_error_if_non_string_path(app, setup):
                          "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"path" must be string')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'] == "1 is not of type 'string'"
 
 
 def test_uploadfile_error_if_no_regular_path(app, setup):
@@ -170,8 +171,8 @@ def test_uploadfile_error_if_no_regular_path(app, setup):
                          "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"path" must be valid path')
+    assert response.json['err'] == 1
+    assert "'no/leading/slash' does not match " in response.json['err_desc']
 
 
 def test_uploadfile_error_if_no_file_content(app, setup):
@@ -181,8 +182,8 @@ def test_uploadfile_error_if_no_file_content(app, setup):
                 "file": {"filename": 'somefile.txt', "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"file[\'content\']" is required')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'] == "'content' is a required property"
 
 
 def test_uploadfile_error_if_non_string_file_content(app, setup):
@@ -192,8 +193,8 @@ def test_uploadfile_error_if_non_string_file_content(app, setup):
                 "file": {"filename": 'somefile.txt', "content": 1, "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith('"file[\'content\']" must be string')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'] == "1 is not of type 'string'"
 
 
 def test_uploadfile_error_if_no_proper_base64_encoding(app, setup):
@@ -203,9 +204,8 @@ def test_uploadfile_error_if_no_proper_base64_encoding(app, setup):
                 "file": {"filename": 'somefile.txt', "content": "1", "content_type": "image/jpeg"}},
         expect_errors=True)
     assert response.status_code == 400
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith(
-        '"file[\'content\']" must be a valid base64 string')
+    assert response.json['err'] == 1
+    assert response.json['err_desc'].startswith('"file[\'content\']" must be a valid base64 string')
 
 
 def test_uploadfile_cmis_gateway_error(app, setup, monkeypatch):
@@ -220,8 +220,8 @@ def test_uploadfile_cmis_gateway_error(app, setup, monkeypatch):
         params={"path": "/some/folder/structure",
                 "file": {"filename": "file_name", "content": b64encode('aaaa'),
                          "content_type": "image/jpeg"}})
-    assert response.json_body['err'] == 1
-    assert response.json_body['err_desc'].startswith("some error")
+    assert response.json['err'] == 1
+    assert response.json['err_desc'].startswith("some error")
 
 
 def test_get_or_create_folder_already_existing(monkeypatch):
@@ -331,7 +331,8 @@ def test_wrap_cmis_error(app, setup, monkeypatch, cmis_exc, err_msg):
 
 
 def test_re_file_path():
-    from passerelle.apps.cmis.models import RE_FILE_PATH
+    from passerelle.apps.cmis.models import FILE_PATH_PATTERN
+    RE_FILE_PATH = re.compile(FILE_PATH_PATTERN)
     assert RE_FILE_PATH.match('/')
     assert RE_FILE_PATH.match('/some')
     assert RE_FILE_PATH.match('/some/path')
@@ -346,6 +347,7 @@ def test_re_file_path():
 
 
 def test_re_file_name():
-    from passerelle.apps.cmis.models import RE_FILE_NAME
+    from passerelle.apps.cmis.models import FILE_NAME_PATTERN
+    RE_FILE_NAME = re.compile(FILE_NAME_PATTERN)
     assert RE_FILE_NAME.match('toto.tata')
     assert RE_FILE_NAME.match('TOTO.TATA')
-- 
2.20.1

