Ensure refs are updated from their origin

It can happen that a related ref is installed from a different remote
than the thing it's related to. We always want to update things from
their origin remote. However as of now FlatpakTransaction resolves the
commit of a related ref to the one available from the main ref origin,
and later sets the remote for the operation to the installed origin (see
commit 6793d90b8). In case there is a newer commit in the main ref
origin than the installed origin, this leads to an update operation
being erroneously created, only to then error out with an HTTP 404
error, because the commit from the main ref origin is being pulled from
the installed ref origin. For specific steps to reproduce see
https://github.com/flatpak/flatpak/issues/3128#issuecomment-948948040

So, ensure that when a FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE
operation is created for something that's installed, whether it's a
related ref or something else, the remote used is always the origin. And
ensure that the remote is set correctly before the stage where the op is
resolved to a commit, to avoid the situation described above. This is
essentially a re-implementation of the fix in commit 6793d90b8.

Also, add a unit test for this behavior.

This commit also makes a few changes to documentation to make it clear
that this related-ref-different-origin situation is possible.

Fixes #3128

(cherry picked from commit 49d9052d2248de8b5f3c4c1c6ca7dfceefcbcd2f)
(only merge conflicts in tests/testlibrary.c)

5 files changed, 154 insertions, 20 deletions

diff --git a/common/flatpak-dir-private.h b/common/flatpak-dir-private.h
index 3c69928c..db160368 100644
--- a/common/flatpak-dir-private.h
+++ b/common/flatpak-dir-private.h
@@ -100,6 +100,7 @@ GType flatpak_deploy_get_type (void);
 typedef struct
 {
   FlatpakDecomposed *ref;
+  char              *remote;
   char              *commit;
   char             **subpaths;
   gboolean           download;
diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
index e6091b99..db784c3d 100644
--- a/common/flatpak-dir.c
+++ b/common/flatpak-dir.c
@@ -14717,6 +14717,7 @@ flatpak_dir_update_remote_configuration (FlatpakDir   *self,
 void
 flatpak_related_free (FlatpakRelated *self)
 {
+  g_free (self->remote);
   flatpak_decomposed_unref (self->ref);
   g_free (self->commit);
   g_strfreev (self->subpaths);
@@ -14726,6 +14727,7 @@ flatpak_related_free (FlatpakRelated *self)
 static void
 add_related (FlatpakDir        *self,
              GPtrArray         *related,
+             const char        *remote,
              const char        *extension,
              FlatpakDecomposed *extension_ref,
              const char        *checksum,
@@ -14755,7 +14757,12 @@ add_related (FlatpakDir        *self,
   branch = flatpak_decomposed_dup_branch (extension_ref);
 
   if (deploy_data)
-    old_subpaths = flatpak_deploy_data_get_subpaths (deploy_data);
+    {
+      old_subpaths = flatpak_deploy_data_get_subpaths (deploy_data);
+      /* If the extension is installed already, its origin overrides the remote
+       * that would otherwise be used */
+      remote = flatpak_deploy_data_get_origin (deploy_data);
+    }
 
   /* Only respect no-autodownload/download-if for uninstalled refs, we
      always want to update if you manually installed something */
@@ -14801,6 +14808,7 @@ add_related (FlatpakDir        *self,
   subpaths = flatpak_subpaths_merge ((char **) old_subpaths, extra_subpaths);
 
   rel = g_new0 (FlatpakRelated, 1);
+  rel->remote = g_strdup (remote);
   rel->ref = flatpak_decomposed_ref (extension_ref);
   rel->commit = g_strdup (checksum);
   rel->subpaths = g_steal_pointer (&subpaths);
@@ -15014,7 +15022,7 @@ flatpak_dir_find_remote_related_for_metadata (FlatpakDir         *self,
               if (flatpak_remote_state_lookup_ref (state, flatpak_decomposed_get_ref (extension_ref), &checksum, NULL, NULL, NULL, NULL))
                 {
                   if (flatpak_filters_allow_ref (NULL, masked, flatpak_decomposed_get_ref (extension_ref)))
-                    add_related (self, related, extension, extension_ref, checksum,
+                    add_related (self, related, state->remote_name, extension, extension_ref, checksum,
                                  no_autodownload, download_if, autoprune_unless, autodelete, locale_subset);
                 }
               else if (subdirectories)
@@ -15028,7 +15036,7 @@ flatpak_dir_find_remote_related_for_metadata (FlatpakDir         *self,
                       if (flatpak_remote_state_lookup_ref (state, flatpak_decomposed_get_ref (subref_ref),
                                                            &subref_checksum, NULL, NULL, NULL, NULL) &&
                           flatpak_filters_allow_ref (NULL, masked,  flatpak_decomposed_get_ref (subref_ref)))
-                        add_related (self, related, extension, subref_ref, subref_checksum,
+                        add_related (self, related, state->remote_name, extension, subref_ref, subref_checksum,
                                      no_autodownload, download_if, autoprune_unless, autodelete, locale_subset);
                     }
                 }
@@ -15267,7 +15275,7 @@ flatpak_dir_find_local_related_for_metadata (FlatpakDir        *self,
                                             NULL,
                                             NULL))
                 {
-                  add_related (self, related, extension, extension_ref,
+                  add_related (self, related, remote_name, extension, extension_ref,
                                checksum, no_autodownload, download_if, autoprune_unless, autodelete, locale_subset);
                 }
               else if ((deploy_data = flatpak_dir_get_deploy_data (self, extension_ref,
@@ -15280,8 +15288,10 @@ flatpak_dir_find_local_related_for_metadata (FlatpakDir        *self,
                    * --force
                    */
                   checksum = g_strdup (flatpak_deploy_data_get_commit (deploy_data));
-                  add_related (self, related, extension, extension_ref,
-                               checksum, no_autodownload, download_if, autoprune_unless, autodelete, locale_subset);
+                  add_related (self, related,
+                               flatpak_deploy_data_get_origin (deploy_data),
+                               extension, extension_ref, checksum,
+                               no_autodownload, download_if, autoprune_unless, autodelete, locale_subset);
                 }
               else if (subdirectories)
                 {
@@ -15301,7 +15311,7 @@ flatpak_dir_find_local_related_for_metadata (FlatpakDir        *self,
                                                     NULL,
                                                     NULL))
                         {
-                          add_related (self, related, extension, match, match_checksum,
+                          add_related (self, related, remote_name, extension, match, match_checksum,
                                        no_autodownload, download_if, autoprune_unless, autodelete, locale_subset);
                         }
                       else if ((match_deploy_data = flatpak_dir_get_deploy_data (self, match,
@@ -15313,7 +15323,9 @@ flatpak_dir_find_local_related_for_metadata (FlatpakDir        *self,
                            * not have a ref in the repo
                            */
                           match_checksum = g_strdup (flatpak_deploy_data_get_commit (match_deploy_data));
-                          add_related (self, related, extension, match, match_checksum,
+                          add_related (self, related,
+                                       flatpak_deploy_data_get_origin (match_deploy_data),
+                                       extension, match, match_checksum,
                                        no_autodownload, download_if, autoprune_unless, autodelete, locale_subset);
                         }
                     }
diff --git a/common/flatpak-installation.c b/common/flatpak-installation.c
index 0daf3d66..4d21da7f 100644
--- a/common/flatpak-installation.c
+++ b/common/flatpak-installation.c
@@ -2751,15 +2751,18 @@ flatpak_installation_list_remote_related_refs_sync (FlatpakInstallation *self,
 /**
  * flatpak_installation_list_installed_related_refs_sync:
  * @self: a #FlatpakInstallation
- * @remote_name: the name of the remote
+ * @remote_name: the name of the remote providing @ref
  * @ref: the ref
  * @cancellable: (nullable): a #GCancellable
  * @error: return location for a #GError
  *
- * Lists all the locally installed refs from @remote_name that are
- * related to @ref. These are things that are interesting to install,
- * update, or uninstall together with @ref. For instance, locale data
- * or debug information.
+ * Lists all the locally installed refs that are related to @ref. These are
+ * things that are interesting to install, update, or uninstall together with
+ * @ref. For instance, locale data or debug information.
+ *
+ * Note that while the related refs are usually installed from the same remote
+ * as @ref (@remote_name), it is possible they were installed from another
+ * remote.
  *
  * This function is similar to flatpak_installation_list_remote_related_refs_sync,
  * but instead of looking at what is available on the remote, it only looks
diff --git a/common/flatpak-transaction.c b/common/flatpak-transaction.c
index 4f98b1c8..e3ce7956 100644
--- a/common/flatpak-transaction.c
+++ b/common/flatpak-transaction.c
@@ -2066,7 +2066,8 @@ op_get_related (FlatpakTransaction           *self,
 
   if (transaction_is_local_only (self, op->kind))
     related = flatpak_dir_find_local_related_for_metadata (priv->dir, op->ref,
-                                                           op->remote, op->resolved_metakey,
+                                                           NULL, /* remote could differ from op->remote */
+                                                           op->resolved_metakey,
                                                            NULL, &related_error);
   else
     related = flatpak_dir_find_remote_related_for_metadata (priv->dir, state, op->ref,
@@ -2109,7 +2110,7 @@ add_related (FlatpakTransaction          *self,
           if (!rel->delete)
             continue;
 
-          related_op = flatpak_transaction_add_op (self, op->remote, rel->ref,
+          related_op = flatpak_transaction_add_op (self, rel->remote, rel->ref,
                                                    NULL, NULL, NULL, NULL,
                                                    FLATPAK_TRANSACTION_OPERATION_UNINSTALL,
                                                    error);
@@ -2132,7 +2133,7 @@ add_related (FlatpakTransaction          *self,
           if (!rel->download)
             continue;
 
-          related_op = flatpak_transaction_add_op (self, op->remote, rel->ref,
+          related_op = flatpak_transaction_add_op (self, rel->remote, rel->ref,
                                                    (const char **) rel->subpaths,
                                                    NULL, NULL, NULL,
                                                    FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE,
@@ -2629,8 +2630,10 @@ flatpak_transaction_add_rebase (FlatpakTransaction *self,
                                 const char        **previous_ids,
                                 GError            **error)
 {
+  FlatpakTransactionPrivate *priv = flatpak_transaction_get_instance_private (self);
   const char *all_paths[] = { NULL };
   g_autoptr(FlatpakDecomposed) decomposed = NULL;
+  g_autofree char *installed_origin = NULL;
 
   g_return_val_if_fail (ref != NULL, FALSE);
   g_return_val_if_fail (remote != NULL, FALSE);
@@ -2645,6 +2648,9 @@ flatpak_transaction_add_rebase (FlatpakTransaction *self,
   if (subpaths == NULL)
     subpaths = all_paths;
 
+  if (dir_ref_is_installed (priv->dir, decomposed, &installed_origin, NULL))
+    remote = installed_origin;
+
   return flatpak_transaction_add_ref (self, remote, decomposed, subpaths, previous_ids, NULL, FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE, NULL, NULL, error);
 }
 
@@ -4462,10 +4468,9 @@ flatpak_transaction_normalize_ops (FlatpakTransaction *self)
 
           if (dir_ref_is_installed (priv->dir, op->ref, NULL, &deploy_data))
             {
-              /* Don't use the remote from related ref on update, always use
-                 the current remote. */
-              g_free (op->remote);
-              op->remote = g_strdup (flatpak_deploy_data_get_origin (deploy_data));
+              /* The remote should have already been set to the installed ref
+               * origin so that the resolved commit definitely exists there */
+              g_assert (g_strcmp0 (op->remote, flatpak_deploy_data_get_origin (deploy_data)) == 0);
 
               op->kind = FLATPAK_TRANSACTION_OPERATION_UPDATE;
             }
diff --git a/tests/testlibrary.c b/tests/testlibrary.c
index c7aff93f..c428ceb6 100644
--- a/tests/testlibrary.c
+++ b/tests/testlibrary.c
@@ -39,6 +39,7 @@ static void empty_installation (FlatpakInstallation *inst);
 static void make_test_app (const char *app_repo_name);
 static void update_test_app (void);
 static void update_test_app_extension_version (void);
+static void update_test_app_extension (void);
 static void update_test_runtime (void);
 static void update_repo (const char *update_repo_name);
 static void rename_test_app (const char *update_repo_name);
@@ -2471,6 +2472,26 @@ rename_test_app (const char *update_repo_name)
 }
 
 static void
+update_test_app_extension (void)
+{
+  g_autofree char *app_plugin_ref = NULL;
+  char *argv[] = { "flatpak", "build-commit-from", "--force",
+                   "--gpg-homedir=", "--gpg-sign=",
+                   "--src-repo=repos/test", "repos/test",
+                   NULL, NULL };
+  g_auto(GStrv) gpgargs = NULL;
+
+  gpgargs = g_strsplit (gpg_args, " ", 0);
+  app_plugin_ref = g_strdup_printf ("runtime/org.test.Hello.Plugin.fun/%s/v1",
+                                    flatpak_get_default_arch ());
+  argv[3] = gpgargs[0];
+  argv[4] = gpgargs[1];
+  argv[7] = app_plugin_ref;
+
+  run_test_subprocess (argv, RUN_TEST_SUBPROCESS_DEFAULT);
+}
+
+static void
 update_test_runtime (void)
 {
   g_autofree char *arg0 = NULL;
@@ -3817,6 +3838,97 @@ test_transaction_app_runtime_same_remote (void)
   remove_remote_user ("aaatest-runtime-only-repo");
 }
 
+/* Test that an installed related ref is updated from its origin remote even if
+ * the thing it's related to comes from a different remote which also provides
+ * the related ref */
+static void
+test_transaction_update_related_from_different_remote (void)
+{
+  g_autoptr(FlatpakInstallation) inst = NULL;
+  g_autoptr(FlatpakTransaction) transaction = NULL;
+  g_autoptr(FlatpakInstalledRef) installed_ref = NULL;
+  g_autoptr(FlatpakRemoteRef) remote_ref = NULL;
+  g_autoptr(GError) error = NULL;
+  g_autofree char *app = NULL;
+  g_autofree char *app_plugin = NULL;
+  const char *app_origin = repo_name;
+  const char *app_plugin_origin = "test-without-runtime-repo";
+  gboolean res;
+
+  app = g_strdup_printf ("app/org.test.Hello/%s/master",
+                         flatpak_get_default_arch ());
+  app_plugin = g_strdup_printf ("runtime/org.test.Hello.Plugin.fun/%s/v1",
+                                flatpak_get_default_arch ());
+
+  inst = flatpak_installation_new_user (NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (inst);
+
+  empty_installation (inst);
+
+  add_remote_user ("test-without-runtime", NULL);
+
+  /* Drop caches so we find the new remote */
+  flatpak_installation_drop_caches (inst, NULL, &error);
+  g_assert_no_error (error);
+
+  /* Install the plugin only from its remote */
+  transaction = flatpak_transaction_new_for_installation (inst, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (transaction);
+
+  res = flatpak_transaction_add_install (transaction, app_plugin_origin, app_plugin, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_true (res);
+
+  res = flatpak_transaction_run (transaction, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_true (res);
+
+  /* Update the related ref in the main repo, so we can check that it's not
+   * updated since we should check for updates in its origin repo */
+  update_test_app_extension ();
+  update_repo ("test");
+
+  /* Install the app from the main remote. The plugin should not be updated */
+  g_clear_object (&transaction);
+  transaction = flatpak_transaction_new_for_installation (inst, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (transaction);
+
+  res = flatpak_transaction_add_install (transaction, app_origin, app, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_true (res);
+
+  res = flatpak_transaction_run (transaction, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_true (res);
+
+  /* Check to make sure the plugin is on a different commit locally than in the
+   * main remote */
+  installed_ref = flatpak_installation_get_installed_ref (inst, FLATPAK_REF_KIND_RUNTIME,
+                                                          "org.test.Hello.Plugin.fun",
+                                                          NULL, "v1", NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (installed_ref);
+
+  remote_ref = flatpak_installation_fetch_remote_ref_sync (inst, app_origin, FLATPAK_REF_KIND_RUNTIME,
+                                                          "org.test.Hello.Plugin.fun",
+                                                          NULL, "v1", NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (remote_ref);
+
+  g_assert_cmpstr (flatpak_installed_ref_get_origin (installed_ref), !=, app_origin);
+  g_assert_cmpstr (flatpak_ref_get_commit (FLATPAK_REF (installed_ref)), ==,
+                   flatpak_installed_ref_get_latest_commit (installed_ref));
+  g_assert_cmpstr (flatpak_ref_get_commit (FLATPAK_REF (installed_ref)), !=,
+                   flatpak_ref_get_commit (FLATPAK_REF (remote_ref)));
+
+  /* Reset things */
+  empty_installation (inst);
+  remove_remote_user ("test-without-runtime-repo");
+}
+
 typedef struct
 {
   GMainLoop *loop;
@@ -4736,6 +4848,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/library/transaction-deps", test_transaction_deps);
   g_test_add_func ("/library/transaction-install-local", test_transaction_install_local);
   g_test_add_func ("/library/transaction-app-runtime-same-remote", test_transaction_app_runtime_same_remote);
+  g_test_add_func ("/library/transaction-update-related-from-different-remote", test_transaction_update_related_from_different_remote);
   g_test_add_func ("/library/instance", test_instance);
   g_test_add_func ("/library/update-subpaths", test_update_subpaths);
   g_test_add_func ("/library/overrides", test_overrides);