Say whether an approval was refused, unanswered, or never shown

client/internal/engine_vnc.go

@@ -321,11 +321,29 @@ func (a *vncApprover) Request(ctx context.Context, info vncserver.ApprovalInfo)
 		Metadata: meta,
 	})
 	if err != nil {
-		return vncserver.ApprovalDecision{}, err
+		return vncserver.ApprovalDecision{}, approvalCause(err)
 	}
 	return vncserver.ApprovalDecision{ViewOnly: d.ViewOnly}, nil
 }
 
+// approvalCause restates a broker failure as the cause the VNC server
+// classifies, so the peer that dialled learns whether the user refused or
+// never answered. The broker's denial and timeout carry nothing beyond the
+// sentinel, so those are returned as the server's own; the two unavailable
+// cases differ in a way worth keeping, so they are wrapped.
+func approvalCause(err error) error {
+	switch {
+	case errors.Is(err, approval.ErrDenied):
+		return vncserver.ErrApprovalDenied
+	case errors.Is(err, approval.ErrTimeout):
+		return vncserver.ErrApprovalTimeout
+	case errors.Is(err, approval.ErrNoSubscriber), errors.Is(err, approval.ErrPromptNotShown):
+		return fmt.Errorf("%w: %w", vncserver.ErrApprovalUnavailable, err)
+	default:
+		return err
+	}
+}
+
 func displayPeer(info vncserver.ApprovalInfo) string {
 	if info.Initiator != "" {
 		return info.Initiator

client/internal/engine_vnc_test.go

@@ -0,0 +1,60 @@
+//go:build !js && !ios && !android
+
+package internal
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/netbirdio/netbird/client/internal/approval"
+	vncserver "github.com/netbirdio/netbird/client/vnc/server"
+)
+
+// TestApprovalCause checks that each way the broker can refuse arrives at
+// the VNC server as the matching cause. Without this the server sees one
+// opaque error and tells every caller it was denied, which reads as "the
+// user said no" even when no prompt was ever answered or shown.
+func TestApprovalCause(t *testing.T) {
+	cases := []struct {
+		name string
+		in   error
+		want error
+	}{
+		{"denied", approval.ErrDenied, vncserver.ErrApprovalDenied},
+		{"timeout", approval.ErrTimeout, vncserver.ErrApprovalTimeout},
+		{"no_subscriber", approval.ErrNoSubscriber, vncserver.ErrApprovalUnavailable},
+		{"prompt_not_shown", approval.ErrPromptNotShown, vncserver.ErrApprovalUnavailable},
+		// Wrapped on the way in: the broker may add context, and the
+		// classification must survive it.
+		{"wrapped_timeout", fmt.Errorf("request: %w", approval.ErrTimeout), vncserver.ErrApprovalTimeout},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := approvalCause(tc.in)
+			assert.ErrorIs(t, got, tc.want, "broker error %v must classify as %v", tc.in, tc.want)
+		})
+	}
+}
+
+// TestApprovalCausePreservesUnavailableDetail: the two unavailable causes
+// mean different things to whoever reads the daemon log ("nobody was
+// listening" versus "somebody was, and never got it"), so collapsing them
+// onto one sentinel must not discard which one occurred.
+func TestApprovalCausePreservesUnavailableDetail(t *testing.T) {
+	got := approvalCause(approval.ErrPromptNotShown)
+	assert.ErrorIs(t, got, vncserver.ErrApprovalUnavailable)
+	assert.ErrorIs(t, got, approval.ErrPromptNotShown, "the specific cause must stay readable in the log")
+}
+
+// TestApprovalCausePassesThroughUnknown keeps an error the mapping does not
+// recognise intact, so the daemon log still shows what actually happened.
+// The server rejects it as a denial either way.
+func TestApprovalCausePassesThroughUnknown(t *testing.T) {
+	for _, err := range []error{context.Canceled, errors.New("something else")} {
+		assert.ErrorIs(t, approvalCause(err), err, "unknown cause must pass through unchanged")
+	}
+}

client/vnc/server/server.go

@@ -33,14 +33,16 @@ const (
 // stable so clients can branch on them without parsing free text.
 // Format: "CODE: human message".
 const (
-	RejectCodeAuthForbidden  = "AUTH_FORBIDDEN"
-	RejectCodeSessionError   = "SESSION_ERROR"
-	RejectCodeCapturerError  = "CAPTURER_ERROR"
-	RejectCodeUnsupportedOS  = "UNSUPPORTED"
-	RejectCodeBadRequest     = "BAD_REQUEST"
-	RejectCodeNoConsoleUser  = "NO_CONSOLE_USER"
-	RejectCodeApprovalDenied = "APPROVAL_DENIED"
-	RejectCodeNoApprover     = "NO_APPROVER"
+	RejectCodeAuthForbidden       = "AUTH_FORBIDDEN"
+	RejectCodeSessionError        = "SESSION_ERROR"
+	RejectCodeCapturerError       = "CAPTURER_ERROR"
+	RejectCodeUnsupportedOS       = "UNSUPPORTED"
+	RejectCodeBadRequest          = "BAD_REQUEST"
+	RejectCodeNoConsoleUser       = "NO_CONSOLE_USER"
+	RejectCodeApprovalDenied      = "APPROVAL_DENIED"
+	RejectCodeApprovalTimeout     = "APPROVAL_TIMEOUT"
+	RejectCodeApprovalUnavailable = "APPROVAL_UNAVAILABLE"
+	RejectCodeNoApprover          = "NO_APPROVER"
 )
 
 // EnvVNCDisableDownscale disables any platform-specific framebuffer
@@ -355,6 +357,23 @@ type Approver interface {
 	Request(ctx context.Context, info ApprovalInfo) (ApprovalDecision, error)
 }
 
+// Causes an Approver can report so the rejection names why the connection
+// was refused rather than blaming the user for every outcome. An error
+// matching none of these still rejects, reported as a denial.
+var (
+	// ErrApprovalDenied means the user answered and said no.
+	ErrApprovalDenied = errors.New("approval denied")
+	// ErrApprovalTimeout means the prompt was raised and nobody answered
+	// it in time. Distinct from a denial because the two call for
+	// different things from whoever is dialling: try again later, versus
+	// stop asking.
+	ErrApprovalTimeout = errors.New("approval timed out")
+	// ErrApprovalUnavailable means the prompt never reached a user, so
+	// there was no answer to wait for. A device with no way to display
+	// one lands here, as does a daemon with no UI attached.
+	ErrApprovalUnavailable = errors.New("no approval prompt reached the user")
+)
+
 // ApprovalDecision carries the parts of the user's response the VNC
 // server acts on. Accept is implicit (errors signal deny). ViewOnly puts
 // the session into read-only mode: the server drops input events.
@@ -618,7 +637,8 @@ func (s *Server) gateApproval(conn net.Conn, header *connectionHeader) (Approval
 	}
 	decision, err := s.approver.Request(s.ctx, info)
 	if err != nil {
-		rejectConnection(conn, codeMessage(RejectCodeApprovalDenied, "approval denied"))
+		code, message := approvalRejection(err)
+		rejectConnection(conn, codeMessage(code, message))
 		return ApprovalDecision{}, fmt.Errorf("approval: %w", err)
 	}
 	return decision, nil
@@ -1259,6 +1279,21 @@ func modeString(m byte) string {
 	}
 }
 
+// approvalRejection maps an approver's error onto the code and message the
+// client is told. An error matching no known cause is reported as a denial:
+// the connection is refused either way, and describing a cause the server
+// could not identify would put a wrong explanation in front of the caller.
+func approvalRejection(err error) (string, string) {
+	switch {
+	case errors.Is(err, ErrApprovalTimeout):
+		return RejectCodeApprovalTimeout, "approval timed out"
+	case errors.Is(err, ErrApprovalUnavailable):
+		return RejectCodeApprovalUnavailable, "no approval prompt reached the user"
+	default:
+		return RejectCodeApprovalDenied, "approval denied"
+	}
+}
+
 // acceptRetryable reports whether an Accept error can plausibly clear on its
 // own. Running out of file descriptors can, once open connections close, and an
 // aborted handshake concerns one client rather than the listener. Anything else

client/vnc/server/server_test.go

@@ -8,10 +8,12 @@ import (
 	"encoding/binary"
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"image"
 	"io"
 	"net"
 	"net/netip"
+	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -443,6 +445,37 @@ func newGateConn(t *testing.T) net.Conn {
 	return srv
 }
 
+// newGateConnWithReason is newGateConn plus the reject reason the server
+// wrote, so a test can assert on the code the client would read.
+func newGateConnWithReason(t *testing.T) (net.Conn, <-chan string) {
+	t.Helper()
+	srv, cli := net.Pipe()
+	t.Cleanup(func() { _ = srv.Close() })
+
+	reasons := make(chan string, 1)
+	go func() {
+		defer cli.Close()
+		var srvVer [12]byte
+		if _, err := io.ReadFull(cli, srvVer[:]); err != nil {
+			return
+		}
+		if _, err := cli.Write([]byte("RFB 003.008\n")); err != nil {
+			return
+		}
+		// Server sends numTypes=0, then a 4-byte reason length, then the reason.
+		var head [5]byte
+		if _, err := io.ReadFull(cli, head[:]); err != nil {
+			return
+		}
+		reason := make([]byte, binary.BigEndian.Uint32(head[1:5]))
+		if _, err := io.ReadFull(cli, reason); err != nil {
+			return
+		}
+		reasons <- string(reason)
+	}()
+	return srv, reasons
+}
+
 func gateTestServer(requireApproval bool, approver Approver) *Server {
 	return &Server{
 		log:             log.WithField("test", "gate"),
@@ -540,6 +573,48 @@ func TestGateApproval_ApproverDenies(t *testing.T) {
 	}
 }
 
+// TestGateApproval_RejectCodeNamesCause pins the reject code the client
+// reads for each cause an approver can report. A device that cannot show a
+// prompt at all, or a user who walked away, must not be reported as a user
+// who said no: the caller acts on that difference, and a peer told "denied"
+// when nobody was ever asked has no reason to retry. Causes are wrapped in
+// the cases that carry one so the mapping cannot regress to == comparison.
+func TestGateApproval_RejectCodeNamesCause(t *testing.T) {
+	cases := []struct {
+		name string
+		err  error
+		code string
+	}{
+		{"denied", ErrApprovalDenied, RejectCodeApprovalDenied},
+		{"denied_wrapped", fmt.Errorf("broker: %w", ErrApprovalDenied), RejectCodeApprovalDenied},
+		{"timeout", ErrApprovalTimeout, RejectCodeApprovalTimeout},
+		{"timeout_wrapped", fmt.Errorf("broker: %w", ErrApprovalTimeout), RejectCodeApprovalTimeout},
+		{"unavailable", ErrApprovalUnavailable, RejectCodeApprovalUnavailable},
+		{"unavailable_wrapped", fmt.Errorf("no UI: %w", ErrApprovalUnavailable), RejectCodeApprovalUnavailable},
+		// An unclassified cause still rejects, reported as a denial.
+		{"ctx_canceled", context.Canceled, RejectCodeApprovalDenied},
+		{"unknown", errors.New("anything else"), RejectCodeApprovalDenied},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			app := &recordingApprover{respond: tc.err}
+			srv := gateTestServer(true, app)
+			conn, reasons := newGateConnWithReason(t)
+
+			_, err := srv.gateApproval(conn, &connectionHeader{mode: ModeAttach})
+			require.Error(t, err, "approver error %v must deny", tc.err)
+
+			select {
+			case reason := <-reasons:
+				assert.True(t, strings.HasPrefix(reason, tc.code+":"),
+					"reject reason %q must name the cause with code %s", reason, tc.code)
+			case <-time.After(2 * time.Second):
+				t.Fatal("did not observe rejection reason")
+			}
+		})
+	}
+}
+
 // TestGateApproval_ApproverAccepts confirms the happy path actually
 // returns true so we know the deny path is not the only outcome the
 // gate can produce.

util/capture/text.go

@@ -557,6 +557,8 @@ var vncRejectCodes = [...]string{
 	"BAD_REQUEST",
 	"NO_CONSOLE_USER",
 	"APPROVAL_DENIED",
+	"APPROVAL_TIMEOUT",
+	"APPROVAL_UNAVAILABLE",
 	"NO_APPROVER",
 }