[Bug 36845] As well as ACLs for DBus calls, we need ACLs to filter which handlers get channels

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=36845

--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-05-04 09:42:40 PDT ---
> +gboolean
> +mcp_dbus_channel_acl_authorised (const TpDBusDaemon *dbus,

This entire function, and also cached_acls(), should be in src/ somewhere. If
you want a wrapper around authorised() in MCP, it should be totally trivial,
just like mcp_dispatch_operation_policy_check().

It occurs to me that a new GInterface for McpDBusChannelAcl (whose name I
dislike anyway, see below) is overkill. Why don't you just add a new method to
McpDispatchOperationPolicy:

    /**
     * Check whether a Handler is eligible to handle the given channels.
     * This is called later than mcp_dispatch_operation_policy_check(),
     * when MC has a particular Handler in mind.
     *
     * Returns: %TRUE if @handler can be considered as a handler for
     *   the channels in @dispatch_op, or %FALSE to disqualify that handler
     */
    gboolean mcp_dispatch_operation_policy_handler_is_available (
        McpDispatchOperationPolicy *policy,
        McpDispatchOperation *dispatch_op,
        TpProxy *handler);

and make your pseudo-plugin implement that method, but not check()?

> + * The contents of the #McpDBusChannelAcl struct are not public,
> + * so to provide an implementation of the virtual methods,
> + * plugins should call mcp_dbus_channel_acl_iface_implement_*()
> + * from the interface initialization function

You don't actually need to do this dance: just show the API user the FooIface
struct and let them fill it. Enlarging a GInterface's FooIface struct is
considered by GObject upstream to be a compatible change, as long as you don't
remove, change or re-order any existing function pointers.

> Implement the Aegis DBus Channel/Handler policy as an ACL plugin

Like the other remarkably similar pseudo-plugin, this should be in plugins/ or
src/.

I don't see any reason why this is a separate object at all - please just have
one Aegis pseudo-plugin object that implements both GInterfaces.

> + creds_t caller = creds_gettask (pid);

AIUI you're now meant to make a call out to some API that has been bolted onto
the Aegis-patched dbus-daemon, to avoid time of check/time of use problems. Use
that instead of GetConnectionUnixProcessID.

> + channels = _mcd_tp_channels_build_from_list (self->priv->channels);

This whole function is unnecessary if you have, and use, a McpDispatchOperation
(which you do - it's accessible as self->priv->plugin_api here). As a bonus,
not allocating @channels means you can revert the bit that adds the
"dispatched" variable and "goto done".

> + DEBUG ("handler %s rejected by ACL: %s", name, error->message);

This code path leaks @error.

> Filter reinvocation of handlers through the DBus Aegis ACL as well

You don't need this commit: the handler is already handling the channels here,
so this can only be a problem if you already got it wrong. There's no transfer
of responsibility.

------------------------------------------

Things which were wrong with McpDBusChannelAcl, but are academic if you follow
the suggestion above:

> +/* Mission Control plugin API - DBus Channel Handler/Observer ACL

Is this Handler/Observer or just Handler? As far as I can see, just Handler.

> + * SECTION:dbus-channel-acl
> + * @title: McpDBusChannelAcl
> + * @short_description: DBus ACLs for channel handlers

Nothing in this API is specific to either D-Bus or ACLs - the plugin
implementing this interface is just asked for a yes/no decision, and it's up to
the plugin how it implements that internally.

I'd call this thing McpHandlerPolicy (analogous to McpDispatchOperationPolicy),
and describe it as:

    @short_description: disqualify certain Handlers from acting on
        certain Channels

> +#include <mission-control-plugins/mcp-signals-marshal.h>

I don't see any signals, so you can probably remove this.

> +#include "config.h"

Should always be the very first code in the file, so it can do archaic
workarounds like "#define inline /* nothing */" if needed.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.