Просмотр исходного кода

Only offer a service link a browser can actually follow

Two bugs, one cause: three places each decided for themselves what a service's
URL was, and all three were wrong.

The card derived every link as http:// regardless of the port, so SSH on 22
rendered as http://10.0.0.5:22/ — a link that looks real, invites a click and
cannot load. It was reading Network.Protocol as though it named a scheme, but
discovery writes the transport there (TCP), which says nothing about what rides
on top of it. The dependency trees had it worse: they fed NetworkString() into
an href, and that was display text — "Ip: 10.0.0.5:3000", prefix and trailing
space included — so the link was dead on arrival.

Both rules now live in one place. A scheme is only inferred where the
convention is genuinely a web one; ssh, smb, mqtt, dns and anything uncurated
get no link at all, because a dead link is worse than none. A URL someone typed
by hand still always wins, and an explicit http or https in the protocol field
is still believed, which is the only thing that can answer on an uncurated port.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tim Jones 1 день назад
Родитель
Сommit
bed8c3ab95

+ 15 - 21
RackPeek.Domain/Resources/Services/Service.cs

@@ -1,30 +1,24 @@
-using System.Text;
-
 namespace RackPeek.Domain.Resources.Services;
 
 public class Service : Resource {
     public const string KindLabel = "Service";
     public Network? Network { get; set; }
 
-    public string NetworkString() {
-        if (Network == null) return string.Empty;
-
-        if (!string.IsNullOrEmpty(Network.Url)) return Network.Url;
-
-        var stringBuilder = new StringBuilder();
-        if (!string.IsNullOrEmpty(Network.Ip)) {
-            stringBuilder.Append("Ip: ");
-            stringBuilder.Append(Network.Ip);
-            if (Network.Port.HasValue) {
-                stringBuilder.Append(':');
-                stringBuilder.Append(Network.Port.Value);
-            }
-
-            stringBuilder.Append(' ');
-        }
-
-        return stringBuilder.ToString();
-    }
+    /// <summary>
+    ///     Where this service answers, for showing to a person. Display text, never a
+    ///     link — <see cref="BrowsableUrl" /> is the link.
+    /// </summary>
+    public string NetworkString() =>
+        !string.IsNullOrEmpty(Network?.Url)
+            ? Network.Url
+            : ServiceEndpoint.Describe(Network);
+
+    /// <summary>
+    ///     A link a browser can follow, or null when this port serves something a browser
+    ///     cannot open.
+    /// </summary>
+    public string? BrowsableUrl(string? fallbackIp = null) =>
+        ServiceEndpoint.BrowsableUrl(Network, fallbackIp);
 }
 
 public class Network {

+ 78 - 0
RackPeek.Domain/Resources/Services/ServiceEndpoint.cs

@@ -0,0 +1,78 @@
+namespace RackPeek.Domain.Resources.Services;
+
+/// <summary>
+///     Where a service answers, and whether a browser can do anything with it.
+/// </summary>
+public static class ServiceEndpoint {
+    /// <summary>
+    ///     Ports a browser opens over TLS. Proxmox serves its management UI on 8006 and
+    ///     redirects plain HTTP, so guessing http there costs the user a round trip.
+    /// </summary>
+    private static readonly HashSet<int> _https = [443, 8006, 8443, 9443];
+
+    /// <summary>Ports a browser opens in the clear.</summary>
+    private static readonly HashSet<int> _http = [
+        80, 631, 3000, 5000, 7860, 8000, 8080, 8096, 8123, 9000, 9090, 11434, 32400
+    ];
+
+    /// <summary>
+    ///     The address a service answers on — <c>10.0.50.105:3000</c> — for showing to a
+    ///     person. Empty when there is no address to show. This is display text and never
+    ///     a link: see <see cref="BrowsableUrl" /> for that.
+    /// </summary>
+    public static string Describe(Network? network) {
+        if (string.IsNullOrWhiteSpace(network?.Ip))
+            return string.Empty;
+
+        return network.Port.HasValue
+            ? $"{network.Ip}:{network.Port.Value}"
+            : network.Ip;
+    }
+
+    /// <summary>
+    ///     A link a browser can actually follow, or null when it cannot.
+    ///     <para>
+    ///         A URL somebody typed always wins. Failing that the scheme has to be
+    ///         inferred, and a port number is a convention rather than a promise — so this
+    ///         answers only where the convention is a web one. SSH, SMB, MQTT, DNS and
+    ///         anything uncurated get no link at all, which is more useful than an
+    ///         <c>http://</c> that cannot load: a dead link invites a click and wastes it.
+    ///     </para>
+    ///     <para>
+    ///         <c>protocol</c> is only consulted when it names a scheme. Discovery writes
+    ///         the transport there — <c>TCP</c> — which says nothing about what rides on
+    ///         top of it.
+    ///     </para>
+    /// </summary>
+    public static string? BrowsableUrl(Network? network, string? fallbackIp = null) {
+        if (network == null)
+            return null;
+
+        if (!string.IsNullOrWhiteSpace(network.Url))
+            return network.Url;
+
+        var ip = !string.IsNullOrWhiteSpace(network.Ip) ? network.Ip : fallbackIp;
+
+        if (string.IsNullOrWhiteSpace(ip) || network.Port is not { } port)
+            return null;
+
+        var scheme = SchemeFor(port, network.Protocol);
+
+        if (scheme == null)
+            return null;
+
+        return new UriBuilder(scheme, ip) { Port = port }.Uri.ToString();
+    }
+
+    private static string? SchemeFor(int port, string? protocol) {
+        var stated = protocol?.Trim().ToLowerInvariant();
+
+        if (stated is "http" or "https")
+            return stated;
+
+        if (_https.Contains(port))
+            return "https";
+
+        return _http.Contains(port) ? "http" : null;
+    }
+}

+ 30 - 11
Shared.Rcl/Components/HardwareDependencyTreeComponent.razor

@@ -55,6 +55,7 @@ else
 
                                     case Service service:
                                         var endpoint = service.NetworkString();
+                                        var browsable = service.BrowsableUrl();
 
                                         <NavLink href="@($"resources/services/{Uri.EscapeDataString(service.Name)}")"
                                                  class="block">
@@ -69,13 +70,34 @@ else
                                                     @if (!string.IsNullOrWhiteSpace(endpoint))
                                                     {
                                                         <span> - </span>
-                                                        <a href="@endpoint"
-                                                           target="_blank"
-                                                           rel="noopener noreferrer"
-                                                           class="underline hover:text-emerald-400"
-                                                           @onclick:stopPropagation>
-                                                            @endpoint
-                                                        </a>
+
+                                                        @if (!string.IsNullOrWhiteSpace(browsable))
+
+                                                        {
+
+                                                            <a href="@browsable"
+
+                                                               target="_blank"
+
+                                                               rel="noopener noreferrer"
+
+                                                               class="underline hover:text-emerald-400"
+
+                                                               @onclick:stopPropagation>
+
+                                                                @endpoint
+
+                                                            </a>
+
+                                                        }
+
+                                                        else
+
+                                                        {
+
+                                                            <span>@endpoint</span>
+
+                                                        }
                                                     }
                                                 </div>
                                             </div>
@@ -107,10 +129,7 @@ else
     {
         var endpoint = service.NetworkString();
 
-        if (string.IsNullOrWhiteSpace(endpoint))
-            return null;
-
-        return endpoint;
+        return string.IsNullOrWhiteSpace(endpoint) ? null : endpoint;
     }
 
 }

+ 4 - 21
Shared.Rcl/Services/ServiceCardComponent.razor

@@ -461,27 +461,10 @@
         Nav.NavigateTo($"resources/services/{Uri.EscapeDataString(newName)}");
     }
 
-    private string? GetBrowsableHref()
-    {
-        var ip = Service.Network?.Ip ?? EffectiveIp;
-        var port = Service.Network?.Port;
-
-        if (string.IsNullOrWhiteSpace(ip) || port is null)
-            return null;
-
-        var proto = Service.Network?.Protocol?.Trim().ToLowerInvariant();
-
-        var scheme = proto switch
-        {
-            "https" => "https",
-            "http" => "http",
-            _ => "http"
-        };
-
-        // Build a correct absolute URL
-        var ub = new UriBuilder(scheme, ip) { Port = port.Value };
-        return ub.Uri.ToString();
-    }
+    // A port is a convention, not a promise: ssh on 22 is not browsable and an http://
+    // link to it only wastes a click. The domain owns that judgement so this page, the
+    // dependency trees and anything else agree on it.
+    private string? GetBrowsableHref() => Service.BrowsableUrl(EffectiveIp);
 }
 
 @code

+ 30 - 11
Shared.Rcl/Systems/SystemDependencyTreeComponent.razor

@@ -18,6 +18,7 @@ else
                 {
                     case Service service:
                         var endpoint = service.NetworkString();
+                                        var browsable = service.BrowsableUrl();
 
                         <NavLink href="@($"resources/services/{Uri.EscapeDataString(service.Name)}")"
                                  class="block">
@@ -32,13 +33,34 @@ else
                                     @if (!string.IsNullOrWhiteSpace(endpoint))
                                     {
                                         <span> - </span>
-                                        <a href="@endpoint"
-                                           target="_blank"
-                                           rel="noopener noreferrer"
-                                           class="underline hover:text-emerald-400"
-                                           @onclick:stopPropagation>
-                                            @endpoint
-                                        </a>
+
+                                        @if (!string.IsNullOrWhiteSpace(browsable))
+
+                                        {
+
+                                            <a href="@browsable"
+
+                                               target="_blank"
+
+                                               rel="noopener noreferrer"
+
+                                               class="underline hover:text-emerald-400"
+
+                                               @onclick:stopPropagation>
+
+                                                @endpoint
+
+                                            </a>
+
+                                        }
+
+                                        else
+
+                                        {
+
+                                            <span>@endpoint</span>
+
+                                        }
                                     }
                                 </div>
 
@@ -83,10 +105,7 @@ else
     {
         var endpoint = service.NetworkString();
 
-        if (string.IsNullOrWhiteSpace(endpoint))
-            return null;
-
-        return endpoint;
+        return string.IsNullOrWhiteSpace(endpoint) ? null : endpoint;
     }
 
 }

+ 96 - 0
Tests.Discovery/ServiceEndpointTests.cs

@@ -0,0 +1,96 @@
+using RackPeek.Domain.Resources.Services;
+
+namespace Tests.Discovery;
+
+/// <summary>
+///     Where a service answers, and whether a browser can do anything with it.
+///     <para>
+///         Both of these were wrong in the UI. The endpoint was rendered as
+///         <c>Ip: 10.0.50.105:3000</c> and then used as a link target, so clicking it did
+///         nothing. And the scheme fell back to <c>http</c> for every port, so an SSH
+///         service showed <c>http://10.0.50.105:22/</c> — a link that looks real, invites
+///         a click and cannot load.
+///     </para>
+/// </summary>
+public class ServiceEndpointTests {
+    private static Network Net(int? port, string? protocol = "TCP", string? ip = "10.0.50.105", string? url = null) =>
+        new() { Ip = ip, Port = port, Protocol = protocol, Url = url };
+
+    private static Service Svc(Network? network) =>
+        new() { Kind = Service.KindLabel, Name = "svc", Network = network };
+
+    [Fact]
+    public void An_endpoint_reads_as_an_address_and_nothing_else() =>
+        // It goes on screen next to the service name; a label belongs in the markup.
+        Assert.Equal("10.0.50.105:3000", Svc(Net(3000)).NetworkString());
+
+    [Fact]
+    public void An_endpoint_with_no_port_is_just_the_address() =>
+        Assert.Equal("10.0.50.105", Svc(Net(null)).NetworkString());
+
+    [Fact]
+    public void A_service_with_no_network_has_no_endpoint() =>
+        Assert.Equal(string.Empty, Svc(null).NetworkString());
+
+    [Theory]
+    [InlineData(22)] // ssh
+    [InlineData(445)] // smb
+    [InlineData(1883)] // mqtt
+    [InlineData(53)] // dns
+    [InlineData(3306)] // mysql
+    [InlineData(9987)] // nothing curated
+    public void A_port_a_browser_cannot_open_gets_no_link(int port) =>
+        Assert.Null(Svc(Net(port)).BrowsableUrl());
+
+    [Theory]
+    [InlineData(80, "http://10.0.50.105/")]
+    [InlineData(3000, "http://10.0.50.105:3000/")]
+    [InlineData(8123, "http://10.0.50.105:8123/")]
+    [InlineData(9000, "http://10.0.50.105:9000/")]
+    public void A_web_port_gets_a_link(int port, string expected) =>
+        Assert.Equal(expected, Svc(Net(port)).BrowsableUrl());
+
+    [Theory]
+    [InlineData(443, "https://10.0.50.105/")]
+    [InlineData(8006, "https://10.0.50.105:8006/")]
+    [InlineData(8443, "https://10.0.50.105:8443/")]
+    public void A_tls_port_gets_an_https_link(int port, string expected) =>
+        // Proxmox on 8006 redirects plain HTTP, so guessing http costs a round trip.
+        Assert.Equal(expected, Svc(Net(port)).BrowsableUrl());
+
+    [Fact]
+    public void The_transport_is_not_mistaken_for_a_scheme() {
+        // Discovery writes "TCP" into protocol, which says nothing about what rides on
+        // top of it. Reading that as a scheme is what produced http:// on port 22.
+        Assert.Null(Svc(Net(22, "TCP")).BrowsableUrl());
+        Assert.Equal("http://10.0.50.105:3000/", Svc(Net(3000, "TCP")).BrowsableUrl());
+    }
+
+    [Theory]
+    [InlineData("https", "https://10.0.50.105:9999/")]
+    [InlineData("HTTP", "http://10.0.50.105:9999/")]
+    public void A_protocol_that_names_a_scheme_is_believed(string protocol, string expected) =>
+        // On an uncurated port this is the only thing that can answer.
+        Assert.Equal(expected, Svc(Net(9999, protocol)).BrowsableUrl());
+
+    [Fact]
+    public void A_url_someone_typed_always_wins() {
+        Service service = Svc(Net(22, "TCP", url: "https://git.example.com/"));
+
+        Assert.Equal("https://git.example.com/", service.BrowsableUrl());
+        Assert.Equal("https://git.example.com/", service.NetworkString());
+    }
+
+    [Fact]
+    public void A_service_with_no_address_of_its_own_can_borrow_its_hosts() {
+        // The card resolves the host's address when the service carries none.
+        Service service = Svc(Net(8080, ip: null));
+
+        Assert.Null(service.BrowsableUrl());
+        Assert.Equal("http://10.0.20.7:8080/", service.BrowsableUrl("10.0.20.7"));
+    }
+
+    [Fact]
+    public void A_service_with_no_port_is_not_guessed_at() =>
+        Assert.Null(Svc(Net(null)).BrowsableUrl());
+}