client/web: add logging of device management type for web client (#10492)

Add logging of device management type for the web client auth flow. Namely,
this differentiates between viewing a node you do not own, viewing a local
tagged node, viewing a remote tagged node, managing a local node, and
managing a remote node.

Updates https://github.com/tailscale/tailscale/issues/10261

Signed-off-by: Mario Minardi <[email protected]>

client/web/auth.go

@@ -14,6 +14,7 @@ import (
 	"time"
 
 	"tailscale.com/client/tailscale/apitype"
+	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
 )
 
@@ -102,48 +103,48 @@ var (
 //
 // The WhoIsResponse is always populated, with a non-nil Node and UserProfile,
 // unless getTailscaleBrowserSession reports errNotUsingTailscale.
-func (s *Server) getSession(r *http.Request) (*browserSession, *apitype.WhoIsResponse, error) {
+func (s *Server) getSession(r *http.Request) (*browserSession, *apitype.WhoIsResponse, *ipnstate.Status, error) {
 	whoIs, whoIsErr := s.lc.WhoIs(r.Context(), r.RemoteAddr)
 	status, statusErr := s.lc.StatusWithoutPeers(r.Context())
 	switch {
 	case whoIsErr != nil:
-		return nil, nil, errNotUsingTailscale
+		return nil, nil, status, errNotUsingTailscale
 	case statusErr != nil:
-		return nil, whoIs, statusErr
+		return nil, whoIs, nil, statusErr
 	case status.Self == nil:
-		return nil, whoIs, errors.New("missing self node in tailscale status")
+		return nil, whoIs, status, errors.New("missing self node in tailscale status")
 	case whoIs.Node.IsTagged() && whoIs.Node.StableID == status.Self.ID:
-		return nil, whoIs, errTaggedLocalSource
+		return nil, whoIs, status, errTaggedLocalSource
 	case whoIs.Node.IsTagged():
-		return nil, whoIs, errTaggedRemoteSource
+		return nil, whoIs, status, errTaggedRemoteSource
 	case !status.Self.IsTagged() && status.Self.UserID != whoIs.UserProfile.ID:
-		return nil, whoIs, errNotOwner
+		return nil, whoIs, status, errNotOwner
 	}
 	srcNode := whoIs.Node.ID
 	srcUser := whoIs.UserProfile.ID
 
 	cookie, err := r.Cookie(sessionCookieName)
 	if errors.Is(err, http.ErrNoCookie) {
-		return nil, whoIs, errNoSession
+		return nil, whoIs, status, errNoSession
 	} else if err != nil {
-		return nil, whoIs, err
+		return nil, whoIs, status, err
 	}
 	v, ok := s.browserSessions.Load(cookie.Value)
 	if !ok {
-		return nil, whoIs, errNoSession
+		return nil, whoIs, status, errNoSession
 	}
 	session := v.(*browserSession)
 	if session.SrcNode != srcNode || session.SrcUser != srcUser {
 		// In this case the browser cookie is associated with another tailscale node.
 		// Maybe the source browser's machine was logged out and then back in as a different node.
 		// Return errNoSession because there is no session for this user.
-		return nil, whoIs, errNoSession
+		return nil, whoIs, status, errNoSession
 	} else if session.isExpired(s.timeNow()) {
 		// Session expired, remove from session map and return errNoSession.
 		s.browserSessions.Delete(session.ID)
-		return nil, whoIs, errNoSession
+		return nil, whoIs, status, errNoSession
 	}
-	return session, whoIs, nil
+	return session, whoIs, status, nil
 }
 
 // newSession creates a new session associated with the given source user/node,

client/web/web.go

@@ -276,9 +276,6 @@ func (s *Server) serve(w http.ResponseWriter, r *http.Request) {
 		s.apiHandler.ServeHTTP(w, r)
 		return
 	}
-	if !s.devMode {
-		s.lc.IncrementCounter(r.Context(), "web_client_page_load", 1)
-	}
 	s.assetsHandler.ServeHTTP(w, r)
 }
 
@@ -328,7 +325,7 @@ func (s *Server) requireTailscaleIP(w http.ResponseWriter, r *http.Request) (han
 // errors to the ResponseWriter itself.
 func (s *Server) authorizeRequest(w http.ResponseWriter, r *http.Request) (ok bool) {
 	if s.mode == ManageServerMode { // client using tailscale auth
-		session, _, err := s.getSession(r)
+		session, _, _, err := s.getSession(r)
 		switch {
 		case errors.Is(err, errNotUsingTailscale):
 			// All requests must be made over tailscale.
@@ -404,7 +401,7 @@ type viewerIdentity struct {
 func (s *Server) serveAPIAuth(w http.ResponseWriter, r *http.Request) {
 	var resp authResponse
 
-	session, whois, err := s.getSession(r)
+	session, whois, status, err := s.getSession(r)
 	switch {
 	case s.mode == LoginServerMode || errors.Is(err, errNotUsingTailscale):
 		// not using tailscale, so perform platform auth
@@ -426,18 +423,28 @@ func (s *Server) serveAPIAuth(w http.ResponseWriter, r *http.Request) {
 		default:
 			// no additional auth for this distro
 		}
-	case err != nil && (errors.Is(err, errNotOwner) ||
-		errors.Is(err, errNotUsingTailscale) ||
-		errors.Is(err, errTaggedLocalSource) ||
-		errors.Is(err, errTaggedRemoteSource)):
-		// These cases are all restricted to the readonly view.
-		// No auth action to take.
+	case err != nil && errors.Is(err, errNotOwner):
+		// Restricted to the readonly view, no auth action to take.
+		s.lc.IncrementCounter(r.Context(), "web_client_viewing_not_owner", 1)
+		resp.AuthNeeded = ""
+	case err != nil && errors.Is(err, errTaggedLocalSource):
+		// Restricted to the readonly view, no auth action to take.
+		s.lc.IncrementCounter(r.Context(), "web_client_viewing_local_tag", 1)
+		resp.AuthNeeded = ""
+	case err != nil && errors.Is(err, errTaggedRemoteSource):
+		// Restricted to the readonly view, no auth action to take.
+		s.lc.IncrementCounter(r.Context(), "web_client_viewing_remote_tag", 1)
 		resp.AuthNeeded = ""
 	case err != nil && !errors.Is(err, errNoSession):
 		// Any other error.
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	case session.isAuthorized(s.timeNow()):
+		if whois.Node.StableID == status.Self.ID {
+			s.lc.IncrementCounter(r.Context(), "web_client_managing_local", 1)
+		} else {
+			s.lc.IncrementCounter(r.Context(), "web_client_managing_remote", 1)
+		}
 		resp.CanManageNode = true
 		resp.AuthNeeded = ""
 	default:
@@ -463,7 +470,7 @@ type newSessionAuthResponse struct {
 
 // serveAPIAuthSessionNew handles requests to the /api/auth/session/new endpoint.
 func (s *Server) serveAPIAuthSessionNew(w http.ResponseWriter, r *http.Request) {
-	session, whois, err := s.getSession(r)
+	session, whois, _, err := s.getSession(r)
 	if err != nil && !errors.Is(err, errNoSession) {
 		// Source associated with request not allowed to create
 		// a session for this web client.
@@ -493,7 +500,7 @@ func (s *Server) serveAPIAuthSessionNew(w http.ResponseWriter, r *http.Request)
 
 // serveAPIAuthSessionWait handles requests to the /api/auth/session/wait endpoint.
 func (s *Server) serveAPIAuthSessionWait(w http.ResponseWriter, r *http.Request) {
-	session, _, err := s.getSession(r)
+	session, _, _, err := s.getSession(r)
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusUnauthorized)
 		return

client/web/web_test.go

@@ -13,6 +13,7 @@ import (
 	"net/http/httptest"
 	"net/netip"
 	"net/url"
+	"slices"
 	"strings"
 	"testing"
 	"time"
@@ -168,7 +169,7 @@ func TestGetTailscaleBrowserSession(t *testing.T) {
 
 	lal := memnet.Listen("local-tailscaled.sock:80")
 	defer lal.Close()
-	localapi := mockLocalAPI(t, tailnetNodes, func() *ipnstate.PeerStatus { return selfNode }, nil)
+	localapi := mockLocalAPI(t, tailnetNodes, func() *ipnstate.PeerStatus { return selfNode }, nil, nil)
 	defer localapi.Close()
 	go localapi.Serve(lal)
 
@@ -305,7 +306,7 @@ func TestGetTailscaleBrowserSession(t *testing.T) {
 			if tt.cookie != "" {
 				r.AddCookie(&http.Cookie{Name: sessionCookieName, Value: tt.cookie})
 			}
-			session, _, err := s.getSession(r)
+			session, _, _, err := s.getSession(r)
 			if !errors.Is(err, tt.wantError) {
 				t.Errorf("wrong error; want=%v, got=%v", tt.wantError, err)
 			}
@@ -336,6 +337,7 @@ func TestAuthorizeRequest(t *testing.T) {
 		map[string]*apitype.WhoIsResponse{remoteIP: remoteNode},
 		func() *ipnstate.PeerStatus { return self },
 		nil,
+		nil,
 	)
 	defer localapi.Close()
 	go localapi.Serve(lal)
@@ -445,6 +447,7 @@ func TestServeAuth(t *testing.T) {
 		func() *ipn.Prefs {
 			return &ipn.Prefs{ControlURL: *testControlURL}
 		},
+		nil,
 	)
 	defer localapi.Close()
 	go localapi.Serve(lal)
@@ -713,6 +716,175 @@ func TestServeAuth(t *testing.T) {
 	}
 }
 
+// TestServeAPIAuthMetricLogging specifically tests metric logging in the serveAPIAuth function.
+// For each given test case, we assert that the local API received a request to log the expected metric.
+func TestServeAPIAuthMetricLogging(t *testing.T) {
+	user := &tailcfg.UserProfile{LoginName: "[email protected]", ID: tailcfg.UserID(1)}
+	otherUser := &tailcfg.UserProfile{LoginName: "[email protected]", ID: tailcfg.UserID(2)}
+	self := &ipnstate.PeerStatus{
+		ID:           "self",
+		UserID:       user.ID,
+		TailscaleIPs: []netip.Addr{netip.MustParseAddr("100.1.2.3")},
+	}
+	remoteIP := "100.100.100.101"
+	remoteNode := &apitype.WhoIsResponse{
+		Node: &tailcfg.Node{
+			Name:      "remote-managed",
+			ID:        1,
+			Addresses: []netip.Prefix{netip.MustParsePrefix(remoteIP + "/32")},
+		},
+		UserProfile: user,
+	}
+	remoteTaggedIP := "100.123.100.213"
+	remoteTaggedNode := &apitype.WhoIsResponse{
+		Node: &tailcfg.Node{
+			Name:      "remote-tagged",
+			ID:        2,
+			Addresses: []netip.Prefix{netip.MustParsePrefix(remoteTaggedIP + "/32")},
+			Tags:      []string{"dev-machine"},
+		},
+		UserProfile: user,
+	}
+	localIP := "100.1.2.3"
+	localNode := &apitype.WhoIsResponse{
+		Node: &tailcfg.Node{
+			Name:      "local-managed",
+			ID:        3,
+			StableID:  "self",
+			Addresses: []netip.Prefix{netip.MustParsePrefix(localIP + "/32")},
+		},
+		UserProfile: user,
+	}
+	localTaggedIP := "100.1.2.133"
+	localTaggedNode := &apitype.WhoIsResponse{
+		Node: &tailcfg.Node{
+			Name:      "local-tagged",
+			ID:        4,
+			StableID:  "self",
+			Addresses: []netip.Prefix{netip.MustParsePrefix(localTaggedIP + "/32")},
+			Tags:      []string{"prod-machine"},
+		},
+		UserProfile: user,
+	}
+	otherIP := "100.100.2.3"
+	otherNode := &apitype.WhoIsResponse{
+		Node: &tailcfg.Node{
+			Name:      "other-node",
+			ID:        5,
+			Addresses: []netip.Prefix{netip.MustParsePrefix(otherIP + "/32")},
+		},
+		UserProfile: otherUser,
+	}
+
+	testControlURL := &defaultControlURL
+	var loggedMetrics []string
+
+	lal := memnet.Listen("local-tailscaled.sock:80")
+	defer lal.Close()
+	localapi := mockLocalAPI(t,
+		map[string]*apitype.WhoIsResponse{remoteIP: remoteNode, localIP: localNode, otherIP: otherNode, localTaggedIP: localTaggedNode, remoteTaggedIP: remoteTaggedNode},
+		func() *ipnstate.PeerStatus { return self },
+		func() *ipn.Prefs {
+			return &ipn.Prefs{ControlURL: *testControlURL}
+		},
+		func(metricName string) {
+			loggedMetrics = append(loggedMetrics, metricName)
+		},
+	)
+	defer localapi.Close()
+	go localapi.Serve(lal)
+
+	timeNow := time.Now()
+	oneHourAgo := timeNow.Add(-time.Hour)
+
+	s := &Server{
+		mode:        ManageServerMode,
+		lc:          &tailscale.LocalClient{Dial: lal.Dial},
+		timeNow:     func() time.Time { return timeNow },
+		newAuthURL:  mockNewAuthURL,
+		waitAuthURL: mockWaitAuthURL,
+	}
+
+	remoteNodeCookie := "ts-cookie-remote-node"
+	s.browserSessions.Store(remoteNodeCookie, &browserSession{
+		ID:            remoteNodeCookie,
+		SrcNode:       remoteNode.Node.ID,
+		SrcUser:       user.ID,
+		Created:       oneHourAgo,
+		AuthID:        testAuthPathSuccess,
+		AuthURL:       *testControlURL + testAuthPathSuccess,
+		Authenticated: true,
+	})
+	localNodeCookie := "ts-cookie-local-node"
+	s.browserSessions.Store(localNodeCookie, &browserSession{
+		ID:            localNodeCookie,
+		SrcNode:       localNode.Node.ID,
+		SrcUser:       user.ID,
+		Created:       oneHourAgo,
+		AuthID:        testAuthPathSuccess,
+		AuthURL:       *testControlURL + testAuthPathSuccess,
+		Authenticated: true,
+	})
+
+	tests := []struct {
+		name       string
+		cookie     string // cookie attached to request
+		remoteAddr string // remote address to hit
+
+		wantLoggedMetric string // expected metric to be logged
+	}{
+		{
+			name:             "managing-remote",
+			cookie:           remoteNodeCookie,
+			remoteAddr:       remoteIP,
+			wantLoggedMetric: "web_client_managing_remote",
+		},
+		{
+			name:             "managing-local",
+			cookie:           localNodeCookie,
+			remoteAddr:       localIP,
+			wantLoggedMetric: "web_client_managing_local",
+		},
+		{
+			name:             "viewing-not-owner",
+			cookie:           remoteNodeCookie,
+			remoteAddr:       otherIP,
+			wantLoggedMetric: "web_client_viewing_not_owner",
+		},
+		{
+			name:             "viewing-local-tagged",
+			cookie:           localNodeCookie,
+			remoteAddr:       localTaggedIP,
+			wantLoggedMetric: "web_client_viewing_local_tag",
+		},
+		{
+			name:             "viewing-remote-tagged",
+			cookie:           remoteNodeCookie,
+			remoteAddr:       remoteTaggedIP,
+			wantLoggedMetric: "web_client_viewing_remote_tag",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			testControlURL = &defaultControlURL
+
+			r := httptest.NewRequest("GET", "http://100.1.2.3:5252/api/auth", nil)
+			r.RemoteAddr = tt.remoteAddr
+			r.AddCookie(&http.Cookie{Name: sessionCookieName, Value: tt.cookie})
+			w := httptest.NewRecorder()
+			s.serveAPIAuth(w, r)
+
+			if !slices.Contains(loggedMetrics, tt.wantLoggedMetric) {
+				t.Errorf("expected logged metrics to contain: '%s' but was: '%v'", tt.wantLoggedMetric, loggedMetrics)
+			}
+			loggedMetrics = []string{}
+
+			res := w.Result()
+			defer res.Body.Close()
+		})
+	}
+}
+
 func TestRequireTailscaleIP(t *testing.T) {
 	self := &ipnstate.PeerStatus{
 		TailscaleIPs: []netip.Addr{
@@ -723,7 +895,7 @@ func TestRequireTailscaleIP(t *testing.T) {
 
 	lal := memnet.Listen("local-tailscaled.sock:80")
 	defer lal.Close()
-	localapi := mockLocalAPI(t, nil, func() *ipnstate.PeerStatus { return self }, nil)
+	localapi := mockLocalAPI(t, nil, func() *ipnstate.PeerStatus { return self }, nil, nil)
 	defer localapi.Close()
 	go localapi.Serve(lal)
 
@@ -812,7 +984,7 @@ var (
 // self accepts a function that resolves to a self node status,
 // so that tests may swap out the /localapi/v0/status response
 // as desired.
-func mockLocalAPI(t *testing.T, whoIs map[string]*apitype.WhoIsResponse, self func() *ipnstate.PeerStatus, prefs func() *ipn.Prefs) *http.Server {
+func mockLocalAPI(t *testing.T, whoIs map[string]*apitype.WhoIsResponse, self func() *ipnstate.PeerStatus, prefs func() *ipn.Prefs, metricCapture func(string)) *http.Server {
 	return &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch r.URL.Path {
 		case "/localapi/v0/whois":
@@ -832,6 +1004,19 @@ func mockLocalAPI(t *testing.T, whoIs map[string]*apitype.WhoIsResponse, self fu
 		case "/localapi/v0/prefs":
 			writeJSON(w, prefs())
 			return
+		case "/localapi/v0/upload-client-metrics":
+			type metricName struct {
+				Name string `json:"name"`
+			}
+
+			var metricNames []metricName
+			if err := json.NewDecoder(r.Body).Decode(&metricNames); err != nil {
+				http.Error(w, "invalid JSON body", http.StatusBadRequest)
+				return
+			}
+			metricCapture(metricNames[0].Name)
+			writeJSON(w, struct{}{})
+			return
 		default:
 			t.Fatalf("unhandled localapi test endpoint %q, add to localapi handler func in test", r.URL.Path)
 		}