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.

Sign up for GitHub

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

fix(_streaming_download): handle retry-level JSON error responses #2329

Merged
merged 2 commits into from
Dec 20, 2024
Merged

fix(_streaming_download): handle retry-level JSON error responses #2329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5d0b87
fix(_streaming_download): handle retry-level JSON error responses
cfm Dec 10, 2024
d233eb3
test(test_download_submission_autoresume): break initial download req…
cfm Dec 11, 2024
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
8 changes: 7 additions & 1 deletion client/securedrop_client/sdk/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ def _streaming_download(
download_finished = False

while not download_finished and retry < self.download_retry_limit:
bytes_written_prev = bytes_written
logger.debug(f"Streaming download, retry {retry}")

try:
Expand Down Expand Up @@ -213,10 +214,15 @@ def _streaming_download(
contents = fobj.read()
fobj.close()

# Check for an error response
# Check for an error response first at the request level...
if contents[0:1] == b"{":
logger.error(f"Retry {retry}, received JSON error response.")
return self._handle_json_response(contents)
# ...and then at the retry level.
partial = contents[bytes_written_prev:bytes_written]
Copy link
Member

Choose a reason for hiding this comment

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

Should/can we skip this check if bytes_written_prev == 0 (a no-retry request)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wondered about this. In terms of control flow:

  1. In a retry request:
    1. bytes_written_prev < len(contents) and
    2. bytes_written = len(contents), so
    3. len(partial) < len(contents), so
    4. partial[0:1] != contents[0:1] and is worth checking,
    5. unless we've already returned on line 220.
  2. In a non-retry request:
    1. bytes_written_prev = 0 and
    2. bytes_written = len(contents), so
    3. len(partial) == len(contents), so
    4. partial[0:1] == contents[0:1] and is indeed redundant,
    5. unless we've already returned on line 220.

So the "should" here for me comes down to: Is this check too expensive to do twice for a successful request? Probably not. But it is confusing, so let me make it explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in c5d0b87. I've kept the conditionals parallel between contents and partial, but let me know if you'd prefer a nested one for partial.

if retry > 0 and partial[0:1] == b"{":
logger.error(f"Retry {retry}, received JSON error response.")
return self._handle_json_response(partial)

# Get the headers
if proc.stderr is not None:
Expand Down
10 changes: 5 additions & 5 deletions client/tests/sdk/data/setup_method.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
interactions:
- request:
body: '{"username": "journalist", "passphrase": "correct horse battery staple
profanity oil chewy", "one_time_code": "112097"}'
profanity oil chewy", "one_time_code": "578466"}'
headers: {}
method: POST
uri: api/v1/token
response: !!python/object:securedrop_client.sdk.JSONResponse
data:
expiration: '2024-02-29T23:13:01.444028Z'
expiration: '2024-12-11T02:37:22.232423Z'
journalist_first_name: null
journalist_last_name: null
journalist_uuid: ed64b0fa-0565-4b12-9c48-6cfe27a73fd9
token: Ilhha2ZHZWpVcFZQVmpKdWJOUzJYRG1YVUc4QXZWS0JiOGVNQXJycE5GUFUi.ZeDzXQ.10ODTj49yF7kxKbeMm9eaayR3Xo
journalist_uuid: e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
token: IlAzb1p1MjRubnlLOFo0SDVaajZPc2JPZWRRcjRYRUxLM3RrUmhsUEFWVTQi.Z1jewg.4VNFOk_5HSHXhgn319zzDh9JVyg
headers:
connection: close
content-length: '290'
content-type: application/json
date: Thu, 29 Feb 2024 21:13:01 GMT
date: Wed, 11 Dec 2024 00:37:22 GMT
server: Werkzeug/2.2.3 Python/3.8.10
status: 200
version: 1
2 changes: 1 addition & 1 deletion client/tests/sdk/data/test_auth_badpassword.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
interactions:
- request:
body: '{"username": "journalist", "passphrase": "no", "one_time_code": "112097"}'
body: '{"username": "journalist", "passphrase": "no", "one_time_code": "578466"}'
headers: {}
method: POST
uri: api/v1/token
Expand Down
2 changes: 1 addition & 1 deletion client/tests/sdk/data/test_auth_baduser.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
interactions:
- request:
body: '{"username": "no", "passphrase": "correct horse battery staple profanity
oil chewy", "one_time_code": "112097"}'
oil chewy", "one_time_code": "578466"}'
headers: {}
method: POST
uri: api/v1/token
Expand Down
650 changes: 325 additions & 325 deletions client/tests/sdk/data/test_delete_conversation.yml
View file
Open in desktop

Large diffs are not rendered by default.

176 changes: 127 additions & 49 deletions client/tests/sdk/data/test_delete_reply.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,45 +5,71 @@ interactions:
Accept:
- application/json
Authorization:
- Token Ilhha2ZHZWpVcFZQVmpKdWJOUzJYRG1YVUc4QXZWS0JiOGVNQXJycE5GUFUi.ZeDzXQ.10ODTj49yF7kxKbeMm9eaayR3Xo
- Token IlAzb1p1MjRubnlLOFo0SDVaajZPc2JPZWRRcjRYRUxLM3RrUmhsUEFWVTQi.Z1jewg.4VNFOk_5HSHXhgn319zzDh9JVyg
Content-Type:
- application/json
method: GET
uri: api/v1/replies
response: !!python/object:securedrop_client.sdk.JSONResponse
data:
replies:
- filename: 5-hand-to-mouth_greasewood-reply.gpg
- filename: 5-intricate_hock-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: dellsberg
journalist_uuid: e101e4e2-230e-4e27-9f58-ad094079c230
reply_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0/replies/8d0b333a-082f-4c01-a4c4-5763795af7f7
journalist_uuid: a6e99098-ca63-403c-a76d-38530a2e88dd
reply_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8/replies/417d50e9-6e12-4aee-b11a-4d2e2bb58cde
seen_by:
- ed64b0fa-0565-4b12-9c48-6cfe27a73fd9
- e101e4e2-230e-4e27-9f58-ad094079c230
size: 1331
source_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0
uuid: 8d0b333a-082f-4c01-a4c4-5763795af7f7
- filename: 6-hand-to-mouth_greasewood-reply.gpg
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- a6e99098-ca63-403c-a76d-38530a2e88dd
size: 1174
source_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8
uuid: 417d50e9-6e12-4aee-b11a-4d2e2bb58cde
- filename: 6-intricate_hock-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: deleted
journalist_uuid: b2857ac1-3765-4a3e-95d2-528209ef9525
reply_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0/replies/40460ca9-8d80-40d6-8181-c5175ad961cc
journalist_uuid: c98cd9c4-a45a-42cb-a704-9d9af34f4563
reply_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8/replies/c8142538-e670-42dd-9126-fb67242166dd
seen_by:
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- c98cd9c4-a45a-42cb-a704-9d9af34f4563
size: 1293
source_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8
uuid: c8142538-e670-42dd-9126-fb67242166dd
- filename: 5-generational_catchment-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: deleted
journalist_uuid: c98cd9c4-a45a-42cb-a704-9d9af34f4563
reply_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0/replies/946ab2d4-a13d-4fd9-9372-e9a7aac55aca
seen_by:
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- c98cd9c4-a45a-42cb-a704-9d9af34f4563
size: 1331
source_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0
uuid: 946ab2d4-a13d-4fd9-9372-e9a7aac55aca
- filename: 6-generational_catchment-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: dellsberg
journalist_uuid: a6e99098-ca63-403c-a76d-38530a2e88dd
reply_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0/replies/97ad15ab-64a2-4fb9-8d26-80cf46fedfae
seen_by:
- ed64b0fa-0565-4b12-9c48-6cfe27a73fd9
- b2857ac1-3765-4a3e-95d2-528209ef9525
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- a6e99098-ca63-403c-a76d-38530a2e88dd
size: 1237
source_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0
uuid: 40460ca9-8d80-40d6-8181-c5175ad961cc
source_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0
uuid: 97ad15ab-64a2-4fb9-8d26-80cf46fedfae
headers:
connection: close
content-length: '1395'
content-length: '2745'
content-type: application/json
date: Thu, 29 Feb 2024 21:13:15 GMT
date: Wed, 11 Dec 2024 00:37:40 GMT
server: Werkzeug/2.2.3 Python/3.8.10
status: 200
- request:
Expand All @@ -52,45 +78,71 @@ interactions:
Accept:
- application/json
Authorization:
- Token Ilhha2ZHZWpVcFZQVmpKdWJOUzJYRG1YVUc4QXZWS0JiOGVNQXJycE5GUFUi.ZeDzXQ.10ODTj49yF7kxKbeMm9eaayR3Xo
- Token IlAzb1p1MjRubnlLOFo0SDVaajZPc2JPZWRRcjRYRUxLM3RrUmhsUEFWVTQi.Z1jewg.4VNFOk_5HSHXhgn319zzDh9JVyg
Content-Type:
- application/json
method: GET
uri: api/v1/replies
response: !!python/object:securedrop_client.sdk.JSONResponse
data:
replies:
- filename: 5-hand-to-mouth_greasewood-reply.gpg
- filename: 5-intricate_hock-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: dellsberg
journalist_uuid: e101e4e2-230e-4e27-9f58-ad094079c230
reply_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0/replies/8d0b333a-082f-4c01-a4c4-5763795af7f7
journalist_uuid: a6e99098-ca63-403c-a76d-38530a2e88dd
reply_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8/replies/417d50e9-6e12-4aee-b11a-4d2e2bb58cde
seen_by:
- ed64b0fa-0565-4b12-9c48-6cfe27a73fd9
- e101e4e2-230e-4e27-9f58-ad094079c230
size: 1331
source_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0
uuid: 8d0b333a-082f-4c01-a4c4-5763795af7f7
- filename: 6-hand-to-mouth_greasewood-reply.gpg
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- a6e99098-ca63-403c-a76d-38530a2e88dd
size: 1174
source_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8
uuid: 417d50e9-6e12-4aee-b11a-4d2e2bb58cde
- filename: 6-intricate_hock-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: deleted
journalist_uuid: c98cd9c4-a45a-42cb-a704-9d9af34f4563
reply_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8/replies/c8142538-e670-42dd-9126-fb67242166dd
seen_by:
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- c98cd9c4-a45a-42cb-a704-9d9af34f4563
size: 1293
source_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8
uuid: c8142538-e670-42dd-9126-fb67242166dd
- filename: 5-generational_catchment-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: deleted
journalist_uuid: b2857ac1-3765-4a3e-95d2-528209ef9525
reply_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0/replies/40460ca9-8d80-40d6-8181-c5175ad961cc
journalist_uuid: c98cd9c4-a45a-42cb-a704-9d9af34f4563
reply_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0/replies/946ab2d4-a13d-4fd9-9372-e9a7aac55aca
seen_by:
- ed64b0fa-0565-4b12-9c48-6cfe27a73fd9
- b2857ac1-3765-4a3e-95d2-528209ef9525
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- c98cd9c4-a45a-42cb-a704-9d9af34f4563
size: 1331
source_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0
uuid: 946ab2d4-a13d-4fd9-9372-e9a7aac55aca
- filename: 6-generational_catchment-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: dellsberg
journalist_uuid: a6e99098-ca63-403c-a76d-38530a2e88dd
reply_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0/replies/97ad15ab-64a2-4fb9-8d26-80cf46fedfae
seen_by:
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- a6e99098-ca63-403c-a76d-38530a2e88dd
size: 1237
source_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0
uuid: 40460ca9-8d80-40d6-8181-c5175ad961cc
source_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0
uuid: 97ad15ab-64a2-4fb9-8d26-80cf46fedfae
headers:
connection: close
content-length: '1395'
content-length: '2745'
content-type: application/json
date: Thu, 29 Feb 2024 21:13:15 GMT
date: Wed, 11 Dec 2024 00:37:40 GMT
server: Werkzeug/2.2.3 Python/3.8.10
status: 200
- request:
Expand All @@ -99,19 +151,19 @@ interactions:
Accept:
- application/json
Authorization:
- Token Ilhha2ZHZWpVcFZQVmpKdWJOUzJYRG1YVUc4QXZWS0JiOGVNQXJycE5GUFUi.ZeDzXQ.10ODTj49yF7kxKbeMm9eaayR3Xo
- Token IlAzb1p1MjRubnlLOFo0SDVaajZPc2JPZWRRcjRYRUxLM3RrUmhsUEFWVTQi.Z1jewg.4VNFOk_5HSHXhgn319zzDh9JVyg
Content-Type:
- application/json
method: DELETE
uri: api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0/replies/8d0b333a-082f-4c01-a4c4-5763795af7f7
uri: api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8/replies/417d50e9-6e12-4aee-b11a-4d2e2bb58cde
response: !!python/object:securedrop_client.sdk.JSONResponse
data:
message: Reply deleted
headers:
connection: close
content-length: '33'
content-type: application/json
date: Thu, 29 Feb 2024 21:13:15 GMT
date: Wed, 11 Dec 2024 00:37:40 GMT
server: Werkzeug/2.2.3 Python/3.8.10
status: 200
- request:
Expand All @@ -120,32 +172,58 @@ interactions:
Accept:
- application/json
Authorization:
- Token Ilhha2ZHZWpVcFZQVmpKdWJOUzJYRG1YVUc4QXZWS0JiOGVNQXJycE5GUFUi.ZeDzXQ.10ODTj49yF7kxKbeMm9eaayR3Xo
- Token IlAzb1p1MjRubnlLOFo0SDVaajZPc2JPZWRRcjRYRUxLM3RrUmhsUEFWVTQi.Z1jewg.4VNFOk_5HSHXhgn319zzDh9JVyg
Content-Type:
- application/json
method: GET
uri: api/v1/replies
response: !!python/object:securedrop_client.sdk.JSONResponse
data:
replies:
- filename: 6-hand-to-mouth_greasewood-reply.gpg
- filename: 6-intricate_hock-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: deleted
journalist_uuid: b2857ac1-3765-4a3e-95d2-528209ef9525
reply_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0/replies/40460ca9-8d80-40d6-8181-c5175ad961cc
journalist_uuid: c98cd9c4-a45a-42cb-a704-9d9af34f4563
reply_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8/replies/c8142538-e670-42dd-9126-fb67242166dd
seen_by:
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- c98cd9c4-a45a-42cb-a704-9d9af34f4563
size: 1293
source_url: /api/v1/sources/c0f0153d-0cb0-4ad3-9f0e-f8540891b5c8
uuid: c8142538-e670-42dd-9126-fb67242166dd
- filename: 5-generational_catchment-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: deleted
journalist_uuid: c98cd9c4-a45a-42cb-a704-9d9af34f4563
reply_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0/replies/946ab2d4-a13d-4fd9-9372-e9a7aac55aca
seen_by:
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- c98cd9c4-a45a-42cb-a704-9d9af34f4563
size: 1331
source_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0
uuid: 946ab2d4-a13d-4fd9-9372-e9a7aac55aca
- filename: 6-generational_catchment-reply.gpg
is_deleted_by_source: false
journalist_first_name: ''
journalist_last_name: ''
journalist_username: dellsberg
journalist_uuid: a6e99098-ca63-403c-a76d-38530a2e88dd
reply_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0/replies/97ad15ab-64a2-4fb9-8d26-80cf46fedfae
seen_by:
- ed64b0fa-0565-4b12-9c48-6cfe27a73fd9
- b2857ac1-3765-4a3e-95d2-528209ef9525
- e7e047d5-aa5e-4afe-9a4f-c24e5f2cec8d
- a6e99098-ca63-403c-a76d-38530a2e88dd
size: 1237
source_url: /api/v1/sources/127ce85b-2c3e-4f06-a46f-89882ecb82d0
uuid: 40460ca9-8d80-40d6-8181-c5175ad961cc
source_url: /api/v1/sources/ee212060-9cd5-4031-b596-581ea15cffa0
uuid: 97ad15ab-64a2-4fb9-8d26-80cf46fedfae
headers:
connection: close
content-length: '707'
content-length: '2067'
content-type: application/json
date: Thu, 29 Feb 2024 21:13:15 GMT
date: Wed, 11 Dec 2024 00:37:40 GMT
server: Werkzeug/2.2.3 Python/3.8.10
status: 200
version: 1
Loading
Loading