notification: Don't fail requests with unsupported properties #1303

jsparber · 2024-03-15T16:18:24Z

This filters unsupported properties from a notification request without
failing it. This allows backwards compatibility after adding new
properties. This should had been the case from the beginning so that we
don't have to break API when extending the notification interface. See
[1].

[1] https://dbus.freedesktop.org/doc/dbus-api-design.html#extensibility

Also improve tests and add icon tests to make sure that this doesn't break icons.

src/notification.c

swick · 2024-03-15T20:10:49Z

src/notification.c

+        {
+          if (!check_value_type (key, value, G_VARIANT_TYPE_STRING, error))
+            return FALSE;
+          label = g_steal_pointer (&value);


this is leaking when there are multiple "label" entries. some for the other keys. either g_clear_pointer (&label, g_variant_unref) or change the if before to check for label == NULL depending on if you want to use the first or last occurrence of the key.

Seeing the changed code: is it documented what happens with multiple keys? Should this maybe even return an error?

The documentation of the portal doesn't mention anything so this seems to be undefined and the code should be fine. Either way, we should update the spec to either throw an error when a key exists multiple times (which might not be possible if there are actual apps which already do this), or at least update if the first or last occurrence of a key is used.

not sure, the docs don't say anything about it, and the old code just hands what ever keys are present to the backend. The gtk backend simple passes everything to GNOME Shell or uses g_variant_lookup_value() when using FDO. The docs for g_variant_lookup_value() don't specify a behavior, but a quick look at the code it seams that it just returns the first found value.

Alright, this is fine then.

It would still be good to define what happens with multiple keys in a dict for all portals (first or last occurrence wins) but let's not block this MR on it.

src/notification.c

jsparber · 2024-03-21T14:59:26Z

@swick thanks for the review :)

swick · 2024-04-03T17:09:19Z

LGTM. Even though portals are versioned and this isn't really required, this makes it much nicer for apps to use.

e: I'll redact this. The notifications test is crashing. This needs fixing first.

jsparber · 2024-04-03T17:51:35Z

LGTM. Even though portals are versioned and this isn't really required, this makes it much nicer for apps to use.

e: I'll redact this. The notifications test is crashing. This needs fixing first.

The tests fails on CI because, xdg-desktop-portal-validate-icon isn't installed. But all tests are run XDP_UNINSTALLED=1. I need to figure out what's the best solution.

swick · 2024-04-03T20:36:22Z

Probably just some environment variable to override the icon_validator executable path that you set in test-portals.c and the python test framework.

jsparber · 2024-04-04T13:02:07Z

Probably just some environment variable to override the icon_validator executable path that you set in test-portals.c and the python test framework.

Not sure if that's so easy, we don't want to add the possibility to set the icon_validator path via env variable.

swick · 2024-04-04T16:44:07Z

Might want to take a look at https://github.com/flatpak/xdg-desktop-portal/pull/1309/commits.

If the env var is only for testing then we probably want the names to be consistent (XDG_DESKTOP_PORTAL_TEST_VALIDATE_ICON or something like that).

Also, let's set the environment from within the test harnesses and not the meson environment.

jsparber · 2024-04-04T17:22:44Z

If the env var is only for testing then we probably want the names to be consistent (XDG_DESKTOP_PORTAL_TEST_VALIDATE_ICON or something like that).

I think the env var can be generally useful, not just for testing.

jsparber · 2024-04-04T17:25:36Z

the icon validator doesn't work, but the CI logs don't show any reason. Locally it works just fine. It could have to do something with the sandbox.

swick · 2024-04-04T17:48:59Z

It's running into a timeout (TIMEOUT 30.01s killed by signal 15 SIGTERM) which suggests that it's either actually taking a really long time or your new test is getting stuck somehow.

jsparber · 2024-04-04T17:50:49Z

It's running into a timeout (TIMEOUT 30.01s killed by signal 15 SIGTERM) which suggests that it's either actually taking a really long time or your new test is getting stuck somehow.

that's normal, if a test fails. Since the mocked backend just stops talking back. The real issue is in the meson log.

(/src/builddir/tests/../src/xdg-desktop-portal:4770): xdg-desktop-portal-WARNING **: 15:51:47.056: : Child process exited with code 1
error: **

swick · 2024-04-04T18:07:44Z

Sight that should be fixed in the test harness...

Anyway, the test backend crashes in response to test_bytes_icon when validate-icon isn't the actual executable:

(/var/home/swick/Projects/xdg-desktop-portal/build/tests/../src/xdg-desktop-portal:807442): xdg-desktop-portal-WARNING **: 20:05:23.472: Icon validation: /var/home/swick/Projects/xdg-desktop-portal/build/src/xdg-desktop-portal-validate-icon/asd.asd not found, rejecting icon by default.
error: **
xdg-desktop-portal:ERROR:../tests/backend/notification.c:65:handle_add_notification: 'g_variant_equal (notification, arg_notification)' should be TRUE

Bail out! xdg-desktop-portal:ERROR:../tests/backend/notification.c:65:handle_add_notification: 'g_variant_equal (notification, arg_notification)' should be TRUE

Thread 2.1 "test-backends" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff78cb800 (LWP 807432)]

0x00007ffff7ae8834 in __pthread_kill_implementation () from /lib64/libc.so.6
Missing separate debuginfos, use: dnf debuginfo-install glib2-2.78.3-1.fc39.x86_64 glibc-2.38-16.fc39.x86_64 libblkid-2.39.3-6.fc39.x86_64 libffi-3.4.4-4.fc39.x86_64 libmount-2.39.3-6.fc39.x86_64 libselinux-3.5-5.fc39.x86_64 pcre2-10.42-1.fc39.2.x86_64 zlib-1.2.13-4.fc39.x86_64
(gdb) bt
#0  0x00007ffff7ae8834 in __pthread_kill_implementation () at /lib64/libc.so.6
#1  0x00007ffff7a968ee in raise () at /lib64/libc.so.6
#2  0x00007ffff7a7e8ff in abort () at /lib64/libc.so.6
#3  0x00007ffff7e90056 in g_assertion_message[cold] () at /lib64/libglib-2.0.so.0
#4  0x000000000045056e in handle_add_notification (object=0x4a3060, invocation=0x7fffe40059d0, arg_app_id=0x7fffe40038a0 "org.example.App", arg_id=0x4abbc0 "test7", arg_notification=0x7fffe40042b0)
    at ../tests/backend/notification.c:65
#5  0x000000000040fdde in _g_dbus_codegen_marshal_BOOLEAN__OBJECT_STRING_STRING_VARIANT
    (closure=0x4a30f0, return_value=0x7fffffffd640, n_param_values=5, param_values=0x4abc20, invocation_hint=0x7fffffffd620, marshal_data=0x0) at src/xdp-impl-dbus.c:1322
#6  0x000000000042e185 in xdp_dbus_impl_notification_method_marshal_add_notification
    (closure=0x4a30f0, return_value=0x7fffffffd640, n_param_values=5, param_values=0x4abc20, invocation_hint=0x7fffffffd620, marshal_data=0x0) at src/xdp-impl-dbus.c:23474
#7  0x00007ffff7c4e52a in g_closure_invoke () at /lib64/libgobject-2.0.so.0
#8  0x00007ffff7c7cfec in signal_emit_unlocked_R.isra.0 () at /lib64/libgobject-2.0.so.0
#9  0x00007ffff7c6ad3b in g_signal_emitv () at /lib64/libgobject-2.0.so.0
#10 0x000000000042f51c in _xdp_dbus_impl_notification_skeleton_handle_method_call
    (connection=0x4914c0, sender=0x7fffe4003bb0 ":1.3", object_path=0x7fffe40054f0 "/org/freedesktop/portal/desktop", interface_name=0x499d20 "org.freedesktop.impl.portal.Notification", method_name=0x7fffe4002a80 "AddNotification", parameters=0x7fffe4003c00, invocation=0x7fffe40059d0, user_data=0x4a3060) at src/xdp-impl-dbus.c:24283
#11 0x00007ffff7dc55d3 in g_dbus_interface_method_dispatch_helper () at /lib64/libgio-2.0.so.0
#12 0x00007ffff7da76e8 in call_in_idle_cb.lto_priv () at /lib64/libgio-2.0.so.0
#13 0x00007ffff7ec878d in g_idle_dispatch () at /lib64/libglib-2.0.so.0
#14 0x00007ffff7ecbe5c in g_main_context_dispatch_unlocked.lto_priv () at /lib64/libglib-2.0.so.0
#15 0x00007ffff7f26f18 in g_main_context_iterate_unlocked.isra () at /lib64/libglib-2.0.so.0
#16 0x00007ffff7ecd447 in g_main_loop_run () at /lib64/libglib-2.0.so.0
#17 0x000000000044af0d in main (argc=1, argv=0x7fffffffdcf8) at ../tests/backend/test-backends.c:151

Adding a notification may fail, instead of waiting for the timeout we can check the result we get in the callback.

jsparber · 2024-04-05T16:17:39Z

Fixing the CI took way to long. 😮‍💨

For debugging it's important that the backend doesn't just crash without telling the reason. It would be even nicer if the test would fail immediately instead of hitting the timeout, but that' currently not possible since the client doesn't know whether the backend failed.

We can reuse most of the test setup for all tests. This will make it easier to test more properties.

This filters unsupported properties from a notification request without failing it. This allows backwards compatibility after adding new properties. This should had been the case from the beginning so that we don't have to break API when extending the notification interface. See [1]. [1] https://dbus.freedesktop.org/doc/dbus-api-design.html#extensibility

This allows setting a custom path for the icon validator, especially useful when testing without installing.

The sandbox for the icon-validator needs a privileged container. And it needs librsvg to be able to validate SVGs.

If debug messages are turned on the output of validate-icon isn't a valid key file since it contains log output as well.

swick · 2024-04-16T13:53:03Z

Alright, this looks all good to me!

jsparber · 2024-06-26T10:04:20Z

@GeorgesStavracas This was the merge request that included pretty much the same commits as the once you cherry picked in #1386. Closing.

swick reviewed Mar 15, 2024

View reviewed changes

jsparber force-pushed the filter_notification branch from 0b80213 to 90b194f Compare March 21, 2024 14:58

jsparber force-pushed the filter_notification branch 2 times, most recently from c98a948 to d1d6492 Compare March 21, 2024 15:03

jsparber force-pushed the filter_notification branch from d1d6492 to 730c5db Compare April 4, 2024 15:50

jsparber force-pushed the filter_notification branch 4 times, most recently from d452635 to e5eca5e Compare April 4, 2024 19:31

tests/notification: Check result in notification add callback

6e2f90a

Adding a notification may fail, instead of waiting for the timeout we can check the result we get in the callback.

jsparber force-pushed the filter_notification branch 6 times, most recently from a958c42 to 0df47d2 Compare April 5, 2024 16:06

jsparber added 2 commits April 5, 2024 19:36

tests/notification: Remove redundant code

13f78fa

We can reuse most of the test setup for all tests. This will make it easier to test more properties.

jsparber added 6 commits April 5, 2024 19:39

test: Implement tests for notification icons

0dcec82

icon-validator: Allow custom path for icon validator via env

a16aee0

This allows setting a custom path for the icon validator, especially useful when testing without installing.

utils: Print error log when icon-validator fails to validate a image

3482e33

CI: Fix running icon-validator

a66d939

The sandbox for the icon-validator needs a privileged container. And it needs librsvg to be able to validate SVGs.

validate-icon: Don't log command ever

e66041b

If debug messages are turned on the output of validate-icon isn't a valid key file since it contains log output as well.

jsparber force-pushed the filter_notification branch from 0df47d2 to e66041b Compare April 5, 2024 17:48

jsparber closed this Jun 26, 2024

jsparber mentioned this pull request Jun 26, 2024

Implement v2 notification spec #1298

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notification: Don't fail requests with unsupported properties #1303

notification: Don't fail requests with unsupported properties #1303

jsparber commented Mar 15, 2024

swick Mar 15, 2024

swick Mar 21, 2024

swick Mar 21, 2024

jsparber Mar 21, 2024

swick Apr 3, 2024

jsparber commented Mar 21, 2024

swick commented Apr 3, 2024 •

edited

Loading

jsparber commented Apr 3, 2024

swick commented Apr 3, 2024

jsparber commented Apr 4, 2024

swick commented Apr 4, 2024

jsparber commented Apr 4, 2024

jsparber commented Apr 4, 2024

swick commented Apr 4, 2024

jsparber commented Apr 4, 2024

swick commented Apr 4, 2024 •

edited

Loading

jsparber commented Apr 5, 2024

swick commented Apr 16, 2024

jsparber commented Jun 26, 2024

notification: Don't fail requests with unsupported properties #1303

notification: Don't fail requests with unsupported properties #1303

Conversation

jsparber commented Mar 15, 2024

swick Mar 15, 2024

Choose a reason for hiding this comment

swick Mar 21, 2024

Choose a reason for hiding this comment

swick Mar 21, 2024

Choose a reason for hiding this comment

jsparber Mar 21, 2024

Choose a reason for hiding this comment

swick Apr 3, 2024

Choose a reason for hiding this comment

jsparber commented Mar 21, 2024

swick commented Apr 3, 2024 • edited Loading

jsparber commented Apr 3, 2024

swick commented Apr 3, 2024

jsparber commented Apr 4, 2024

swick commented Apr 4, 2024

jsparber commented Apr 4, 2024

jsparber commented Apr 4, 2024

swick commented Apr 4, 2024

jsparber commented Apr 4, 2024

swick commented Apr 4, 2024 • edited Loading

jsparber commented Apr 5, 2024

swick commented Apr 16, 2024

jsparber commented Jun 26, 2024

swick commented Apr 3, 2024 •

edited

Loading

swick commented Apr 4, 2024 •

edited

Loading