diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 576f01b6b..c506f1376 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3620,7 +3620,7 @@ func (b *LocalBackend) shouldUploadServices() bool { } // SetCurrentUser is used to implement support for multi-user systems (only -// Windows 2022-11-25). On such systems, the uid is used to determine which +// Windows 2022-11-25). On such systems, the actor is used to determine which // user's state should be used. The current user is maintained by active // connections open to the backend. // @@ -3634,11 +3634,8 @@ func (b *LocalBackend) shouldUploadServices() bool { // unattended mode. The user must disable unattended mode before the user can be // changed. // -// On non-multi-user systems, the user should be set to nil. -// -// SetCurrentUser returns the ipn.WindowsUserID associated with the user -// when successful. -func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) (ipn.WindowsUserID, error) { +// On non-multi-user systems, the actor should be set to nil. +func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) { var uid ipn.WindowsUserID if actor != nil { uid = actor.UserID() @@ -3647,16 +3644,17 @@ func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) (ipn.WindowsUserID, e unlock := b.lockAndGetUnlock() defer unlock() - if b.pm.CurrentUserID() == uid { - return uid, nil + if actor != b.currentUser { + if c, ok := b.currentUser.(ipnauth.ActorCloser); ok { + c.Close() + } + b.currentUser = actor } - b.pm.SetCurrentUserID(uid) - if c, ok := b.currentUser.(ipnauth.ActorCloser); ok { - c.Close() + + if b.pm.CurrentUserID() != uid { + b.pm.SetCurrentUserID(uid) + b.resetForProfileChangeLockedOnEntry(unlock) } - b.currentUser = actor - b.resetForProfileChangeLockedOnEntry(unlock) - return uid, nil } // CurrentUserForTest returns the current user and the associated WindowsUserID. diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 574d1a55c..c0e99f5d8 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -21,7 +21,6 @@ import ( "unicode" "tailscale.com/envknob" - "tailscale.com/ipn" "tailscale.com/ipn/ipnauth" "tailscale.com/ipn/ipnlocal" "tailscale.com/ipn/localapi" @@ -51,7 +50,6 @@ type Server struct { // mu guards the fields that follow. // lock order: mu, then LocalBackend.mu mu sync.Mutex - lastUserID ipn.WindowsUserID // tracks last userid; on change, Reset state for paranoia activeReqs map[*http.Request]ipnauth.Actor backendWaiter waiterSet // of LocalBackend waiters zeroReqWaiter waiterSet // of blockUntilZeroConnections waiters @@ -376,16 +374,6 @@ func (s *Server) addActiveHTTPRequest(req *http.Request, actor ipnauth.Actor) (o lb := s.mustBackend() - // If the connected user changes, reset the backend server state to make - // sure node keys don't leak between users. - var doReset bool - defer func() { - if doReset { - s.logf("identity changed; resetting server") - lb.ResetForClientDisconnect() - } - }() - s.mu.Lock() defer s.mu.Unlock() @@ -400,16 +388,7 @@ func (s *Server) addActiveHTTPRequest(req *http.Request, actor ipnauth.Actor) (o // Tell the LocalBackend about the identity we're now running as, // unless its the SYSTEM user. That user is not a real account and // doesn't have a home directory. - uid, err := lb.SetCurrentUser(actor) - if err != nil { - return nil, err - } - if s.lastUserID != uid { - if s.lastUserID != "" { - doReset = true - } - s.lastUserID = uid - } + lb.SetCurrentUser(actor) } } diff --git a/ipn/ipnserver/server_test.go b/ipn/ipnserver/server_test.go index 8a9324fab..7f6131328 100644 --- a/ipn/ipnserver/server_test.go +++ b/ipn/ipnserver/server_test.go @@ -158,6 +158,35 @@ func TestIPNAlreadyInUseOnWindows(t *testing.T) { server.checkCurrentUser(clientA.User) } +func TestSequentialOSUserSwitchingOnWindows(t *testing.T) { + enableLogging := false + setGOOSForTest(t, "windows") + + ctx := context.Background() + server := startDefaultTestIPNServer(t, ctx, enableLogging) + + connectDisconnectAsUser := func(name string) { + // User connects and starts watching the IPN bus. + client := server.getClientAs(name) + watcher, cancelWatcher := client.WatchIPNBus(ctx, 0) + defer cancelWatcher() + go pumpIPNBus(watcher) + + // It should be the current user from the LocalBackend's perspective... + server.checkCurrentUser(client.User) + // until it disconnects. + cancelWatcher() + server.blockWhileInUse(ctx) + // Now, the current user should be unset. + server.checkCurrentUser(nil) + } + + // UserA logs in, uses Tailscale for a bit, then logs out. + connectDisconnectAsUser("UserA") + // Same for UserB. + connectDisconnectAsUser("UserB") +} + func setGOOSForTest(tb testing.TB, goos string) { tb.Helper() envknob.Setenv("TS_DEBUG_FAKE_GOOS", goos)