diff --git a/admin/base/middleware.py b/admin/base/middleware.py new file mode 100644 index 00000000000..13fe392a09b --- /dev/null +++ b/admin/base/middleware.py @@ -0,0 +1,24 @@ +from django.contrib.auth.views import redirect_to_login +from django.core.exceptions import PermissionDenied +from django.urls import reverse + + +class AdminLoginRequiredMiddleware: + """Require an active staff user for every admin view not marked with ``login_not_required``. + Anonymous users are sent to the admin login page; signed-in users who aren't staff get a 403. + """ + + def __init__(self, get_response): + self.get_response = get_response + + def __call__(self, request): + return self.get_response(request) + + def process_view(self, request, view_func, view_args, view_kwargs): + if not getattr(view_func, 'login_required', True): + return None + if not request.user.is_authenticated: + return redirect_to_login(request.get_full_path(), reverse('auth:login')) + if not request.user.is_staff: + raise PermissionDenied + return None diff --git a/admin/base/settings/defaults.py b/admin/base/settings/defaults.py index 98a970c4a00..acf28e1a0bf 100644 --- a/admin/base/settings/defaults.py +++ b/admin/base/settings/defaults.py @@ -157,6 +157,7 @@ 'django.contrib.sessions.middleware.SessionMiddleware', 'django.contrib.auth.middleware.AuthenticationMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', + 'admin.base.middleware.AdminLoginRequiredMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', 'django.middleware.security.SecurityMiddleware', 'waffle.middleware.WaffleMiddleware', diff --git a/admin/base/urls.py b/admin/base/urls.py index 3d3c65fa6dc..002c41efec3 100644 --- a/admin/base/urls.py +++ b/admin/base/urls.py @@ -12,6 +12,7 @@ base_pattern, include([ re_path(r'^$', views.home, name='home'), + re_path(r'^unprotected_views/$', views.unprotected_views, name='unprotected_views'), re_path(r'^admin/dj-celery-panel/', include('dj_celery_panel.urls')), re_path(r'^admin/', admin.site.urls), re_path(r'^provider_asset_files/', include('admin.provider_asset_files.urls', namespace='provider_asset_files')), diff --git a/admin/base/utils.py b/admin/base/utils.py index a612e06c185..34543c33e38 100644 --- a/admin/base/utils.py +++ b/admin/base/utils.py @@ -4,7 +4,7 @@ from osf.models import Subject, NodeLicense, Brand, CedarMetadataTemplate from django.core.exceptions import ValidationError, PermissionDenied -from django.urls import reverse +from django.urls import URLPattern, URLResolver, get_resolver, reverse from django.core.validators import RegexValidator, _lazy_re_compile from django.utils.http import urlencode from django.utils.translation import gettext_lazy as _ @@ -33,6 +33,38 @@ def osf_staff_check(user): return user and user.is_authenticated and user.is_staff +def login_not_required(view_func): + """Let a view through AdminLoginRequiredMiddleware without a logged-in staff user. + + For class-based views, decorate ``dispatch`` with ``method_decorator``. + """ + view_func.login_required = False + return view_func + + +def get_unprotected_url_patterns(url_patterns=None, route_prefix='/', name_prefix=''): + """List the URL patterns that AdminLoginRequiredMiddleware lets through without a logged-in staff user. + + Each entry is a dict with the pattern's ``route``, namespaced URL ``name`` and the dotted path of its ``view``. + """ + if url_patterns is None: + url_patterns = get_resolver().url_patterns + unprotected = [] + for pattern in url_patterns: + route = route_prefix + str(pattern.pattern).lstrip('^').removesuffix('$') + if isinstance(pattern, URLResolver): + namespace = f'{name_prefix}{pattern.namespace}:' if pattern.namespace else name_prefix + unprotected += get_unprotected_url_patterns(pattern.url_patterns, route, namespace) + elif isinstance(pattern, URLPattern) and not getattr(pattern.callback, 'login_required', True): + view = getattr(pattern.callback, 'view_class', pattern.callback) + unprotected.append({ + 'route': route, + 'name': f'{name_prefix}{pattern.name}' if pattern.name else '', + 'view': f'{view.__module__}.{view.__qualname__}', + }) + return unprotected + + def get_subject_rules(subjects_selected): """ Take a list of subjects, and parse them into rules consistent with preprint provider diff --git a/admin/base/views.py b/admin/base/views.py index 1d79e1020db..08dea5c418d 100644 --- a/admin/base/views.py +++ b/admin/base/views.py @@ -4,7 +4,7 @@ from django.views.generic import DetailView from django.views.defaults import page_not_found -from admin.base.utils import osf_staff_check +from admin.base.utils import get_unprotected_url_patterns, osf_staff_check @user_passes_test(osf_staff_check) @@ -13,6 +13,12 @@ def home(request): return render(request, 'home.html', context) +@user_passes_test(osf_staff_check) +def unprotected_views(request): + context = {'unprotected_url_patterns': get_unprotected_url_patterns()} + return render(request, 'unprotected_views.html', context) + + class GuidView(DetailView): def get(self, request, *args, **kwargs): try: diff --git a/admin/common_auth/views.py b/admin/common_auth/views.py index 552eb405f05..675a9b049cc 100644 --- a/admin/common_auth/views.py +++ b/admin/common_auth/views.py @@ -11,6 +11,7 @@ from osf.models.user import OSFUser from osf.models import AdminProfile +from admin.base.utils import login_not_required from admin.common_auth.forms import LoginForm, UserRegistrationForm, DeskUserForm, TwoFactorForm @@ -19,6 +20,7 @@ class LoginView(FormView): redirect_field_name = REDIRECT_FIELD_NAME template_name = 'login.html' + @method_decorator(login_not_required) @method_decorator(csrf_protect) @method_decorator(never_cache) def dispatch(self, request, *args, **kwargs): @@ -113,7 +115,6 @@ def get_success_url(self): redirect_to = reverse('home') return redirect_to - def logout_user(request): logout(request) return redirect('auth:login') diff --git a/admin/templates/base.html b/admin/templates/base.html index 4554c0ff1b5..06bd7c8069b 100644 --- a/admin/templates/base.html +++ b/admin/templates/base.html @@ -348,6 +348,9 @@ {% if perms.osf.change_maintenancestate %}
  • Maintenance Alerts
    • {% endif %} + {% if user.is_staff %} +
  • Unprotected Views
    • + {% endif %} diff --git a/admin_tests/base/test_middleware.py b/admin_tests/base/test_middleware.py new file mode 100644 index 00000000000..65a19922fe8 --- /dev/null +++ b/admin_tests/base/test_middleware.py @@ -0,0 +1,92 @@ +import pytest +from django.contrib.auth.models import AnonymousUser, Permission +from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import PermissionDenied +from django.test import RequestFactory +from django.urls import resolve, reverse + +from admin.base import settings as admin_settings +from admin.base.middleware import AdminLoginRequiredMiddleware +from admin.common_auth.views import LoginView +from admin_tests.utilities import all_admin_views, as_view_func, protected_views, public_views +from osf.models import AbstractNode, OSFUser +from osf_tests.factories import AuthUserFactory + +pytestmark = pytest.mark.django_db + +LOGIN_REDIRECT_PREFIX = f"{reverse('auth:login', urlconf='admin.base.urls')}?next=" + + +def process_view(view_func, user): + request = RequestFactory().get('/fake_path') + request.user = user + middleware = AdminLoginRequiredMiddleware(lambda r: None) + return middleware.process_view(request, view_func, (), {}) + + +def staff_user(**kwargs): + user = AuthUserFactory(**kwargs) + user.is_staff = True + user.save() + return user + + +@pytest.mark.urls('admin.base.urls') +class TestAdminLoginRequiredMiddleware: + + @pytest.mark.parametrize('view', protected_views()) + def test_anonymous_user_is_sent_to_login(self, view): + response = process_view(as_view_func(view), AnonymousUser()) + assert response.status_code == 302 + assert response.url.startswith(LOGIN_REDIRECT_PREFIX) + + @pytest.mark.parametrize('view', protected_views()) + def test_non_staff_user_is_denied(self, view): + with pytest.raises(PermissionDenied): + process_view(as_view_func(view), OSFUser(is_active=True, is_staff=False)) + + def test_staff_user_passes(self): + assert process_view(resolve('/nodes/').func, staff_user()) is None + + def test_django_admin_site_requires_login(self): + response = process_view(resolve('/admin/').func, AnonymousUser()) + assert response.status_code == 302 + assert response.url.startswith(LOGIN_REDIRECT_PREFIX) + + def test_login_is_public(self): + assert process_view(as_view_func(LoginView), AnonymousUser()) is None + + def test_only_login_is_public(self): + public = {view for view in all_admin_views() if not getattr(as_view_func(view), 'login_required', True)} + assert public == public_views() + + +@pytest.mark.urls('admin.base.urls') +class TestAdminLoginRequiredMiddlewareEnabled: + """ + Each request gets a different response from /nodes/ without AdminLoginRequiredMiddleware: the view itself + returns 403 to anonymous users and lets in a non-staff user who holds osf.view_node. + """ + + @pytest.fixture(autouse=True) + def admin_middleware(self, settings): + settings.MIDDLEWARE = admin_settings.MIDDLEWARE + + def test_middleware_is_in_admin_settings(self): + middleware = list(admin_settings.MIDDLEWARE) + assert 'admin.base.middleware.AdminLoginRequiredMiddleware' in middleware + + def test_anonymous_request_is_redirected_to_login(self, client): + response = client.get('/nodes/') + assert response.status_code == 302 + assert response.url == f'{LOGIN_REDIRECT_PREFIX}/nodes/' + + def test_non_staff_request_is_denied(self, client): + user = AuthUserFactory() + user.user_permissions.add(Permission.objects.get( + codename='view_node', + content_type=ContentType.objects.get_for_model(AbstractNode), + )) + client.force_login(user) + response = client.get('/nodes/') + assert response.status_code == 403 diff --git a/admin_tests/utilities.py b/admin_tests/utilities.py index 44a3709fc44..3dcaa585e6b 100644 --- a/admin_tests/utilities.py +++ b/admin_tests/utilities.py @@ -2,6 +2,9 @@ Utilities to help run Django tests * setup_view - replaces as_view """ +from django.urls import URLPattern, URLResolver, get_resolver + +from admin.common_auth.views import LoginView from osf_tests.factories import UserFactory @@ -53,3 +56,37 @@ def handle_post_view_request(request, view, obj, guid): view.post(request) obj.refresh_from_db() return obj + + +def get_all_views(url_patterns, views=None, module=None): + if views is None: + views = set() + for pattern in url_patterns: + if isinstance(pattern, URLResolver): + get_all_views(pattern.url_patterns, views, module=module) + elif isinstance(pattern, URLPattern): + # Handle function-based and class-based views + view = pattern.callback + if hasattr(view, 'view_class'): + view = view.view_class + if module and not view.__module__.startswith(f'{module}.'): + continue + views.add(view) + return views + + +def as_view_func(view): + return view.as_view() if isinstance(view, type) else view + + +def all_admin_views(): + return get_all_views(get_resolver('admin.base.urls').url_patterns, module='admin') + + +def public_views(): + return {LoginView} + + +def protected_views(): + # Sorted so every xdist worker collects the same tests in the same order + return sorted(all_admin_views() - public_views(), key=lambda view: (view.__name__, view.__module__))