From 356f6bdda06357bebe39f039de36275136623773 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sun, 9 Aug 2026 07:31:18 +0000 Subject: [PATCH] [management] Record proxy logins and mesh activity for active-user accounting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Activity accounting counts a user as active from their last login or from a peer of theirs being seen. Neither timestamp moved when someone reached a service through the reverse proxy, so a person who only ever uses proxy-protected services and never opens the dashboard has no login on record at all and is skipped outright. The two proxy entry points mean different things, so they write different things. GenerateSessionToken is only reached after an ID token was verified, so a completed SSO sign-in records a login on the user. ValidateTunnelPeer authorises by tunnel IP with no IdP involved, so it records that the peer was seen instead; the owner counts through that. Both write on the granted path only, in UTC, and log and drop failures — no authorisation decision reads them back. The peer write is throttled to once an hour against the peer already in hand, so a busy peer does not rewrite its row behind every request. Both store methods update one column and leave the session-ownership fields to the sync stream that owns them. Peers that accounting excludes, embedded proxy peers and browser clients, are skipped rather than written for nothing. --- management/internals/modules/peers/manager.go | 6 + .../internals/modules/peers/manager_mock.go | 15 ++ management/internals/shared/grpc/proxy.go | 53 +++++- .../shared/grpc/proxy_group_access_test.go | 143 +++++++++++++++ .../shared/grpc/validate_session_test.go | 4 + .../proxy/auth_callback_integration_test.go | 49 ++++++ management/server/peer/peer.go | 7 + management/server/store/sql_store.go | 41 +++++ .../server/store/sql_store_activity_test.go | 166 ++++++++++++++++++ management/server/store/store.go | 10 ++ management/server/store/store_mock.go | 28 +++ management/server/users/manager.go | 12 ++ 12 files changed, 533 insertions(+), 1 deletion(-) create mode 100644 management/server/store/sql_store_activity_test.go diff --git a/management/internals/modules/peers/manager.go b/management/internals/modules/peers/manager.go index 6f292f6ed..fd234103e 100644 --- a/management/internals/modules/peers/manager.go +++ b/management/internals/modules/peers/manager.go @@ -44,6 +44,8 @@ type Manager interface { // to. Used by the proxy's auth path to authorise a request by the calling // peer's group memberships. GetPeerWithGroups(ctx context.Context, accountID, peerID string) (*peer.Peer, []*types.Group, error) + // RefreshLastSeen records that a peer was seen at seenAt. + RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error } type managerImpl struct { @@ -128,6 +130,10 @@ func (m *managerImpl) GetPeerWithGroups(ctx context.Context, accountID, peerID s return p, groups, nil } +func (m *managerImpl) RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { + return m.store.RefreshPeerLastSeen(ctx, accountID, peerID, seenAt) +} + func (m *managerImpl) DeletePeers(ctx context.Context, accountID string, peerIDs []string, userID string, checkConnected bool) error { settings, err := m.store.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) if err != nil { diff --git a/management/internals/modules/peers/manager_mock.go b/management/internals/modules/peers/manager_mock.go index 3836ac909..21f91c06a 100644 --- a/management/internals/modules/peers/manager_mock.go +++ b/management/internals/modules/peers/manager_mock.go @@ -8,6 +8,7 @@ import ( context "context" net "net" reflect "reflect" + time "time" gomock "github.com/golang/mock/gomock" network_map "github.com/netbirdio/netbird/management/internals/controllers/network_map" @@ -174,6 +175,20 @@ func (mr *MockManagerMockRecorder) GetPeersByGroupIDs(ctx, accountID, groupsIDs return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPeersByGroupIDs", reflect.TypeOf((*MockManager)(nil).GetPeersByGroupIDs), ctx, accountID, groupsIDs) } +// RefreshLastSeen mocks base method. +func (m *MockManager) RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RefreshLastSeen", ctx, accountID, peerID, seenAt) + ret0, _ := ret[0].(error) + return ret0 +} + +// RefreshLastSeen indicates an expected call of RefreshLastSeen. +func (mr *MockManagerMockRecorder) RefreshLastSeen(ctx, accountID, peerID, seenAt interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshLastSeen", reflect.TypeOf((*MockManager)(nil).RefreshLastSeen), ctx, accountID, peerID, seenAt) +} + // SetAccountManager mocks base method. func (m *MockManager) SetAccountManager(accountManager account.Manager) { m.ctrl.T.Helper() diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 4cae872e2..28c326677 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -1672,7 +1672,7 @@ func (s *ProxyServiceServer) GenerateSessionToken(ctx context.Context, domain, u groupIDs, groupNames := pairGroupIDsAndNames(userGroups) - return sessionkey.SignToken( + token, err := sessionkey.SignToken( service.SessionPrivateKey, userID, user.Email, @@ -1682,6 +1682,28 @@ func (s *ProxyServiceServer) GenerateSessionToken(ctx context.Context, domain, u groupNames, proxyauth.DefaultSessionExpiry, ) + if err != nil { + return "", err + } + + s.recordUserLogin(ctx, service.AccountID, user) + + return token, nil +} + +// recordUserLogin marks a completed reverse proxy SSO login on the user. The +// timestamp is what activity accounting reads to count someone who only ever +// reaches proxy-protected services and never opens the dashboard. Service users +// have no interactive login to record. A failure is logged and dropped: the +// next login marks it again and no authorization decision reads it. +func (s *ProxyServiceServer) recordUserLogin(ctx context.Context, accountID string, user *types.User) { + if user.IsServiceUser { + return + } + + if err := s.usersManager.RefreshLastLogin(ctx, accountID, user.Id, time.Now().UTC()); err != nil { + log.WithContext(ctx).Debugf("record proxy login for user %s: %v", user.Id, err) + } } // ValidateUserGroupAccess checks if a user has access to a service. @@ -2031,6 +2053,8 @@ func (s *ProxyServiceServer) ValidateTunnelPeer(ctx context.Context, req *proto. return nil, err } + s.recordPeerSeen(ctx, service.AccountID, peer) + log.WithFields(log.Fields{ "domain": domain, "tunnel_ip": tunnelIPStr, @@ -2048,6 +2072,33 @@ func (s *ProxyServiceServer) ValidateTunnelPeer(ctx context.Context, req *proto. }, nil } +// proxyPeerSeenInterval is how stale a peer's LastSeen must be before reaching +// a private service refreshes it. Positive tunnel validations are cached on the +// proxy for five minutes, so without a floor a busy peer would rewrite its row +// all day; an hour still sits well inside the window activity accounting asks +// about. +const proxyPeerSeenInterval = time.Hour + +// recordPeerSeen marks a peer as seen when it reaches a private service over +// the mesh, which is what lets its owner count as active. Peers that activity +// accounting excludes are skipped rather than written for nothing, and so is a +// peer already seen inside the interval — the row is in hand, so the throttle +// costs nothing. A failure is logged and dropped: the next request marks it +// again. +func (s *ProxyServiceServer) recordPeerSeen(ctx context.Context, accountID string, peer *peer.Peer) { + if !peer.CountsTowardActivity() { + return + } + + if peer.Status != nil && time.Since(peer.Status.LastSeen) < proxyPeerSeenInterval { + return + } + + if err := s.peersManager.RefreshLastSeen(ctx, accountID, peer.ID, time.Now().UTC()); err != nil { + log.WithContext(ctx).Debugf("record proxy activity for peer %s: %v", peer.ID, err) + } +} + // resolvePeerOwner returns the user a peer is linked to, once per request so // the status gate and the identity resolution below share a single lookup. // Unlinked peers (machine agents) have no owner. A lookup that fails returns diff --git a/management/internals/shared/grpc/proxy_group_access_test.go b/management/internals/shared/grpc/proxy_group_access_test.go index cb30a07b9..fc0fbc6ac 100644 --- a/management/internals/shared/grpc/proxy_group_access_test.go +++ b/management/internals/shared/grpc/proxy_group_access_test.go @@ -5,6 +5,7 @@ import ( "errors" "net" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -122,6 +123,20 @@ type mockUsersManager struct { users map[string]*types.User err error getUserCalls int + loginMarks []loginMark +} + +// loginMark records a RefreshLastLogin call so tests can assert what the proxy +// wrote, rather than that it merely called something. +type loginMark struct { + accountID string + userID string + at time.Time +} + +func (m *mockUsersManager) RefreshLastLogin(_ context.Context, accountID, userID string, loginAt time.Time) error { + m.loginMarks = append(m.loginMarks, loginMark{accountID: accountID, userID: userID, at: loginAt}) + return nil } func (m *mockUsersManager) GetUser(ctx context.Context, userID string) (*types.User, error) { @@ -153,6 +168,19 @@ type mockTunnelPeersManager struct { peerErr error groups []*types.Group groupsErr error + seenMarks []seenMark +} + +// seenMark records a RefreshLastSeen call. +type seenMark struct { + accountID string + peerID string + at time.Time +} + +func (m *mockTunnelPeersManager) RefreshLastSeen(_ context.Context, accountID, peerID string, seenAt time.Time) error { + m.seenMarks = append(m.seenMarks, seenMark{accountID: accountID, peerID: peerID, at: seenAt}) + return nil } func (m *mockTunnelPeersManager) GetPeerByTunnelIP(_ context.Context, _ string, _ net.IP) (*peer.Peer, error) { @@ -745,6 +773,121 @@ func TestValidateTunnelPeerOwnerStatus(t *testing.T) { } } +// TestValidateTunnelPeerRecordsActivity covers the activity write on the mesh +// fast-path: a peer reaching a private service is what lets its owner count as +// active, but only peers that activity accounting actually counts are written, +// and only once per interval. +func TestValidateTunnelPeerRecordsActivity(t *testing.T) { + const ( + domain = "app.example.com" + accountID = "account1" + peerID = "peer1" + ) + + tests := []struct { + name string + peer *peer.Peer + expectMark bool + }{ + { + name: "peer seen long ago is marked", + peer: &peer.Peer{ID: peerID, Name: "agent", Status: &peer.PeerStatus{LastSeen: time.Now().Add(-3 * time.Hour)}}, + expectMark: true, + }, + { + name: "peer never seen is marked", + peer: &peer.Peer{ID: peerID, Name: "agent", Status: &peer.PeerStatus{}}, + expectMark: true, + }, + { + // The throttle. The peer row is already in hand, so a recently seen + // peer costs nothing to skip. + name: "peer seen inside the interval is skipped", + peer: &peer.Peer{ID: peerID, Name: "agent", Status: &peer.PeerStatus{LastSeen: time.Now().Add(-10 * time.Minute)}}, + expectMark: false, + }, + { + name: "embedded proxy peer is skipped", + peer: &peer.Peer{ID: peerID, Name: "embedded", ProxyMeta: peer.ProxyMeta{Embedded: true}, Status: &peer.PeerStatus{LastSeen: time.Now().Add(-3 * time.Hour)}}, + expectMark: false, + }, + { + name: "browser client is skipped", + peer: &peer.Peer{ID: peerID, Name: "browser", Meta: peer.PeerSystemMeta{KernelVersion: "wasm"}, Status: &peer.PeerStatus{LastSeen: time.Now().Add(-3 * time.Hour)}}, + expectMark: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + peersManager := &mockTunnelPeersManager{peer: tt.peer} + server := &ProxyServiceServer{ + serviceManager: &mockReverseProxyManager{ + proxiesByAccount: map[string][]*service.Service{ + accountID: {{Domain: domain, AccountID: accountID}}, + }, + }, + peersManager: peersManager, + usersManager: &mockUsersManager{users: map[string]*types.User{}}, + } + + resp, err := server.ValidateTunnelPeer(context.Background(), &proto.ValidateTunnelPeerRequest{ + Domain: domain, + TunnelIp: "100.64.0.1", + }) + + require.NoError(t, err) + require.True(t, resp.GetValid(), "peer should be granted access") + + if !tt.expectMark { + assert.Empty(t, peersManager.seenMarks, "peer should not have been marked seen") + return + } + + require.Len(t, peersManager.seenMarks, 1, "peer should have been marked seen exactly once") + mark := peersManager.seenMarks[0] + assert.Equal(t, accountID, mark.accountID, "activity must be recorded against the service account") + assert.Equal(t, peerID, mark.peerID, "activity must be recorded against the calling peer") + assert.Equal(t, time.UTC, mark.at.Location(), "timestamps are written in UTC") + assert.WithinDuration(t, time.Now().UTC(), mark.at, time.Minute, "seen timestamp should be now") + }) + } +} + +// TestValidateTunnelPeerDeniedRecordsNoActivity keeps the write on the granted +// path only: a refused peer is not evidence its owner was active. +func TestValidateTunnelPeerDeniedRecordsNoActivity(t *testing.T) { + const ( + domain = "app.example.com" + accountID = "account1" + ) + + peersManager := &mockTunnelPeersManager{ + peer: &peer.Peer{ID: "peer1", Name: "agent", UserID: "user1", Status: &peer.PeerStatus{LastSeen: time.Now().Add(-3 * time.Hour)}}, + } + server := &ProxyServiceServer{ + serviceManager: &mockReverseProxyManager{ + proxiesByAccount: map[string][]*service.Service{ + accountID: {{Domain: domain, AccountID: accountID}}, + }, + }, + peersManager: peersManager, + // The owner is blocked, so the tunnel gate denies before the mint. + usersManager: &mockUsersManager{users: map[string]*types.User{ + "user1": {Id: "user1", AccountID: accountID, Blocked: true}, + }}, + } + + resp, err := server.ValidateTunnelPeer(context.Background(), &proto.ValidateTunnelPeerRequest{ + Domain: domain, + TunnelIp: "100.64.0.1", + }) + + require.NoError(t, err) + require.False(t, resp.GetValid(), "blocked owner should be denied") + assert.Empty(t, peersManager.seenMarks, "a denied peer must not be marked seen") +} + 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 03f200414..9a99c42f4 100644 --- a/management/internals/shared/grpc/validate_session_test.go +++ b/management/internals/shared/grpc/validate_session_test.go @@ -587,6 +587,10 @@ type testValidateSessionUsersManager struct { store store.Store } +func (m *testValidateSessionUsersManager) RefreshLastLogin(ctx context.Context, accountID, userID string, loginAt time.Time) error { + return m.store.RefreshUserLastLogin(ctx, accountID, userID, loginAt) +} + func (m *testValidateSessionUsersManager) GetUser(ctx context.Context, userID string) (*types.User, error) { return m.store.GetUserByUserID(ctx, store.LockingStrengthNone, userID) } 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 51e54ebce..c7a05c1c8 100644 --- a/management/server/http/handlers/proxy/auth_callback_integration_test.go +++ b/management/server/http/handlers/proxy/auth_callback_integration_test.go @@ -538,6 +538,55 @@ func TestAuthCallback_UserAllowedToLogin(t *testing.T) { // TestAuthCallback_UserDeniedByAccountStatus asserts that a user whose account // is pending approval or blocked never receives a session token from the OIDC // callback, and that the redirect carries a description the proxy can render. +// TestAuthCallback_RecordsUserLogin drives the real OIDC callback and asserts +// the login lands on the user row. That timestamp is what activity accounting +// reads, and it is the only signal that can ever count someone who reaches +// proxy-protected services from a browser and never opens the dashboard. +func TestAuthCallback_RecordsUserLogin(t *testing.T) { + setup := setupAuthCallbackTest(t) + defer setup.cleanup() + + ctx := context.Background() + + before, err := setup.store.GetUserByUserID(ctx, store.LockingStrengthNone, "allowedUserId") + require.NoError(t, err) + require.Nil(t, before.LastLogin, "fixture user starts with no login on record") + + setup.oidcServer.tokenSubject = "allowedUserId" + state := createTestState(t, setup.proxyService, "https://test-proxy.example.com/dashboard") + + req := httptest.NewRequest(http.MethodGet, "/reverse-proxy/callback?code=test-auth-code&state="+url.QueryEscape(state), nil) + rec := httptest.NewRecorder() + setup.router.ServeHTTP(rec, req) + require.Equal(t, http.StatusFound, rec.Code) + + after, err := setup.store.GetUserByUserID(ctx, store.LockingStrengthNone, "allowedUserId") + require.NoError(t, err) + require.NotNil(t, after.LastLogin, "a completed proxy SSO login must be recorded on the user") + require.WithinDuration(t, time.Now().UTC(), after.LastLogin.UTC(), time.Minute, "login should be stamped at sign-in time") +} + +// TestAuthCallback_DeniedUserLoginNotRecorded keeps the write on the granted +// path: a refused sign-in is not a login. +func TestAuthCallback_DeniedUserLoginNotRecorded(t *testing.T) { + setup := setupAuthCallbackTest(t) + defer setup.cleanup() + + ctx := context.Background() + + setup.oidcServer.tokenSubject = "blockedUserId" + state := createTestState(t, setup.proxyService, "https://test-proxy.example.com/dashboard") + + req := httptest.NewRequest(http.MethodGet, "/reverse-proxy/callback?code=test-auth-code&state="+url.QueryEscape(state), nil) + rec := httptest.NewRecorder() + setup.router.ServeHTTP(rec, req) + require.Equal(t, http.StatusFound, rec.Code) + + after, err := setup.store.GetUserByUserID(ctx, store.LockingStrengthNone, "blockedUserId") + require.NoError(t, err) + require.Nil(t, after.LastLogin, "a denied user must not be recorded as having logged in") +} + func TestAuthCallback_UserDeniedByAccountStatus(t *testing.T) { tests := []struct { name string diff --git a/management/server/peer/peer.go b/management/server/peer/peer.go index 7c4971285..a8567a1fd 100644 --- a/management/server/peer/peer.go +++ b/management/server/peer/peer.go @@ -477,6 +477,13 @@ func sameMultiset[T comparable](a, b []T) bool { return len(counts) == 0 } +// CountsTowardActivity reports whether the peer represents a device a person +// actually runs. Embedded proxy peers are infrastructure and browser (WASM) +// clients are ephemeral sessions, so activity accounting ignores both. +func (p *Peer) CountsTowardActivity() bool { + return !p.ProxyMeta.Embedded && p.Meta.KernelVersion != "wasm" +} + // GetLastLogin returns the last login time of the peer. func (p *Peer) GetLastLogin() time.Time { if p.LastLogin != nil { diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 670d9f781..3578a3ff2 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -599,6 +599,27 @@ func (s *SqlStore) ApproveAccountPeers(ctx context.Context, accountID string) (i return int(result.RowsAffected), nil } +// RefreshPeerLastSeen updates only peer_status_last_seen. Every other status +// column is left untouched: peer_status_connected and +// peer_status_session_started_at belong to the sync stream that owns the +// session, and a blind write here would corrupt the fencing +// MarkPeerConnectedIfNewerSession relies on. +func (s *SqlStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { + if seenAt.IsZero() { + return nil + } + + result := s.db.WithContext(ctx). + Model(&nbpeer.Peer{}). + Where(accountAndIDQueryCondition, accountID, peerID). + Update("peer_status_last_seen", seenAt) + if result.Error != nil { + return status.Errorf(status.Internal, "refresh peer last seen: %v", result.Error) + } + + return nil +} + // SaveUsers saves the given list of users to the database. func (s *SqlStore) SaveUsers(ctx context.Context, users []*types.User) error { if len(users) == 0 { @@ -3001,6 +3022,26 @@ func (s *SqlStore) SaveUserLastLogin(ctx context.Context, accountID, userID stri return nil } +// RefreshUserLastLogin updates only the last_login column, and only when it +// moves the timestamp forward. A user who has never logged in has a NULL that +// must also be written, so it counts as older than anything. +func (s *SqlStore) RefreshUserLastLogin(ctx context.Context, accountID, userID string, loginAt time.Time) error { + if loginAt.IsZero() { + return nil + } + + result := s.db.WithContext(ctx). + Model(&types.User{}). + Where(accountAndIDQueryCondition, accountID, userID). + Where("last_login IS NULL OR last_login < ?", loginAt). + Update("last_login", loginAt) + if result.Error != nil { + return status.Errorf(status.Internal, "refresh user last login: %v", result.Error) + } + + return nil +} + func (s *SqlStore) GetPostureCheckByChecksDefinition(accountID string, checks *posture.ChecksDefinition) (*posture.Checks, error) { definitionJSON, err := json.Marshal(checks) if err != nil { diff --git a/management/server/store/sql_store_activity_test.go b/management/server/store/sql_store_activity_test.go new file mode 100644 index 000000000..596139394 --- /dev/null +++ b/management/server/store/sql_store_activity_test.go @@ -0,0 +1,166 @@ +package store + +import ( + "context" + "net/netip" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + nbpeer "github.com/netbirdio/netbird/management/server/peer" + "github.com/netbirdio/netbird/management/server/types" +) + +const activityAccountID = "activityAccountId" + +func newActivityTestStore(t *testing.T) Store { + t.Helper() + + store, cleanUp, err := NewTestStoreFromSQL(context.Background(), "", t.TempDir()) + require.NoError(t, err) + t.Cleanup(cleanUp) + + require.NoError(t, store.SaveAccount(context.Background(), &types.Account{ + Id: activityAccountID, + Domain: "activity.example.com", + CreatedAt: time.Now().UTC(), + })) + + return store +} + +func TestRefreshUserLastLogin(t *testing.T) { + ctx := context.Background() + base := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC) + + tests := []struct { + name string + stored *time.Time + loginAt time.Time + expect *time.Time + }{ + { + // A user who has only ever reached proxy services has no login on + // record at all, and activity accounting skips those users. + name: "never logged in gets the timestamp", + stored: nil, + loginAt: base, + expect: &base, + }, + { + name: "older timestamp moves forward", + stored: ptrTime(base.Add(-2 * time.Hour)), + loginAt: base, + expect: &base, + }, + { + name: "newer timestamp is left alone", + stored: ptrTime(base.Add(time.Hour)), + loginAt: base, + expect: ptrTime(base.Add(time.Hour)), + }, + { + name: "zero login is ignored", + stored: ptrTime(base), + loginAt: time.Time{}, + expect: ptrTime(base), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := newActivityTestStore(t) + + require.NoError(t, store.SaveUser(ctx, &types.User{ + Id: "activityUser", + AccountID: activityAccountID, + Role: types.UserRoleUser, + Issued: "api", + LastLogin: tt.stored, + CreatedAt: base.Add(-24 * time.Hour), + })) + + require.NoError(t, store.RefreshUserLastLogin(ctx, activityAccountID, "activityUser", tt.loginAt)) + + user, err := store.GetUserByUserID(ctx, LockingStrengthNone, "activityUser") + require.NoError(t, err) + require.NotNil(t, user.LastLogin, "user should have a login timestamp") + assert.WithinDuration(t, *tt.expect, user.LastLogin.UTC(), time.Second, "unexpected stored last login") + }) + } +} + +func TestRefreshUserLastLoginUnknownUserIsNotAnError(t *testing.T) { + ctx := context.Background() + store := newActivityTestStore(t) + + // The write is best-effort telemetry on an auth path; a row that no longer + // exists must not surface as a failure to the caller. + assert.NoError(t, store.RefreshUserLastLogin(ctx, activityAccountID, "goneUser", time.Now().UTC())) +} + +func TestRefreshPeerLastSeen(t *testing.T) { + ctx := context.Background() + store := newActivityTestStore(t) + require.NoError(t, store.AddPeerToAccount(ctx, activityPeer(time.Date(2026, 3, 1, 9, 0, 0, 0, time.UTC)))) + + seenAt := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC) + require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", seenAt)) + + peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer") + require.NoError(t, err) + assert.WithinDuration(t, seenAt, peer.Status.LastSeen.UTC(), time.Second, "unexpected stored last seen") +} + +func TestRefreshPeerLastSeenZeroIsIgnored(t *testing.T) { + ctx := context.Background() + store := newActivityTestStore(t) + stored := time.Date(2026, 3, 1, 9, 0, 0, 0, time.UTC) + require.NoError(t, store.AddPeerToAccount(ctx, activityPeer(stored))) + + require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", time.Time{})) + + peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer") + require.NoError(t, err) + assert.WithinDuration(t, stored, peer.Status.LastSeen.UTC(), time.Second, "a zero timestamp must not clear last seen") +} + +// TestRefreshPeerLastSeenLeavesSessionStateAlone pins the column boundary: the +// connected flag and the session token belong to the sync stream that owns the +// peer's session, and a blind write here would corrupt its fencing. +func TestRefreshPeerLastSeenLeavesSessionStateAlone(t *testing.T) { + ctx := context.Background() + store := newActivityTestStore(t) + + stored := activityPeer(time.Date(2026, 3, 1, 9, 0, 0, 0, time.UTC)) + stored.Status.Connected = true + stored.Status.SessionStartedAt = 1234567890 + require.NoError(t, store.AddPeerToAccount(ctx, stored)) + + seenAt := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC) + require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", seenAt)) + + peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer") + require.NoError(t, err) + assert.WithinDuration(t, seenAt, peer.Status.LastSeen.UTC(), time.Second, "last seen should move forward") + assert.True(t, peer.Status.Connected, "connected flag must survive an activity write") + assert.Equal(t, int64(1234567890), peer.Status.SessionStartedAt, "session token must survive an activity write") +} + +func activityPeer(lastSeen time.Time) *nbpeer.Peer { + return &nbpeer.Peer{ + ID: "activityPeer", + AccountID: activityAccountID, + Key: "activityPeerKey", + IP: netip.MustParseAddr("100.64.0.9"), + Name: "activity-peer", + DNSLabel: "activity-peer", + Status: &nbpeer.PeerStatus{LastSeen: lastSeen}, + } +} + +func ptrTime(t time.Time) *time.Time { + return &t +} diff --git a/management/server/store/store.go b/management/server/store/store.go index 1beea72fd..ebd6bcc3c 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -94,6 +94,11 @@ type Store interface { SaveUsers(ctx context.Context, users []*types.User) error SaveUser(ctx context.Context, user *types.User) error SaveUserLastLogin(ctx context.Context, accountID, userID string, lastLogin time.Time) error + // RefreshUserLastLogin moves a user's last login forward to loginAt, + // touching no other column and leaving a newer stored value alone. Used by + // login paths that only need the timestamp, so they neither read the row + // first nor rewrite fields they did not change. + RefreshUserLastLogin(ctx context.Context, accountID, userID string, loginAt time.Time) error DeleteUser(ctx context.Context, accountID, userID string) error GetTokenIDByHashedToken(ctx context.Context, secret string) (string, error) DeleteHashedPAT2TokenIDIndex(hashedToken string) error @@ -180,6 +185,11 @@ type Store interface { // Returns true when the update happened, false when this stream lost // the race against a newer session. MarkPeerConnectedIfNewerSession(ctx context.Context, accountID, peerID string, newSessionStartedAt int64) (bool, error) + // RefreshPeerLastSeen records that a peer was seen at seenAt. Connected and + // SessionStartedAt are left alone, so this never interferes with the + // session-ownership protocol MarkPeerConnectedIfNewerSession implements. + // Callers decide how often to call it; the store does not throttle. + RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error // MarkPeerDisconnectedIfSameSession sets the peer to disconnected and // resets SessionStartedAt to zero, but only when the stored // SessionStartedAt equals the given sessionStartedAt. LastSeen is diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 428632a86..2fe261547 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3203,6 +3203,34 @@ func (mr *MockStoreMockRecorder) MarkProxyAccessTokenUsed(ctx, tokenID interface return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MarkProxyAccessTokenUsed", reflect.TypeOf((*MockStore)(nil).MarkProxyAccessTokenUsed), ctx, tokenID) } +// RefreshPeerLastSeen mocks base method. +func (m *MockStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RefreshPeerLastSeen", ctx, accountID, peerID, seenAt) + ret0, _ := ret[0].(error) + return ret0 +} + +// RefreshPeerLastSeen indicates an expected call of RefreshPeerLastSeen. +func (mr *MockStoreMockRecorder) RefreshPeerLastSeen(ctx, accountID, peerID, seenAt interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshPeerLastSeen", reflect.TypeOf((*MockStore)(nil).RefreshPeerLastSeen), ctx, accountID, peerID, seenAt) +} + +// RefreshUserLastLogin mocks base method. +func (m *MockStore) RefreshUserLastLogin(ctx context.Context, accountID, userID string, loginAt time.Time) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RefreshUserLastLogin", ctx, accountID, userID, loginAt) + ret0, _ := ret[0].(error) + return ret0 +} + +// RefreshUserLastLogin indicates an expected call of RefreshUserLastLogin. +func (mr *MockStoreMockRecorder) RefreshUserLastLogin(ctx, accountID, userID, loginAt interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshUserLastLogin", reflect.TypeOf((*MockStore)(nil).RefreshUserLastLogin), ctx, accountID, userID, loginAt) +} + // RemovePeerFromAllGroups mocks base method. func (m *MockStore) RemovePeerFromAllGroups(ctx context.Context, peerID string) error { m.ctrl.T.Helper() diff --git a/management/server/users/manager.go b/management/server/users/manager.go index 1a05b1a7c..9f6a74df6 100644 --- a/management/server/users/manager.go +++ b/management/server/users/manager.go @@ -3,6 +3,7 @@ package users import ( "context" "errors" + "time" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" @@ -11,6 +12,9 @@ import ( type Manager interface { GetUser(ctx context.Context, userID string) (*types.User, error) GetUserWithGroups(ctx context.Context, userID string) (*types.User, []*types.Group, error) + // RefreshLastLogin records an interactive login on the user without + // rewriting the rest of the row, keeping a newer stored timestamp. + RefreshLastLogin(ctx context.Context, accountID, userID string, loginAt time.Time) error } type managerImpl struct { @@ -26,6 +30,10 @@ func NewManager(store store.Store) Manager { } } +func (m *managerImpl) RefreshLastLogin(ctx context.Context, accountID, userID string, loginAt time.Time) error { + return m.store.RefreshUserLastLogin(ctx, accountID, userID, loginAt) +} + func (m *managerImpl) GetUser(ctx context.Context, userID string) (*types.User, error) { return m.store.GetUserByUserID(ctx, store.LockingStrengthNone, userID) } @@ -74,6 +82,10 @@ func (m *managerMock) GetUser(ctx context.Context, userID string) (*types.User, } } +func (m *managerMock) RefreshLastLogin(_ context.Context, _, _ string, _ time.Time) error { + return nil +} + func (m *managerMock) GetUserWithGroups(ctx context.Context, userID string) (*types.User, []*types.Group, error) { user, err := m.GetUser(ctx, userID) if err != nil {