mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-05 21:19:08 +02:00
[management] Confine the activity writes to the reverse proxy
The user half reused nothing: SaveUserLastLogin already exists and is the same call the dashboard and device login paths make, so the parallel RefreshUserLastLogin is gone and the proxy uses the established one. Reaching it no longer widens shared interfaces. The proxy service already receives the store, narrowed to ProxyTokenChecker; that interface now carries the two writes the proxy makes, so users.Manager, peers.Manager and Peer are untouched and the exclusion predicate moved into the proxy package next to its only caller. RefreshPeerLastSeen stays on the store because nothing there fits: SavePeerStatus rewrites the connected flag and session token from a caller snapshot, which would race the sync stream that owns them.
This commit is contained in:
@@ -217,7 +217,7 @@ func setupAuthCallbackTest(t *testing.T) *testSetup {
|
||||
usersManager,
|
||||
nil,
|
||||
nil,
|
||||
nil,
|
||||
testStore,
|
||||
)
|
||||
|
||||
proxyService.SetServiceManager(&testServiceManager{store: testStore})
|
||||
|
||||
@@ -477,13 +477,6 @@ 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 {
|
||||
|
||||
@@ -3022,26 +3022,6 @@ 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 {
|
||||
|
||||
@@ -31,76 +31,6 @@ func newActivityTestStore(t *testing.T) Store {
|
||||
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)
|
||||
@@ -117,7 +47,8 @@ func TestRefreshPeerLastSeen(t *testing.T) {
|
||||
|
||||
// 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.
|
||||
// peer's session, and a blind write here would corrupt its fencing. This is why
|
||||
// SavePeerStatus is not reused for an activity bump.
|
||||
func TestRefreshPeerLastSeenLeavesSessionStateAlone(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
store := newActivityTestStore(t)
|
||||
@@ -147,7 +78,3 @@ func activityPeer(lastSeen time.Time) *nbpeer.Peer {
|
||||
Status: &nbpeer.PeerStatus{LastSeen: lastSeen},
|
||||
}
|
||||
}
|
||||
|
||||
func ptrTime(t time.Time) *time.Time {
|
||||
return &t
|
||||
}
|
||||
|
||||
@@ -94,11 +94,6 @@ 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
|
||||
|
||||
@@ -3217,20 +3217,6 @@ func (mr *MockStoreMockRecorder) RefreshPeerLastSeen(ctx, accountID, peerID inte
|
||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshPeerLastSeen", reflect.TypeOf((*MockStore)(nil).RefreshPeerLastSeen), ctx, accountID, peerID)
|
||||
}
|
||||
|
||||
// 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()
|
||||
|
||||
@@ -3,7 +3,6 @@ package users
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"time"
|
||||
|
||||
"github.com/netbirdio/netbird/management/server/store"
|
||||
"github.com/netbirdio/netbird/management/server/types"
|
||||
@@ -12,9 +11,6 @@ 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 {
|
||||
@@ -30,10 +26,6 @@ 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)
|
||||
}
|
||||
@@ -82,10 +74,6 @@ 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 {
|
||||
|
||||
Reference in New Issue
Block a user