[client] Allow logging out of the active profile when profiles are disabled (#7360)

* [client] Allow logging out of the active profile when profiles are disabled

A profile-addressed logout was refused outright when the profiles feature is
disabled: handleProfileLogout ran validateProfileOperation, which returned
Unavailable ("profiles are disabled, you cannot use this feature without
profiles enabled") before looking at which profile was targeted.

The desktop UI always addresses logout by profile — both the profile menu and
the session-expiration dialog send the active profile's ID — so a client with
profiles disabled could not log out at all; only a plain `netbird logout`,
which takes the profile-less path, still worked. Logging out of the profile the
daemon is already running is a deregistration, not profile management, and with
profiles disabled there is a single profile anyway, so every profile-addressed
logout is by definition an active-profile logout.

Replace validateProfileOperation with validateProfileLogout, which skips the
profiles-disabled check when the target is the active profile and keeps gating
logout of any other profile. This mirrors switchProfileIfNeeded, which already
gates only the branch that actually manages profiles. The dropped
allowActiveProfile parameter was always true, leaving canRemoveProfile
unreachable, so both are removed.

* [client] Compare the username and propagate state errors on profile logout

Review follow-ups on the logout gate:

Propagate the GetActiveProfileState failure instead of discarding it. A failed
lookup made the target look non-active, so a caller with profiles disabled got
"profiles are disabled" in place of the real error.

Compare the username along with the ID when deciding whether the target is the
active profile, matching switchProfileIfNeeded. Legacy profile IDs are display
names, so two users can hold the same ID in their own profile directories, and
an ID-only match let one user's logout pass the gate against the other user's
active profile. The default profile is shared and carries no username, so it
keeps matching on the ID alone.

Re-read the active profile before the connection teardown rather than reusing
the pre-flight snapshot. Login switches profiles under guardedConfigMu, which
the logout path does not hold, so a login that landed while the deregistration
was in flight would otherwise lose its fresh connection to a stale flag.

* [client] Address review on the profile logout gate

Pass the username down to logoutFromProfile and reuse the running config only
when the target is the active profile for that username. On an ID-only match a
legacy profile ID shared between two users made the connected-client path
deregister the active peer while its connection stayed up, which the gate fix
alone did not cover.

Split the setup-key-less branch of Login into beginSSOLogin, with the
reuse-the-pending-flow decision in pendingOAuthFlowResponse. Login's cognitive
complexity drops from 37 to 21 (gocognit), clearing the SonarQube report on
this file with no behaviour change.

Point the test fixture at an https URL, since the profiles a gated logout must
not touch only need to be unreachable, not plaintext.

client/server/logout_gate_test.go

@@ -0,0 +1,200 @@
+package server
+
+import (
+	"context"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/codes"
+	gstatus "google.golang.org/grpc/status"
+
+	"github.com/netbirdio/netbird/client/internal"
+	"github.com/netbirdio/netbird/client/internal/profilemanager"
+	"github.com/netbirdio/netbird/client/proto"
+)
+
+// unreachableManagementURL keeps a test that is expected to stop at a gate from
+// reaching the network if the gate ever regresses: the profiles a logout must
+// not touch point here, so a leak fails fast instead of contacting a real
+// management server.
+const unreachableManagementURL = "https://127.0.0.1:9"
+
+// enableSSHOnProfile rewrites the profile config at cfgPath with the SSH server
+// enabled. Deregistering an SSH-enabled profile is a privileged change, so an
+// unprivileged caller is refused by requirePrivilegeForDeregistration before any
+// management connection is attempted, which is what keeps these tests offline.
+func enableSSHOnProfile(t *testing.T, cfgPath string) {
+	t.Helper()
+	_, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
+		ConfigPath:       cfgPath,
+		ManagementURL:    "https://api.netbird.io:443",
+		ServerSSHAllowed: boolPtr(true),
+	})
+	require.NoError(t, err)
+}
+
+// Logging out of the profile the daemon is already running is a deregistration,
+// not profile management, so the profiles-disabled kill switch must not block
+// it. The desktop UI always addresses logout by profile (both the profile menu
+// and the session-expiration dialog), so gating it left users with
+// disableProfiles enforced unable to log out at all.
+func TestLogout_ActiveProfileAllowedWhenProfilesDisabled(t *testing.T) {
+	s, _, activeProfile, username, cfgPath := setupServerWithProfile(t)
+	s.rootCtx = internal.CtxInitState(context.Background())
+	enableSSHOnProfile(t, cfgPath)
+
+	s.profilesDisabled = true
+
+	_, err := s.Logout(userCtx(), &proto.LogoutRequest{
+		ProfileName: &activeProfile,
+		Username:    &username,
+	})
+
+	require.Error(t, err, "the SSH privilege gate is expected to refuse this unprivileged caller")
+	require.Equal(t, codes.PermissionDenied, gstatus.Code(err),
+		"logout of the active profile must reach the deregistration path, not be refused as profile management: %v", err)
+	require.NotContains(t, gstatus.Convert(err).Message(), errProfilesDisabled)
+}
+
+// A profile-addressed logout that targets some *other* profile does manage
+// profiles, so it stays gated: with profiles disabled the daemon must not
+// deregister a peer the user is not currently running.
+func TestLogout_OtherProfileStaysGatedWhenProfilesDisabled(t *testing.T) {
+	s, _, _, username, _ := setupServerWithProfile(t)
+	s.rootCtx = internal.CtxInitState(context.Background())
+
+	other := "other-profile"
+	_, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
+		ConfigPath:    filepath.Join(profilemanager.DefaultConfigPathDir, other+".json"),
+		ManagementURL: unreachableManagementURL,
+	})
+	require.NoError(t, err)
+
+	s.profilesDisabled = true
+
+	_, err = s.Logout(userCtx(), &proto.LogoutRequest{
+		ProfileName: &other,
+		Username:    &username,
+	})
+
+	require.Error(t, err)
+	require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the profiles-disabled refusal, got %v", err)
+	require.Contains(t, gstatus.Convert(err).Message(), errProfilesDisabled)
+}
+
+// A legacy profile ID is a display name, so two users can hold the same ID in
+// their own profile directories. Matching on the ID alone would let one user's
+// logout pass the gate against the other user's active profile, so the username
+// is part of the comparison.
+func TestLogout_ForeignUserProfileStaysGatedWhenProfilesDisabled(t *testing.T) {
+	s, _, _, username, _ := setupServerWithProfile(t)
+	s.rootCtx = internal.CtxInitState(context.Background())
+
+	// A legacy-style profile whose ID is its filename stem, and an active state
+	// claiming that same ID for a different user.
+	shared := "shared-legacy-name"
+	_, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
+		ConfigPath:    filepath.Join(profilemanager.DefaultConfigPathDir, shared+".json"),
+		ManagementURL: unreachableManagementURL,
+	})
+	require.NoError(t, err)
+	require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{
+		ID:       profilemanager.ID(shared),
+		Username: "someone-else",
+	}))
+
+	s.profilesDisabled = true
+
+	_, err = s.Logout(userCtx(), &proto.LogoutRequest{
+		ProfileName: &shared,
+		Username:    &username,
+	})
+
+	require.Error(t, err)
+	require.Equal(t, codes.Unavailable, gstatus.Code(err),
+		"another user's profile must not pass the gate on an ID match alone: %v", err)
+}
+
+// Deregistering a namesake profile must not go out with the running config.
+// logoutFromProfile reuses the connected client's config when the target is the
+// active profile, and on an ID-only match a shared legacy ID made it reuse it
+// for another user's profile, deregistering the active peer instead.
+func TestLogout_ForeignUserProfileDoesNotUseTheRunningConfig(t *testing.T) {
+	s, _, _, username, cfgPath := setupServerWithProfile(t)
+	s.rootCtx = internal.CtxInitState(context.Background())
+
+	// The running config has the SSH server enabled, so reusing it would be
+	// refused with PermissionDenied. The namesake profile does not, so the
+	// correct path gets as far as dialing its own unreachable management URL.
+	enableSSHOnProfile(t, cfgPath)
+	running, err := profilemanager.GetConfig(cfgPath)
+	require.NoError(t, err)
+	s.config = running
+	s.connectClient = newDummyConnectClient(context.Background())
+
+	shared := "shared-legacy-name"
+	_, err = profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
+		ConfigPath:    filepath.Join(profilemanager.DefaultConfigPathDir, shared+".json"),
+		ManagementURL: unreachableManagementURL,
+	})
+	require.NoError(t, err)
+	require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{
+		ID:       profilemanager.ID(shared),
+		Username: "someone-else",
+	}))
+
+	// Bounded so the deregistration the fixed path attempts fails on the dial
+	// rather than sitting in gRPC backoff for the whole test timeout.
+	ctx, cancel := context.WithTimeout(userCtx(), 2*time.Second)
+	t.Cleanup(cancel)
+
+	_, err = s.Logout(ctx, &proto.LogoutRequest{
+		ProfileName: &shared,
+		Username:    &username,
+	})
+
+	require.Error(t, err)
+	require.NotEqual(t, codes.PermissionDenied, gstatus.Code(err),
+		"the namesake profile was deregistered with the running config: %v", err)
+}
+
+// The connection teardown follows the profile that is active when the logout
+// completes, not the one seen before it started: Login switches profiles under
+// guardedConfigMu, which the logout path does not hold, so a login that landed
+// meanwhile must keep its connection.
+func TestCleanupAfterProfileLogout_FollowsTheCurrentActiveProfile(t *testing.T) {
+	s, _, activeProfile, username, _ := setupServerWithProfile(t)
+	s.rootCtx = internal.CtxInitState(context.Background())
+
+	state := internal.CtxGetState(s.rootCtx)
+
+	s.cleanupAfterProfileLogout("some-other-profile", username)
+	status, err := state.Status()
+	require.NoError(t, err)
+	require.NotEqual(t, internal.StatusNeedsLogin, status,
+		"logging out of a profile that is not active must not ask for a new login")
+
+	s.cleanupAfterProfileLogout(profilemanager.ID(activeProfile), username)
+	status, err = state.Status()
+	require.NoError(t, err)
+	require.Equal(t, internal.StatusNeedsLogin, status,
+		"logging out of the active profile must ask for a new login")
+}
+
+// With profiles enabled the gate is out of the way on both surfaces; the active
+// profile still reaches the deregistration path.
+func TestLogout_ActiveProfileAllowedWhenProfilesEnabled(t *testing.T) {
+	s, _, activeProfile, username, cfgPath := setupServerWithProfile(t)
+	s.rootCtx = internal.CtxInitState(context.Background())
+	enableSSHOnProfile(t, cfgPath)
+
+	_, err := s.Logout(userCtx(), &proto.LogoutRequest{
+		ProfileName: &activeProfile,
+		Username:    &username,
+	})
+
+	require.Error(t, err, "the SSH privilege gate is expected to refuse this unprivileged caller")
+	require.Equal(t, codes.PermissionDenied, gstatus.Code(err), "want the privilege refusal, got %v", err)
+}

client/server/server.go

@@ -710,54 +710,7 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro
 	}
 
 	if msg.SetupKey == "" {
-		hint := ""
-		if msg.Hint != nil {
-			hint = *msg.Hint
-		}
-		oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.IsUnixDesktopClient, false, hint)
-		if err != nil {
-			state.Set(internal.StatusLoginFailed)
-			return nil, err
-		}
-
-		if s.oauthAuthFlow.flow != nil && s.oauthAuthFlow.flow.GetClientID(ctx) == oAuthFlow.GetClientID(ctx) {
-			if s.oauthAuthFlow.expiresAt.After(time.Now().Add(90 * time.Second)) {
-				log.Debugf("using previous oauth flow info")
-				state.Set(internal.StatusNeedsLogin)
-				return &proto.LoginResponse{
-					NeedsSSOLogin:           true,
-					VerificationURI:         s.oauthAuthFlow.info.VerificationURI,
-					VerificationURIComplete: s.oauthAuthFlow.info.VerificationURIComplete,
-					UserCode:                s.oauthAuthFlow.info.UserCode,
-				}, nil
-			} else {
-				log.Warnf("canceling previous waiting execution")
-				if s.oauthAuthFlow.waitCancel != nil {
-					s.oauthAuthFlow.waitCancel()
-				}
-			}
-		}
-
-		authInfo, err := oAuthFlow.RequestAuthInfo(ctx)
-		if err != nil {
-			log.Errorf("getting a request OAuth flow failed: %v", err)
-			return nil, err
-		}
-
-		s.mutex.Lock()
-		s.oauthAuthFlow.flow = oAuthFlow
-		s.oauthAuthFlow.info = authInfo
-		s.oauthAuthFlow.expiresAt = time.Now().Add(time.Duration(authInfo.ExpiresIn) * time.Second)
-		s.mutex.Unlock()
-
-		state.Set(internal.StatusNeedsLogin)
-
-		return &proto.LoginResponse{
-			NeedsSSOLogin:           true,
-			VerificationURI:         authInfo.VerificationURI,
-			VerificationURIComplete: authInfo.VerificationURIComplete,
-			UserCode:                authInfo.UserCode,
-		}, nil
+		return s.beginSSOLogin(ctx, config, msg)
 	}
 
 	// Setup-key path: we are about to dial Management with the key, so the
@@ -773,6 +726,76 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro
 	return &proto.LoginResponse{}, nil
 }
 
+// beginSSOLogin starts the browser leg of a login that carries no setup key and
+// returns the response that parks the caller on it.
+func (s *Server) beginSSOLogin(ctx context.Context, config *profilemanager.Config, msg *proto.LoginRequest) (*proto.LoginResponse, error) {
+	state := internal.CtxGetState(s.rootCtx)
+
+	hint := ""
+	if msg.Hint != nil {
+		hint = *msg.Hint
+	}
+	oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.IsUnixDesktopClient, false, hint)
+	if err != nil {
+		state.Set(internal.StatusLoginFailed)
+		return nil, err
+	}
+
+	if resp := s.pendingOAuthFlowResponse(ctx, oAuthFlow); resp != nil {
+		state.Set(internal.StatusNeedsLogin)
+		return resp, nil
+	}
+
+	authInfo, err := oAuthFlow.RequestAuthInfo(ctx)
+	if err != nil {
+		log.Errorf("getting a request OAuth flow failed: %v", err)
+		return nil, err
+	}
+
+	s.mutex.Lock()
+	s.oauthAuthFlow.flow = oAuthFlow
+	s.oauthAuthFlow.info = authInfo
+	s.oauthAuthFlow.expiresAt = time.Now().Add(time.Duration(authInfo.ExpiresIn) * time.Second)
+	s.mutex.Unlock()
+
+	state.Set(internal.StatusNeedsLogin)
+
+	return &proto.LoginResponse{
+		NeedsSSOLogin:           true,
+		VerificationURI:         authInfo.VerificationURI,
+		VerificationURIComplete: authInfo.VerificationURIComplete,
+		UserCode:                authInfo.UserCode,
+	}, nil
+}
+
+// pendingOAuthFlowResponse returns the in-flight flow's response when it
+// targets the same IdP client and has enough time left for the user to finish
+// the browser leg, so a second login joins the pending flow instead of opening
+// a competing one. A flow too close to expiry has its waiter cancelled and nil
+// returned, leaving the caller to start a fresh flow.
+func (s *Server) pendingOAuthFlowResponse(ctx context.Context, oAuthFlow auth.OAuthFlow) *proto.LoginResponse {
+	if s.oauthAuthFlow.flow == nil || s.oauthAuthFlow.flow.GetClientID(ctx) != oAuthFlow.GetClientID(ctx) {
+		return nil
+	}
+
+	if s.oauthAuthFlow.expiresAt.After(time.Now().Add(90 * time.Second)) {
+		log.Debugf("using previous oauth flow info")
+		return &proto.LoginResponse{
+			NeedsSSOLogin:           true,
+			VerificationURI:         s.oauthAuthFlow.info.VerificationURI,
+			VerificationURIComplete: s.oauthAuthFlow.info.VerificationURIComplete,
+			UserCode:                s.oauthAuthFlow.info.UserCode,
+		}
+	}
+
+	log.Warnf("canceling previous waiting execution")
+	if s.oauthAuthFlow.waitCancel != nil {
+		s.oauthAuthFlow.waitCancel()
+	}
+
+	return nil
+}
+
 // WaitSSOLogin validates the supplied userCode against the in-flight OAuth
 // device/PKCE flow and blocks until the user finishes the browser leg.
 //
@@ -1347,11 +1370,16 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque
 		return nil, err
 	}
 
-	if err := s.validateProfileOperation(resolved.ID, true); err != nil {
+	activeProf, err := s.profileManager.GetActiveProfileState()
+	if err != nil {
+		return nil, gstatus.Errorf(codes.FailedPrecondition, "failed to get active profile state: %v", err)
+	}
+
+	if err := s.validateProfileLogout(resolved.ID, isActiveProfile(activeProf, resolved.ID, username)); err != nil {
 		return nil, err
 	}
 
-	if err := s.logoutFromProfile(ctx, resolved); err != nil {
+	if err := s.logoutFromProfile(ctx, resolved, username); err != nil {
 		log.Errorf("failed to logout from profile %s: %v", resolved.ID, err)
 		// A refused deregistration is already a status error carrying the reason
 		// and the command to run; rewrapping it as Internal would flatten both
@@ -1362,18 +1390,35 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque
 		return nil, gstatus.Errorf(codes.Internal, "logout: %v", err)
 	}
 
-	activeProf, _ := s.profileManager.GetActiveProfileState()
-	if activeProf != nil && activeProf.ID == resolved.ID {
-		if err := s.cleanupConnection(); err != nil && !errors.Is(err, ErrServiceNotUp) {
-			log.Errorf("failed to cleanup connection: %v", err)
-		}
-		state := internal.CtxGetState(s.rootCtx)
-		state.Set(internal.StatusNeedsLogin)
-	}
+	s.cleanupAfterProfileLogout(resolved.ID, username)
 
 	return &proto.LogoutResponse{}, nil
 }
 
+// cleanupAfterProfileLogout tears the connection down and asks for a new login
+// when the profile that was just deregistered is the one the daemon is running.
+// The active profile is read again here rather than reused from the pre-flight
+// check: Login switches profiles under guardedConfigMu, which this path does not
+// hold, so a login that landed meanwhile must not have its fresh connection
+// dropped by a logout that targeted the profile it replaced.
+func (s *Server) cleanupAfterProfileLogout(id profilemanager.ID, username string) {
+	activeProf, err := s.profileManager.GetActiveProfileState()
+	if err != nil {
+		log.Errorf("failed to get active profile state after logout from profile %s: %v", id, err)
+		return
+	}
+
+	if !isActiveProfile(activeProf, id, username) {
+		return
+	}
+
+	if err := s.cleanupConnection(); err != nil && !errors.Is(err, ErrServiceNotUp) {
+		log.Errorf("failed to cleanup connection: %v", err)
+	}
+	state := internal.CtxGetState(s.rootCtx)
+	state.Set(internal.StatusNeedsLogin)
+}
+
 func (s *Server) handleActiveProfileLogout(ctx context.Context) (*proto.LogoutResponse, error) {
 	if s.config == nil {
 		activeProf, err := s.profileManager.GetActiveProfileState()
@@ -1425,40 +1470,47 @@ func (s *Server) getConfig(activeProf *profilemanager.ActiveProfileState) (*prof
 	return config, configExisted, nil
 }
 
-func (s *Server) canRemoveProfile(id profilemanager.ID) error {
-	if id == profilemanager.DefaultProfileName {
-		return fmt.Errorf("remove profile with reserved name: %s", profilemanager.DefaultProfileName)
-	}
-
-	activeProf, err := s.profileManager.GetActiveProfileState()
-	if err == nil && activeProf.ID == id {
-		return fmt.Errorf("remove active profile: %s", id)
-	}
-
-	return nil
-}
-
-func (s *Server) validateProfileOperation(id profilemanager.ID, allowActiveProfile bool) error {
-	if s.checkProfilesDisabled() {
-		return gstatus.Errorf(codes.Unavailable, errProfilesDisabled)
-	}
-
+// validateProfileLogout gates a profile-addressed logout. Deregistering the
+// profile the daemon already runs is what a plain `netbird logout` does, so the
+// profiles-disabled kill switch must not block it. Logging out of any other
+// profile is profile management and stays gated.
+func (s *Server) validateProfileLogout(id profilemanager.ID, isActive bool) error {
 	if id == "" {
 		return gstatus.Errorf(codes.InvalidArgument, "profile name must be provided")
 	}
 
-	if !allowActiveProfile {
-		if err := s.canRemoveProfile(id); err != nil {
-			return gstatus.Errorf(codes.InvalidArgument, "%v", err)
-		}
+	if isActive {
+		return nil
+	}
+
+	if s.checkProfilesDisabled() {
+		return gstatus.Errorf(codes.Unavailable, errProfilesDisabled)
 	}
 
 	return nil
 }
 
-func (s *Server) logoutFromProfile(ctx context.Context, profile *profilemanager.Profile) error {
+// isActiveProfile reports whether id is the profile the daemon runs for
+// username. The username is part of the comparison because legacy profile IDs
+// are display names, which two users can both hold; the default profile is
+// shared by every user and carries no username.
+func isActiveProfile(activeProf *profilemanager.ActiveProfileState, id profilemanager.ID, username string) bool {
+	if activeProf == nil || activeProf.ID != id {
+		return false
+	}
+
+	return id == profilemanager.DefaultProfileName || activeProf.Username == username
+}
+
+// logoutFromProfile deregisters profile, reusing the running config when
+// profile is the one the daemon is connected with. The username takes part in
+// that decision for the same reason it does in the logout gate: a legacy
+// profile ID is a display name two users can share, and sending the running
+// config for a namesake would deregister the active peer instead of the
+// requested one.
+func (s *Server) logoutFromProfile(ctx context.Context, profile *profilemanager.Profile, username string) error {
 	activeProf, err := s.profileManager.GetActiveProfileState()
-	if err == nil && activeProf.ID == profile.ID && s.connectClient != nil {
+	if err == nil && isActiveProfile(activeProf, profile.ID, username) && s.connectClient != nil {
 		return s.sendLogoutRequest(ctx)
 	}
 
@@ -2227,7 +2279,7 @@ func (s *Server) RemoveProfile(ctx context.Context, msg *proto.RemoveProfileRequ
 		return nil, err
 	}
 
-	if err := s.logoutFromProfile(ctx, resolved); err != nil {
+	if err := s.logoutFromProfile(ctx, resolved, msg.Username); err != nil {
 		// Deregistration is best-effort here: the local profile is removed
 		// either way, so an unprivileged caller leaves the peer registered on
 		// the management server rather than being blocked from removing it.