Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feat(/sources): add bulk DELETE endpoint #7228

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 59 additions & 1 deletion securedrop/journalist_app/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import flask
import werkzeug
from db import db
from flask import Blueprint, abort, jsonify, request
from flask import Blueprint, abort, current_app, jsonify, request
from journalist_app import utils
from journalist_app.sessions import session
from models import (
Expand All @@ -23,10 +23,13 @@
)
from sqlalchemy import Column
from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm.exc import NoResultFound
from store import NotEncrypted, Storage
from two_factor import OtpSecretInvalid, OtpTokenInvalid
from werkzeug.exceptions import default_exceptions

MAX_BATCH_SIZE = 50 # requests


def get_or_404(model: db.Model, object_id: str, column: Column) -> db.Model:
result = model.query.filter(column == object_id).one_or_none()
Expand Down Expand Up @@ -124,6 +127,61 @@ def get_all_sources() -> Tuple[flask.Response, int]:
sources = Source.query.filter_by(pending=False, deleted_at=None).all()
return jsonify({"sources": [source.to_json() for source in sources]}), 200

@api.route("/sources", methods=["DELETE"])
def delete_sources() -> Tuple[flask.Response, int]:
"""
Given a list of at most `MAX_BATCH_SIZE `Source` UUIDs, iterate over the
list and try to delete each `Source`. Return HTTP 200 "Success" if all
`Source`s were deleted or HTTP 207 "Multi-Status" if some failed.
(There's an argument for HTTP 202 "Accepted", since filesystem-level
deletion is still deferred to the shredder; but that's an implementation
detail that we can hide from the client.)

Batching (under `MAX_BATCH_SIZE`) and retrying (of `Source`s returned as
`failed`) are responsibilities of the client.

NB. According to RFC 9110 §9.3.5, a client may not assume that a DELETE
endpoint will accept a request body, but a DELETE endpoint may do so,
and in our case we can rule out middleboxes that might mangle it in
transit.
"""
data = request.json
if not isinstance(data, list):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also want to check that all the contents of the list are strings?

abort(400, "no sources specified")
elif len(data) > MAX_BATCH_SIZE:
abort(413, f"bulk requests may have at most {MAX_BATCH_SIZE} items")

succeeded = []
failed = []
for source_uuid in data:
try:
# Don't use `get_or_404()`: we'll handle the `NoResultFound`
# case ourselves, rather than abort with HTTP 404.
source = Source.query.filter(Source.uuid == source_uuid).one()
utils.delete_collection(source.filesystem_id)
succeeded.append(source_uuid)

# Deletion is idempotent, so count nonexistent `Source`s as
# successes.
except NoResultFound:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try/except should only cover exactly the Source.query.filter().one() line, so any error in delete_collection/append is not caught by this.

Or just use .one_or_none() and avoid catching NoResultFound entirely.

succeeded.append(source_uuid)

except Exception as exc:
current_app.logger.error(f"Failed to delete source {source_uuid}: {exc}")
failed.append(source_uuid)

# Return the lists of both failed and succeeded deletion operations no
# matter what, so that the client can act directly on the results.
return (
jsonify(
{
"failed": failed,
"succeeded": succeeded,
}
),
200 if len(failed) == 0 else 207, # Success or Multi-Status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they all fail, should that still be 207? I think that's an acceptable compromise to keep the complexity of the client part down.

)

@api.route("/sources/<source_uuid>", methods=["GET", "DELETE"])
def single_source(source_uuid: str) -> Tuple[flask.Response, int]:
if request.method == "GET":
Expand Down