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
Author
|
RCOSDP/RDM-waterbutler#93 の修正が完了し、本 PR も合わせて更新しました。
再レビューをお願いします。 |
念の為ですが、リンク先ですが、RDM-osf.io側になってしまっているようでした。 RCOSDP/RDM-waterbutler#93 (comment) のことと理解し、承知しました。 |
yacchin1205
approved these changes
Oct 14, 2026
hide24
approved these changes
Oct 16, 2026
Author
リンクを修正いたしました。誤記申し訳ありません。 |
Merged
Author
|
RCOSDP/RDM-waterbutler#93 の修正(f76537f4、8a984734)にあわせて、本 PR と #93 を同時に入れた検証環境で移行互換の確認を行いました。結果の要点です。 develop で接続・投入した機関ストレージとアドオンのデータは、本 PR + #93 でそのまま一覧・取得・ダウンロード・移動・削除できました。DB マイグレーションはなく、 一方、逆方向で 1 点、注意が必要な挙動がありました。本 PR の稼働中に新規接続したアドオンは、develop に戻すとファイル一覧が "couldn't load" になります。原因は本 PR がルート選択でも CI は develop を取り込んだ状態で success です。 |
Merged
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.
概要
既存の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 はルート直下を選んだ場合も
folder_idを<bucket>:/の形式で保存します(develop は<bucket>)。develop のserialize_waterbutler_settingsはこの値をそのままバケット名として WaterButler に渡すため、本 PR の稼働中に新規接続または接続先の変更を行った S3 アドオンは、develop に戻すとファイル一覧を開けなくなります(検証環境で確認済み。エラーは出ず "couldn't load" の表示になります)。develop の時点で接続済みだったアドオンには影響しません。develop に戻す場合は、本 PR の稼働中に接続・変更したアドオンの再接続が必要です。この形式はサブフォルダ選択時の
<bucket>:<path>と保存形式を揃えるためのものです。切り戻しは再接続で復旧できるため、本 PR では形式を変えず、上記を運用手順に記載する対応としています。ルート選択時のみ<bucket>で保存して develop と揃える変更は、必要であれば別 PR で対応可能です。関連PR
Based on anqiuy/RDM-osf.io@feature/s3-sigv4
2026-10-02 更新: develop(0381d90)を取り込みました(merge commit 2a4e715、競合なし)。あわせて見直しで見つかった 3 件を修正しています: us-east-1 バケットの
LocationConstraintがNoneで表示名が崩れる問題(14a8027)、get_bucket_prefixesの失敗が 500 になる退行(10873ee)、create_bucketでBotoCoreErrorが未捕捉だった問題(94d57ea)。レビュー以降のコミットは論理単位に整理しています(整理前後でツリーは同一)。RCOSDP/RDM-waterbutler#93 と合わせた実機確認の結果は RCOSDP/RDM-waterbutler#93 のコメントを参照してください。