Repository navigation
[GRDM-60922] Add permission checks and update token verification for admin endpoints #796
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
106c84b
14943c8
47424ed
e92d12f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| Utility functions and classes | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| from functools import wraps | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from osf.models import Subject, NodeLicense, Brand | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from django.core.exceptions import ValidationError, PermissionDenied | ||||||||||||||||||||
|
|
@@ -35,6 +37,29 @@ def osf_staff_check(user): | |||||||||||||||||||
| return user.is_authenticated and user.is_staff | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def osf_superuser_check(user): | ||||||||||||||||||||
| """An inactive or unregistered superuser is not an Integrated Admin.""" | ||||||||||||||||||||
| return bool( | ||||||||||||||||||||
| user.is_authenticated and | ||||||||||||||||||||
| user.is_active and | ||||||||||||||||||||
| user.is_registered and | ||||||||||||||||||||
| user.is_superuser | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def superuser_required(view_func): | ||||||||||||||||||||
| """Function-view decorator that returns 403 (PermissionDenied) instead of | ||||||||||||||||||||
| redirecting to login, unlike django.contrib.auth.decorators.user_passes_test.""" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @wraps(view_func) | ||||||||||||||||||||
| def wrapped_view(request, *args, **kwargs): | ||||||||||||||||||||
| if not osf_superuser_check(request.user): | ||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 無効化されたsuperuserを許可してしまうコードに見えます。他のコードと合わせるという点で、 Lines 16 to 24 in 0381d90
is_active と is_registered の確認をしておくのが手堅いかと思いました。
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. こちらも同様に修正完了後に、改めてレビュー依頼させていただきます。 対応
同じファイルの 経緯
同種の漏れがないことの確認
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yacchin1205 |
||||||||||||||||||||
| raise PermissionDenied | ||||||||||||||||||||
| return view_func(request, *args, **kwargs) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return wrapped_view | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def get_subject_rules(subjects_selected): | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| Take a list of subjects, and parse them into rules consistent with preprpint provider | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| */30 * * * * curl http://localhost:8001/custom_storage_location/external_acc_update/d610ef95f0b0f5868f13919b8ed64070b9acb9c19b8da9f2c514ed938203ec3e236c9cad4f4146bdf22b4e79cf0d92f6d4f4c996d236c6b0ee79a1336b26afb7/ | jq -c . | ||
| */30 * * * * TOKEN=$(cd /code && python3 manage.py generate_signed_cron_token | tail -n1) && [ -n "$TOKEN" ] && curl "http://localhost:8001/custom_storage_location/external_acc_update/$TOKEN/" | jq -c . |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 0 2 * * 1 curl http://localhost:8001/statistics/gather/2A85563B2B0F7D3168199F475365F57DA1D56E4BB2CE2B7044EB058AE5E287637E7C636A772682D92C8D6B1830B9A97C5A5DC3DE7016C60BDE4BAA7CC3B38AEB/ | jq -c . | ||
| 0 2 * * 1 TOKEN=$(cd /code && python3 manage.py generate_signed_cron_token | tail -n1) && [ -n "$TOKEN" ] && curl "http://localhost:8001/statistics/gather/$TOKEN/" | jq -c . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENABLE_LOGIN_FORM=Falseでも、ここで公開している/admin/login/からパスワード認証が可能なように見えます。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yacchin1205
ご指摘ありがとうございます。2件とも、ご指摘のとおりでした。コメントごとに、対応・経緯・同種の漏れがないことの確認を記載します。修正完了後に、改めてレビュー依頼させていただきます。
対応
ENABLE_LOGIN_FORMが無効の場合、/admin/login/を 404 とします(admin/base/urls.pyでadmin.site.urlsより前に登録します)。学認でログイン済みのユーザは、引き続き Django 管理サイトを利用できます。UT・結合試験・総合テストに、404 となることの確認を追加します。経緯
/admin/配下は対象に含まれていませんでした。ENABLE_LOGIN_FORM連動)を採りました。この時点でパスワード認証の入口をすべて洗い出すべきところ、本リポジトリ内のログイン処理(LoginView)のみを対象としていました。Django 管理サイトのログインは Django 本体側で認証を呼び出すため、リポジトリ内の確認では対象から漏れていました。同種の漏れがないことの確認
LoginViewと Django 管理サイトのログインの2か所のみです(管理画面の DRF の認証はSessionAuthenticationのみ)。今回の修正で、ENABLE_LOGIN_FORMが無効の場合は両方とも使えなくなります。AdminSite.admin_viewにより、is_activeかつis_staffのユーザ以外はログイン画面へ転送されます。未ログインで利用できる URL は、ログイン以外にありません。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yacchin1205
対応が完了しましたので、改めて、レビュー依頼を投げさせていただきました。