Access to request object in register_user_listing_buttons

[tomusher]

Is your proposal related to a problem?

In Wagtail 6.1, the register_user_listing_buttons hook signature changed to remove the context.

In migrating one of my sites to 6.1, I realised that one of the the buttons I add to the user listing is an 'Impersonate' button, making use of django-hijack to impersonate a specific user.

This button is implemented like:

class HijackFormButton(UserListingButton):
    def __init__(self, *, user_pk, request):
        self.user_pk = user_pk
        self.request = request
        super().__init__(
            "Impersonate",
            None,
            priority=10,
        )

    def render(self):
        csrf_token = csrf.get_token(self.request)
        return f"""
        <form method="POST" action="{reverse("hijack:acquire")}">
            <input type="hidden" name="csrfmiddlewaretoken" value="{csrf_token}">
            <input type="hidden" name="user_pk" value="{self.user_pk}">
            <button class="button button-secondary button-small" title="Login as this user">{self.label}</button>
        </form>
        """

@hooks.register("register_user_listing_buttons")
def user_listing_hijack_button(context, user):
    yield HijackFormButton(
        user_pk=user.pk,
        request=context.request,
    )

Where the request object is used to generate a CSRF token for the form submission.

With the new hook signature, I don't have a clean way to make the request object available to the button.

I'm wondering what the justification for removing access to the request was, and whether there's any reason it couldn't be added back?

Describe the solution you'd like

Adding a request parameter to the register_user_listing_buttons hook.

Alternatively, a hook for adding a common context to all buttons might be useful - in the above example this could be used to generate a single CSRF token that could be reused between instances, and may be useful for performance optimisations in cases where buttons need to retrieve additional data.

Working on this

I'm happy to work on this once I understand the reason it was removed in the first place to avoid repeating or reintroducing any issues that this caused.

[zerolab]

The change happened in #11823

[laymonage]

Thanks for the report, and sorry for the trouble! Here's my investigation results.

Rationale

The idea was that I wanted to allow the UserViewSet to be customised, just like the GroupViewSet. From there, overriding the listing buttons would be done by creating a custom subclass of the view and overriding the appropriate methods, e.g.

wagtail/wagtail/admin/views/generic/models.py

Line 375 in 538365f

def get_list_more_buttons(self, instance):

Then, the custom view subclass would be specified in the custom viewset. This would align better with how we're moving towards the viewsets being the one place to configure your views, instead of individual hooks. It also encourages working with the view directly to do any logic instead of treating the context as a grab-bag and relying on whatever is in it.

Another reason is that, none of the other register_{foo}_buttons (i.e. register_snippet_listing_buttons, register_page_listing_buttons, register_page_listing_more_buttons, register_page_header_buttons) hooks give you the context. Apart from a few small differences (e.g. view_name next_url), they all have one thing in common: passing the row's instance as the first argument, and the request's user as the second. The hook was never documented until #9162 and even after that PR, it had no tests, so I opted to remove the context support in favour of making it more consistent with the other hooks.

In the future, I'm imagining a dedicated/expanded section in the Wagtail docs on how to work with and customise Wagtail's generic views and viewsets, so that it includes things like customising the listing view buttons, and the header buttons (the ones you see in the breadcrumbs). Ideally, the guide would be applicable to all views that are customisable, including ModelViewSet, SnippetViewSet, etc.

Unfortunately, we were short on time and I didn't get to implement the ability to customise the UserViewSet before the feature freeze.

Alternatively, a hook for adding a common context to all buttons might be useful - in the above example this could be used to generate a single CSRF token that could be reused between instances, and may be useful for performance optimisations in cases where buttons need to retrieve additional data.

With the move towards using Component classes (now extracted into the laces library), I don't think this is something we want to do (read https://github.com/tbrlpld/laces#using-the-parent-context for more, um, context).

The preferred way would be to pass anything you need in the button directly to the button's constructor, which makes it more explicit compared to accessing the context (which may or may not have what you want, e.g. due to the use of only keyword when using {% include %} in the templates). Moving the button customisation to the view allows you to pass any information available in the view to the button more easily and explicitly.

Reinstating support for the given customisation

Anyway... back to the issue at hand. I couldn't get the provided example to work as expected in 6.0.x nor 5.2.x. Which version was the latest where this still worked @tomusher?

To make it work, I had to change render to render_html (accepting a parent_context argument) and wrapping the return value in mark_safe():

class HijackFormButton(UserListingButton):
    def __init__(self, *, user_pk, request):
        self.user_pk = user_pk
        self.request = request
        super().__init__(
            "Impersonate",
            None,
            priority=10,
        )

    def render_html(self, parent_context):
        csrf_token = get_token(self.request)
        return mark_safe(
            f"""
        <form method="POST" action="{reverse("hijack:acquire")}">
            <input type="hidden" name="csrfmiddlewaretoken" value="{csrf_token}">
            <input type="hidden" name="user_pk" value="{self.user_pk}">
            <button class="button button-secondary button-small" title="Login as this user">{self.label}</button>
        </form>
        """
        )


@hooks.register("register_user_listing_buttons")
def user_listing_hijack_button(context, user):
    yield HijackFormButton(
        user_pk=user.pk,
        request=context.request,
    )

After the above changes, the hook works as expected on 5.2.x and 6.0.x.

With that out of the way, there are a few things that unfortunately make it tricky to fix this in a 6.1.x patch release with minimal impact. I'll separate this into two steps.

1. Ensuring each button can define how it can render itself

The BaseDropdownMenuButton we use for rendering the ... button in listing views unfortunately render the buttons using a template include directly instead of rendering each Button instance as a component (with {% component %}):

wagtail/wagtail/admin/widgets/button.py

Lines 170 to 171 in 8aaa579

	class BaseDropdownMenuButton(Button):
	template_name = "wagtailadmin/pages/listing/_button_with_dropdown.html"

wagtail/wagtail/admin/templates/wagtailadmin/pages/listing/_button_with_dropdown.html

Line 3 in 8aaa579

{% include "wagtailadmin/pages/listing/_dropdown_items.html" with buttons=buttons only %}

wagtail/wagtail/admin/templates/wagtailadmin/pages/listing/_dropdown_items.html

Lines 3 to 10 in 8aaa579

	{% for button in buttons %}
	<a href="{{ button.url }}" {{ button.base_attrs_string }}>
	{% if button.icon_name %}
	{% icon name=button.icon_name %}
	{% endif %}
	{{ button.label }}
	</a>
	{% endfor %}

This means that the Button's render_html (or the "deprecated" render() like in your example) isn't used in the rendering process.

We need to fix this if we want to allow a button to have its own markup. This potentially affects the page listing buttons too, so I'd be wary to do it in a patch release. Might be OK if we're extra careful, though.

2. Passing the request

The hook is now called in the view before the context is finalised by get_context_data:

wagtail/wagtail/users/views/users.py

Lines 200 to 236 in 8aec935

	def get_list_buttons(self, instance):
	more_buttons = self.get_list_more_buttons(instance)
	list_buttons = []
	for hook in hooks.get_hooks("register_user_listing_buttons"):
	if accepts_kwarg(hook, "request_user"):
	hook_buttons = hook(user=instance, request_user=self.request.user)
	else:
	# old-style hook that accepts a context argument instead of request_user
	hook_buttons = hook(RequestContext(self.request), instance)
	warn(
	"`register_user_listing_buttons` hook functions should accept a `request_user` argument instead of `context` -"
	f" {hook.__module__}.{hook.__name__} needs to be updated",
	category=RemovedInWagtail70Warning,
	)
	for button in hook_buttons:
	if isinstance(button, BaseDropdownMenuButton):
	# If the button is a dropdown menu, add it to the top-level
	# because we do not support nested dropdowns
	list_buttons.append(button)
	else:
	# Otherwise, add it to the default "More" dropdown
	more_buttons.append(button)
	list_buttons.append(
	ButtonWithDropdown(
	buttons=sorted(more_buttons),
	icon_name="dots-horizontal",
	attrs={
	"aria-label": _("More options for '%(title)s'")
	% {"title": str(instance)},
	},
	)
	)
	return sorted(list_buttons)

As such, we're no longer able to give the full context to the hook. However, we can still include the request. In fact, we pass a RequestContext to support the old signature during the deprecation process, as seen in the code above.

This means that with the fix for (1), the customisation will continue to work using the old hook signature (since request is all you need from the context), but you'll get a deprecation warning due to the signature change.

If we want to use the new signature for the hook, we can't retrieve the request from the parent_context argument in Component.get_context_data/Component.render_html because the "parent" BaseDropdownMenuButton component only specifies the following context:

wagtail/wagtail/admin/widgets/button.py

Lines 180 to 187 in 8aaa579

	def get_context_data(self, parent_context):
	return {
	"buttons": sorted(self.dropdown_buttons),
	"label": self.label,
	"title": self.aria_label,
	"toggle_classname": self.classname,
	"icon_name": self.icon_name,
	}

Option A: pass the request via parent_context

If we want the individual child button to have access to the request in the context, we need to include it in the parent BaseDropdownMenuButton's context. So if we add something like "request": parent_context.get("request") to the get_context_data() above, we can achieve the customisation with the following:

class HijackFormButton(UserListingButton):
    def __init__(self, *, user_pk):
        self.user_pk = user_pk
        super().__init__("Impersonate", None, priority=10)

    def render_html(self, parent_context):
        csrf_token = get_token(parent_context.get("request"))
        return mark_safe(
            f"""
        <form method="POST" action="{reverse("hijack:acquire")}">
            <input type="hidden" name="csrfmiddlewaretoken" value="{csrf_token}">
            <input type="hidden" name="user_pk" value="{self.user_pk}">
            <button title="Login as this user">{self.label}</button>
        </form>
        """
        )


@hooks.register("register_user_listing_buttons")
def user_listing_hijack_button(user, request_user):
    yield HijackFormButton(user_pk=user.pk)

I'm not too keen with this, as it goes against the plan of not relying on the parent context. That said, I can accept request as an exception to the rule.

Option B: pass the request directly to the hook

Alternatively, we can also pass the request to the hook directly, either by replacing the request_user parameter, or adding a new parameter. If we do it by replacing request_user, this is a breaking change to a breaking change we already made. If we add a new parameter, it's also going to be a breaking change unless developers update the signature to match (or use *args/**kwargs). We can do this a bit more safely by inspecting the hook's signature with accepts_kwarg or catching any TypeErrors, but that adds noise to the code. Not to mention that this goes against the other register_{foo}_buttons hooks.

Option C: do nothing

Or, as another alternative, we can also skip fixing (2) by saying "if you need the request, you need to use the old signature for now, and disregard the deprecation warning until we have a better way to support the use case". (Or, maybe remove the warning for 6.1 and keep it for 6.2...)

Thoughts @gasman @zerolab?