From fb87f751a5f2da6d463a333e6ccb6e470269e12c Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Fri, 19 Jun 2026 11:39:21 +0200 Subject: [PATCH 1/5] [management] Fetch complete user data in ValidateTunnelPeer (#6457) * [management] Fetch complete user data in ValidateTunnelPeer Previously the `ValidateTunnelPeer` method used by the ProxyService would fetch user information from the database if the connected peer was associated with a user ID, but it would not consult the IdP data for cached info from JWT claims like email. This caused the value of the injected `X-Netbird-User` header to always display the peer ID and never the user email associated with the peer as expected. This change adds an optional IdP manager to the ProxyService and fetches the complete user data from it if present. * [management] Refactor ValidateTunnelPeer principal info gathering This refactors the gathering of info on proxy tunnel peer principals into its own method to keep the complexity down and make Sonar happy. --- .../service/manager/manager_test.go | 6 +- management/internals/server/boot.go | 2 +- management/internals/shared/grpc/proxy.go | 64 ++++-- .../shared/grpc/proxy_group_access_test.go | 208 ++++++++++++++++++ .../shared/grpc/validate_session_test.go | 2 +- management/server/account_test.go | 2 +- .../proxy/auth_callback_integration_test.go | 1 + .../testing/testing_tools/channel/channel.go | 4 +- proxy/management_byop_integration_test.go | 1 + proxy/management_integration_test.go | 1 + 10 files changed, 266 insertions(+), 25 deletions(-) diff --git a/management/internals/modules/reverseproxy/service/manager/manager_test.go b/management/internals/modules/reverseproxy/service/manager/manager_test.go index ace105b31..29a117921 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/service/manager/manager_test.go @@ -434,7 +434,7 @@ func TestDeletePeerService_SourcePeerValidation(t *testing.T) { t.Helper() tokenStore := nbgrpc.NewOneTimeTokenStore(context.Background(), testCacheStore(t)) pkceStore := nbgrpc.NewPKCEVerifierStore(context.Background(), testCacheStore(t)) - srv := nbgrpc.NewProxyServiceServer(nil, tokenStore, pkceStore, nbgrpc.ProxyOIDCConfig{}, nil, nil, nil, nil) + srv := nbgrpc.NewProxyServiceServer(nil, tokenStore, pkceStore, nbgrpc.ProxyOIDCConfig{}, nil, nil, nil, nil, nil) return srv } @@ -723,7 +723,7 @@ func setupIntegrationTest(t *testing.T) (*Manager, store.Store) { tokenStore := nbgrpc.NewOneTimeTokenStore(ctx, testCacheStore(t)) pkceStore := nbgrpc.NewPKCEVerifierStore(ctx, testCacheStore(t)) - proxySrv := nbgrpc.NewProxyServiceServer(nil, tokenStore, pkceStore, nbgrpc.ProxyOIDCConfig{}, nil, nil, nil, nil) + proxySrv := nbgrpc.NewProxyServiceServer(nil, tokenStore, pkceStore, nbgrpc.ProxyOIDCConfig{}, nil, nil, nil, nil, nil) proxyController, err := proxymanager.NewGRPCController(proxySrv, noop.NewMeterProvider().Meter("")) require.NoError(t, err) @@ -1147,7 +1147,7 @@ func TestDeleteService_DeletesTargets(t *testing.T) { tokenStore := nbgrpc.NewOneTimeTokenStore(ctx, testCacheStore(t)) pkceStore := nbgrpc.NewPKCEVerifierStore(ctx, testCacheStore(t)) - proxySrv := nbgrpc.NewProxyServiceServer(nil, tokenStore, pkceStore, nbgrpc.ProxyOIDCConfig{}, nil, nil, nil, nil) + proxySrv := nbgrpc.NewProxyServiceServer(nil, tokenStore, pkceStore, nbgrpc.ProxyOIDCConfig{}, nil, nil, nil, nil, nil) proxyController, err := proxymanager.NewGRPCController(proxySrv, noop.NewMeterProvider().Meter("")) require.NoError(t, err) diff --git a/management/internals/server/boot.go b/management/internals/server/boot.go index 46e475143..ae82b60fe 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -219,7 +219,7 @@ func (s *BaseServer) GRPCServer() *grpc.Server { func (s *BaseServer) ReverseProxyGRPCServer() *nbgrpc.ProxyServiceServer { return Create(s, func() *nbgrpc.ProxyServiceServer { - proxyService := nbgrpc.NewProxyServiceServer(s.AccessLogsManager(), s.ProxyTokenStore(), s.PKCEVerifierStore(), s.proxyOIDCConfig(), s.PeersManager(), s.UsersManager(), s.ProxyManager(), s.Store()) + proxyService := nbgrpc.NewProxyServiceServer(s.AccessLogsManager(), s.ProxyTokenStore(), s.PKCEVerifierStore(), s.proxyOIDCConfig(), s.PeersManager(), s.UsersManager(), s.IdpManager(), s.ProxyManager(), s.Store()) s.AfterInit(func(s *BaseServer) { proxyService.SetServiceManager(s.ServiceManager()) proxyService.SetProxyController(s.ServiceProxyController()) diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 0feb807f6..76663f898 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -33,6 +33,8 @@ import ( "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/sessionkey" + "github.com/netbirdio/netbird/management/server/idp" + "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/management/server/users" proxyauth "github.com/netbirdio/netbird/proxy/auth" @@ -82,6 +84,9 @@ type ProxyServiceServer struct { // Manager for users usersManager users.Manager + // Manager for IdP-enriched user data (may be nil when no IdP is configured) + idpManager idp.Manager + // Store for one-time authentication tokens tokenStore *OneTimeTokenStore @@ -157,7 +162,7 @@ func enforceAccountScope(ctx context.Context, requestAccountID string) error { } // NewProxyServiceServer creates a new proxy service server. -func NewProxyServiceServer(accessLogMgr accesslogs.Manager, tokenStore *OneTimeTokenStore, pkceStore *PKCEVerifierStore, oidcConfig ProxyOIDCConfig, peersManager peers.Manager, usersManager users.Manager, proxyMgr proxy.Manager, tokenChecker ProxyTokenChecker) *ProxyServiceServer { +func NewProxyServiceServer(accessLogMgr accesslogs.Manager, tokenStore *OneTimeTokenStore, pkceStore *PKCEVerifierStore, oidcConfig ProxyOIDCConfig, peersManager peers.Manager, usersManager users.Manager, idpManager idp.Manager, proxyMgr proxy.Manager, tokenChecker ProxyTokenChecker) *ProxyServiceServer { ctx, cancel := context.WithCancel(context.Background()) s := &ProxyServiceServer{ accessLogManager: accessLogMgr, @@ -166,6 +171,7 @@ func NewProxyServiceServer(accessLogMgr accesslogs.Manager, tokenStore *OneTimeT pkceVerifierStore: pkceStore, peersManager: peersManager, usersManager: usersManager, + idpManager: idpManager, proxyManager: proxyMgr, tokenChecker: tokenChecker, snapshotBatchSize: snapshotBatchSizeFromEnv(), @@ -1702,22 +1708,7 @@ func (s *ProxyServiceServer) ValidateTunnelPeer(ctx context.Context, req *proto. } groupIDs, groupNames := pairGroupIDsAndNames(peerGroups) - - // Resolve the principal: when the peer is linked to a user, the human - // is the principal so multiple peers owned by the same user share a - // single identity. Unlinked peers (machine agents) are their own - // principal keyed on peer.ID. displayIdentity is what upstream gateways - // tag spend with — user.Email when linked, peer.Name when not. - principalID := peer.ID - displayIdentity := peer.Name - if peer.UserID != "" { - if user, uerr := s.usersManager.GetUser(ctx, peer.UserID); uerr == nil && user != nil { - principalID = user.Id - if user.Email != "" { - displayIdentity = user.Email - } - } - } + principalID, displayIdentity := s.getTunnelPeerInfo(ctx, domain, service, peer) if err := checkPeerGroupAccess(service, groupIDs); err != nil { log.WithFields(log.Fields{"domain": domain, "peer_id": peer.ID, "error": err.Error()}).Debug("ValidateTunnelPeer: access denied") @@ -1754,6 +1745,45 @@ func (s *ProxyServiceServer) ValidateTunnelPeer(ctx context.Context, req *proto. }, nil } +// getTunnelPeerInfo returns the principal ID and display name for a peer, e.g. a +// user or peer ID, and peer name or user email. +func (s *ProxyServiceServer) getTunnelPeerInfo(ctx context.Context, domain string, service *rpservice.Service, peer *peer.Peer) (string, string) { + // Resolve the principal: when the peer is linked to a user, the human is the + // principal so multiple peers owned by the same user share a single + // identity. Unlinked peers (machine agents) are their own principal keyed on + // peer.ID. displayIdentity is what upstream gateways tag spend with — + // user.Email when linked, peer.Name when not. + + // If the peer isn't associated with a user, return the peer info directly. + if peer.UserID == "" { + return peer.ID, peer.Name + } + + // Otherwise, if the peer is linked to a user, the user is the principal and + // if an IdP is available, we gather details on the user from it. + principalID := peer.UserID + displayIdentity := peer.Name + // Stored column first (cheap, but often empty for OIDC-provisioned users). + if user, uerr := s.usersManager.GetUser(ctx, peer.UserID); uerr == nil && user != nil { + principalID = user.Id + if user.Email != "" { + displayIdentity = user.Email + } + } + // IdP enrichment wins when available — the stored email column is a + // best-effort cache and is frequently empty for OIDC users. Enrichment + // failures must never fail the RPC; we simply keep the stored/peer identity. + if s.idpManager != nil { + if ud, uerr := s.idpManager.GetUserDataByID(ctx, peer.UserID, idp.AppMetadata{WTAccountID: service.AccountID}); uerr == nil && ud != nil && ud.Email != "" { + displayIdentity = ud.Email + } else if uerr != nil { + log.WithFields(log.Fields{"domain": domain, "user_id": peer.UserID, "error": uerr.Error()}).Debug("ValidateTunnelPeer: IdP user enrichment failed; using stored/peer identity") + } + } + + return principalID, displayIdentity +} + // checkPeerGroupAccess gates ValidateTunnelPeer by the service's required // groups. Private services authorise against AccessGroups (empty list fails // closed — Validate() rejects that at save time but the RPC is the security diff --git a/management/internals/shared/grpc/proxy_group_access_test.go b/management/internals/shared/grpc/proxy_group_access_test.go index 76da7ddbc..532cb7cc3 100644 --- a/management/internals/shared/grpc/proxy_group_access_test.go +++ b/management/internals/shared/grpc/proxy_group_access_test.go @@ -3,14 +3,19 @@ package grpc import ( "context" "errors" + "net" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/management/internals/modules/peers" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" + "github.com/netbirdio/netbird/management/server/idp" + "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/shared/management/proto" ) type mockReverseProxyManager struct { @@ -137,6 +142,52 @@ func (m *mockUsersManager) GetUserWithGroups(ctx context.Context, userID string) return user, nil, nil } +// mockTunnelPeersManager implements only the two peers.Manager methods that +// ValidateTunnelPeer calls; the embedded interface satisfies the rest (and +// panics if any unexpected method is invoked). +type mockTunnelPeersManager struct { + peers.Manager + peer *peer.Peer + peerErr error + groups []*types.Group + groupsErr error +} + +func (m *mockTunnelPeersManager) GetPeerByTunnelIP(_ context.Context, _ string, _ net.IP) (*peer.Peer, error) { + return m.peer, m.peerErr +} + +func (m *mockTunnelPeersManager) GetPeerWithGroups(_ context.Context, _, _ string) (*peer.Peer, []*types.Group, error) { + return m.peer, m.groups, m.groupsErr +} + +// mockTunnelIdpManager implements only GetUserDataByID; the embedded interface +// satisfies the rest of idp.Manager. hasData==false returns (nil, nil) to model +// an IdP that knows nothing about the user. +type mockTunnelIdpManager struct { + idp.Manager + email string + hasData bool + err error + gotCalls int + gotMeta []idp.AppMetadata +} + +func (m *mockTunnelIdpManager) GetUserDataByID(_ context.Context, userID string, meta idp.AppMetadata) (*idp.UserData, error) { + m.gotCalls++ + m.gotMeta = append(m.gotMeta, meta) + if m.err != nil { + return nil, m.err + } + if !m.hasData { + // This might not be a thing any of the actual IDP implementations do, + // i.e. return a nil value with no error, but it seems valuable to test + // that behavior here. + return nil, nil //nolint:nilnil + } + return &idp.UserData{ID: userID, Email: m.email}, nil +} + func TestValidateUserGroupAccess(t *testing.T) { tests := []struct { name string @@ -354,6 +405,163 @@ func TestValidateUserGroupAccess(t *testing.T) { } } +// TestValidateTunnelPeerUserEmailEnrichment verifies the UserEmail/UserId +// resolution in ValidateTunnelPeer, including the IdP-enrichment fallback order +// (IdP email -> stored User.Email -> peer.Name). +func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) { + const ( + domain = "app.example.com" + accountID = "account1" + peerID = "peer1" + peerName = "peer-display-name" + userID = "user1" + ) + + storedUser := map[string]*types.User{userID: {Id: userID, AccountID: accountID, Email: "stored@example.com"}} + storedUserNoEmail := map[string]*types.User{userID: {Id: userID, AccountID: accountID, Email: ""}} + + tests := []struct { + name string + peerUserID string + storedUsers map[string]*types.User + storedErr error + noIdP bool + idpEmail string + idpHasData bool + idpErr error + expectEmail string + expectUserID string + expectIdPHit bool + }{ + { + name: "idp email wins over stored email", + peerUserID: userID, + storedUsers: storedUser, + idpEmail: "idp@example.com", + idpHasData: true, + expectEmail: "idp@example.com", + expectUserID: userID, + expectIdPHit: true, + }, + { + name: "stored email when idp returns empty email", + peerUserID: userID, + storedUsers: storedUser, + idpEmail: "", + idpHasData: true, + expectEmail: "stored@example.com", + expectUserID: userID, + expectIdPHit: true, + }, + { + name: "stored email when idp has no data", + peerUserID: userID, + storedUsers: storedUser, + idpHasData: false, + expectEmail: "stored@example.com", + expectUserID: userID, + expectIdPHit: true, + }, + { + name: "stored email when idp errors", + peerUserID: userID, + storedUsers: storedUser, + idpErr: errors.New("idp unreachable"), + expectEmail: "stored@example.com", + expectUserID: userID, + expectIdPHit: true, + }, + { + name: "stored email when no idp manager", + peerUserID: userID, + storedUsers: storedUser, + noIdP: true, + expectEmail: "stored@example.com", + expectUserID: userID, + }, + { + name: "idp email when stored email is empty", + peerUserID: userID, + storedUsers: storedUserNoEmail, + idpEmail: "idp@example.com", + idpHasData: true, + expectEmail: "idp@example.com", + expectUserID: userID, + expectIdPHit: true, + }, + { + name: "idp email when stored user missing keeps peer.UserID as principal", + peerUserID: userID, + storedUsers: map[string]*types.User{}, + idpEmail: "idp@example.com", + idpHasData: true, + expectEmail: "idp@example.com", + expectUserID: userID, + expectIdPHit: true, + }, + { + name: "unlinked peer uses peer name and never consults idp", + peerUserID: "", + storedUsers: storedUser, + idpEmail: "idp@example.com", + idpHasData: true, + expectEmail: peerName, + expectUserID: peerID, + expectIdPHit: false, + }, + { + name: "linked peer with empty stored email and no idp falls back to peer name", + peerUserID: userID, + storedUsers: storedUserNoEmail, + noIdP: true, + expectEmail: peerName, + expectUserID: userID, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := &service.Service{Domain: domain, AccountID: accountID} + server := &ProxyServiceServer{ + serviceManager: &mockReverseProxyManager{ + proxiesByAccount: map[string][]*service.Service{accountID: {svc}}, + }, + peersManager: &mockTunnelPeersManager{ + peer: &peer.Peer{ID: peerID, Name: peerName, UserID: tt.peerUserID}, + }, + usersManager: &mockUsersManager{users: tt.storedUsers, err: tt.storedErr}, + } + + var idpMock *mockTunnelIdpManager + if !tt.noIdP { + idpMock = &mockTunnelIdpManager{email: tt.idpEmail, hasData: tt.idpHasData, err: tt.idpErr} + server.idpManager = idpMock + } + + resp, err := server.ValidateTunnelPeer(context.Background(), &proto.ValidateTunnelPeerRequest{ + Domain: domain, + TunnelIp: "100.64.0.1", + }) + + require.NoError(t, err) + require.NotNil(t, resp) + assert.True(t, resp.GetValid(), "expected access granted") + assert.Equal(t, tt.expectEmail, resp.GetUserEmail()) + assert.Equal(t, tt.expectUserID, resp.GetUserId()) + + if idpMock != nil { + if tt.expectIdPHit { + assert.Equal(t, 1, idpMock.gotCalls, "expected IdP to be consulted") + require.Len(t, idpMock.gotMeta, 1) + assert.Equal(t, accountID, idpMock.gotMeta[0].WTAccountID) + } else { + assert.Equal(t, 0, idpMock.gotCalls, "expected IdP to not be consulted") + } + } + }) + } +} + func TestGetAccountProxyByDomain(t *testing.T) { tests := []struct { name string diff --git a/management/internals/shared/grpc/validate_session_test.go b/management/internals/shared/grpc/validate_session_test.go index 27d9a65e7..d649102a1 100644 --- a/management/internals/shared/grpc/validate_session_test.go +++ b/management/internals/shared/grpc/validate_session_test.go @@ -42,7 +42,7 @@ func setupValidateSessionTest(t *testing.T) *validateSessionTestSetup { tokenStore := NewOneTimeTokenStore(ctx, testCacheStore(t)) pkceStore := NewPKCEVerifierStore(ctx, testCacheStore(t)) - proxyService := NewProxyServiceServer(nil, tokenStore, pkceStore, ProxyOIDCConfig{}, nil, usersManager, proxyManager, nil) + proxyService := NewProxyServiceServer(nil, tokenStore, pkceStore, ProxyOIDCConfig{}, nil, usersManager, nil, proxyManager, nil) proxyService.SetServiceManager(serviceManager) createTestProxies(t, ctx, testStore) diff --git a/management/server/account_test.go b/management/server/account_test.go index 256b71f18..2e26ac222 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -3215,7 +3215,7 @@ func createManager(t testing.TB) (*DefaultAccountManager, *update_channel.PeersU return nil, nil, err } - proxyGrpcServer := nbgrpc.NewProxyServiceServer(nil, nil, nil, nbgrpc.ProxyOIDCConfig{}, peersManager, nil, proxyManager, nil) + proxyGrpcServer := nbgrpc.NewProxyServiceServer(nil, nil, nil, nbgrpc.ProxyOIDCConfig{}, peersManager, nil, nil, proxyManager, nil) proxyController, err := proxymanager.NewGRPCController(proxyGrpcServer, noop.Meter{}) if err != nil { return nil, nil, err diff --git a/management/server/http/handlers/proxy/auth_callback_integration_test.go b/management/server/http/handlers/proxy/auth_callback_integration_test.go index f08d5daf1..a24857066 100644 --- a/management/server/http/handlers/proxy/auth_callback_integration_test.go +++ b/management/server/http/handlers/proxy/auth_callback_integration_test.go @@ -217,6 +217,7 @@ func setupAuthCallbackTest(t *testing.T) *testSetup { usersManager, nil, nil, + nil, ) proxyService.SetServiceManager(&testServiceManager{store: testStore}) diff --git a/management/server/http/testing/testing_tools/channel/channel.go b/management/server/http/testing/testing_tools/channel/channel.go index 8da9c7ad4..61584a615 100644 --- a/management/server/http/testing/testing_tools/channel/channel.go +++ b/management/server/http/testing/testing_tools/channel/channel.go @@ -110,7 +110,7 @@ func BuildApiBlackBoxWithDBState(t testing_tools.TB, sqlFile string, expectedPee if err != nil { t.Fatalf("Failed to create proxy manager: %v", err) } - proxyServiceServer := nbgrpc.NewProxyServiceServer(accessLogsManager, proxyTokenStore, pkceverifierStore, nbgrpc.ProxyOIDCConfig{}, peersManager, userManager, proxyMgr, nil) + proxyServiceServer := nbgrpc.NewProxyServiceServer(accessLogsManager, proxyTokenStore, pkceverifierStore, nbgrpc.ProxyOIDCConfig{}, peersManager, userManager, nil, proxyMgr, nil) domainManager := manager.NewManager(store, proxyMgr, permissionsManager, am) serviceProxyController, err := proxymanager.NewGRPCController(proxyServiceServer, noopMeter) if err != nil { @@ -240,7 +240,7 @@ func BuildApiBlackBoxWithDBStateAndPeerChannel(t testing_tools.TB, sqlFile strin if err != nil { t.Fatalf("Failed to create proxy manager: %v", err) } - proxyServiceServer := nbgrpc.NewProxyServiceServer(accessLogsManager, proxyTokenStore, pkceverifierStore, nbgrpc.ProxyOIDCConfig{}, peersManager, userManager, proxyMgr, nil) + proxyServiceServer := nbgrpc.NewProxyServiceServer(accessLogsManager, proxyTokenStore, pkceverifierStore, nbgrpc.ProxyOIDCConfig{}, peersManager, userManager, nil, proxyMgr, nil) domainManager := manager.NewManager(store, proxyMgr, permissionsManager, am) serviceProxyController, err := proxymanager.NewGRPCController(proxyServiceServer, noopMeter) if err != nil { diff --git a/proxy/management_byop_integration_test.go b/proxy/management_byop_integration_test.go index c0fbe682a..d075e47ec 100644 --- a/proxy/management_byop_integration_test.go +++ b/proxy/management_byop_integration_test.go @@ -125,6 +125,7 @@ func setupBYOPIntegrationTest(t *testing.T) *byopTestSetup { oidcConfig, nil, usersManager, + nil, realProxyManager, nil, ) diff --git a/proxy/management_integration_test.go b/proxy/management_integration_test.go index bf5067b85..cb82813b0 100644 --- a/proxy/management_integration_test.go +++ b/proxy/management_integration_test.go @@ -140,6 +140,7 @@ func setupIntegrationTest(t *testing.T) *integrationTestSetup { oidcConfig, nil, usersManager, + nil, proxyManager, nil, ) From 35b465fa4a1667b99877d78c554bb1d1231aa616 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:43:01 +0200 Subject: [PATCH 2/5] [management] reduce sync and login transaction (#6472) --- management/server/peer.go | 42 ++++++++++++++------------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/management/server/peer.go b/management/server/peer.go index bd6b2b6c5..83236d961 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -982,8 +982,6 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy var peer *nbpeer.Peer var updated, versionChanged, ipv6CapabilityChanged bool var err error - var postureChecks []*posture.Checks - var peerGroupIDs []string settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) if err != nil { @@ -1011,11 +1009,6 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy return status.NewPeerLoginExpiredError() } - peerGroupIDs, err = getPeerGroupIDs(ctx, transaction, accountID, peer.ID) - if err != nil { - return err - } - oldHasIPv6Cap := peer.HasCapability(nbpeer.PeerCapabilityIPv6Overlay) updated, versionChanged = peer.UpdateMetaIfNew(sync.Meta) ipv6CapabilityChanged = oldHasIPv6Cap != peer.HasCapability(nbpeer.PeerCapabilityIPv6Overlay) @@ -1025,16 +1018,6 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy if err = transaction.SavePeer(ctx, accountID, peer); err != nil { return err } - - policies, err := transaction.GetAccountPolicies(ctx, store.LockingStrengthNone, accountID) - if err != nil { - return err - } - - postureChecks, err = getPeerPostureChecks(ctx, transaction, accountID, peerGroupIDs, policies) - if err != nil { - return err - } } return nil }) @@ -1042,6 +1025,11 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy return nil, nil, nil, 0, err } + peerGroupIDs, err := getPeerGroupIDs(ctx, am.Store, accountID, peer.ID) + if err != nil { + return nil, nil, nil, 0, err + } + peerNotValid, isStatusChanged, err := am.integratedPeerValidator.IsNotValidPeer(ctx, accountID, peer, peerGroupIDs, settings.Extra) if err != nil { return nil, nil, nil, 0, err @@ -1052,9 +1040,9 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy return nil, nil, nil, 0, err } - if isStatusChanged || sync.UpdateAccountPeers || ipv6CapabilityChanged || (updated && (len(postureChecks) > 0 || versionChanged)) { + if isStatusChanged || sync.UpdateAccountPeers || ipv6CapabilityChanged || (updated && (len(resPostureChecks) > 0 || versionChanged)) { changedPeerIDs := []string{peer.ID} - affectedPeerIDs := am.syncPeerAffectedPeers(ctx, accountID, peer.ID, nmap, peerNotValid, updated, len(postureChecks) > 0) + affectedPeerIDs := am.syncPeerAffectedPeers(ctx, accountID, peer.ID, nmap, peerNotValid, updated, len(resPostureChecks) > 0) if err = am.networkMapController.OnPeersUpdated(ctx, accountID, changedPeerIDs, affectedPeerIDs); err != nil { return nil, nil, nil, 0, fmt.Errorf("notify network map controller of peer update: %w", err) } @@ -1160,11 +1148,6 @@ func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.Peer } } - peerGroupIDs, err = getPeerGroupIDs(ctx, transaction, accountID, peer.ID) - if err != nil { - return err - } - if peer.SSHKey != login.SSHKey { peer.SSHKey = login.SSHKey shouldStorePeer = true @@ -1180,15 +1163,20 @@ func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.Peer } } - // This is needed to keep in memory for the peer config. Otherwise browser client will end in a retry loop - peer.UpdateMetaIfNew(login.Meta) - return nil }) if err != nil { return nil, nil, nil, false, err } + // This is needed to keep in memory for the peer config. Otherwise browser client will end in a retry loop + peer.UpdateMetaIfNew(login.Meta) + + peerGroupIDs, err = getPeerGroupIDs(ctx, am.Store, accountID, peer.ID) + if err != nil { + return nil, nil, nil, false, err + } + isRequiresApproval, _, err := am.integratedPeerValidator.IsNotValidPeer(ctx, accountID, peer, peerGroupIDs, settings.Extra) if err != nil { return nil, nil, nil, false, err From 85116872706057cd8063e4ec22deb19da32799f1 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 19 Jun 2026 13:30:52 +0200 Subject: [PATCH 3/5] [management] log peer meta diff (#6468) --- management/server/peer.go | 4 +- management/server/peer/peer.go | 167 +++++++++++++------ management/server/peer/peer_metadiff_test.go | 113 +++++++++++++ 3 files changed, 233 insertions(+), 51 deletions(-) create mode 100644 management/server/peer/peer_metadiff_test.go diff --git a/management/server/peer.go b/management/server/peer.go index 83236d961..c54c1dc7b 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -1010,7 +1010,7 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy } oldHasIPv6Cap := peer.HasCapability(nbpeer.PeerCapabilityIPv6Overlay) - updated, versionChanged = peer.UpdateMetaIfNew(sync.Meta) + updated, versionChanged = peer.UpdateMetaIfNew(ctx, sync.Meta) ipv6CapabilityChanged = oldHasIPv6Cap != peer.HasCapability(nbpeer.PeerCapabilityIPv6Overlay) if updated { am.metrics.AccountManagerMetrics().CountPeerMetUpdate() @@ -1170,7 +1170,7 @@ func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.Peer } // This is needed to keep in memory for the peer config. Otherwise browser client will end in a retry loop - peer.UpdateMetaIfNew(login.Meta) + peer.UpdateMetaIfNew(ctx, login.Meta) peerGroupIDs, err = getPeerGroupIDs(ctx, am.Store, accountID, peer.ID) if err != nil { diff --git a/management/server/peer/peer.go b/management/server/peer/peer.go index e5475c07d..591ac074e 100644 --- a/management/server/peer/peer.go +++ b/management/server/peer/peer.go @@ -1,12 +1,16 @@ package peer import ( + "context" + "fmt" "net" "net/netip" "slices" - "sort" + "strings" "time" + log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/management/server/util" "github.com/netbirdio/netbird/shared/management/http/api" ) @@ -162,49 +166,7 @@ type PeerSystemMeta struct { //nolint:revive } func (p PeerSystemMeta) isEqual(other PeerSystemMeta) bool { - sort.Slice(p.NetworkAddresses, func(i, j int) bool { - return p.NetworkAddresses[i].Mac < p.NetworkAddresses[j].Mac - }) - sort.Slice(other.NetworkAddresses, func(i, j int) bool { - return other.NetworkAddresses[i].Mac < other.NetworkAddresses[j].Mac - }) - equalNetworkAddresses := slices.EqualFunc(p.NetworkAddresses, other.NetworkAddresses, func(addr NetworkAddress, oAddr NetworkAddress) bool { - return addr.Mac == oAddr.Mac && addr.NetIP == oAddr.NetIP - }) - if !equalNetworkAddresses { - return false - } - - sort.Slice(p.Files, func(i, j int) bool { - return p.Files[i].Path < p.Files[j].Path - }) - sort.Slice(other.Files, func(i, j int) bool { - return other.Files[i].Path < other.Files[j].Path - }) - equalFiles := slices.EqualFunc(p.Files, other.Files, func(file File, oFile File) bool { - return file.Path == oFile.Path && file.Exist == oFile.Exist && file.ProcessIsRunning == oFile.ProcessIsRunning - }) - if !equalFiles { - return false - } - - return p.Hostname == other.Hostname && - p.GoOS == other.GoOS && - p.Kernel == other.Kernel && - p.KernelVersion == other.KernelVersion && - p.Core == other.Core && - p.Platform == other.Platform && - p.OS == other.OS && - p.OSVersion == other.OSVersion && - p.WtVersion == other.WtVersion && - p.UIVersion == other.UIVersion && - p.SystemSerialNumber == other.SystemSerialNumber && - p.SystemProductName == other.SystemProductName && - p.SystemManufacturer == other.SystemManufacturer && - p.Environment.Cloud == other.Environment.Cloud && - p.Environment.Platform == other.Environment.Platform && - p.Flags.isEqual(other.Flags) && - capabilitiesEqual(p.Capabilities, other.Capabilities) + return len(metaDiff(p, other)) == 0 } func (p PeerSystemMeta) isEmpty() bool { @@ -296,7 +258,7 @@ func (p *Peer) Copy() *Peer { // UpdateMetaIfNew updates peer's system metadata if new information is provided // returns true if meta was updated, false otherwise -func (p *Peer) UpdateMetaIfNew(meta PeerSystemMeta) (updated, versionChanged bool) { +func (p *Peer) UpdateMetaIfNew(ctx context.Context, meta PeerSystemMeta) (updated, versionChanged bool) { if meta.isEmpty() { return updated, versionChanged } @@ -308,14 +270,121 @@ func (p *Peer) UpdateMetaIfNew(meta PeerSystemMeta) (updated, versionChanged boo meta.UIVersion = p.Meta.UIVersion } - if p.Meta.isEqual(meta) { - return updated, versionChanged + oldVersion := p.Meta.WtVersion + + diff := metaDiff(p.Meta, meta) + if len(diff) != 0 { + p.Meta = meta + updated = true } - p.Meta = meta - updated = true + + versionInfo := "" + if versionChanged { + versionInfo = fmt.Sprintf("version changed: %s -> %s, ", oldVersion, meta.WtVersion) + } + + if len(diff) > 0 || versionChanged { + log.WithContext(ctx). + Debugf("peer meta updated, %s%d field(s) changed: %s", versionInfo, len(diff), strings.Join(diff, ", ")) + } + return updated, versionChanged } +// metaDiff returns a human-readable list of the fields that differ between the +// old and new meta, each formatted as `field: -> `. It is the single +// source of truth for meta comparison: isEqual reports equality as an empty +// diff, so the log line can never disagree with the change decision. Slices are +// cloned before sorting, so callers' meta is not mutated. +func metaDiff(oldMeta, newMeta PeerSystemMeta) []string { + var diff []string + add := func(field string, oldVal, newVal any) { + diff = append(diff, fmt.Sprintf("%s: %v -> %v", field, oldVal, newVal)) + } + + if oldMeta.Hostname != newMeta.Hostname { + add("hostname", oldMeta.Hostname, newMeta.Hostname) + } + if oldMeta.GoOS != newMeta.GoOS { + add("goos", oldMeta.GoOS, newMeta.GoOS) + } + if oldMeta.Kernel != newMeta.Kernel { + add("kernel", oldMeta.Kernel, newMeta.Kernel) + } + if oldMeta.KernelVersion != newMeta.KernelVersion { + add("kernel_version", oldMeta.KernelVersion, newMeta.KernelVersion) + } + if oldMeta.Core != newMeta.Core { + add("core", oldMeta.Core, newMeta.Core) + } + if oldMeta.Platform != newMeta.Platform { + add("platform", oldMeta.Platform, newMeta.Platform) + } + if oldMeta.OS != newMeta.OS { + add("os", oldMeta.OS, newMeta.OS) + } + if oldMeta.OSVersion != newMeta.OSVersion { + add("os_version", oldMeta.OSVersion, newMeta.OSVersion) + } + if oldMeta.WtVersion != newMeta.WtVersion { + add("wt_version", oldMeta.WtVersion, newMeta.WtVersion) + } + if oldMeta.UIVersion != newMeta.UIVersion { + add("ui_version", oldMeta.UIVersion, newMeta.UIVersion) + } + if oldMeta.SystemSerialNumber != newMeta.SystemSerialNumber { + add("system_serial_number", oldMeta.SystemSerialNumber, newMeta.SystemSerialNumber) + } + if oldMeta.SystemProductName != newMeta.SystemProductName { + add("system_product_name", oldMeta.SystemProductName, newMeta.SystemProductName) + } + if oldMeta.SystemManufacturer != newMeta.SystemManufacturer { + add("system_manufacturer", oldMeta.SystemManufacturer, newMeta.SystemManufacturer) + } + if oldMeta.Environment.Cloud != newMeta.Environment.Cloud { + add("environment_cloud", oldMeta.Environment.Cloud, newMeta.Environment.Cloud) + } + if oldMeta.Environment.Platform != newMeta.Environment.Platform { + add("environment_platform", oldMeta.Environment.Platform, newMeta.Environment.Platform) + } + if !oldMeta.Flags.isEqual(newMeta.Flags) { + add("flags", fmt.Sprintf("%+v", oldMeta.Flags), fmt.Sprintf("%+v", newMeta.Flags)) + } + if !capabilitiesEqual(oldMeta.Capabilities, newMeta.Capabilities) { + add("capabilities", oldMeta.Capabilities, newMeta.Capabilities) + } + + if !sameMultiset(oldMeta.NetworkAddresses, newMeta.NetworkAddresses) { + add("network_addresses", fmt.Sprintf("%v", oldMeta.NetworkAddresses), fmt.Sprintf("%v", newMeta.NetworkAddresses)) + } + + if !sameMultiset(oldMeta.Files, newMeta.Files) { + add("files", fmt.Sprintf("%v", oldMeta.Files), fmt.Sprintf("%v", newMeta.Files)) + } + + return diff +} + +// sameMultiset reports whether two slices contain the same elements with the +// same multiplicity, ignoring order. The element type is the comparison key, so +// every field participates in equality. +func sameMultiset[T comparable](a, b []T) bool { + if len(a) != len(b) { + return false + } + counts := make(map[T]int, len(a)) + for _, v := range a { + counts[v]++ + } + for _, v := range b { + counts[v]-- + if counts[v] == 0 { + delete(counts, v) + } + } + return len(counts) == 0 +} + // GetLastLogin returns the last login time of the peer. func (p *Peer) GetLastLogin() time.Time { if p.LastLogin != nil { diff --git a/management/server/peer/peer_metadiff_test.go b/management/server/peer/peer_metadiff_test.go new file mode 100644 index 000000000..1256cdb02 --- /dev/null +++ b/management/server/peer/peer_metadiff_test.go @@ -0,0 +1,113 @@ +package peer + +import ( + "net/netip" + "reflect" + "testing" + + "github.com/stretchr/testify/require" +) + +// metaDiffExtraEntries accounts for PeerSystemMeta fields that metaDiff does not +// map 1:1 to a single diff entry. Today the only such field is Environment, which +// is exploded into two checks (Cloud, Platform) and therefore yields one extra +// entry beyond its single struct field. If you teach metaDiff to explode another +// field into N entries, bump this by N-1; if you collapse a field, lower it. +const metaDiffExtraEntries = 1 + +// TestMetaDiff_CoversAllFields fully populates a PeerSystemMeta with non-zero +// values and diffs it against the zero value, then asserts metaDiff emits exactly +// one entry per exported field (plus metaDiffExtraEntries for fields it explodes). +// +// The expected count is derived from the struct via reflection, so adding a field +// to PeerSystemMeta raises the expectation automatically — but the actual diff +// only grows if metaDiff was taught to compare the new field. A mismatch means +// someone changed the struct without updating metaDiff (or this test's +// extra-entry accounting), which is exactly what we want to catch. +func TestMetaDiff_CoversAllFields(t *testing.T) { + var full PeerSystemMeta + exported := populateAll(t, reflect.ValueOf(&full).Elem()) + require.NotZero(t, exported, "expected PeerSystemMeta to expose fields") + + diff := metaDiff(PeerSystemMeta{}, full) + + require.Len(t, diff, exported+metaDiffExtraEntries, + "metaDiff entry count no longer matches PeerSystemMeta's fields: a field was "+ + "likely added or removed without updating metaDiff (or metaDiffExtraEntries). "+ + "diff was: %v", diff) + + require.False(t, full.isEqual(PeerSystemMeta{}), + "isEqual must report a fully-populated meta as different from the zero value") +} + +// TestFlags_isEqualChecksEveryField guards the one field that the count-based +// TestMetaDiff_CoversAllFields cannot: metaDiff collapses all of Flags into a +// single "flags" diff entry, so a new Flags field that Flags.isEqual forgets to +// compare would not change the diff count. This flips each Flags field on its own +// and asserts Flags.isEqual notices, so adding a Flags field without comparing it +// fails here. +func TestFlags_isEqualChecksEveryField(t *testing.T) { + typ := reflect.TypeOf(Flags{}) + for i := 0; i < typ.NumField(); i++ { + f := typ.Field(i) + require.Equal(t, reflect.Bool, f.Type.Kind(), + "Flags.%s is not a bool; extend this test to set it non-zero", f.Name) + + var a, b Flags + reflect.ValueOf(&b).Elem().Field(i).SetBool(true) + require.False(t, a.isEqual(b), "Flags.isEqual ignores field %s", f.Name) + } +} + +// populateAll sets every exported field of the struct to a deterministic non-zero +// value, recursing into nested structs and the element type of struct slices so +// that each leaf differs from zero. It returns the number of exported fields on +// the top-level struct. netip.Prefix is treated as an opaque leaf (it has no +// settable exported fields and is comparable with ==). +func populateAll(t *testing.T, v reflect.Value) int { + t.Helper() + + typ := v.Type() + exported := 0 + for i := 0; i < typ.NumField(); i++ { + f := typ.Field(i) + if f.PkgPath != "" { // unexported + continue + } + exported++ + setNonZero(t, v.Field(i)) + } + return exported +} + +// setNonZero assigns a deterministic non-zero value to a field based on its kind, +// recursing into nested structs and populating one element of slice fields. +func setNonZero(t *testing.T, field reflect.Value) { + t.Helper() + + if field.Type() == reflect.TypeOf(netip.Prefix{}) { + field.Set(reflect.ValueOf(netip.MustParsePrefix("10.0.0.0/24"))) + return + } + + switch field.Kind() { + case reflect.String: + field.SetString("non-zero") + case reflect.Bool: + field.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + field.SetInt(7) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + field.SetUint(7) + case reflect.Float32, reflect.Float64: + field.SetFloat(7) + case reflect.Struct: + populateAll(t, field) + case reflect.Slice: + s := reflect.MakeSlice(field.Type(), 1, 1) + setNonZero(t, s.Index(0)) + field.Set(s) + default: + t.Fatalf("unhandled field kind %s; extend setNonZero", field.Kind()) + } +} From 54192a94b7bf3a59e0b86b98845014b5f4a9fb3a Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Fri, 19 Jun 2026 14:10:43 +0200 Subject: [PATCH 4/5] [misc] handle release candidates when fetching tags in FreeBSD port scripts (#6480) * [misc] Exclude release candidates when fetching tags in FreeBSD port scripts --- release_files/freebsd-port-diff.sh | 3 ++- release_files/freebsd-port-issue-body.sh | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/release_files/freebsd-port-diff.sh b/release_files/freebsd-port-diff.sh index b030b9164..6ffa141be 100755 --- a/release_files/freebsd-port-diff.sh +++ b/release_files/freebsd-port-diff.sh @@ -21,7 +21,8 @@ AWK_FIRST_FIELD='{print $1}' fetch_all_tags() { curl -sL "https://github.com/${GITHUB_REPO}/tags" 2>/dev/null | \ - grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+' | \ + grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+([^"]+)?' | \ + grep -iv 'rc' | \ sed 's/.*\/v//' | \ sort -u -V return 0 diff --git a/release_files/freebsd-port-issue-body.sh b/release_files/freebsd-port-issue-body.sh index b7ad0f5b1..1c23dbbbe 100755 --- a/release_files/freebsd-port-issue-body.sh +++ b/release_files/freebsd-port-issue-body.sh @@ -32,7 +32,8 @@ fetch_current_ports_version() { fetch_all_tags() { # Fetch tags from GitHub tags page (no rate limiting, no auth needed) curl -sL "https://github.com/${GITHUB_REPO}/tags" 2>/dev/null | \ - grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+' | \ + grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+([^"]+)?' | \ + grep -iv 'rc' | \ sed 's/.*\/v//' | \ sort -u -V return 0 From 883a1a8961ff181bd61cbd23dd603c541190df17 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:23:51 +0200 Subject: [PATCH 5/5] [client] Fix profile regressions in `up --profile` and `status` (#6479) * Restores behavior to create profile if not there on Up * Allows to restore nerbird status showing of the profile name * [client] Reduce upFunc cognitive complexity Extract the profile switch/auto-create logic from upFunc into a dedicated switchOrCreateProfile helper. The inlined NotFound-retry branch pushed upFunc over SonarCloud's cognitive complexity threshold (S3776). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) * [client] Make up --profile auto-create idempotent under concurrent runs Don't fail switchOrCreateProfile on a createProfile error: a concurrent run may create the profile between the NotFound check and our create call. Retry the switch regardless and only surface the create error if the switch also fails. Addresses CodeRabbit race-condition feedback. Co-Authored-By: Claude Opus 4.8 (1M context) * Share createProfile with addProfileFunc * But allow conn reusage * moves switchOrCreateProfile to where it's used --------- Co-authored-by: Claude Opus 4.8 (1M context) --- client/cmd/login.go | 2 +- client/cmd/profile.go | 34 ++++++++++++++++++--------- client/cmd/status.go | 29 ++++++++++++++++++----- client/cmd/up.go | 54 +++++++++++++++++++++++++++++++++++++------ 4 files changed, 94 insertions(+), 25 deletions(-) diff --git a/client/cmd/login.go b/client/cmd/login.go index 2f7677901..a7ee960b1 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -227,7 +227,7 @@ func switchProfile(ctx context.Context, handle string, username string) (profile Username: &username, }) if err != nil { - return "", fmt.Errorf("switch profile failed: %v", err) + return "", fmt.Errorf("switch profile failed: %w", err) } return profilemanager.ID(resp.Id), nil diff --git a/client/cmd/profile.go b/client/cmd/profile.go index 4de2d754e..268034e70 100644 --- a/client/cmd/profile.go +++ b/client/cmd/profile.go @@ -138,26 +138,23 @@ func addProfileFunc(cmd *cobra.Command, args []string) error { return err } + currUser, err := user.Current() + if err != nil { + return fmt.Errorf("get current user: %w", err) + } + conn, err := DialClientGRPCServer(cmd.Context(), daemonAddr) if err != nil { return fmt.Errorf("connect to service CLI interface: %w", err) } defer conn.Close() - currUser, err := user.Current() - if err != nil { - return fmt.Errorf("get current user: %w", err) - } - daemonClient := proto.NewDaemonServiceClient(conn) profileName := args[0] - resp, err := daemonClient.AddProfile(cmd.Context(), &proto.AddProfileRequest{ - ProfileName: profileName, - Username: currUser.Username, - }) + id, err := addProfileOnDaemon(cmd.Context(), daemonClient, profileName, currUser.Username) if err != nil { - return fmt.Errorf("add profile request: %w", err) + return err } dupCount, _ := countProfilesWithName(cmd.Context(), daemonClient, currUser.Username, profileName) @@ -166,7 +163,6 @@ func addProfileFunc(cmd *cobra.Command, args []string) error { cmd.Println("Use `netbird profile list --show-id` to disambiguate later.") } - id := profilemanager.ID(resp.Id) cmd.Printf("Profile added: %s %s\n", id.ShortID(), profilemanager.StripCtrlChars(profileName)) return nil @@ -330,3 +326,19 @@ func wrapAmbiguityError(err error, handle string) error { } return err } + +// addProfileOnDaemon issues the AddProfile RPC on an existing daemon client +// and returns the new profile's ID. It is the single entry point for profile +// creation, shared by `netbird profile add` and the `netbird up --profile +// ` auto-create path. +func addProfileOnDaemon(ctx context.Context, client proto.DaemonServiceClient, profileName, username string) (profilemanager.ID, error) { + resp, err := client.AddProfile(ctx, &proto.AddProfileRequest{ + ProfileName: profileName, + Username: username, + }) + if err != nil { + return "", fmt.Errorf("add profile failed: %w", err) + } + + return profilemanager.ID(resp.Id), nil +} diff --git a/client/cmd/status.go b/client/cmd/status.go index 103b3044a..5a7559cf1 100644 --- a/client/cmd/status.go +++ b/client/cmd/status.go @@ -11,7 +11,6 @@ import ( "google.golang.org/grpc/status" "github.com/netbirdio/netbird/client/internal" - "github.com/netbirdio/netbird/client/internal/profilemanager" "github.com/netbirdio/netbird/client/proto" nbstatus "github.com/netbirdio/netbird/client/status" "github.com/netbirdio/netbird/util" @@ -111,11 +110,10 @@ func statusFunc(cmd *cobra.Command, args []string) error { return nil } - pm := profilemanager.NewProfileManager() - var profName string - if activeProf, err := pm.GetActiveProfile(); err == nil { - profName = activeProf.Name - } + // Resolve the active profile's display name via the daemon, which runs + // as root and can read the per-user profile files. The local profile + // manager only knows the active profile ID, not its display name. + profName := getActiveProfileName(ctx) var outputInformationHolder = nbstatus.ConvertToStatusOutputOverview(resp.GetFullStatus(), nbstatus.ConvertOptions{ Anonymize: anonymizeFlag, @@ -167,6 +165,25 @@ func getStatus(ctx context.Context, fullPeerStatus bool, shouldRunProbes bool) ( return resp, nil } +// getActiveProfileName asks the daemon for the active profile's display +// name. The daemon runs as root and can read the per-user profile files to +// resolve the ID to its human-readable name. Returns an empty string on any +// error so status output degrades gracefully. +func getActiveProfileName(ctx context.Context) string { + conn, err := DialClientGRPCServer(ctx, daemonAddr) + if err != nil { + return "" + } + defer conn.Close() + + resp, err := proto.NewDaemonServiceClient(conn).GetActiveProfile(ctx, &proto.GetActiveProfileRequest{}) + if err != nil { + return "" + } + + return resp.GetProfileName() +} + func parseFilters() error { switch strings.ToLower(statusFilter) { case "", "idle", "connecting", "connected": diff --git a/client/cmd/up.go b/client/cmd/up.go index 2761cf74a..0506bc65b 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -128,15 +128,9 @@ func upFunc(cmd *cobra.Command, args []string) error { var profileSwitched bool // switch profile if provided if profileName != "" { - resolvedID, err := switchProfile(cmd.Context(), profileName, username.Username) - if err != nil { + if err := switchOrCreateProfile(cmd.Context(), pm, profileName, username.Username); err != nil { return fmt.Errorf("switch profile: %v", err) } - - if err := pm.SwitchProfile(resolvedID); err != nil { - return fmt.Errorf("switch profile: %v", err) - } - profileSwitched = true } @@ -151,6 +145,52 @@ func upFunc(cmd *cobra.Command, args []string) error { return runInDaemonMode(ctx, cmd, pm, activeProf, profileSwitched) } +// switchOrCreateProfile switches the active profile to the one identified by +// handle, creating it first when it does not exist yet. This restores the +// pre-0.73 behaviour where `netbird up --profile ` auto-creates a +// missing profile instead of failing. +func switchOrCreateProfile(ctx context.Context, pm *profilemanager.ProfileManager, handle, username string) error { + resolvedID, err := switchProfile(ctx, handle, username) + if err != nil { + st, ok := gstatus.FromError(err) + if !ok || st.Code() != codes.NotFound { + return err + } + // Don't fail immediately on a create error: a concurrent run may + // have created the profile between the NotFound above and this + // call, in which case the retried switch still succeeds. Only + // surface the create error if the switch also fails. + _, createErr := createProfile(ctx, handle, username) + if resolvedID, err = switchProfile(ctx, handle, username); err != nil { + if createErr != nil { + return fmt.Errorf("create profile: %w", createErr) + } + return err + } + } + + if err := pm.SwitchProfile(resolvedID); err != nil { + return err + } + return nil +} + +// createProfile dials the daemon and creates a new profile with the given +// display name, returning its generated ID. Use addProfileOnDaemon directly +// when a daemon client is already available to reuse the connection. +func createProfile(ctx context.Context, profileName, username string) (profilemanager.ID, error) { + conn, err := DialClientGRPCServer(ctx, daemonAddr) + if err != nil { + //nolint + return "", fmt.Errorf("failed to connect to daemon error: %v\n"+ + "If the daemon is not running please run: "+ + "\nnetbird service install \nnetbird service start\n", err) + } + defer conn.Close() + + return addProfileOnDaemon(ctx, proto.NewDaemonServiceClient(conn), profileName, username) +} + func runInForegroundMode(ctx context.Context, cmd *cobra.Command, activeProf *profilemanager.Profile) error { // override the default profile filepath if provided if configPath != "" {