client/web: move auth wait to backend

Signed-off-by: Gesa Stupperich <gesa@tailscale.com>
2 months ago · cbf92413a5
parent fba4f8ec23
commit cbf92413a5
4 changed files with 55 additions and 80 deletions
--- a/client/web/auth.go
+++ b/client/web/auth.go
@ -37,6 +37,7 @@ type browserSession struct {
 	AuthURL       string // from tailcfg.WebClientAuthResponse
 	Created       time.Time
 	Authenticated bool
+	PendingAuth   bool
 }

 // isAuthorized reports true if the given session is authorized
@ -127,23 +128,12 @@ func (s *Server) getSession(r *http.Request) (*browserSession, *apitype.WhoIsRes

 	cookie, err := r.Cookie(sessionCookieName)
 	if errors.Is(err, http.ErrNoCookie) {
-		s.logf("auth: no session cookie found")
 		return nil, whoIs, status, errNoSession
 	} else if err != nil {
 		return nil, whoIs, status, err
 	}
 	v, ok := s.browserSessions.Load(cookie.Value)
 	if !ok {
-		s.logf("auth: session for %v not found in session store", cookie.Value)
-		count := 0
-		s.browserSessions.Range(func(k, v interface{}) bool {
-			count++
-			bs := v.(*browserSession)
-			s.logf("auth: session for %v: ID=%v, AuthID=%v, SrcNode=%v, SrcUser=%v, Created=%v, Authenticated=%v",
-				k, bs.ID, bs.AuthID, bs.SrcNode, bs.SrcUser, bs.Created, bs.Authenticated)
-			return true
-		})
-		s.logf("auth: total sessions in store: %d", count)
 		return nil, whoIs, status, errNoSession
 	}
 	session := v.(*browserSession)
@ -175,7 +165,7 @@ func (s *Server) newSession(ctx context.Context, src *apitype.WhoIsResponse) (*b
 		Created: s.timeNow(),
 	}

-	if true {
+	if s.controlSupportsCheckMode(ctx) {
 		// control supports check mode, so get a new auth URL and return.
 		a, err := s.newAuthURL(ctx, src.Node.ID)
 		if err != nil {
@ -183,22 +173,14 @@ func (s *Server) newSession(ctx context.Context, src *apitype.WhoIsResponse) (*b
 		}
 		session.AuthID = a.ID
 		session.AuthURL = a.URL
+		session.PendingAuth = true
 	} else {
 		// control does not support check mode, so there is no additional auth we can do.
 		session.Authenticated = true
 	}

-	s.logf("auth: session created for %v, %v", session.AuthID, session.ID)
-	// Make a deep copy of the session to ensure it's properly stored
-	sessionCopy := *session
-	s.browserSessions.Store(sid, &sessionCopy)
-	value, ok := s.browserSessions.Load(sid)
-	if !ok {
-		s.logf("auth: session not stored for %v", session.AuthID)
-	} else {
-		session = value.(*browserSession)
-		s.logf("auth: session stored for %v, %v: %v", session.AuthID, session.ID, value)
-	}
+	s.browserSessions.Store(sid, session)
+
 	return session, nil
 }

@ -227,23 +209,24 @@ func (s *Server) awaitUserAuth(ctx context.Context, session *browserSession) err
 	if session.isAuthorized(s.timeNow()) {
 		return nil // already authorized
 	}
+
 	a, err := s.waitAuthURL(ctx, session.AuthID, session.SrcNode)
 	if err != nil {
 		// Don't delete the session on context cancellation, as this is expected
-		// when users navigate away or refresh the page
+		// when users navigate away or refresh the page.
 		if errors.Is(err, context.Canceled) {
-			s.logf("awaitUserAuth: context canceled for session %v - not deleting session", session.ID)
 			return err
 		}

-		s.logf("awaitUserAuth: ERROR from waitAuthURL: %v - DELETING SESSION %v", err, session.ID)
 		// Clean up the session for non-cancellation errors from control server
 		// to avoid the user getting stuck with a bad session cookie.
 		s.browserSessions.Delete(session.ID)
 		return err
 	}
+
 	if a.Complete {
 		session.Authenticated = a.Complete
+		session.PendingAuth = false
 		s.browserSessions.Store(session.ID, session)
 	}
 	return nil
--- a/client/web/src/hooks/auth.ts
+++ b/client/web/src/hooks/auth.ts
@ -81,25 +81,7 @@ export default function useAuth() {
    return apiFetch<{ authUrl?: string }>("/auth/session/new", "GET")
      .then((d) => {
        if (d.authUrl) {
-          // Store a flag to signal that auth is in progress.
-          // We try to open the auth URL in a new tab, but it might open
-          // in the same tab depending on browser settings
-          // (https://github.com/tailscale/tailscale/issues/11905).
-          // The flag will be used when returning to the app (in any tab).
-          localStorage.setItem("ts-web-needs-auth-wait", "true")
-
-          // Open the auth URL - this might open in a new tab or
-          // replace the current tab depending on browser settings.
          window.open(d.authUrl, "_blank")
-
-          // If execution continues here, we're still in the original tab
-          // (the URL opened in a new tab or the popup was blocked).
-          // If it opened in the same tab, we'll handle this when the user
-          // returns via the effect that checks for the pending auth flag.
-          // If it opened in a new tab, we can wait for auth to complete now.
-          // (If the popup was blocked, we'll also wait here.)
-
-          // Wait for the auth session to complete.
          return apiFetch("/auth/session/wait", "GET")
        }
      })
@ -133,23 +115,6 @@ export default function useAuth() {
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [ranSynoAuth])

-  // Handle returning to the UI after having completed the auth flow in any tab.
-  useEffect(() => {
-    // Check if we have a pending auth session when the component mounts.
-    const pendingAuth = localStorage.getItem("ts-web-needs-auth-wait")
-    if (pendingAuth === "true") {
-      // Wait for the auth session to complete.
-      apiFetch("/auth/session/wait", "GET")
-        .then(() => {
-          loadAuth()
-          localStorage.removeItem("ts-web-needs-auth-wait")
-        })
-        .catch((error) => {
-          console.error(error)
-        })
-    }
-  }, [loadAuth])
-
  return {
    data,
    loading,
--- a/client/web/web.go
+++ b/client/web/web.go
@ -370,7 +370,6 @@ func (s *Server) serve(w http.ResponseWriter, r *http.Request) {
 			s.serveAPIAuthSessionNew(w, r) // create new session
 			return
 		case r.URL.Path == "/api/auth/session/wait" && r.Method == httpm.GET:
-			s.logf("web: wait for auth session")
 			s.serveAPIAuthSessionWait(w, r) // wait for session to be authorized
 			return
 		}
@ -772,6 +771,19 @@ func (s *Server) serveAPIAuth(w http.ResponseWriter, r *http.Request) {
 		}
 	}

+	// We might have a session for which we haven't awaited the result yet.
+	// This can happen when the AuthURL opens in the same browser tab instead
+	// of a new one due to browser settings.
+	// (See https://github.com/tailscale/tailscale/issues/11905)
+	// We therefore set a PendingAuth flag when creating a new session, check
+	// it here and call awaitUserAuth if we find it to be true. Once the auth
+	// wait completes, awaitUserAuth will set PendingAuth to false.
+	if sErr == nil && session.PendingAuth == true {
+		if err := s.awaitUserAuth(r.Context(), session); err != nil {
+			sErr = err
+		}
+	}
+
 	switch {
 	case sErr != nil && errors.Is(sErr, errNotUsingTailscale):
 		s.lc.IncrementCounter(r.Context(), "web_client_viewing_local", 1)
--- a/client/web/web_test.go
+++ b/client/web/web_test.go
@ -582,12 +582,23 @@ func TestServeAuth(t *testing.T) {

 	successCookie := "ts-cookie-success"
 	s.browserSessions.Store(successCookie, &browserSession{
-		ID:      successCookie,
-		SrcNode: remoteNode.Node.ID,
-		SrcUser: user.ID,
-		Created: oneHourAgo,
-		AuthID:  testAuthPathSuccess,
-		AuthURL: *testControlURL + testAuthPathSuccess,
+		ID:          successCookie,
+		SrcNode:     remoteNode.Node.ID,
+		SrcUser:     user.ID,
+		Created:     oneHourAgo,
+		AuthID:      testAuthPathSuccess,
+		AuthURL:     *testControlURL + testAuthPathSuccess,
+		PendingAuth: true,
+	})
+	successCookie2 := "ts-cookie-success-2"
+	s.browserSessions.Store(successCookie2, &browserSession{
+		ID:          successCookie2,
+		SrcNode:     remoteNode.Node.ID,
+		SrcUser:     user.ID,
+		Created:     oneHourAgo,
+		AuthID:      testAuthPathSuccess,
+		AuthURL:     *testControlURL + testAuthPathSuccess,
+		PendingAuth: true,
 	})
 	failureCookie := "ts-cookie-failure"
 	s.browserSessions.Store(failureCookie, &browserSession{
@ -642,14 +653,15 @@ func TestServeAuth(t *testing.T) {
 				AuthID:        testAuthPath,
 				AuthURL:       *testControlURL + testAuthPath,
 				Authenticated: false,
+				PendingAuth:   true,
 			},
 		},
 		{
-			name:       "query-existing-incomplete-session",
-			path:       "/api/auth",
+			name:       "existing-session-used",
+			path:       "/api/auth/session/new", // should not create new session
 			cookie:     successCookie,
 			wantStatus: http.StatusOK,
-			wantResp:   &authResponse{ViewerIdentity: vi, ServerMode: ManageServerMode},
+			wantResp:   &newSessionAuthResponse{AuthURL: *testControlURL + testAuthPathSuccess},
 			wantSession: &browserSession{
 				ID:            successCookie,
 				SrcNode:       remoteNode.Node.ID,
@ -658,14 +670,15 @@ func TestServeAuth(t *testing.T) {
 				AuthID:        testAuthPathSuccess,
 				AuthURL:       *testControlURL + testAuthPathSuccess,
 				Authenticated: false,
+				PendingAuth:   true,
 			},
 		},
 		{
-			name:       "existing-session-used",
-			path:       "/api/auth/session/new", // should not create new session
+			name:       "transition-to-successful-session-via-api-auth-session-wait",
+			path:       "/api/auth/session/wait",
 			cookie:     successCookie,
 			wantStatus: http.StatusOK,
-			wantResp:   &newSessionAuthResponse{AuthURL: *testControlURL + testAuthPathSuccess},
+			wantResp:   nil,
 			wantSession: &browserSession{
 				ID:            successCookie,
 				SrcNode:       remoteNode.Node.ID,
@ -673,17 +686,17 @@ func TestServeAuth(t *testing.T) {
 				Created:       oneHourAgo,
 				AuthID:        testAuthPathSuccess,
 				AuthURL:       *testControlURL + testAuthPathSuccess,
-				Authenticated: false,
+				Authenticated: true,
 			},
 		},
 		{
-			name:       "transition-to-successful-session",
-			path:       "/api/auth/session/wait",
-			cookie:     successCookie,
+			name:       "transition-to-successful-session-via-api-auth",
+			path:       "/api/auth",
+			cookie:     successCookie2,
 			wantStatus: http.StatusOK,
-			wantResp:   nil,
+			wantResp:   &authResponse{Authorized: true, ViewerIdentity: vi, ServerMode: ManageServerMode},
 			wantSession: &browserSession{
-				ID:            successCookie,
+				ID:            successCookie2,
 				SrcNode:       remoteNode.Node.ID,
 				SrcUser:       user.ID,
 				Created:       oneHourAgo,
@ -731,6 +744,7 @@ func TestServeAuth(t *testing.T) {
 				AuthID:        testAuthPath,
 				AuthURL:       *testControlURL + testAuthPath,
 				Authenticated: false,
+				PendingAuth:   true,
 			},
 		},
 		{
@ -748,6 +762,7 @@ func TestServeAuth(t *testing.T) {
 				AuthID:        testAuthPath,
 				AuthURL:       *testControlURL + testAuthPath,
 				Authenticated: false,
+				PendingAuth:   true,
 			},
 		},
 		{