context: Keep fallback-x11 separate from x11 conditionals

If we convert fallback-x11 internally to a conditional x11 permission,
we cannot express current fallback-x11 stacking behavior:

lower: empty + upper: !fallback-x11 -> no x11 access
lower: fallback-x11 + upper: !fallback-x11 -> x11 access

The reason is that conditionals have no view of the lower level.

This changes things in a way that fallback-x11 stays its own socket
permission with two interactions with the x11 socket permission:

* If a upper level resets x11 (--socket=x11, --nosocket=x11), the lower
  level fallback-x11 permission gets dropped
* When computing the allowed sockets, --socket=fallback-x11 gets
  converted to --socket=if:x11:!has-wayland

Fixes: #6556

common/flatpak-context.c

@@ -216,28 +216,6 @@ flatpak_permission_set_allowed_if (FlatpakPermission *permission,
   g_ptr_array_sort (permission->conditionals, flatpak_strcmp0_ptr);
 }
 
-static void
-flatpak_permission_remove_conditional (FlatpakPermission *permission,
-                                       const char        *condition)
-{
-  guint index;
-
-  /* If we are already unconditionally allowed, we don't have conditions */
-  if (permission->allowed)
-    return;
-
-  /* The only way to correctly layer removal of conditional is to completely
-     remove everything from the lower layer */
-  permission->reset = TRUE;
-
-  if (!g_ptr_array_find_with_equal_func (permission->conditionals,
-                                         condition,
-                                         g_str_equal, &index))
-    return;
-
-  g_ptr_array_remove_index (permission->conditionals, index);
-}
-
 static void
 flatpak_permission_serialize (FlatpakPermission *permission,
                               const char        *name,
@@ -564,17 +542,6 @@ flatpak_permissions_set_allowed_if (GHashTable *permissions,
                                       condition);
 }
 
-
-static void
-flatpak_permissions_remove_conditional (GHashTable *permissions,
-                                        const char *name,
-                                        const char *condition)
-{
-  flatpak_permission_remove_conditional (flatpak_permissions_ensure (permissions,
-                                                                     name),
-                                         condition);
-}
-
 static gboolean
 flatpak_permissions_allows_unconditionally (GHashTable *permissions,
                                             const char *name)
@@ -649,56 +616,6 @@ flatpak_permissions_compute_allowed (GHashTable                        *permissi
   return bitmask;
 }
 
-static void
-flatpak_canonicalize_x11_permissions (GHashTable  *permissions)
-{
-  /* The on-disk format for sockets supports the old fallback-x11
-   * permission, but in-memory we remove that converting it to a modern.
-   * conditional check if-wayland.
-   */
-
-  FlatpakPermission *fallback_x11 = g_hash_table_lookup (permissions, "fallback-x11");
-  if (fallback_x11)
-    {
-      /* Remove full-access plain x11, which used to be added when
-         fallback-x11 was added.  */
-      FlatpakPermission *x11 = flatpak_permissions_ensure (permissions, "x11");
-      x11->allowed = FALSE;
-      x11->reset = FALSE;
-
-      if (fallback_x11->allowed)
-        flatpak_permission_set_allowed_if (x11, "!has-wayland");
-      else
-        flatpak_permission_remove_conditional (x11, "!has-wayland");
-
-      /* Remove fallback-x11 (which is deprecated) */
-      g_hash_table_remove (permissions, "fallback-x11");
-    }
-}
-
-static GHashTable *
-flatpak_decanonicalize_x11_permissions (GHashTable  *permissions)
-{
-  /* Convert from internal format to on-disk backwards compatible format.
-   * Note: This only handles the specific case where there is only
-   * the fallback-x11 conditional. More complex cases are handled
-   * with the full conditional syntax.
-   */
-
-  FlatpakPermission *x11 = g_hash_table_lookup (permissions, "x11");
-  if (x11 != NULL && !x11->allowed && x11->conditionals->len == 1 &&
-      strcmp (x11->conditionals->pdata[0], "!has-wayland") == 0)
-    {
-      GHashTable *copy = flatpak_permissions_dup (permissions);
-      flatpak_permissions_set_allowed (copy, "fallback-x11");
-      g_hash_table_remove (copy, "x11");
-
-      return copy;
-    }
-
-  return g_hash_table_ref (permissions);
-}
-
 static gboolean
 flatpak_permissions_from_strv (GHashTable  *permissions,
                                const char **strv,
@@ -750,6 +667,13 @@ flatpak_permissions_merge (GHashTable *permissions,
   const char *name;
   FlatpakPermission *other_permission;
   GHashTableIter iter;
+  FlatpakPermission *x11;
+
+  /* If we reset the x11 conditionals, the fallback-x11 permission also must go
+   * because it is a conditional. */
+  x11 = g_hash_table_lookup (other, "x11");
+  if (x11 && x11->reset)
+    g_hash_table_remove (permissions, "fallback-x11");
 
   g_hash_table_iter_init (&iter, other);
   while (g_hash_table_iter_next (&iter,
@@ -1023,9 +947,10 @@ static void flatpak_permissions_test_backwards_compat (void)
 static void flatpak_permissions_test_fallback_x11 (void)
 {
   g_autoptr(GHashTable) perms = NULL;
+  g_autoptr(GHashTable) res_perms = NULL;
 
   {
-    FlatpakPermission *x11;
+    FlatpakPermission *fallback_x11;
     FlatpakPermission *wayland;
     g_autoptr(GError) error = NULL;
     gboolean ok;
@@ -1041,77 +966,31 @@ static void flatpak_permissions_test_fallback_x11 (void)
     g_assert_true (ok);
     g_assert_no_error (error);
     g_assert_nonnull (perms);
-    flatpak_canonicalize_x11_permissions (perms);
 
     g_assert_cmpint (g_hash_table_size (perms), ==, 2);
 
-    x11 = g_hash_table_lookup (perms, "x11");
-    g_assert_nonnull (x11);
+    fallback_x11 = g_hash_table_lookup (perms, "fallback-x11");
+    g_assert_nonnull (fallback_x11);
+    g_assert_true (fallback_x11->allowed);
     wayland = g_hash_table_lookup (perms, "wayland");
     g_assert_nonnull (wayland);
-
-    g_assert_false (x11->allowed);
-    g_assert_cmpint (x11->conditionals->len, ==, 1);
-    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "!has-wayland");
     g_assert_true (wayland->allowed);
   }
 
+  /* If we only add a conditional, we don't reset fallback-x11 */
   {
     g_autoptr(GHashTable) perms2 = NULL;
     FlatpakPermission *x11;
+    FlatpakPermission *fallback_x11;
     FlatpakPermission *wayland;
     g_autoptr(GError) error = NULL;
     gboolean ok;
 
-    perms2 = flatpak_permissions_new ();
-    ok = flatpak_permissions_from_strv (perms2,
-                                        (const char * []) {
-                                          "if:x11:!has-wayland",
-                                          NULL,
-                                        },
-                                        &error);
-    g_assert_true (ok);
-    g_assert_no_error (error);
-    g_assert_nonnull (perms2);
-    flatpak_canonicalize_x11_permissions (perms2);
-
-    g_assert_cmpint (g_hash_table_size (perms2), ==, 1);
-
-    x11 = g_hash_table_lookup (perms, "x11");
-    g_assert_nonnull (x11);
-    g_assert_false (x11->allowed);
-    g_assert_cmpint (x11->conditionals->len, ==, 1);
-    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "!has-wayland");
-
-    /* lower: fallback-x11
-     * upper: if:x11:!has-wayland
-     * -> if:x11:!has-wayland */
-    flatpak_permissions_merge (perms, perms2);
-
-    g_assert_cmpint (g_hash_table_size (perms), ==, 2);
-
-    x11 = g_hash_table_lookup (perms, "x11");
-    g_assert_nonnull (x11);
-    wayland = g_hash_table_lookup (perms, "wayland");
-    g_assert_nonnull (wayland);
-
-    g_assert_false (x11->allowed);
-    g_assert_cmpint (x11->conditionals->len, ==, 1);
-    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "!has-wayland");
-    g_assert_true (wayland->allowed);
-  }
-
-  {
-    g_autoptr(GHashTable) perms2 = NULL;
-    g_autoptr(GHashTable) perms3 = NULL;
-    FlatpakPermission *x11;
-    g_autoptr(GError) error = NULL;
-    gboolean ok;
+    res_perms = flatpak_permissions_dup (perms);
 
     perms2 = flatpak_permissions_new ();
     ok = flatpak_permissions_from_strv (perms2,
                                         (const char * []) {
-                                          "fallback-x11",
                                           "if:x11:foo",
                                           NULL,
                                         },
@@ -1119,7 +998,6 @@ static void flatpak_permissions_test_fallback_x11 (void)
     g_assert_true (ok);
     g_assert_no_error (error);
     g_assert_nonnull (perms2);
-    flatpak_canonicalize_x11_permissions (perms2);
 
     g_assert_cmpint (g_hash_table_size (perms2), ==, 1);
 
@@ -1127,47 +1005,124 @@ static void flatpak_permissions_test_fallback_x11 (void)
     g_assert_nonnull (x11);
     g_assert_false (x11->allowed);
     g_assert_false (x11->reset);
-    g_assert_cmpint (x11->conditionals->len, ==, 2);
-    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "!has-wayland");
-    g_assert_cmpstr (x11->conditionals->pdata[1], ==, "foo");
+    g_assert_cmpint (x11->conditionals->len, ==, 1);
+    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "foo");
 
-    perms3 = flatpak_permissions_new ();
-    ok = flatpak_permissions_from_strv (perms3,
+    flatpak_permissions_merge (res_perms, perms2);
+
+    g_assert_cmpint (g_hash_table_size (res_perms), ==, 3);
+
+    x11 = g_hash_table_lookup (res_perms, "x11");
+    g_assert_nonnull (x11);
+    g_assert_false (x11->allowed);
+    g_assert_false (x11->reset);
+    g_assert_cmpint (x11->conditionals->len, ==, 1);
+    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "foo");
+
+    fallback_x11 = g_hash_table_lookup (res_perms, "fallback-x11");
+    g_assert_nonnull (fallback_x11);
+    g_assert_true (fallback_x11->allowed);
+
+    wayland = g_hash_table_lookup (res_perms, "wayland");
+    g_assert_nonnull (wayland);
+    g_assert_true (wayland->allowed);
+  }
+
+  /* If we set x11, we reset conditionals *and* fallback-x11 */
+  {
+    g_autoptr(GHashTable) perms2 = NULL;
+    FlatpakPermission *x11;
+    FlatpakPermission *wayland;
+    g_autoptr(GError) error = NULL;
+    gboolean ok;
+
+    res_perms = flatpak_permissions_dup (perms);
+
+    perms2 = flatpak_permissions_new ();
+    ok = flatpak_permissions_from_strv (perms2,
                                         (const char * []) {
-                                          "if:x11:!has-wayland",
-                                          "!fallback-x11",
-                                          "if:x11:bar",
+                                          "x11",
                                           NULL,
                                         },
                                         &error);
     g_assert_true (ok);
     g_assert_no_error (error);
-    g_assert_nonnull (perms3);
-    flatpak_canonicalize_x11_permissions (perms3);
-
-    g_assert_cmpint (g_hash_table_size (perms3), ==, 1);
-
-    x11 = g_hash_table_lookup (perms3, "x11");
-    g_assert_nonnull (x11);
-    g_assert_false (x11->allowed);
-    g_assert_true (x11->reset);
-    g_assert_cmpint (x11->conditionals->len, ==, 1);
-    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "bar");
-
-    /* lower: fallback-x11;if:x11:foo
-     * upper: if:x11:!has-wayland;!fallback-x11;if:x11:bar
-     * -> if:x11:bar
-     * The !fallback-x11 removes the if:x11:!has-wayland conditional which
-     * turns into !x11. */
-    flatpak_permissions_merge (perms2, perms3);
+    g_assert_nonnull (perms2);
 
     g_assert_cmpint (g_hash_table_size (perms2), ==, 1);
 
+    x11 = g_hash_table_lookup (perms2, "x11");
+    g_assert_nonnull (x11);
+    g_assert_true (x11->allowed);
+    g_assert_true (x11->reset);
+
+    flatpak_permissions_merge (res_perms, perms2);
+
+    g_assert_cmpint (g_hash_table_size (res_perms), ==, 2);
+
+    x11 = g_hash_table_lookup (res_perms, "x11");
+    g_assert_nonnull (x11);
+    g_assert_true (x11->allowed);
+
+    wayland = g_hash_table_lookup (res_perms, "wayland");
+    g_assert_nonnull (wayland);
+    g_assert_true (wayland->allowed);
+  }
+
+  /* Only add a conditional and nosocket fallback-x11, gives nosocket fallback-x11 */
+  {
+    g_autoptr(GHashTable) perms2 = NULL;
+    FlatpakPermission *x11;
+    FlatpakPermission *fallback_x11;
+    FlatpakPermission *wayland;
+    g_autoptr(GError) error = NULL;
+    gboolean ok;
+
+    res_perms = flatpak_permissions_dup (perms);
+
+    perms2 = flatpak_permissions_new ();
+    ok = flatpak_permissions_from_strv (perms2,
+                                        (const char * []) {
+                                          "if:x11:foo",
+                                          "!fallback-x11",
+                                          NULL,
+                                        },
+                                        &error);
+    g_assert_true (ok);
+    g_assert_no_error (error);
+    g_assert_nonnull (perms2);
+
+    g_assert_cmpint (g_hash_table_size (perms2), ==, 2);
+
     x11 = g_hash_table_lookup (perms2, "x11");
     g_assert_nonnull (x11);
     g_assert_false (x11->allowed);
+    g_assert_false (x11->reset);
     g_assert_cmpint (x11->conditionals->len, ==, 1);
-    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "bar");
+    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "foo");
+
+    fallback_x11 = g_hash_table_lookup (perms2, "fallback-x11");
+    g_assert_nonnull (fallback_x11);
+    g_assert_false (fallback_x11->allowed);
+
+    flatpak_permissions_merge (res_perms, perms2);
+
+    g_assert_cmpint (g_hash_table_size (res_perms), ==, 3);
+
+    x11 = g_hash_table_lookup (res_perms, "x11");
+    g_assert_nonnull (x11);
+    g_assert_false (x11->allowed);
+    g_assert_false (x11->reset);
+    g_assert_cmpint (x11->conditionals->len, ==, 1);
+    g_assert_cmpstr (x11->conditionals->pdata[0], ==, "foo");
+
+    fallback_x11 = g_hash_table_lookup (res_perms, "fallback-x11");
+    g_assert_nonnull (fallback_x11);
+    g_assert_false (fallback_x11->allowed);
+
+    wayland = g_hash_table_lookup (res_perms, "wayland");
+    g_assert_nonnull (wayland);
+    g_assert_true (wayland->allowed);
   }
 }
 
@@ -2138,14 +2093,6 @@ option_socket_cb (const gchar *option_name,
   if (socket == 0)
     return FALSE;
 
-  if (socket == FLATPAK_CONTEXT_SOCKET_FALLBACK_X11)
-    {
-      flatpak_permissions_set_allowed_if (context->socket_permissions,
-                                          "x11",
-                                          "!has-wayland");
-      return TRUE;
-    }
-
   flatpak_permissions_set_allowed (context->socket_permissions,
                                    value);
 
@@ -2165,13 +2112,6 @@ option_nosocket_cb (const gchar *option_name,
   if (socket == 0)
     return FALSE;
 
-  if (socket == FLATPAK_CONTEXT_SOCKET_FALLBACK_X11)
-    {
-      flatpak_permissions_remove_conditional (context->socket_permissions,
-                                              "x11", "!has-wayland");
-      return TRUE;
-    }
-
   flatpak_permissions_set_not_allowed (context->socket_permissions,
                                        value);
 
@@ -2826,7 +2766,6 @@ flatpak_context_load_metadata (FlatpakContext *context,
 
       if (!flatpak_permissions_from_strv (context->socket_permissions, (const char **)sockets, error))
         return FALSE;
-      flatpak_canonicalize_x11_permissions (context->socket_permissions);
     }
 
   if (g_key_file_has_key (metakey, FLATPAK_METADATA_GROUP_CONTEXT, FLATPAK_METADATA_KEY_DEVICES, NULL))
@@ -3105,8 +3044,7 @@ flatpak_context_save_metadata (FlatpakContext *context,
   int i;
 
   shared = flatpak_permissions_to_strv (context->shares_permissions, flatten);
-  g_autoptr(GHashTable) socket_permissions = flatpak_decanonicalize_x11_permissions (context->socket_permissions);
-  sockets = flatpak_permissions_to_strv (socket_permissions, flatten);
+  sockets = flatpak_permissions_to_strv (context->socket_permissions, flatten);
   devices = flatpak_permissions_to_strv (context->device_permissions, flatten);
   features = flatpak_permissions_to_strv (context->features_permissions, flatten);
 
@@ -4500,7 +4438,27 @@ FlatpakContextSockets
 flatpak_context_compute_allowed_sockets (FlatpakContext                   *context,
                                          FlatpakContextConditionEvaluator  evaluator)
 {
-  return flatpak_permissions_compute_allowed (context->socket_permissions,
+  g_autoptr(GHashTable) permissions =
+    g_hash_table_new_similar (context->socket_permissions);
+  GHashTableIter iter;
+  gpointer key, value;
+  FlatpakPermission *fallback_x11;
+
+  g_hash_table_iter_init (&iter, context->socket_permissions);
+  while (g_hash_table_iter_next (&iter, &key, &value))
+    g_hash_table_insert (permissions, g_strdup (key), flatpak_permission_dup (value));
+
+  fallback_x11 = g_hash_table_lookup (context->socket_permissions, "fallback-x11");
+  if (fallback_x11 && fallback_x11->allowed)
+    {
+      FlatpakPermission *x11 = flatpak_permissions_ensure (permissions, "x11");
+
+      x11->allowed = FALSE;
+      flatpak_permission_set_allowed_if (x11, "!has-wayland");
+    }
+  g_hash_table_remove (permissions, "fallback-x11");
+
+  return flatpak_permissions_compute_allowed (permissions,
                                               flatpak_context_sockets,
                                               evaluator);
 }

tests/test-exports.c

@@ -371,6 +371,7 @@ test_full_context (void)
   g_assert_cmpstr (strv[i++], ==, "ssh-auth");
   g_assert_cmpstr (strv[i++], ==, "system-bus");
   g_assert_cmpstr (strv[i++], ==, "wayland");
+  g_assert_cmpstr (strv[i++], ==, "x11");
   g_assert_cmpstr (strv[i], ==, NULL);
   g_assert_cmpuint (i, ==, n);
   g_clear_pointer (&strv, g_strfreev);