Merge remote-tracking branch 'origin/main' into claude/spelunk-api-key-tenant-6373cf

src/DodoSSH.Client.App/Platform/LinuxUpdateChannel.cs

@@ -127,17 +127,61 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
         this.installedVersion = installedVersion;
         this.handToInstaller = handToInstaller;
 
-        packageName = string.Equals(channel, "nightly", StringComparison.Ordinal)
-            ? "dodossh-nightly"
-            : "dodossh";
+        packageName = PackageFor(channel);
+    }
+
+    /// <summary>The channel for a copy under the RPM's install root, if rpm says the package is there.</summary>
+    internal static IUpdateChannel ForInstall(string channel) =>
+        ForInstall(channel, InstalledRpmVersion, () => new LinuxUpdateChannel(channel));
+
+    /// <summary>
+    /// The channel for a copy under the RPM's install root, with rpm supplied, which is how the tests
+    /// reach it.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// <b>The path says where this copy is and only rpm says who put it there.</b>
+    /// <c>UpdateChannels.LinuxChannel</c> has already established the first, and on an RPM distribution
+    /// that is the whole story. It is not on any other: a locally built pacman or dpkg package that mirrors
+    /// the spec puts the same tree at the same path, and this channel then offered every nightly as newer
+    /// than a version nobody could read, fetched a package that machine has no way to install, and did it
+    /// again every six hours.
+    /// </para>
+    /// <para>
+    /// Asked once, at composition, for the reason the rest of this decision is: who installed this copy
+    /// does not change while it runs. It costs one <c>rpm</c> launch at start on a machine whose path
+    /// already matched, and nothing at all anywhere else.
+    /// </para>
+    /// </remarks>
+    /// <param name="channel">The build's channel, which names the package.</param>
+    /// <param name="installedVersion">What rpm says is installed under a package name, or null.</param>
+    /// <param name="whenOwned">The channel to use when rpm does own this copy.</param>
+    internal static IUpdateChannel ForInstall(
+        string channel,
+        Func<string, string?> installedVersion,
+        Func<IUpdateChannel> whenOwned)
+    {
+        ArgumentException.ThrowIfNullOrEmpty(channel);
+        ArgumentNullException.ThrowIfNull(installedVersion);
+        ArgumentNullException.ThrowIfNull(whenOwned);
+
+        var package = PackageFor(channel);
+
+        return installedVersion(package) is null
+            ? new UnavailableUpdateChannel(
+                $"This copy of DodoSSH was installed outside the {package} RPM — rpm has no record of that "
+                + "package here — and an RPM is the only kind of update it can fetch. Update it with the "
+                + "package manager that installed it.")
+            : whenOwned();
     }
 
     /// <inheritdoc />
     /// <remarks>
     /// True unconditionally, because whether this copy is one that can be updated was already decided by
-    /// <see cref="UpdateChannels.ForThisMachine"/> — a build run from a checkout never reaches this class
-    /// at all. The same split Velopack's side uses, where <c>IsInstalled</c> is checked once at
-    /// composition rather than at every call site.
+    /// <see cref="UpdateChannels.ForThisMachine"/> and <see cref="ForInstall(string)"/> — a build run from
+    /// a checkout, or installed by something other than rpm, never reaches this class at all. The same
+    /// split Velopack's side uses, where <c>IsInstalled</c> is checked once at composition rather than at
+    /// every call site.
     /// </remarks>
     public bool IsSupported => true;
 
@@ -184,9 +228,20 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
                 $"The {channel} release's {manifestName} could not be read as a manifest.");
         }
 
+        // ◆ No answer from rpm is a failure, never a version. ForInstall only built this class because
+        // rpm answered, so silence now is a package removed under a running process or an rpm that
+        // stopped working — and any stand-in number would make every published build look newer and
+        // fetch a package for a machine that cannot install it, which is what this once did.
+        if (installedVersion(packageName) is not { } installed)
+        {
+            throw new InvalidOperationException(
+                $"rpm has no record of {packageName}, so there is no installed version to compare the "
+                + $"{channel} release against.");
+        }
+
         // The one place null is returned, and it means what null is documented to mean: this build is
         // current. Compared the way rpm compares, because rpm is what accepts or refuses the install.
-        if (RpmVersion.Compare(manifest.RpmVersion, installedVersion(packageName) ?? "0") <= 0)
+        if (RpmVersion.Compare(manifest.RpmVersion, installed) <= 0)
         {
             return null;
         }
@@ -324,6 +379,12 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
             ? $"{RepositoryApi}/releases/tags/nightly-desktop"
             : $"{RepositoryApi}/releases/latest";
 
+    /// <summary>The package a channel's builds are published as.</summary>
+    private static string PackageFor(string channel) =>
+        string.Equals(channel, "nightly", StringComparison.Ordinal)
+            ? "dodossh-nightly"
+            : "dodossh";
+
     private static string? Asset(LinuxForgeRelease? release, string name) =>
         release?.Assets?.FirstOrDefault(
             asset => string.Equals(asset.Name, name, StringComparison.Ordinal))?.DownloadUrl;
@@ -365,9 +426,11 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
     /// <c>scripts/ci-linux-rpm.sh</c> performs, and two copies of one rule is a pair that can disagree.
     /// </para>
     /// <para>
-    /// A machine that cannot answer gets null, which the caller reads as <c>0</c> and makes every
-    /// published build look newer. That is the wrong way to fail, chosen deliberately: the failure is then
-    /// an install PackageKit declines rather than a security fix that never arrives.
+    /// A machine that cannot answer gets null — no <c>rpm</c> at all, or one with no such package — and
+    /// null means "rpm does not own this copy", never a version. It used to be read as <c>0</c>, on the
+    /// argument that the worst case was an install PackageKit declined. That holds only where PackageKit
+    /// installs RPMs: on any other distribution it made every published build look newer, forever, and
+    /// fetched a package that machine could not install. See <see cref="ForInstall(string)"/>.
     /// </para>
     /// </remarks>
     private static string? InstalledRpmVersion(string packageName)

src/DodoSSH.Client.App/Platform/VelopackUpdateChannel.cs

@@ -66,21 +66,23 @@ internal static class UpdateChannels
         }
     }
 
-    /// <summary>The Linux channel, if this copy is one a package manager put here.</summary>
+    /// <summary>The Linux channel, if this copy is one the RPM put here.</summary>
     /// <remarks>
     /// <para>
     /// <b>The question Velopack answers with <c>IsInstalled</c>, answered by looking at where this process
-    /// lives.</b> The RPM owns <c>/opt/DodoSSH</c> or <c>/opt/DodoSSH.Nightly</c> — the spec puts it there
-    /// and nothing else writes to <c>/opt</c> under those names — so a build running from anywhere else is
-    /// a checkout somebody started, an extracted archive, or a tree copied by hand. None of those can be
-    /// updated by installing a package over them, and offering it would be an offer that cannot be carried
-    /// out.
+    /// lives and then asking rpm.</b> The RPM owns <c>/opt/DodoSSH</c> or <c>/opt/DodoSSH.Nightly</c> — the
+    /// spec puts it there — so a build running from anywhere else is a checkout somebody started, an
+    /// extracted archive, or a tree copied by hand. None of those can be updated by installing a package
+    /// over them, and offering it would be an offer that cannot be carried out.
     /// </para>
     /// <para>
-    /// Checked by path rather than by asking rpm which package owns the executable, which would be the
-    /// more thorough answer and costs a process launch on every start for a distinction nothing acts on
-    /// differently. The install root is a constant in two places — here and the packaging script — and
-    /// they are named in each other's comments.
+    /// <b>The path is necessary and not sufficient.</b> It used to be the whole check, on the argument
+    /// that asking rpm cost a process launch for a distinction nothing acted on differently. Something
+    /// does: a locally built pacman package that mirrors the spec puts the same tree at the same path,
+    /// and that copy was offered every nightly as an RPM it could never install. So a matching path is
+    /// then put to rpm — see <see cref="LinuxUpdateChannel.ForInstall(string)"/> — and the launch is paid
+    /// only where the path already matched. The install root is a constant in two places — here and the
+    /// packaging script — and they are named in each other's comments.
     /// </para>
     /// </remarks>
     [SupportedOSPlatform("linux")]
@@ -91,7 +93,7 @@ internal static class UpdateChannels
         var here = AppContext.BaseDirectory.TrimEnd(Path.DirectorySeparatorChar);
 
         return string.Equals(here, root, StringComparison.Ordinal)
-            ? new LinuxUpdateChannel(DesktopChannel.Name)
+            ? LinuxUpdateChannel.ForInstall(DesktopChannel.Name)
             : new UnavailableUpdateChannel();
     }
 }

src/DodoSSH.Client.App/Views/SettingsGeneralPage.axaml

@@ -84,7 +84,10 @@
                          IsVisible="{Binding Updates.Status, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
               <TextBlock Classes="settingsrowcaption"
                          Text="This copy of DodoSSH cannot replace itself, so nothing above does anything. That is what a build run from a source checkout looks like, and also what a copy somebody unzipped by hand looks like — it is the installer that registers the update path."
-                         IsVisible="{Binding Updates.IsUnsupported}" />
+                         IsVisible="{Binding Updates.IsUnsupportedForTheUsualReason}" />
+              <TextBlock Classes="settingsrowcaption"
+                         Text="{Binding Updates.UnsupportedReason}"
+                         IsVisible="{Binding Updates.UnsupportedReason, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
             </StackPanel>
           </Border>
 

src/DodoSSH.Client.Session/ClientUpdates.cs

@@ -141,9 +141,32 @@ public interface IUpdateChannel
 /// <see cref="UnavailableDeviceKeyStore"/>: the caller then has one shape to write against, and "no
 /// updater here" is a thing the interface can express rather than a case every call site has to remember.
 /// </para>
+/// <para>
+/// <b>One situation outside those four carries a <see cref="Reason"/>.</b> A Linux copy sitting exactly
+/// where the RPM puts it that no RPM owns — built locally and installed by another distribution's package
+/// manager. It looks installed, so the sentence about checkouts and unzipped copies would describe the
+/// wrong machine, and the person reading it needs telling which updater is theirs.
+/// </para>
 /// </remarks>
 public sealed class UnavailableUpdateChannel : IUpdateChannel
 {
+    /// <summary>Creates one for the situations that need no telling apart.</summary>
+    public UnavailableUpdateChannel()
+    {
+    }
+
+    /// <summary>Creates one whose situation is worth a sentence of its own.</summary>
+    /// <param name="reason">What to tell the user, as a sentence they can act on.</param>
+    public UnavailableUpdateChannel(string reason)
+    {
+        ArgumentException.ThrowIfNullOrWhiteSpace(reason);
+
+        Reason = reason;
+    }
+
+    /// <summary>Why this copy cannot replace itself, where that is not the usual why.</summary>
+    public string? Reason { get; }
+
     /// <inheritdoc />
     public bool IsSupported => false;
 

src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs

@@ -126,6 +126,7 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
         this.restart = restart;
 
         CurrentVersion = updates.CurrentVersion;
+        UnsupportedReason = (updates as UnavailableUpdateChannel)?.Reason;
         isAutomatic = settings.Read().AutomaticUpdateChecks;
         state = updates.IsSupported ? UpdateState.Idle : UpdateState.Unsupported;
     }
@@ -133,6 +134,16 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
     /// <summary>What this build calls itself.</summary>
     internal string CurrentVersion { get; }
 
+    /// <summary>Why this copy cannot replace itself, when the channel had something particular to say.</summary>
+    /// <remarks>
+    /// Null in the ordinary cases, where the preferences screen's own sentence about checkouts and unzipped
+    /// copies is the right one. See <see cref="UnavailableUpdateChannel"/> for the case where it is not.
+    /// </remarks>
+    internal string? UnsupportedReason { get; }
+
+    /// <summary>Unsupported, with nothing more particular to say than the screen's own sentence.</summary>
+    internal bool IsUnsupportedForTheUsualReason => IsUnsupported && UnsupportedReason is null;
+
     [ObservableProperty]
     private UpdateState state;
 
@@ -248,6 +259,7 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
     {
         OnPropertyChanged(nameof(IsSupported));
         OnPropertyChanged(nameof(IsUnsupported));
+        OnPropertyChanged(nameof(IsUnsupportedForTheUsualReason));
         OnPropertyChanged(nameof(IsChecking));
         OnPropertyChanged(nameof(IsDownloading));
         OnPropertyChanged(nameof(IsReady));