Repository navigation
feat(s3): S3アドオンにSigV4署名を追加 - #746
Open
tishin-endou wants to merge 10 commits into
Open
tishin-endou wants to merge 10 commits into
tishin-endou wants to merge 10 commits into
Conversation
This was referenced Jun 5, 2026
tishin-endou
changed the title
yacchin1205
requested changes
Jul 10, 2026
yacchin1205
reviewed
Aug 10, 2026
yacchin1205
left a comment
yacchin1205
left a comment
There was a problem hiding this comment.
ありがとうございます。対応を確認しました。一方、関連する WaterButler側のPR RCOSDP/RDM-waterbutler#93 について、既存仕様や既知問題への対策が失われている箇所が見つかったため、変更コメントを記載しました。
RCOSDP/RDM-waterbutler#93 (review)
本PRはWaterButler側の変更と一体で動作するため、#93の修正が完了した後、両PRのインターフェースとテスト結果を改めて確認したいと思います。それまでApproveは保留します。
tishin-endou
added a commit
to tishin-endou/RDM-e2e-test-nb
that referenced
this pull request
Oct 1, 2026
run-20261001-130318 の実行で分かった、証跡と最終確認の表示の誤りを直す。 - 最終確認セル [73]: 後始末で generated_buckets が空になるため、3 つ作って 3 つ消していても cleanup_buckets() が「生成したバケットは無い(すべて パラメータ指定)」と出していた。生成時の名前を generated_bucket_names に 控え、list_buckets の実際と突き合わせて「生成 N / 削除 N / 残存 N」を 出すようにする。BucketAlreadyExists の再試行で作りかけを消したときは 控えからも落とす - INDEX.md の対象 SHA: osf_deployed_sha が未記入のままだと 「RCOSDP/RDM-osf.io#746 (None)」という証跡が残るので、未設定のときは 「未設定(S-0-jenkins-build.txt を参照)」と書く。セル [11] の警告は従来どおり - INDEX.md の取りこぼし: セル [67] は S-1-download-redirect.json(HAR の判定) と S-9-cleanup.txt(後始末)より先に走るため、この 2 つが表に載っていなかった。 生成処理を write_index() に切り出し、後始末セル [71] の末尾で作り直す。 INDEX 自身と finish_pw_context の成果物(har*/video-*/last-*/console.log)は 表に出さない(末尾の注で説明しているもの)
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.
tishin-endou
force-pushed
the
feature/s3-sigv4-clean
branch
from
October 2, 2026 07:07
a2068ba to
94d57ea
Compare
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
2 participants
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.
概要
既存のS3アドオンをレガシーSignature V2からSignature V4認証に移行し、SigV4が必要なAWSリージョン（例:
ap-northeast-1）との互換性を実現します。変更内容
モデル (
addons/s3/models.py)s3_auth_typeフィールドを追加ビュー (
addons/s3/views.py)ユーティリティ (
addons/s3/utils.py)get_bucket_names()をSigV4署名リクエストに対応フロントエンド (
addons/s3/static/)s3NodeConfig.js、s3UserConfig.jsをSigV4認証フローに対応テスト (
addons/s3/tests/)test_model.py、test_view.py、test_serializer.pyを新認証フローに適合その他
README.mdを追加requirements.txtに必要な依存関係を追加addons/s3/requirements.txtを削除関連PR
Based on anqiuy/RDM-osf.io@feature/s3-sigv4
2026-10-01 更新: develop(0381d90)を取り込みました(merge commit 548dc5e、競合なし)。あわせて見直しで見つかった 3 件を修正しています: us-east-1 バケットの LocationConstraint が None で表示名が崩れる問題(077e11b)、get_bucket_prefixes の失敗が 500 になる退行(60930c2)、create_bucket で BotoCoreError が未捕捉だった問題(a2068ba)。RDM-waterbutler#93 と合わせた実機確認の結果は #93 のコメントを参照してください。