Repository navigation
Feature/s3 sigv4 clean - #817
Merged
hide24 merged 10 commits intoOct 22, 2026
Merged
Conversation
Replace the boto 2 client with boto3/botocore so that the S3 addon can talk
to regions that only accept Signature Version 4 (ap-northeast-1 and later).
- utils.py / views.py / models.py build the client through boto3 with a
region-specific endpoint, and surface botocore exceptions as HTTPError
- folder_id becomes "bucket:/prefix/" so that a subfolder can be selected;
the legacy "bucket" form is still accepted
- the node/user config JS, the Rubeus config and the log action lists follow
the new folder shape
- requirements.txt gains boto3/botocore; addons/s3/requirements.txt is kept
as an empty file because the Jenkins Dockerfile still does
COPY ./addons/s3/requirements.txt and would fail the image build without it
- MAX_UPLOAD_SIZE stays at 50GB: multipart upload handles up to 5TB, so the
lower limit the first draft introduced was unnecessary
- the ${ _("...") } gettext wrappers are kept in all three mako templates
(20 translatable strings); only the aria-label/alt attributes and the th
class change are new
Co-Authored-By: An Qiuyu <qiuyu.an@hotmail.com>
…lready registered
Registering the same access key twice has to end up on the existing
ExternalAccount instead of creating a second row. The duplicate is caught by
the unique constraint on (provider, provider_id) — provider_id holds
"{aws_account_id}\t{access_key}" (GRDM-53044) — so the save raises
IntegrityError, not the ValidationError the handler was catching, and the
failed INSERT aborted the surrounding transaction.
Wrap the save in transaction.atomic() so the failure only rolls back to a
savepoint, and catch IntegrityError alongside ValidationError before falling
back to the account that is already stored.
…igV4 folder shape - test_view.py still used assert_equals/assert_in and a missing InstitutionFactory import left over from the nose era; replace them with plain pytest asserts against rv.text - _mock_folder_result still returned the pre-SigV4 get_folders payload, so the node settings tests asserted against a shape the view no longer produces
Extend addons/s3/tests/test_view.py so the SigV4 flow is exercised end to end:
- both folder_id forms ("bucket" and "bucket:/prefix/") and a non-existent
bucket raising InvalidFolderError
- the provider_id format "{aws_account_id}\t{access_key}" and the dedup path
when the same key is registered twice (GRDM-53044)
- the error branches: a missing key field, get_user_info returning None, an
invalid bucket name, an empty bucket_exists result, can_list returning None,
and the ClientError / InvalidAuthError conversions
Three test-harness fixes come with it:
- query ExternalAccount by provider_id, a plain CharField. oauth_key is an
EncryptedTextField (JWE), so its ciphertext differs on every save and
filtering on it never matches; the plaintext is checked by attribute access
instead
- use post_json/put_json instead of the json= keyword, which the webtest_plus
version used in CI does not accept, and pass expect_errors=True wherever a
4xx/401/403 response is expected
- patch get_user_info with mock.patch(..., return_value=mock.Mock(...)); the
.return_value attribute on the patcher object never reaches the started mock,
so user_info.id / user_info.display_name were missing in the view
…display name - stop lower-casing the bucket name in the settings check. S3 bucket names are case-sensitive in the legacy us-east-1 naming rules, so folding the case can point the addon at a different bucket; validate_bucket_name already rejects the names that are actually invalid - fall back gracefully when the owner returned by GetUserInfo has no display name, instead of raising while building the account label - keep the Owner entry of the admin storage-location response in its original shape so that the existing consumers keep working
…rametrize pytest.mark.parametrize has no effect on a method of a unittest.TestCase: the case is collected once and the extra arguments are never supplied, so test_serialize_settings_supports_legacy_and_prefixed_folder_ids errored on a missing fixture instead of checking the three folder_id forms. Iterate over the cases inside the test body instead.
get_bucket_location() returns a null LocationConstraint for us-east-1, but BUCKET_LOCATIONS keys that region as '', so the null fell through to set_folder and labelled the bucket '(None)' in the node settings UI.
The ListObjects call was unguarded, so expired or revoked keys and a missing bucket escaped as a raw botocore exception and reached the user as a 500 with no actionable message. Map NoCredentialsError to 403 and reuse get_status_for_error so a ClientError keeps the status S3 returned.
create_bucket only handled NoCredentialsError and ClientError, so the non-response failures - bad parameters, an unreachable endpoint - left the user with a 500. ClientError is not a BotoCoreError subclass and NoCredentialsError is matched earlier, so the existing clauses keep their behaviour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
総合テスト用 #746 のコピー