Close security loopholes to make non-superuser admins usable

allianceauth/authentication/admin.py

@@ -1,26 +1,44 @@
 from django.contrib import admin
 from django.contrib.auth.admin import UserAdmin as BaseUserAdmin
-from django.contrib.auth.models import User as BaseUser, \
-    Permission as BasePermission, Group
+from django.contrib.auth.models import Group
+from django.contrib.auth.models import Permission as BasePermission
+from django.contrib.auth.models import User as BaseUser
 from django.db.models import Count, Q
-from allianceauth.services.hooks import ServicesHook
-from django.db.models.signals import pre_save, post_save, pre_delete, \
-    post_delete, m2m_changed
 from django.db.models.functions import Lower
+from django.db.models.signals import (
+    m2m_changed,
+    post_delete,
+    post_save,
+    pre_delete,
+    pre_save
+)
 from django.dispatch import receiver
-from django.forms import ModelForm
-from django.utils.html import format_html
 from django.urls import reverse
+from django.utils.html import format_html
 from django.utils.text import slugify
 
-from allianceauth.authentication.models import State, get_guest_state,\
-    CharacterOwnership, UserProfile, OwnershipRecord
-from allianceauth.hooks import get_hooks
-from allianceauth.eveonline.models import EveCharacter, EveCorporationInfo,\
-    EveAllianceInfo, EveFactionInfo
+from allianceauth.authentication.models import (
+    CharacterOwnership,
+    OwnershipRecord,
+    State,
+    UserProfile,
+    get_guest_state
+)
+from allianceauth.eveonline.models import (
+    EveAllianceInfo,
+    EveCharacter,
+    EveCorporationInfo,
+    EveFactionInfo
+)
 from allianceauth.eveonline.tasks import update_character
-from .app_settings import AUTHENTICATION_ADMIN_USERS_MAX_GROUPS, \
-    AUTHENTICATION_ADMIN_USERS_MAX_CHARS
+from allianceauth.hooks import get_hooks
+from allianceauth.services.hooks import ServicesHook
+
+from .app_settings import (
+    AUTHENTICATION_ADMIN_USERS_MAX_CHARS,
+    AUTHENTICATION_ADMIN_USERS_MAX_GROUPS
+)
+from .forms import UserChangeForm, UserProfileForm
 
 
 def make_service_hooks_update_groups_action(service):
@@ -59,19 +77,10 @@ def make_service_hooks_sync_nickname_action(service):
     return sync_nickname
 
 
-class QuerysetModelForm(ModelForm):
-    # allows specifying FK querysets through kwarg
-    def __init__(self, querysets=None, *args, **kwargs):
-        querysets = querysets or {}
-        super().__init__(*args, **kwargs)
-        for field, qs in querysets.items():
-            self.fields[field].queryset = qs
-
-
 class UserProfileInline(admin.StackedInline):
     model = UserProfile
     readonly_fields = ('state',)
-    form = QuerysetModelForm
+    form = UserProfileForm
     verbose_name = ''
     verbose_name_plural = 'Profile'
 
@@ -99,6 +108,7 @@ class UserProfileInline(admin.StackedInline):
         return False
 
 
+@admin.display(description="")
 def user_profile_pic(obj):
     """profile pic column data for user objects
 
@@ -111,13 +121,10 @@ def user_profile_pic(obj):
             '<img src="{}" class="img-circle">',
             user_obj.profile.main_character.portrait_url(size=32)
         )
-    else:
-        return None
-
-
-user_profile_pic.short_description = ''
+    return None
 
 
+@admin.display(description="user / main", ordering="username")
 def user_username(obj):
     """user column data for user objects
 
@@ -139,18 +146,17 @@ def user_username(obj):
             user_obj.username,
             user_obj.profile.main_character.character_name
         )
-    else:
-        return format_html(
-            '<strong><a href="{}">{}</a></strong>',
-            link,
-            user_obj.username,
-        )
-
-
-user_username.short_description = 'user / main'
-user_username.admin_order_field = 'username'
+    return format_html(
+        '<strong><a href="{}">{}</a></strong>',
+        link,
+        user_obj.username,
+    )
 
 
+@admin.display(
+    description="Corporation / Alliance (Main)",
+    ordering="profile__main_character__corporation_name"
+)
 def user_main_organization(obj):
     """main organization column data for user objects
 
@@ -159,21 +165,15 @@ def user_main_organization(obj):
     """
     user_obj = obj.user if hasattr(obj, 'user') else obj
     if not user_obj.profile.main_character:
-        result = ''
-    else:
-        result = user_obj.profile.main_character.corporation_name
-        if user_obj.profile.main_character.alliance_id:
-            result += f'<br>{user_obj.profile.main_character.alliance_name}'
-        elif user_obj.profile.main_character.faction_name:
-            result += f'<br>{user_obj.profile.main_character.faction_name}'
+        return ''
+    result = user_obj.profile.main_character.corporation_name
+    if user_obj.profile.main_character.alliance_id:
+        result += f'<br>{user_obj.profile.main_character.alliance_name}'
+    elif user_obj.profile.main_character.faction_name:
+        result += f'<br>{user_obj.profile.main_character.faction_name}'
     return format_html(result)
 
 
-user_main_organization.short_description = 'Corporation / Alliance (Main)'
-user_main_organization.admin_order_field = \
-    'profile__main_character__corporation_name'
-
-
 class MainCorporationsFilter(admin.SimpleListFilter):
     """Custom filter to filter on corporations from mains only
 
@@ -196,15 +196,13 @@ class MainCorporationsFilter(admin.SimpleListFilter):
     def queryset(self, request, qs):
         if self.value() is None:
             return qs.all()
-        else:
-            if qs.model == User:
-                return qs.filter(
-                    profile__main_character__corporation_id=self.value()
-                )
-            else:
-                return qs.filter(
-                    user__profile__main_character__corporation_id=self.value()
-                )
+        if qs.model == User:
+            return qs.filter(
+                profile__main_character__corporation_id=self.value()
+            )
+        return qs.filter(
+            user__profile__main_character__corporation_id=self.value()
+        )
 
 
 class MainAllianceFilter(admin.SimpleListFilter):
@@ -217,12 +215,14 @@ class MainAllianceFilter(admin.SimpleListFilter):
     parameter_name = 'main_alliance_id__exact'
 
     def lookups(self, request, model_admin):
-        qs = EveCharacter.objects\
-            .exclude(alliance_id=None)\
-            .exclude(userprofile=None)\
-            .values('alliance_id', 'alliance_name')\
-            .distinct()\
+        qs = (
+            EveCharacter.objects
+            .exclude(alliance_id=None)
+            .exclude(userprofile=None)
+            .values('alliance_id', 'alliance_name')
+            .distinct()
             .order_by(Lower('alliance_name'))
+        )
         return tuple(
             (x['alliance_id'], x['alliance_name']) for x in qs
         )
@@ -230,13 +230,11 @@ class MainAllianceFilter(admin.SimpleListFilter):
     def queryset(self, request, qs):
         if self.value() is None:
             return qs.all()
-        else:
-            if qs.model == User:
-                return qs.filter(profile__main_character__alliance_id=self.value())
-            else:
-                return qs.filter(
-                    user__profile__main_character__alliance_id=self.value()
-                )
+        if qs.model == User:
+            return qs.filter(profile__main_character__alliance_id=self.value())
+        return qs.filter(
+            user__profile__main_character__alliance_id=self.value()
+        )
 
 
 class MainFactionFilter(admin.SimpleListFilter):
@@ -249,12 +247,14 @@ class MainFactionFilter(admin.SimpleListFilter):
     parameter_name = 'main_faction_id__exact'
 
     def lookups(self, request, model_admin):
-        qs = EveCharacter.objects\
-            .exclude(faction_id=None)\
-            .exclude(userprofile=None)\
-            .values('faction_id', 'faction_name')\
-            .distinct()\
+        qs = (
+            EveCharacter.objects
+            .exclude(faction_id=None)
+            .exclude(userprofile=None)
+            .values('faction_id', 'faction_name')
+            .distinct()
             .order_by(Lower('faction_name'))
+        )
         return tuple(
             (x['faction_id'], x['faction_name']) for x in qs
         )
@@ -262,15 +262,14 @@ class MainFactionFilter(admin.SimpleListFilter):
     def queryset(self, request, qs):
         if self.value() is None:
             return qs.all()
-        else:
-            if qs.model == User:
-                return qs.filter(profile__main_character__faction_id=self.value())
-            else:
-                return qs.filter(
-                    user__profile__main_character__faction_id=self.value()
-                )
+        if qs.model == User:
+            return qs.filter(profile__main_character__faction_id=self.value())
+        return qs.filter(
+            user__profile__main_character__faction_id=self.value()
+        )
 
 
+@admin.display(description="Update main character model from ESI")
 def update_main_character_model(modeladmin, request, queryset):
     tasks_count = 0
     for obj in queryset:
@@ -279,21 +278,48 @@ def update_main_character_model(modeladmin, request, queryset):
             tasks_count += 1
 
     modeladmin.message_user(
-        request,
-        f'Update from ESI started for {tasks_count} characters'
+        request, f'Update from ESI started for {tasks_count} characters'
     )
 
 
-update_main_character_model.short_description = \
-    'Update main character model from ESI'
-
-
 class UserAdmin(BaseUserAdmin):
     """Extending Django's UserAdmin model
 
     Behavior of groups and characters columns can be configured via settings
     """
 
+    inlines = BaseUserAdmin.inlines + [UserProfileInline]
+    ordering = ('username', )
+    list_select_related = ('profile__state', 'profile__main_character')
+    show_full_result_count = True
+    list_display = (
+        user_profile_pic,
+        user_username,
+        '_state',
+        '_groups',
+        user_main_organization,
+        '_characters',
+        'is_active',
+        'date_joined',
+        '_role'
+    )
+    list_display_links = None
+    list_filter = (
+        'profile__state',
+        'groups',
+        MainCorporationsFilter,
+        MainAllianceFilter,
+        MainFactionFilter,
+        'is_active',
+        'date_joined',
+        'is_staff',
+        'is_superuser'
+    )
+    search_fields = ('username', 'character_ownerships__character__character_name')
+    readonly_fields = ('date_joined', 'last_login')
+    filter_horizontal = ('groups', 'user_permissions',)
+    form = UserChangeForm
+
     class Media:
         css = {
             "all": ("authentication/css/admin.css",)
@@ -303,9 +329,21 @@ class UserAdmin(BaseUserAdmin):
         qs = super().get_queryset(request)
         return qs.prefetch_related("character_ownerships__character", "groups")
 
-    def get_actions(self, request):
-        actions = super(BaseUserAdmin, self).get_actions(request)
+    def get_form(self, request, obj=None, **kwargs):
+        """Inject current request into change form object."""
 
+        MyForm = super().get_form(request, obj, **kwargs)
+        if obj:
+            class MyFormInjected(MyForm):
+                def __new__(cls, *args, **kwargs):
+                    kwargs['request'] = request
+                    return MyForm(*args, **kwargs)
+
+            return MyFormInjected
+        return MyForm
+
+    def get_actions(self, request):
+        actions = super().get_actions(request)
         actions[update_main_character_model.__name__] = (
             update_main_character_model,
             update_main_character_model.__name__,
@@ -349,39 +387,6 @@ class UserAdmin(BaseUserAdmin):
             )
         return result
 
-    inlines = BaseUserAdmin.inlines + [UserProfileInline]
-    ordering = ('username', )
-    list_select_related = ('profile__state', 'profile__main_character')
-    show_full_result_count = True
-    list_display = (
-        user_profile_pic,
-        user_username,
-        '_state',
-        '_groups',
-        user_main_organization,
-        '_characters',
-        'is_active',
-        'date_joined',
-        '_role'
-    )
-    list_display_links = None
-    list_filter = (
-        'profile__state',
-        'groups',
-        MainCorporationsFilter,
-        MainAllianceFilter,
-        MainFactionFilter,
-        'is_active',
-        'date_joined',
-        'is_staff',
-        'is_superuser'
-    )
-    search_fields = (
-        'username',
-        'character_ownerships__character__character_name'
-    )
-    readonly_fields = ('date_joined', 'last_login')
-
     def _characters(self, obj):
         character_ownerships = list(obj.character_ownerships.all())
         characters = [obj.character.character_name for obj in character_ownerships]
@@ -390,22 +395,16 @@ class UserAdmin(BaseUserAdmin):
             AUTHENTICATION_ADMIN_USERS_MAX_CHARS
         )
 
-    _characters.short_description = 'characters'
-
+    @admin.display(ordering="profile__state")
     def _state(self, obj):
         return obj.profile.state.name
 
-    _state.short_description = 'state'
-    _state.admin_order_field = 'profile__state'
-
     def _groups(self, obj):
         my_groups = sorted(group.name for group in list(obj.groups.all()))
         return self._list_2_html_w_tooltips(
             my_groups, AUTHENTICATION_ADMIN_USERS_MAX_GROUPS
         )
 
-    _groups.short_description = 'groups'
-
     def _role(self, obj):
         if obj.is_superuser:
             role = 'Superuser'
@@ -415,8 +414,6 @@ class UserAdmin(BaseUserAdmin):
             role = 'User'
         return role
 
-    _role.short_description = 'role'
-
     def has_change_permission(self, request, obj=None):
         return request.user.has_perm('auth.change_user')
 
@@ -438,9 +435,16 @@ class UserAdmin(BaseUserAdmin):
             if obj_state:
                 matching_groups_qs = Group.objects.filter(authgroup__states=obj_state)
                 groups_qs = groups_qs | matching_groups_qs
-            kwargs["queryset"] = groups_qs.order_by(Lower('name'))
+            kwargs["queryset"] = groups_qs.order_by(Lower("name"))
         return super().formfield_for_manytomany(db_field, request, **kwargs)
 
+    def get_readonly_fields(self, request, obj=None):
+        if obj and not request.user.is_superuser:
+            return self.readonly_fields + (
+                "is_staff", "is_superuser", "user_permissions"
+            )
+        return self.readonly_fields
+
 
 @admin.register(State)
 class StateAdmin(admin.ModelAdmin):
@@ -451,10 +455,9 @@ class StateAdmin(admin.ModelAdmin):
         qs = super().get_queryset(request)
         return qs.annotate(user_count=Count("userprofile__id"))
 
+    @admin.display(description="Users", ordering="user_count")
     def _user_count(self, obj):
         return obj.user_count
-    _user_count.short_description = 'Users'
-    _user_count.admin_order_field = 'user_count'
 
     fieldsets = (
         (None, {
@@ -510,13 +513,13 @@ class StateAdmin(admin.ModelAdmin):
             )
         return super().get_fieldsets(request, obj=obj)
 
+    def get_readonly_fields(self, request, obj=None):
+        if not request.user.is_superuser:
+            return self.readonly_fields + ("permissions",)
+        return self.readonly_fields
+
 
 class BaseOwnershipAdmin(admin.ModelAdmin):
-    class Media:
-        css = {
-            "all": ("authentication/css/admin.css",)
-        }
-
     list_select_related = (
         'user__profile__state', 'user__profile__main_character', 'character')
     list_display = (
@@ -537,6 +540,11 @@ class BaseOwnershipAdmin(admin.ModelAdmin):
         MainAllianceFilter,
     )
 
+    class Media:
+        css = {
+            "all": ("authentication/css/admin.css",)
+        }
+
     def get_readonly_fields(self, request, obj=None):
         if obj and obj.pk:
             return 'owner_hash', 'character'