From 4ce4bb6271b252d80b20af772c763884a624bd15 Mon Sep 17 00:00:00 2001
From: Will Norris <will@tailscale.com>
Date: Thu, 2 Nov 2023 11:28:07 -0700
Subject: [PATCH] client/web: limit authorization checks to API calls

This completes the migration to setting up authentication state in the
client first before fetching any node data or rendering the client view.

Notable changes:
 - `authorizeRequest` is now only enforced on `/api/*` calls (with the
   exception of /api/auth, which is handled early because it's needed to
   initially setup auth, particularly for synology)
 - re-separate the App and WebClient components to ensure that auth is
   completed before moving on
 - refactor platform auth (synology and QNAP) to fit into this new
   structure. Synology no longer returns redirect for auth, but returns
   authResponse instructing the client to fetch a SynoToken

Updates tailscale/corp#14335

Signed-off-by: Will Norris <will@tailscale.com>
---
 client/web/qnap.go                | 15 +++----
 client/web/src/components/app.tsx | 29 +++++++++++---
 client/web/src/hooks/auth.ts      | 23 ++++++-----
 client/web/synology.go            | 51 ++++++++----------------
 client/web/web.go                 | 65 +++++++++++++++++--------------
 client/web/web_test.go            |  8 +---
 6 files changed, 97 insertions(+), 94 deletions(-)

diff --git a/client/web/qnap.go b/client/web/qnap.go
index 5ccd76798..07145a77c 100644
--- a/client/web/qnap.go
+++ b/client/web/qnap.go
@@ -9,6 +9,7 @@ package web
 import (
 	"crypto/tls"
 	"encoding/xml"
+	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -18,21 +19,17 @@ import (
 
 // authorizeQNAP authenticates the logged-in QNAP user and verifies that they
 // are authorized to use the web client.
-// It reports true if the request is authorized to continue, and false otherwise.
-// authorizeQNAP manages writing out any relevant authorization errors to the
-// ResponseWriter itself.
-func authorizeQNAP(w http.ResponseWriter, r *http.Request) (ok bool) {
+// If the user is not authorized to use the client, an error is returned.
+func authorizeQNAP(r *http.Request) (ar authResponse, err error) {
 	_, resp, err := qnapAuthn(r)
 	if err != nil {
-		http.Error(w, err.Error(), http.StatusUnauthorized)
-		return false
+		return ar, err
 	}
 	if resp.IsAdmin == 0 {
-		http.Error(w, "user is not an admin", http.StatusForbidden)
-		return false
+		return ar, errors.New("user is not an admin")
 	}
 
-	return true
+	return authResponse{OK: true}, nil
 }
 
 type qnapAuthResponse struct {
diff --git a/client/web/src/components/app.tsx b/client/web/src/components/app.tsx
index 8b10ae475..edbd885aa 100644
--- a/client/web/src/components/app.tsx
+++ b/client/web/src/components/app.tsx
@@ -3,17 +3,36 @@ import React from "react"
 import LegacyClientView from "src/components/views/legacy-client-view"
 import LoginClientView from "src/components/views/login-client-view"
 import ReadonlyClientView from "src/components/views/readonly-client-view"
-import useAuth from "src/hooks/auth"
+import useAuth, { AuthResponse } from "src/hooks/auth"
 import useNodeData from "src/hooks/node-data"
 import ManagementClientView from "./views/management-client-view"
 
 export default function App() {
-  const { data, refreshData, updateNode } = useNodeData()
   const { data: auth, loading: loadingAuth, waitOnAuth } = useAuth()
 
   return (
     <div className="flex flex-col items-center min-w-sm max-w-lg mx-auto py-14">
-      {!data || loadingAuth ? (
+      {loadingAuth ? (
+        <div className="text-center py-14">Loading...</div> // TODO(sonia): add a loading view
+      ) : (
+        <WebClient auth={auth} waitOnAuth={waitOnAuth} />
+      )}
+    </div>
+  )
+}
+
+function WebClient({
+  auth,
+  waitOnAuth,
+}: {
+  auth?: AuthResponse
+  waitOnAuth: () => Promise<void>
+}) {
+  const { data, refreshData, updateNode } = useNodeData()
+
+  return (
+    <>
+      {!data ? (
         <div className="text-center py-14">Loading...</div> // TODO(sonia): add a loading view
       ) : data?.Status === "NeedsLogin" || data?.Status === "NoState" ? (
         // Client not on a tailnet, render login.
@@ -35,8 +54,8 @@ export default function App() {
           updateNode={updateNode}
         />
       )}
-      {data && !loadingAuth && <Footer licensesURL={data.LicensesURL} />}
-    </div>
+      {data && <Footer licensesURL={data.LicensesURL} />}
+    </>
   )
 }
 
diff --git a/client/web/src/hooks/auth.ts b/client/web/src/hooks/auth.ts
index 238049288..ca1a3add1 100644
--- a/client/web/src/hooks/auth.ts
+++ b/client/web/src/hooks/auth.ts
@@ -16,7 +16,7 @@ export type AuthResponse = {
 // for the web client.
 export default function useAuth() {
   const [data, setData] = useState<AuthResponse>()
-  const [loading, setLoading] = useState<boolean>(false)
+  const [loading, setLoading] = useState<boolean>(true)
 
   const loadAuth = useCallback((wait?: boolean) => {
     const url = wait ? "/auth?wait=true" : "/auth"
@@ -24,16 +24,19 @@ export default function useAuth() {
     return apiFetch(url, "GET")
       .then((r) => r.json())
       .then((d) => {
-        if ((d as AuthResponse).authNeeded == AuthType.synology) {
-          fetch("/webman/login.cgi")
-            .then((r) => r.json())
-            .then((data) => {
-              setSynoToken(data.SynoToken)
-            })
-        }
-
-        setLoading(false)
         setData(d)
+        switch ((d as AuthResponse).authNeeded) {
+          case AuthType.synology:
+            fetch("/webman/login.cgi")
+              .then((r) => r.json())
+              .then((a) => {
+                setSynoToken(a.SynoToken)
+                setLoading(false)
+              })
+            break
+          default:
+            setLoading(false)
+        }
       })
       .catch((error) => {
         setLoading(false)
diff --git a/client/web/synology.go b/client/web/synology.go
index 5f36cc93e..126e8ed84 100644
--- a/client/web/synology.go
+++ b/client/web/synology.go
@@ -7,6 +7,7 @@
 package web
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"os/exec"
@@ -17,62 +18,44 @@ import (
 
 // authorizeSynology authenticates the logged-in Synology user and verifies
 // that they are authorized to use the web client.
-// It reports true if the request is authorized to continue, and false otherwise.
-// authorizeSynology manages writing out any relevant authorization errors to the
-// ResponseWriter itself.
-func authorizeSynology(w http.ResponseWriter, r *http.Request) (ok bool) {
-	if synoTokenRedirect(w, r) {
-		return false
+// The returned authResponse indicates if the user is authorized,
+// and if additional steps are needed to authenticate the user.
+// If the user is authenticated, but not authorized to use the client, an error is returned.
+func authorizeSynology(r *http.Request) (resp authResponse, err error) {
+	if !hasSynoToken(r) {
+		return authResponse{OK: false, AuthNeeded: synoAuth}, nil
 	}
 
 	// authenticate the Synology user
 	cmd := exec.Command("/usr/syno/synoman/webman/modules/authenticate.cgi")
 	out, err := cmd.CombinedOutput()
 	if err != nil {
-		http.Error(w, fmt.Sprintf("auth: %v: %s", err, out), http.StatusUnauthorized)
-		return false
+		return resp, fmt.Errorf("auth: %v: %s", err, out)
 	}
 	user := strings.TrimSpace(string(out))
 
 	// check if the user is in the administrators group
 	isAdmin, err := groupmember.IsMemberOfGroup("administrators", user)
 	if err != nil {
-		http.Error(w, err.Error(), http.StatusForbidden)
-		return false
+		return resp, err
 	}
 	if !isAdmin {
-		http.Error(w, "not a member of administrators group", http.StatusForbidden)
-		return false
+		return resp, errors.New("not a member of administrators group")
 	}
 
-	return true
+	return authResponse{OK: true}, nil
 }
 
-func synoTokenRedirect(w http.ResponseWriter, r *http.Request) bool {
+// hasSynoToken returns true if the request include a SynoToken used for synology auth.
+func hasSynoToken(r *http.Request) bool {
 	if r.Header.Get("X-Syno-Token") != "" {
-		return false
+		return true
 	}
 	if r.URL.Query().Get("SynoToken") != "" {
-		return false
+		return true
 	}
 	if r.Method == "POST" && r.FormValue("SynoToken") != "" {
-		return false
+		return true
 	}
-	// We need a SynoToken for authenticate.cgi.
-	// So we tell the client to get one.
-	_, _ = fmt.Fprint(w, synoTokenRedirectHTML)
-	return true
+	return false
 }
-
-const synoTokenRedirectHTML = `<html>
-Redirecting with session token...
-<script>
-  fetch("/webman/login.cgi")
-    .then(r => r.json())
-    .then(data => {
-	u = new URL(window.location)
-	u.searchParams.set("SynoToken", data.SynoToken)
-	document.location = u
-    })
-</script>
-`
diff --git a/client/web/web.go b/client/web/web.go
index 8f3a20ae0..2560bfbb1 100644
--- a/client/web/web.go
+++ b/client/web/web.go
@@ -178,10 +178,15 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 }
 
 func (s *Server) serve(w http.ResponseWriter, r *http.Request) {
-	if ok := s.authorizeRequest(w, r); !ok {
-		return
-	}
 	if strings.HasPrefix(r.URL.Path, "/api/") {
+		if r.Method == httpm.GET && r.URL.Path == "/api/auth" {
+			s.serveAPIAuth(w, r)
+			return
+		}
+		if ok := s.authorizeRequest(w, r); !ok {
+			http.Error(w, "not authorized", http.StatusUnauthorized)
+			return
+		}
 		// Pass API requests through to the API handler.
 		s.apiHandler.ServeHTTP(w, r)
 		return
@@ -208,9 +213,6 @@ func (s *Server) authorizeRequest(w http.ResponseWriter, r *http.Request) (ok bo
 		case r.URL.Path == "/api/data" && r.Method == httpm.GET:
 			// Readonly endpoint allowed without browser session.
 			return true
-		case r.URL.Path == "/api/auth":
-			// Endpoint for browser to request auth allowed without browser session.
-			return true
 		case strings.HasPrefix(r.URL.Path, "/api/"):
 			// All other /api/ endpoints require a valid browser session.
 			//
@@ -229,15 +231,13 @@ func (s *Server) authorizeRequest(w http.ResponseWriter, r *http.Request) (ok bo
 		}
 	}
 	// Client using system-specific auth.
-	d := distro.Get()
-	switch {
-	case strings.HasPrefix(r.URL.Path, "/assets/") && r.Method == httpm.GET:
-		// Don't require authorization for static assets.
-		return true
-	case d == distro.Synology:
-		return authorizeSynology(w, r)
-	case d == distro.QNAP:
-		return authorizeQNAP(w, r)
+	switch distro.Get() {
+	case distro.Synology:
+		resp, _ := authorizeSynology(r)
+		return resp.OK
+	case distro.QNAP:
+		resp, _ := authorizeQNAP(r)
+		return resp.OK
 	default:
 		return true // no additional auth for this distro
 	}
@@ -252,11 +252,6 @@ func (s *Server) serveLoginAPI(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "invalid endpoint", http.StatusNotFound)
 		return
 	}
-	if r.URL.Path != "/api/auth" {
-		// empty JSON response until we serve auth for the login client
-		fmt.Fprintf(w, "{}")
-		return
-	}
 	switch r.Method {
 	case httpm.GET:
 		// TODO(soniaappasamy): we may want a minimal node data response here
@@ -282,7 +277,9 @@ type authResponse struct {
 	AuthNeeded authType `json:"authNeeded,omitempty"` // filled when user needs to complete a specific type of auth
 }
 
-func (s *Server) serveTailscaleAuth(w http.ResponseWriter, r *http.Request) {
+// serverAPIAuth handles requests to the /api/auth endpoint
+// and returns an authResponse indicating the current auth state and any steps the user needs to take.
+func (s *Server) serveAPIAuth(w http.ResponseWriter, r *http.Request) {
 	if r.Method != httpm.GET {
 		http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
 		return
@@ -291,6 +288,24 @@ func (s *Server) serveTailscaleAuth(w http.ResponseWriter, r *http.Request) {
 
 	session, whois, err := s.getSession(r)
 	switch {
+	case err != nil && errors.Is(err, errNotUsingTailscale):
+		// not using tailscale, so perform platform auth
+		switch distro.Get() {
+		case distro.Synology:
+			resp, err = authorizeSynology(r)
+			if err != nil {
+				http.Error(w, err.Error(), http.StatusUnauthorized)
+				return
+			}
+		case distro.QNAP:
+			resp, err = authorizeQNAP(r)
+			if err != nil {
+				http.Error(w, err.Error(), http.StatusUnauthorized)
+				return
+			}
+		default:
+			resp.OK = true // no additional auth for this distro
+		}
 	case err != nil && !errors.Is(err, errNoSession):
 		http.Error(w, err.Error(), http.StatusUnauthorized)
 		return
@@ -341,14 +356,6 @@ func (s *Server) serveAPI(w http.ResponseWriter, r *http.Request) {
 	w.Header().Set("X-CSRF-Token", csrf.Token(r))
 	path := strings.TrimPrefix(r.URL.Path, "/api")
 	switch {
-	case path == "/auth":
-		if s.tsDebugMode == "full" { // behind debug flag
-			s.serveTailscaleAuth(w, r)
-		} else {
-			// empty JSON response until we serve auth for other modes
-			fmt.Fprintf(w, "{}")
-		}
-		return
 	case path == "/data":
 		switch r.Method {
 		case httpm.GET:
diff --git a/client/web/web_test.go b/client/web/web_test.go
index a3a94d599..d1efb1abd 100644
--- a/client/web/web_test.go
+++ b/client/web/web_test.go
@@ -369,12 +369,6 @@ func TestAuthorizeRequest(t *testing.T) {
 		wantOkNotOverTailscale: false,
 		wantOkWithoutSession:   false,
 		wantOkWithSession:      true,
-	}, {
-		reqPath:                "/api/auth",
-		reqMethod:              httpm.GET,
-		wantOkNotOverTailscale: false,
-		wantOkWithoutSession:   true,
-		wantOkWithSession:      true,
 	}, {
 		reqPath:                "/api/somethingelse",
 		reqMethod:              httpm.GET,
@@ -587,7 +581,7 @@ func TestServeTailscaleAuth(t *testing.T) {
 			r.RemoteAddr = remoteIP
 			r.AddCookie(&http.Cookie{Name: sessionCookieName, Value: tt.cookie})
 			w := httptest.NewRecorder()
-			s.serveTailscaleAuth(w, r)
+			s.serveAPIAuth(w, r)
 			res := w.Result()
 			defer res.Body.Close()