Repository navigation
Conversation
…ndpoints and update UT.
…nt /custom_storage_location/external_acc_update/<access_token>/ /statistics/gather/<access_token>/
|
|
||
| admin_urls = [ | ||
| url(r'^$', views.home, name='home'), | ||
| url(r'^admin/', admin.site.urls), |
There was a problem hiding this comment.
ENABLE_LOGIN_FORM=False でも、ここで公開している /admin/login/ からパスワード認証が可能なように見えます。
There was a problem hiding this comment.
ご指摘ありがとうございます。2件とも、ご指摘のとおりでした。コメントごとに、対応・経緯・同種の漏れがないことの確認を記載します。修正完了後に、改めてレビュー依頼させていただきます。
対応
ENABLE_LOGIN_FORM が無効の場合、/admin/login/ を 404 とします(admin/base/urls.py で admin.site.urls より前に登録します)。学認でログイン済みのユーザは、引き続き Django 管理サイトを利用できます。UT・結合試験・総合テストに、404 となることの確認を追加します。
経緯
- 本対応は、保守チケットで示された URL パスを対象に始めており、
/admin/配下は対象に含まれていませんでした。 - その後、パスワード関連の URL への対処として、パスワードによるログインを止めて学認ログインに統一する方針(
ENABLE_LOGIN_FORM連動)を採りました。この時点でパスワード認証の入口をすべて洗い出すべきところ、本リポジトリ内のログイン処理(LoginView)のみを対象としていました。Django 管理サイトのログインは Django 本体側で認証を呼び出すため、リポジトリ内の確認では対象から漏れていました。
同種の漏れがないことの確認
- パスワード認証の入口:管理画面でパスワード認証を行うのは、
LoginViewと Django 管理サイトのログインの2か所のみです(管理画面の DRF の認証はSessionAuthenticationのみ)。今回の修正で、ENABLE_LOGIN_FORMが無効の場合は両方とも使えなくなります。 - Django 管理サイト配下のほかの URL:ログイン以外の URL は、Django の
AdminSite.admin_viewにより、is_activeかつis_staffのユーザ以外はログイン画面へ転送されます。未ログインで利用できる URL は、ログイン以外にありません。
There was a problem hiding this comment.
@yacchin1205
対応が完了しましたので、改めて、レビュー依頼を投げさせていただきました。
|
|
||
| @wraps(view_func) | ||
| def wrapped_view(request, *args, **kwargs): | ||
| if not osf_superuser_check(request.user): |
There was a problem hiding this comment.
無効化されたsuperuserを許可してしまうコードに見えます。他のコードと合わせるという点で、
Lines 16 to 24 in 0381d90
is_active と is_registered の確認をしておくのが手堅いかと思いました。
There was a problem hiding this comment.
こちらも同様に修正完了後に、改めてレビュー依頼させていただきます。
対応
osf_superuser_check を、RdmPermissionMixin.is_super_admin と同じ条件(is_active と is_registered を確認)に揃えます。@superuser_required を使用する3つの view について、無効化・未登録の superuser が 403 となる確認を UT と結合試験に追加します。
同じファイルの osf_staff_check も is_active・is_registered を確認していませんが、使用箇所はトップ画面(admin/base/views.py の home)のみで、ログインユーザ本人の名前と各画面へのメニューを表示するだけのため、今回は変更しません。
経緯
superuser_requiredは、関数で書かれた view 向けに今回新設したデコレータです。判定(osf_superuser_check)は、同じファイルの既存のosf_staff_checkと同じ形(ログイン済みかと権限フラグのみ)で作成しており、既存のRdmPermissionMixin.is_super_adminと条件を揃えていませんでした。- 試験も権限の種類ごとの許可・拒否のみを確認しており、無効化・未登録の状態を条件に含めていませんでした。
同種の漏れがないことの確認
- 本 PR のほかのユーザ権限の判定は、既存の
RdmPermissionMixin、または同じアプリの既存の画面と同じPermissionRequiredMixinを使用しており、いずれも無効化されたユーザを拒否します。既存の判定と条件が異なる新設の判定は、superuser_requiredのみでした。
There was a problem hiding this comment.
@yacchin1205
対応が完了しましたので、改めて、レビュー依頼を投げさせていただきました。
Purpose
Implement institutional admin API authorization and secure cron-triggered endpoints using HMAC-signed URLs instead of hardcoded tokens.
Changes
Deployment Notes
The following must be deployed together with this PR.
QA Notes
Documentation
Side Effects
Ticket
60922