Implement project undelete as PATCH

This is done via a custom PATCH due to the lack of transactions of MongoDB;
we cannot undelete both project-referenced files and file-referenced
projects in one atomic operation.

pillar/api/projects/__init__.py

@@ -3,13 +3,13 @@ from .routes import blueprint_api
 
 
 def setup_app(app, api_prefix):
+    from . import patch
+    patch.setup_app(app)
+
     app.on_replace_projects += hooks.override_is_private_field
     app.on_replace_projects += hooks.before_edit_check_permissions
     app.on_replace_projects += hooks.protect_sensitive_fields
 
-    app.on_replaced_projects += hooks.after_undelete_project
-    app.on_updated_projects += hooks.after_undelete_project
-
     app.on_update_projects += hooks.override_is_private_field
     app.on_update_projects += hooks.before_edit_check_permissions
     app.on_update_projects += hooks.protect_sensitive_fields

pillar/api/projects/hooks.py

@@ -1,8 +1,6 @@
 import copy
-import datetime
 import logging
 
-import bson.tz_util
 from flask import request, abort
 
 from pillar import current_app
@@ -13,7 +11,7 @@ from pillar.api.node_types.group_texture import node_type_group_texture
 from pillar.api.node_types.texture import node_type_texture
 from pillar.api.file_storage_backends import default_storage_backend
 from pillar.api.utils import authorization, authentication
-from pillar.api.utils import remove_private_keys, random_etag
+from pillar.api.utils import remove_private_keys
 from pillar.api.utils.authorization import user_has_role, check_permissions
 from pillar.auth import current_user
 from .utils import abort_with_error
@@ -84,37 +82,6 @@ def after_delete_project(project: dict):
         log.warning('Unable to delete files of project %s: %s', pid, r)
 
 
-def after_undelete_project(project: dict, original: dict):
-    """Undelete the files that belong to this project.
-
-    We cannot do this via Eve, as it doesn't support PATCHing collections, so
-    direct MongoDB modification is used to set _deleted=False and provide
-    new _etag and _updated values.
-    """
-
-    if not original:
-        return
-
-    was_deleted = original.get('_deleted', False)
-    now_deleted = project.get('_deleted', False)
-    if not was_deleted or now_deleted:
-        # This is not an undelete.
-        return
-
-    pid = project['_id']
-    new_etag = random_etag()
-    now = datetime.datetime.now(tz=bson.tz_util.utc)
-
-    files_coll = current_app.db('files')
-    update_result = files_coll.update_many(
-        {'project': pid},
-        {'$set': {'_deleted': False,
-                  '_etag': new_etag,
-                  '_updated': now}})
-    log.info('undeleted %d of %d file documents of project %s',
-             update_result.modified_count, update_result.matched_count, pid)
-
-
 def protect_sensitive_fields(document, original):
     """When not logged in as admin, prevents update to certain fields."""
 

pillar/api/projects/patch.py

@@ -0,0 +1,88 @@
+"""Project patching support."""
+
+import datetime
+import logging
+
+import bson.tz_util
+import flask
+from flask import Blueprint, request
+import werkzeug.exceptions as wz_exceptions
+
+from pillar import current_app
+from pillar.auth import current_user
+from pillar.api.utils import random_etag, str2id
+from pillar.api.utils import authorization
+
+log = logging.getLogger(__name__)
+blueprint = Blueprint('projects.patch', __name__)
+
+
+@blueprint.route('/<project_id>', methods=['PATCH'])
+@authorization.require_login()
+def patch_project(project_id: str):
+    """Undelete a project.
+
+    This is done via a custom PATCH due to the lack of transactions of MongoDB;
+    we cannot undelete both project-referenced files and file-referenced
+    projects in one atomic operation.
+    """
+
+    # Parse the request
+    pid = str2id(project_id)
+    patch = request.get_json()
+    if not patch:
+        raise wz_exceptions.BadRequest('Expected JSON body')
+
+    log.debug('User %s wants to PATCH project %s: %s', current_user, pid, patch)
+
+    # 'undelete' is the only operation we support now, so no fancy handler registration.
+    op = patch.get('op', '')
+    if op != 'undelete':
+        log.warning('User %s sent unsupported PATCH op %r to project %s: %s',
+                    current_user, op, pid, patch)
+        raise wz_exceptions.BadRequest(f'unsupported operation {op!r}')
+
+    # Get the project to find the user's permissions.
+    proj_coll = current_app.db('projects')
+    proj = proj_coll.find_one({'_id': pid})
+    if not proj:
+        raise wz_exceptions.NotFound(f'project {pid} not found')
+    allowed = authorization.compute_allowed_methods('projects', proj)
+    if 'PUT' not in allowed:
+        log.warning('User %s tried to undelete project %s but only has permissions %r',
+                    current_user, pid, allowed)
+        raise wz_exceptions.Forbidden(f'no PUT access to project {pid}')
+
+    if not proj.get('_deleted', False):
+        raise wz_exceptions.BadRequest(f'project {pid} was not deleted, unable to undelete')
+
+    # Undelete the files. We cannot do this via Eve, as it doesn't support
+    # PATCHing collections, so direct MongoDB modification is used to set
+    # _deleted=False and provide new _etag and _updated values.
+    new_etag = random_etag()
+    now = datetime.datetime.now(tz=bson.tz_util.utc)
+
+    log.debug('undeleting files before undeleting project %s', pid)
+    files_coll = current_app.db('files')
+    update_result = files_coll.update_many(
+        {'project': pid},
+        {'$set': {'_deleted': False,
+                  '_etag': new_etag,
+                  '_updated': now}})
+    log.info('undeleted %d of %d file documents of project %s',
+             update_result.modified_count, update_result.matched_count, pid)
+
+    log.info('undeleting project %s on behalf of user %s', pid, current_user)
+    update_result = proj_coll.update_one({'_id': pid},
+                                         {'$set': {'_deleted': False}})
+    log.info('undeleted %d project document %s', update_result.modified_count, pid)
+
+    resp = flask.Response('', status=204)
+    resp.location = flask.url_for('projects.view', project_url=proj['url'])
+    return resp
+
+
+def setup_app(app):
+    # This needs to be on the same URL prefix as Eve uses for the collection,
+    # and not /p as used for the other Projects API calls.
+    app.register_api_blueprint(blueprint, url_prefix='/projects')

pillar/web/projects/routes.py

@@ -861,37 +861,3 @@ def edit_extension(project: Project, extension_name):
 
     return ext.project_settings(project,
                                 ext_pages=find_extension_pages())
-
-
-@blueprint.route('/undelete', methods=['POST'])
-@login_required
-def undelete():
-    """Undelete a deleted project.
-
-    Can only be done by the owner of the project or an admin.
-    """
-    # This function takes an API-style approach, even though it's a web
-    # endpoint. Undeleting via a REST approach would mean GETting the
-    # deleted project, which now causes a 404 exception to bubble to the
-    # client.
-    from pillar.api.utils import mongo, remove_private_keys
-    from pillar.api.utils.authorization import check_permissions
-
-    project_id = request.form.get('project_id')
-    if not project_id:
-        raise wz_exceptions.BadRequest('missing project ID')
-
-    # Check that the user has PUT permissions on the project itself.
-    project = mongo.find_one_or_404('projects', project_id)
-    check_permissions('projects', project, 'PUT')
-
-    pid = project['_id']
-    log.info('Undeleting project %s on behalf of %s', pid, current_user.email)
-    r, _, _, status = current_app.put_internal('projects', remove_private_keys(project), _id=pid)
-    if status != 200:
-        log.warning('Error %d un-deleting project %s: %s', status, pid, r)
-        return 'Error un-deleting project', 500
-
-    resp = flask.Response('', status=204)
-    resp.location = flask.url_for('projects.view', project_url=project['url'])
-    return resp

src/templates/projects/index_dashboard.pug

@@ -327,9 +327,16 @@ script.
 		hopToTop(); // Display jump to top button
 	});
 
+
+	var patch_url = '{{ url_for('projects.patch.patch_project', project_id='PROJECTID') }}';
 	function undelete_project(project_id) {
 		console.log('undeleting project', project_id);
-		$.post('{{ url_for('projects.undelete') }}', {project_id: project_id})
+		$.ajax({
+			url: patch_url.replace('PROJECTID', project_id),
+			method: 'PATCH',
+			data: JSON.stringify({'op': 'undelete'}),
+			contentType: 'application/json'
+		})
 		.done(function(data, textStatus, jqXHR) {
 			location.href = jqXHR.getResponseHeader('Location');
 		})

tests/test_api/test_project_management.py

@@ -398,42 +398,67 @@ class ProjectEditTest(AbstractProjectTest):
         self.assertGreater(db_file_after['_updated'], db_file_before['_updated'])
         self.assertNotEqual(db_file_after['_etag'], db_file_before['_etag'])
 
-    def test_undelete_with_put__files_too(self):
+    def _create_delete_project(self):
-        from pillar.api.utils import remove_private_keys
+        """Create and then delete a project."""
 
+        from pillar.api.utils import remove_private_keys
         # Create test project with a file.
         project_info = self._create_user_and_project(['subscriber'])
         project_id = project_info['_id']
-
-        fid, _ = self.ensure_file_exists({'project': ObjectId(project_id)})
-
-        # DELETE by owner should also soft-delete the file documents.
         proj_url = f'/api/projects/{project_id}'
-        self.delete(proj_url, auth_token='token', etag=project_info['_etag'],
+        etag = project_info['_etag']
-                    expected_status=204)
 
-        resp = self.get(proj_url, auth_token='token', expected_status=404)
+        # Assign the file as picture_header so that we have a nice circular reference.
+        fid, _ = self.ensure_file_exists({'project': ObjectId(project_id)})
+        project_info['picture_header'] = str(fid)
+        resp = self.put(proj_url, auth_token='token', etag=etag,
+                        json=remove_private_keys(project_info))
         etag = resp.json()['_etag']
 
+        # DELETE the project.
+        self.delete(proj_url, auth_token='token', etag=etag, expected_status=204)
+
         with self.app.app_context():
             files_coll = self.app.db('files')
             now = datetime.datetime.now(tz=bson.tz_util.utc) - datetime.timedelta(seconds=5)
             db_file_before = files_coll.find_one_and_update({'_id': fid},
                                                             {'$set': {'_updated': now}},
                                                             return_document=ReturnDocument.AFTER)
+        return db_file_before, fid, proj_url
 
-        # PUT on the project should also restore the files.
+    def test_undelete_with_patch(self):
-        self.put(proj_url, auth_token='token', etag=etag,
+        db_file_before, fid, proj_url = self._create_delete_project()
-                 json=remove_private_keys(project_info))
+
+        # PATCH on the project should also restore the files.
+        self.patch(proj_url, auth_token='token', json={'op': 'undelete'}, expected_status=204)
 
         resp = self.get(f'/api/files/{fid}')
         self.assertEqual(str(fid), resp.json()['_id'])
 
         with self.app.app_context():
+            files_coll = self.app.db('files')
             db_file_after = files_coll.find_one(fid)
         self.assertGreater(db_file_after['_updated'], db_file_before['_updated'])
         self.assertNotEqual(db_file_after['_etag'], db_file_before['_etag'])
 
+    def test_undelete_other_user(self):
+        _, fid, proj_url = self._create_delete_project()
+
+        self.create_user(user_id='baddf00dbaddf00dbaddf00d', token='baduser')
+
+        # PATCH on the project should be denied.
+        self.patch(proj_url, auth_token='baduser', json={'op': 'undelete'}, expected_status=403)
+
+        self.get(f'/api/files/{fid}', expected_status=404)
+
+        with self.app.app_context():
+            files_coll = self.app.db('files')
+            db_file_after = files_coll.find_one(fid)
+        self.assertTrue(db_file_after['_deleted'])
+
+        resp = self.get(proj_url, auth_token='token', expected_status=404).json()
+        self.assertTrue(resp['_deleted'])
+
 
 class ProjectNodeAccess(AbstractProjectTest):
     def setUp(self, **kwargs):