From a3805d40ee94693c9f46a8093fbb92a9b4d3071f Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Tue, 2 Apr 2024 16:33:23 +0100 Subject: [PATCH 1/6] Force export wins group to not see anything apart from export wins pages --- datahub/company/admin/adviser.py | 3 +- datahub/core/admin.py | 74 +++++++++++++++++++- datahub/investment/investor_profile/admin.py | 3 +- datahub/investment/opportunity/admin.py | 3 +- 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/datahub/company/admin/adviser.py b/datahub/company/admin/adviser.py index 576fb0e6c1..e1676bbf6a 100644 --- a/datahub/company/admin/adviser.py +++ b/datahub/company/admin/adviser.py @@ -5,10 +5,11 @@ from datahub.company.admin.adviser_forms import AddAdviserFromSSOForm from datahub.company.models import Advisor +from datahub.core.admin import ExportWinsAdminMixin @admin.register(Advisor) -class AdviserAdmin(VersionAdmin, UserAdmin): +class AdviserAdmin(ExportWinsAdminMixin, VersionAdmin, UserAdmin): """Adviser admin.""" fieldsets = ( diff --git a/datahub/core/admin.py b/datahub/core/admin.py index c22f0ad95c..614e44ca78 100644 --- a/datahub/core/admin.py +++ b/datahub/core/admin.py @@ -75,7 +75,74 @@ def has_change_permission(self, request, obj=None): return False -class BaseModelAdminMixin: +class ExportWinsAdminMixin: + def has_module_permission(self, request): + user = request.user + if user.groups.filter(name='ExportWinAdmin').exists(): + # print('self.opts', self.opts) + if self.opts.app_label == 'export_win': + return True + return False + return super().has_module_permission(request) + + def has_view_permission(self, request, obj=None): + # print('******has_view_permission, request.user', request.user) + user = request.user + + return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_view_permission(request, obj)) + + if user.groups.filter(name='ExportWinAdmin').exists(): + print('self.opts.model_name', self.opts.model_name) + if self.opts.model_name == 'win': + return True + return False + return super().has_view_permission(request, obj) + + def has_add_permission(self, request): + # print('******has_add_permission, request.user', request.user) + user = request.user + return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_add_permission(request)) + if user.groups.filter(name='ExportWinAdmin').exists(): + print('self.opts.model_name', self.opts.model_name) + if self.opts.model_name == 'win': + return True + return False + return super().has_add_permission(request) + + def has_delete_permission(self, request, obj=None): + # print('******has_delete_permission, request.user', request.user) + user = request.user + return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_delete_permission(request, obj)) + if user.groups.filter(name='ExportWinAdmin').exists(): + print('self.opts.model_name', self.opts.model_name) + if self.opts.model_name == 'win': + return True + return False + return super().has_delete_permission(request, obj) + + def has_change_permission(self, request, obj=None): + # print('******has_change_permission, request.user', request.user) + user = request.user + return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_change_permission(request, obj)) + if user.groups.filter(name='ExportWinAdmin').exists(): + print('self.opts.model_name', self.opts.model_name) + if self.opts.model_name == 'win': + return True + return False + return super().has_change_permission(request, obj) + + def _handle_export_wins_admin_permissions(self, request, model_name, function): + user = request.user + # print('self.opts.model_name', model_name) + if user.groups.filter(name='ExportWinAdmin').exists(): + if model_name == 'win': + return True + return False + + return function + + +class BaseModelAdminMixin(ExportWinsAdminMixin): """ Mixin for ModelAdmins which adds extra functionalities. Useful when the model extends core.BaseModel @@ -355,6 +422,11 @@ def pretty_source(self, obj): def _make_admin_permission_getter(codename): def _has_permission(self, request, obj=None): + if request.user.groups.filter(name='ExportWinAdmin').exists(): + return False + print('****self.opts', self.opts) + print('****_make_admin_permission_getter._has_permission') + print('****_make_admin_permission_getter._has_permission.request.user', request.user) app_label = self.opts.app_label qualified_name = f'{app_label}.{codename}' return request.user.has_perm(qualified_name) diff --git a/datahub/investment/investor_profile/admin.py b/datahub/investment/investor_profile/admin.py index bcf2a4e463..2c00f4c5b7 100644 --- a/datahub/investment/investor_profile/admin.py +++ b/datahub/investment/investor_profile/admin.py @@ -1,10 +1,11 @@ from django.contrib import admin +from datahub.core.admin import ExportWinsAdminMixin from datahub.investment.investor_profile import models @admin.register(models.LargeCapitalInvestorProfile) -class LargeCapitalInvestorProfileAdmin(admin.ModelAdmin): +class LargeCapitalInvestorProfileAdmin(ExportWinsAdminMixin, admin.ModelAdmin): """Large capital investor profile admin.""" autocomplete_fields = ( diff --git a/datahub/investment/opportunity/admin.py b/datahub/investment/opportunity/admin.py index 84de1a5850..16b47345f8 100644 --- a/datahub/investment/opportunity/admin.py +++ b/datahub/investment/opportunity/admin.py @@ -1,10 +1,11 @@ from django.contrib import admin +from datahub.core.admin import ExportWinsAdminMixin from datahub.investment.opportunity import models @admin.register(models.LargeCapitalOpportunity) -class LargeCapitalOpportunityAdmin(admin.ModelAdmin): +class LargeCapitalOpportunityAdmin(ExportWinsAdminMixin, admin.ModelAdmin): """Large capital opportunity admin.""" autocomplete_fields = ( From de69f4defaaccab1129f311e7f5f34a653eb2e4c Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Wed, 3 Apr 2024 09:16:25 +0100 Subject: [PATCH 2/6] let superuser see everything --- datahub/core/admin.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/datahub/core/admin.py b/datahub/core/admin.py index 614e44ca78..0a15ac3dd5 100644 --- a/datahub/core/admin.py +++ b/datahub/core/admin.py @@ -78,10 +78,12 @@ def has_change_permission(self, request, obj=None): class ExportWinsAdminMixin: def has_module_permission(self, request): user = request.user - if user.groups.filter(name='ExportWinAdmin').exists(): + print(user.is_superuser) + if not user.is_superuser and user.groups.filter(name='ExportWinAdmin').exists(): # print('self.opts', self.opts) if self.opts.app_label == 'export_win': return True + print('******hiding permission in has_module_permission for ', self.opts) return False return super().has_module_permission(request) @@ -134,9 +136,10 @@ def has_change_permission(self, request, obj=None): def _handle_export_wins_admin_permissions(self, request, model_name, function): user = request.user # print('self.opts.model_name', model_name) - if user.groups.filter(name='ExportWinAdmin').exists(): + if not user.is_superuser and user.groups.filter(name='ExportWinAdmin').exists(): if model_name == 'win': return True + print('******hiding permission in _handle_export_wins_admin_permissions for ', model_name) return False return function @@ -422,13 +425,14 @@ def pretty_source(self, obj): def _make_admin_permission_getter(codename): def _has_permission(self, request, obj=None): - if request.user.groups.filter(name='ExportWinAdmin').exists(): - return False - print('****self.opts', self.opts) - print('****_make_admin_permission_getter._has_permission') - print('****_make_admin_permission_getter._has_permission.request.user', request.user) + app_label = self.opts.app_label qualified_name = f'{app_label}.{codename}' + if not request.user.is_superuser and request.user.groups.filter(name='ExportWinAdmin').exists(): + if self.opts.app_label == 'export_win': + return True + print('******hiding permission in _make_admin_permission_getter for ', app_label) + return False return request.user.has_perm(qualified_name) return _has_permission From aecb886d4b9177d5d501d5168da57227826d96c0 Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Wed, 3 Apr 2024 09:53:06 +0100 Subject: [PATCH 3/6] refactor into single function --- datahub/core/admin.py | 78 +++++++------------- datahub/investment/investor_profile/admin.py | 1 + datahub/investment/opportunity/admin.py | 1 + 3 files changed, 27 insertions(+), 53 deletions(-) diff --git a/datahub/core/admin.py b/datahub/core/admin.py index 0a15ac3dd5..d1424b1175 100644 --- a/datahub/core/admin.py +++ b/datahub/core/admin.py @@ -77,69 +77,39 @@ def has_change_permission(self, request, obj=None): class ExportWinsAdminMixin: def has_module_permission(self, request): - user = request.user - print(user.is_superuser) - if not user.is_superuser and user.groups.filter(name='ExportWinAdmin').exists(): - # print('self.opts', self.opts) - if self.opts.app_label == 'export_win': - return True - print('******hiding permission in has_module_permission for ', self.opts) - return False - return super().has_module_permission(request) + return self._handle_export_wins_admin_permissions( + request.user, self.opts.app_label, super().has_module_permission(request), + ) def has_view_permission(self, request, obj=None): - # print('******has_view_permission, request.user', request.user) - user = request.user - return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_view_permission(request, obj)) - - if user.groups.filter(name='ExportWinAdmin').exists(): - print('self.opts.model_name', self.opts.model_name) - if self.opts.model_name == 'win': - return True - return False - return super().has_view_permission(request, obj) + return self._handle_export_wins_admin_permissions( + request.user, self.opts.app_label, super().has_view_permission(request, obj), + ) def has_add_permission(self, request): - # print('******has_add_permission, request.user', request.user) - user = request.user - return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_add_permission(request)) - if user.groups.filter(name='ExportWinAdmin').exists(): - print('self.opts.model_name', self.opts.model_name) - if self.opts.model_name == 'win': - return True - return False - return super().has_add_permission(request) + return self._handle_export_wins_admin_permissions( + request.user, self.opts.app_label, super().has_add_permission(request), + ) def has_delete_permission(self, request, obj=None): - # print('******has_delete_permission, request.user', request.user) - user = request.user - return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_delete_permission(request, obj)) - if user.groups.filter(name='ExportWinAdmin').exists(): - print('self.opts.model_name', self.opts.model_name) - if self.opts.model_name == 'win': - return True - return False - return super().has_delete_permission(request, obj) + return self._handle_export_wins_admin_permissions( + request.user, + self.opts.app_label, + super().has_delete_permission(request, obj), + ) def has_change_permission(self, request, obj=None): - # print('******has_change_permission, request.user', request.user) - user = request.user - return self._handle_export_wins_admin_permissions(request, self.opts.model_name, super().has_change_permission(request, obj)) - if user.groups.filter(name='ExportWinAdmin').exists(): - print('self.opts.model_name', self.opts.model_name) - if self.opts.model_name == 'win': - return True - return False - return super().has_change_permission(request, obj) + return self._handle_export_wins_admin_permissions( + request.user, + self.opts.app_label, + super().has_change_permission(request, obj), + ) - def _handle_export_wins_admin_permissions(self, request, model_name, function): - user = request.user - # print('self.opts.model_name', model_name) + def _handle_export_wins_admin_permissions(self, user, app_label, function): if not user.is_superuser and user.groups.filter(name='ExportWinAdmin').exists(): - if model_name == 'win': + if app_label == 'export_win': return True - print('******hiding permission in _handle_export_wins_admin_permissions for ', model_name) return False return function @@ -428,10 +398,12 @@ def _has_permission(self, request, obj=None): app_label = self.opts.app_label qualified_name = f'{app_label}.{codename}' - if not request.user.is_superuser and request.user.groups.filter(name='ExportWinAdmin').exists(): + if ( + not request.user.is_superuser + and request.user.groups.filter(name='ExportWinAdmin').exists() + ): if self.opts.app_label == 'export_win': return True - print('******hiding permission in _make_admin_permission_getter for ', app_label) return False return request.user.has_perm(qualified_name) diff --git a/datahub/investment/investor_profile/admin.py b/datahub/investment/investor_profile/admin.py index 2c00f4c5b7..e3882836f9 100644 --- a/datahub/investment/investor_profile/admin.py +++ b/datahub/investment/investor_profile/admin.py @@ -1,4 +1,5 @@ from django.contrib import admin + from datahub.core.admin import ExportWinsAdminMixin from datahub.investment.investor_profile import models diff --git a/datahub/investment/opportunity/admin.py b/datahub/investment/opportunity/admin.py index 16b47345f8..47498e48bb 100644 --- a/datahub/investment/opportunity/admin.py +++ b/datahub/investment/opportunity/admin.py @@ -1,4 +1,5 @@ from django.contrib import admin + from datahub.core.admin import ExportWinsAdminMixin from datahub.investment.opportunity import models From a9c4d99f64e8b526364893e7412b3837302d4d48 Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Wed, 3 Apr 2024 10:30:20 +0100 Subject: [PATCH 4/6] added unit tests --- datahub/core/admin.py | 53 +++++++++++++++++---------------- datahub/core/test/test_admin.py | 45 ++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 25 deletions(-) diff --git a/datahub/core/admin.py b/datahub/core/admin.py index d1424b1175..58d21ed7ed 100644 --- a/datahub/core/admin.py +++ b/datahub/core/admin.py @@ -77,43 +77,40 @@ def has_change_permission(self, request, obj=None): class ExportWinsAdminMixin: def has_module_permission(self, request): - return self._handle_export_wins_admin_permissions( - request.user, self.opts.app_label, super().has_module_permission(request), + return handle_export_wins_admin_permissions( + request.user, + self.opts.app_label, + super().has_module_permission(request), ) def has_view_permission(self, request, obj=None): - - return self._handle_export_wins_admin_permissions( - request.user, self.opts.app_label, super().has_view_permission(request, obj), + return handle_export_wins_admin_permissions( + request.user, + self.opts.app_label, + super().has_view_permission(request, obj), ) def has_add_permission(self, request): - return self._handle_export_wins_admin_permissions( - request.user, self.opts.app_label, super().has_add_permission(request), + return handle_export_wins_admin_permissions( + request.user, + self.opts.app_label, + super().has_add_permission(request), ) def has_delete_permission(self, request, obj=None): - return self._handle_export_wins_admin_permissions( + return handle_export_wins_admin_permissions( request.user, self.opts.app_label, super().has_delete_permission(request, obj), ) def has_change_permission(self, request, obj=None): - return self._handle_export_wins_admin_permissions( + return handle_export_wins_admin_permissions( request.user, self.opts.app_label, super().has_change_permission(request, obj), ) - def _handle_export_wins_admin_permissions(self, user, app_label, function): - if not user.is_superuser and user.groups.filter(name='ExportWinAdmin').exists(): - if app_label == 'export_win': - return True - return False - - return function - class BaseModelAdminMixin(ExportWinsAdminMixin): """ @@ -393,18 +390,24 @@ def pretty_source(self, obj): return format_html('
{0}
', json.dumps(value, indent=2)) +def handle_export_wins_admin_permissions(user, app_label, function): + if not user.is_superuser and user.groups.filter(name='ExportWinAdmin').exists(): + if app_label == 'export_win': + return True + return False + + return function + + def _make_admin_permission_getter(codename): def _has_permission(self, request, obj=None): app_label = self.opts.app_label qualified_name = f'{app_label}.{codename}' - if ( - not request.user.is_superuser - and request.user.groups.filter(name='ExportWinAdmin').exists() - ): - if self.opts.app_label == 'export_win': - return True - return False - return request.user.has_perm(qualified_name) + return handle_export_wins_admin_permissions( + request.user, + app_label, + request.user.has_perm(qualified_name), + ) return _has_permission diff --git a/datahub/core/test/test_admin.py b/datahub/core/test/test_admin.py index 38f4b09d36..6f41dcbedb 100644 --- a/datahub/core/test/test_admin.py +++ b/datahub/core/test/test_admin.py @@ -2,6 +2,7 @@ from unittest.mock import Mock import pytest + from django.conf import settings from django.contrib import auth, messages as django_messages from django.contrib.admin.templatetags.admin_urls import admin_urlname @@ -11,6 +12,7 @@ from faker import Faker from rest_framework import status +from datahub.company.test.factories import AdviserFactory from datahub.core.admin import ( custom_add_permission, custom_change_permission, @@ -19,8 +21,10 @@ format_json_as_html, get_change_link, get_change_url, + handle_export_wins_admin_permissions, RawIdWidget, ) +from datahub.core.test.factories import GroupFactory from datahub.core.test.support.factories import BookFactory from datahub.core.test.support.models import Book from datahub.core.test.support.views import MAX_UPLOAD_SIZE @@ -338,3 +342,44 @@ def test_admin_account_lock_out_after_too_many_attempts(self): status.HTTP_403_FORBIDDEN ), (attempt, settings.AXES_FAILURE_LIMIT) assert auth.get_user(client).is_authenticated is False + + +@pytest.mark.django_db +class TestHandleExportWinsAdminPermissions: + def test_user_is_not_superuser_not_in_export_wins_group(self): + permission_group = GroupFactory(name='Group') + user = AdviserFactory(is_superuser=False) + user.groups.add(permission_group) + expected_response = 'ABC' + + result = handle_export_wins_admin_permissions(user, '', function=expected_response) + + assert result == expected_response + + def test_user_is_superuser_in_export_wins_group(self): + permission_group = GroupFactory(name='ExportWinAdmin') + user = AdviserFactory(is_superuser=True) + user.groups.add(permission_group) + expected_response = 'ABC' + + result = handle_export_wins_admin_permissions(user, '', function=expected_response) + + assert result == expected_response + + def test_user_is_not_superuser_in_export_wins_group_accessing_export_win_module(self): + permission_group = GroupFactory(name='ExportWinAdmin') + user = AdviserFactory(is_superuser=False) + user.groups.add(permission_group) + + result = handle_export_wins_admin_permissions(user, 'export_win', function=None) + + assert result is True + + def test_user_is_not_superuser_in_export_wins_group_accessing_company_module(self): + permission_group = GroupFactory(name='ExportWinAdmin') + user = AdviserFactory(is_superuser=False) + user.groups.add(permission_group) + + result = handle_export_wins_admin_permissions(user, 'company', function=None) + + assert result is False From 2880255ff4500d6eba7ff15699ce0167f09a488e Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Wed, 3 Apr 2024 10:54:00 +0100 Subject: [PATCH 5/6] extend modeladmin --- datahub/core/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datahub/core/admin.py b/datahub/core/admin.py index 58d21ed7ed..7d5b769e2f 100644 --- a/datahub/core/admin.py +++ b/datahub/core/admin.py @@ -75,7 +75,7 @@ def has_change_permission(self, request, obj=None): return False -class ExportWinsAdminMixin: +class ExportWinsAdminMixin(admin.ModelAdmin): def has_module_permission(self, request): return handle_export_wins_admin_permissions( request.user, From 889c6cae0b19d69e7550847c220f4ede6a2a9511 Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Wed, 3 Apr 2024 11:31:17 +0100 Subject: [PATCH 6/6] added constant for export wins name --- datahub/core/admin.py | 4 +++- datahub/export_win/admin.py | 8 +++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/datahub/core/admin.py b/datahub/core/admin.py index 7d5b769e2f..8bb274e92a 100644 --- a/datahub/core/admin.py +++ b/datahub/core/admin.py @@ -15,6 +15,8 @@ from django.utils.translation import gettext, gettext_lazy from django.views.decorators.csrf import csrf_exempt, csrf_protect +EXPORT_WIN_GROUP_NAME = 'ExportWinAdmin' + class DisabledOnFilter(admin.SimpleListFilter): """This filter allows us to filter values that have disabled_on value.""" @@ -391,7 +393,7 @@ def pretty_source(self, obj): def handle_export_wins_admin_permissions(user, app_label, function): - if not user.is_superuser and user.groups.filter(name='ExportWinAdmin').exists(): + if not user.is_superuser and user.groups.filter(name=EXPORT_WIN_GROUP_NAME).exists(): if app_label == 'export_win': return True return False diff --git a/datahub/export_win/admin.py b/datahub/export_win/admin.py index c723d642c3..67e9d8837c 100644 --- a/datahub/export_win/admin.py +++ b/datahub/export_win/admin.py @@ -4,8 +4,7 @@ from django.forms import ModelForm from reversion.admin import VersionAdmin - -from datahub.core.admin import BaseModelAdminMixin +from datahub.core.admin import BaseModelAdminMixin, EXPORT_WIN_GROUP_NAME from datahub.export_win.models import Breakdown, CustomerResponse, DeletedWin, Win, WinAdviser @@ -265,6 +264,9 @@ def has_change_permission(self, request, obj=None): def has_view_permission(self, request, obj=None): """Set the desired user group to access view deleted win""" - if request.user.is_superuser or request.user.groups.filter(name='ExportWinAdmin').exists(): + if ( + request.user.is_superuser + or request.user.groups.filter(name=EXPORT_WIN_GROUP_NAME).exists() + ): return True return False