Bug 101256

Created attachment 131610 [details] [review]
[1/4] test/monitor: Assert that BecomeMonitor() on wrong object  path fails

Created attachment 131611 [details] [review]
[2/4] bus/driver: Make non-core interfaces unavailable on most  object paths

The o.fd.DBus interface needs to remain available on arbitrary object
paths for backwards compatibility, and the Introspectable interface
is genuinely useful, but everything else can be skipped.
 
This is arguably an incompatible change for the undocumented Verbose
interface, and for the GetAllMatchRules method on the undocumented
Stats interface: previously those were available at all object paths.

---

In case it isn't obvious: I think that arguable incompatible change is harmless.

Created attachment 131612 [details] [review]
[3/4] bus/driver: Only allow specific methods to be called at  wrong paths

The default for the future should be that new functionality should
only be available at /org/freedesktop/DBus, which is the canonical
path of the "bus driver" object that represents the message bus.
 
The disallowed methods are still in Introspect() output, and 
raise AccessDenied instead of UnknownMethod, to preserve the invariant
that the o.fd.DBus interface has constant contents. 

test/dbus-daemon.c already asserts that UpdateActivationEnvironment()
still raises AccessDenied when invoked on a non-canonical path;
this has been in place since 1.8.14.

Created attachment 131613 [details] [review]
[4/4] spec: Document the canonical object path for the bus   driver

Created attachment 131616 [details] [review]
[3/4] bus/driver: Only allow specific methods to be called at  wrong paths

The default for the future should be that new functionality should
only be available at /org/freedesktop/DBus, which is the canonical
path of the "bus driver" object that represents the message bus.

The disallowed methods are still in Introspect() output, and
raise AccessDenied instead of UnknownMethod, to preserve the invariant
that the o.fd.DBus interface has constant contents.

test/dbus-daemon.c already asserts that UpdateActivationEnvironment()
still raises AccessDenied when invoked on a non-canonical path;
this has been in place since 1.8.14.

---

Remove the declaration of bus_driver_check_message_is_for_us()

Comment on attachment 131610 [details] [review]
[1/4] test/monitor: Assert that BecomeMonitor() on wrong object  path fails

Review of attachment 131610 [details] [review]:
-----------------------------------------------------------------

++

Comment on attachment 131611 [details] [review]
[2/4] bus/driver: Make non-core interfaces unavailable on most  object paths

Review of attachment 131611 [details] [review]:
-----------------------------------------------------------------

Looks good apart from a few nitpicks.

::: bus/driver.c
@@ +2420,5 @@
> +  INTERFACE_FLAG_NONE = 0,
> +  /* Various older interfaces were available at every object path. We have to
> +   * preserve that behaviour for backwards compatibility, but we can at least
> +   * stop doing that for newly added interfaces. */
> +  INTERFACE_FLAG_ANY_PATH = (1 << 0)

Maybe add a reference to this bug in the comment. Probably also want to mention (or at least not imply to the contrary) that it’s actually useful for o.fd.D.Introspectable.

Nitpick: Add a trailing comma after the final enum value.

@@ +2557,4 @@
>    DBusString xml;
>    DBusMessage *reply;
>    const char *v_STRING;
> +  dbus_bool_t canonical_path;

Nitpick: Maybe call this `is_canonical_path` to make it more obvious it’s a boolean (rather than an object path)?

@@ +2655,4 @@
>    const InterfaceHandler *ih;
>    const MessageHandler *mh;
>    dbus_bool_t found_interface = FALSE;
> +  dbus_bool_t canonical_path;

Nitpick: similarly.

Comment on attachment 131616 [details] [review]
[3/4] bus/driver: Only allow specific methods to be called at  wrong paths

Review of attachment 131616 [details] [review]:
-----------------------------------------------------------------

++ with nitpicks

::: bus/driver.c
@@ +2293,5 @@
> +{
> +  METHOD_FLAG_NONE = 0,
> +  /* Various older methods were available at every object path. We have to
> +   * preserve that behaviour for backwards compatibility, but we can at least
> +   * stop doing that for newly added methods.. */

Nitpick: Double full-stop at the end of the comment. Maybe cross-reference this bug report in the comment.

@@ +2294,5 @@
> +  METHOD_FLAG_NONE = 0,
> +  /* Various older methods were available at every object path. We have to
> +   * preserve that behaviour for backwards compatibility, but we can at least
> +   * stop doing that for newly added methods.. */
> +  METHOD_FLAG_ANY_PATH = (1 << 0)

Nitpick: Add a trailing comma.

Comment on attachment 131613 [details] [review]
[4/4] spec: Document the canonical object path for the bus   driver

Review of attachment 131613 [details] [review]:
-----------------------------------------------------------------

++

(In reply to Philip Withnall from comment #7)
> Maybe add a reference to this bug in the comment. Probably also want to
> mention (or at least not imply to the contrary) that it’s actually useful
> for o.fd.D.Introspectable.

Good point - Introspectable is not just there for compatibility.

> Nitpick: Add a trailing comma after the final enum value.

Sadly that isn't Standard C (although gcc, clang and most real compilers accept it).

Perhaps I could move the NONE sentinels to the end of the enum so that the special last entry is special in a different way anyway?

> Nitpick: Maybe call this `is_canonical_path` to make it more obvious it’s a
> boolean (rather than an object path)?

Good idea.

(In reply to Simon McVittie from comment #10)
> Perhaps I could move the NONE sentinels to the end of the enum so that the
> special last entry is special in a different way anyway?

That would be great.

I've pushed these with the changes you recommended. Fixed in git for 1.11.14.