diff --git a/ipn/auditlog/extension.go b/ipn/auditlog/extension.go index 036d8fd36..3b561b2e5 100644 --- a/ipn/auditlog/extension.go +++ b/ipn/auditlog/extension.go @@ -68,7 +68,7 @@ func (e *extension) Name() string { func (e *extension) Init(h ipnext.Host) error { e.cleanup = []func(){ h.RegisterControlClientCallback(e.controlClientChanged), - h.Profiles().RegisterProfileChangeCallback(e.profileChanged), + h.Profiles().RegisterProfileStateChangeCallback(e.profileChanged), h.RegisterAuditLogProvider(e.getCurrentLogger), } return nil @@ -109,7 +109,7 @@ func (e *extension) startNewLogger(cc controlclient.Client, profileID ipn.Profil return logger, nil } -func (e *extension) controlClientChanged(cc controlclient.Client, profile ipn.LoginProfileView, _ ipn.PrefsView) (cleanup func()) { +func (e *extension) controlClientChanged(cc controlclient.Client, profile ipn.LoginProfileView) (cleanup func()) { logger, err := e.startNewLogger(cc, profile.ID()) e.mu.Lock() e.logger = logger // nil on error diff --git a/ipn/ipnext/ipnext.go b/ipn/ipnext/ipnext.go index f8fd500ce..4c7e978e5 100644 --- a/ipn/ipnext/ipnext.go +++ b/ipn/ipnext/ipnext.go @@ -219,12 +219,32 @@ type ExtensionServices interface { // ProfileServices provides access to the [Host]'s profile management services, // such as switching profiles and registering profile change callbacks. type ProfileServices interface { + // CurrentProfileState returns read-only views of the current profile + // and its preferences. The returned views are always valid, + // but the profile's [ipn.LoginProfileView.ID] returns "" + // if the profile is new and has not been persisted yet. + // + // The returned views are immutable snapshots of the current profile + // and prefs at the time of the call. The actual state is only guaranteed + // to remain unchanged and match these views for the duration + // of a callback invoked by the host, if used within that callback. + // + // Extensions that need the current profile or prefs at other times + // should typically subscribe to [ProfileStateChangeCallback] + // to be notified if the profile or prefs change after retrieval. + // CurrentProfileState returns both the profile and prefs + // to guarantee that they are consistent with each other. + CurrentProfileState() (ipn.LoginProfileView, ipn.PrefsView) + + // CurrentPrefs is like [CurrentProfileState] but only returns prefs. + CurrentPrefs() ipn.PrefsView + // SwitchToBestProfileAsync asynchronously selects the best profile to use // and switches to it, unless it is already the current profile. // // If an extension needs to know when a profile switch occurs, - // it must use [ProfileServices.RegisterProfileChangeCallback] - // to register a [ProfileChangeCallback]. + // it must use [ProfileServices.RegisterProfileStateChangeCallback] + // to register a [ProfileStateChangeCallback]. // // The reason indicates why the profile is being switched, such as due // to a client connecting or disconnecting or a change in the desktop @@ -241,10 +261,14 @@ type ProfileServices interface { // only exist on Windows, and we're moving away from them anyway. RegisterBackgroundProfileResolver(ProfileResolver) (unregister func()) - // RegisterProfileChangeCallback registers a function to be called when the current - // [ipn.LoginProfile] changes. The returned function unregisters the callback. + // RegisterProfileStateChangeCallback registers a function to be called when the current + // [ipn.LoginProfile] or its [ipn.Prefs] change. The returned function unregisters the callback. + // + // To get the initial profile or prefs, use [ProfileServices.CurrentProfileState] + // or [ProfileServices.CurrentPrefs] from the extension's [Extension.Init]. + // // It is a runtime error to register a nil callback. - RegisterProfileChangeCallback(ProfileChangeCallback) (unregister func()) + RegisterProfileStateChangeCallback(ProfileStateChangeCallback) (unregister func()) } // ProfileStore provides read-only access to available login profiles and their preferences. @@ -281,23 +305,30 @@ type AuditLogProvider func() ipnauth.AuditLogFunc // The provided [ProfileStore] can only be used for the duration of the callback. type ProfileResolver func(ProfileStore) ipn.LoginProfileView -// ProfileChangeCallback is a function to be called when the current login profile changes. +// ProfileStateChangeCallback is a function to be called when the current login profile +// or its preferences change. +// // The sameNode parameter indicates whether the profile represents the same node as before, -// such as when only the profile metadata is updated but the node ID remains the same, -// or when a new profile is persisted and assigned an [ipn.ProfileID] for the first time. -// The subscribers can use this information to decide whether to reset their state. +// which is true when: +// - Only the profile's [ipn.Prefs] or metadata (e.g., [tailcfg.UserProfile]) have changed, +// but the node ID and [ipn.ProfileID] remain the same. +// - The profile has been persisted and assigned an [ipn.ProfileID] for the first time, +// so while its node ID and [ipn.ProfileID] have changed, it is still the same profile. +// +// It can be used to decide whether to reset state bound to the current profile or node identity. // // The profile and prefs are always valid, but the profile's [ipn.LoginProfileView.ID] // returns "" if the profile is new and has not been persisted yet. -type ProfileChangeCallback func(_ ipn.LoginProfileView, _ ipn.PrefsView, sameNode bool) +type ProfileStateChangeCallback func(_ ipn.LoginProfileView, _ ipn.PrefsView, sameNode bool) // NewControlClientCallback is a function to be called when a new [controlclient.Client] -// is created and before it is first used. The login profile and prefs represent -// the profile for which the cc is created and are always valid; however, the -// profile's [ipn.LoginProfileView.ID] returns "" if the profile is new -// and has not been persisted yet. If the [controlclient.Client] is created -// due to a profile switch, any registered [ProfileChangeCallback]s are called first. +// is created and before it is first used. The specified profile represents the node +// for which the cc is created and is always valid. Its [ipn.LoginProfileView.ID] +// returns "" if it is a new node whose profile has never been persisted. +// +// If the [controlclient.Client] is created due to a profile switch, any registered +// [ProfileStateChangeCallback]s are called first. // // It returns a function to be called when the cc is being shut down, // or nil if no cleanup is needed. -type NewControlClientCallback func(controlclient.Client, ipn.LoginProfileView, ipn.PrefsView) (cleanup func()) +type NewControlClientCallback func(controlclient.Client, ipn.LoginProfileView) (cleanup func()) diff --git a/ipn/ipnlocal/extension_host.go b/ipn/ipnlocal/extension_host.go index 9c6b6d44c..2a8a6a085 100644 --- a/ipn/ipnlocal/extension_host.go +++ b/ipn/ipnlocal/extension_host.go @@ -99,6 +99,13 @@ type ExtensionHost struct { // by the workQueue after all extensions have been initialized. postInitWorkQueue []func(Backend) + // currentProfile is a read-only view of the currently used profile. + // The view is always Valid, but might be of an empty, non-persisted profile. + currentProfile ipn.LoginProfileView + // currentPrefs is a read-only view of the current profile's [ipn.Prefs] + // with any private keys stripped. It is always Valid. + currentPrefs ipn.PrefsView + // auditLoggers are registered [AuditLogProvider]s. // Each provider is called to get an [ipnauth.AuditLogFunc] when an auditable action // is about to be performed. If an audit logger returns an error, the action is denied. @@ -108,11 +115,12 @@ type ExtensionHost struct { backgroundProfileResolvers set.HandleSet[ipnext.ProfileResolver] // newControlClientCbs are the functions to be called when a new control client is created. newControlClientCbs set.HandleSet[ipnext.NewControlClientCallback] - // profileChangeCbs are the callbacks to be invoked when the current login profile changes, - // either because of a profile switch, or because the profile information was updated - // by [LocalBackend.SetControlClientStatus], including when the profile is first populated - // and persisted. - profileChangeCbs set.HandleSet[ipnext.ProfileChangeCallback] + // profileStateChangeCbs are callbacks that are invoked when the current login profile + // or its [ipn.Prefs] change, after those changes have been made. The current login profile + // may be changed either because of a profile switch, or because the profile information + // was updated by [LocalBackend.SetControlClientStatus], including when the profile + // is first populated and persisted. + profileStateChangeCbs set.HandleSet[ipnext.ProfileStateChangeCallback] } // Backend is a subset of [LocalBackend] methods that are used by [ExtensionHost]. @@ -133,6 +141,10 @@ func NewExtensionHost(logf logger.Logf, sys *tsd.System, b Backend, overrideExts host := &ExtensionHost{ logf: logger.WithPrefix(logf, "ipnext: "), workQueue: &execqueue.ExecQueue{}, + // The host starts with an empty profile and default prefs. + // We'll update them once [profileManager] notifies us of the initial profile. + currentProfile: zeroProfile, + currentPrefs: defaultPrefs, } // All operations on the backend must be executed asynchronously by the work queue. @@ -231,7 +243,6 @@ func (h *ExtensionHost) init() { f(b) } }) - } // Extensions implements [ipnext.Host]. @@ -295,6 +306,22 @@ func (h *ExtensionHost) Profiles() ipnext.ProfileServices { return h } +// CurrentProfileState implements [ipnext.ProfileServices]. +func (h *ExtensionHost) CurrentProfileState() (ipn.LoginProfileView, ipn.PrefsView) { + if h == nil { + return zeroProfile, defaultPrefs + } + h.mu.Lock() + defer h.mu.Unlock() + return h.currentProfile, h.currentPrefs +} + +// CurrentPrefs implements [ipnext.ProfileServices]. +func (h *ExtensionHost) CurrentPrefs() ipn.PrefsView { + _, prefs := h.CurrentProfileState() + return prefs +} + // SwitchToBestProfileAsync implements [ipnext.ProfileServices]. func (h *ExtensionHost) SwitchToBestProfileAsync(reason string) { if h == nil { @@ -305,8 +332,8 @@ func (h *ExtensionHost) SwitchToBestProfileAsync(reason string) { }) } -// RegisterProfileChangeCallback implements [ipnext.ProfileServices]. -func (h *ExtensionHost) RegisterProfileChangeCallback(cb ipnext.ProfileChangeCallback) (unregister func()) { +// RegisterProfileStateChangeCallback implements [ipnext.ProfileServices]. +func (h *ExtensionHost) RegisterProfileStateChangeCallback(cb ipnext.ProfileStateChangeCallback) (unregister func()) { if h == nil { return func() {} } @@ -315,31 +342,60 @@ func (h *ExtensionHost) RegisterProfileChangeCallback(cb ipnext.ProfileChangeCal } h.mu.Lock() defer h.mu.Unlock() - handle := h.profileChangeCbs.Add(cb) + handle := h.profileStateChangeCbs.Add(cb) return func() { h.mu.Lock() defer h.mu.Unlock() - delete(h.profileChangeCbs, handle) + delete(h.profileStateChangeCbs, handle) } } -// NotifyProfileChange invokes registered profile change callbacks. -// It strips private keys from the [ipn.Prefs] before passing it to the callbacks. +// NotifyProfileChange invokes registered profile state change callbacks +// and updates the current profile and prefs in the host. +// It strips private keys from the [ipn.Prefs] before preserving +// or passing them to the callbacks. func (h *ExtensionHost) NotifyProfileChange(profile ipn.LoginProfileView, prefs ipn.PrefsView, sameNode bool) { if h == nil { return } h.mu.Lock() - cbs := collectValues(h.profileChangeCbs) + // Strip private keys from the prefs before preserving or passing them to the callbacks. + // Extensions should not need them (unless proven otherwise in the future), + // and this is a good way to ensure that they won't accidentally leak them. + prefs = stripKeysFromPrefs(prefs) + // Update the current profile and prefs in the host, + // so we can provide them to the extensions later if they ask. + h.currentPrefs = prefs + h.currentProfile = profile + // Get the callbacks to be invoked. + cbs := collectValues(h.profileStateChangeCbs) h.mu.Unlock() - if cbs != nil { - // Strip private keys from the prefs before passing it to the callbacks. - // Extensions should not need it (unless proven otherwise in the future), - // and this is a good way to ensure that they won't accidentally leak them. - prefs = stripKeysFromPrefs(prefs) - for _, cb := range cbs { - cb(profile, prefs, sameNode) - } + for _, cb := range cbs { + cb(profile, prefs, sameNode) + } +} + +// NotifyProfilePrefsChanged invokes registered profile state change callbacks, +// and updates the current profile and prefs in the host. +// It strips private keys from the [ipn.Prefs] before preserving or using them. +func (h *ExtensionHost) NotifyProfilePrefsChanged(profile ipn.LoginProfileView, oldPrefs, newPrefs ipn.PrefsView) { + if h == nil { + return + } + h.mu.Lock() + // Strip private keys from the prefs before preserving or passing them to the callbacks. + // Extensions should not need them (unless proven otherwise in the future), + // and this is a good way to ensure that they won't accidentally leak them. + newPrefs = stripKeysFromPrefs(newPrefs) + // Update the current profile and prefs in the host, + // so we can provide them to the extensions later if they ask. + h.currentPrefs = newPrefs + h.currentProfile = profile + // Get the callbacks to be invoked. + stateCbs := collectValues(h.profileStateChangeCbs) + h.mu.Unlock() + for _, cb := range stateCbs { + cb(profile, newPrefs, true) } } @@ -410,7 +466,7 @@ func (h *ExtensionHost) RegisterControlClientCallback(cb ipnext.NewControlClient // NotifyNewControlClient invokes all registered control client callbacks. // It returns callbacks to be executed when the control client shuts down. -func (h *ExtensionHost) NotifyNewControlClient(cc controlclient.Client, profile ipn.LoginProfileView, prefs ipn.PrefsView) (ccShutdownCbs []func()) { +func (h *ExtensionHost) NotifyNewControlClient(cc controlclient.Client, profile ipn.LoginProfileView) (ccShutdownCbs []func()) { if h == nil { return nil } @@ -420,7 +476,7 @@ func (h *ExtensionHost) NotifyNewControlClient(cc controlclient.Client, profile if len(cbs) > 0 { ccShutdownCbs = make([]func(), 0, len(cbs)) for _, cb := range cbs { - if shutdown := cb(cc, profile, prefs); shutdown != nil { + if shutdown := cb(cc, profile); shutdown != nil { ccShutdownCbs = append(ccShutdownCbs, shutdown) } } diff --git a/ipn/ipnlocal/extension_host_test.go b/ipn/ipnlocal/extension_host_test.go index cefe9339d..ced5867e7 100644 --- a/ipn/ipnlocal/extension_host_test.go +++ b/ipn/ipnlocal/extension_host_test.go @@ -32,6 +32,11 @@ import ( "tailscale.com/util/must" ) +// defaultCmpOpts are the default options used for deepcmp comparisons in tests. +var defaultCmpOpts = []deepcmp.Option{ + cmpopts.EquateComparable(key.NodePublic{}, netip.Addr{}, netip.Prefix{}), +} + // TestExtensionInitShutdown tests that [ExtensionHost] correctly initializes // and shuts down extensions. func TestExtensionInitShutdown(t *testing.T) { @@ -508,56 +513,63 @@ func TestExtensionHostEnqueueBackendOperation(t *testing.T) { } } -// TestExtensionHostProfileChangeCallback verifies that [ExtensionHost] correctly handles the registration, -// invocation, and unregistration of profile change callbacks. It also checks that the callbacks are called -// with the correct arguments and that any private keys are stripped from [ipn.Prefs] before being passed to the callback. -func TestExtensionHostProfileChangeCallback(t *testing.T) { +// TestExtensionHostProfileStateChangeCallback verifies that [ExtensionHost] correctly handles the registration, +// invocation, and unregistration of profile state change callbacks. This includes callbacks triggered by profile changes +// and by changes to the profile's [ipn.Prefs]. It also checks that the callbacks are called with the correct arguments +// and that any private keys are stripped from [ipn.Prefs] before being passed to the callback. +func TestExtensionHostProfileStateChangeCallback(t *testing.T) { t.Parallel() - type profileChange struct { + type stateChange struct { Profile *ipn.LoginProfile Prefs *ipn.Prefs SameNode bool } - // newProfileChange creates a new profile change with deep copies of the profile and prefs. - newProfileChange := func(profile ipn.LoginProfileView, prefs ipn.PrefsView, sameNode bool) profileChange { - return profileChange{ + type prefsChange struct { + Profile *ipn.LoginProfile + Old, New *ipn.Prefs + } + + // newStateChange creates a new [stateChange] with deep copies of the profile and prefs. + newStateChange := func(profile ipn.LoginProfileView, prefs ipn.PrefsView, sameNode bool) stateChange { + return stateChange{ Profile: profile.AsStruct(), Prefs: prefs.AsStruct(), SameNode: sameNode, } } - // makeProfileChangeAppender returns a callback that appends profile changes to the extension's state. - makeProfileChangeAppender := func(e *testExtension) ipnext.ProfileChangeCallback { + // makeStateChangeAppender returns a callback that appends profile state changes to the extension's state. + makeStateChangeAppender := func(e *testExtension) ipnext.ProfileStateChangeCallback { return func(profile ipn.LoginProfileView, prefs ipn.PrefsView, sameNode bool) { - UpdateExtState(e, "changes", func(changes []profileChange) []profileChange { - return append(changes, newProfileChange(profile, prefs, sameNode)) + UpdateExtState(e, "changes", func(changes []stateChange) []stateChange { + return append(changes, newStateChange(profile, prefs, sameNode)) }) } } - // getProfileChanges returns the profile changes stored in the extension's state. - getProfileChanges := func(e *testExtension) []profileChange { - changes, _ := GetExtStateOk[[]profileChange](e, "changes") + // getStateChanges returns the profile state changes stored in the extension's state. + getStateChanges := func(e *testExtension) []stateChange { + changes, _ := GetExtStateOk[[]stateChange](e, "changes") return changes } tests := []struct { - name string - ext *testExtension - calls []profileChange - wantCalls []profileChange + name string + ext *testExtension + stateCalls []stateChange + prefsCalls []prefsChange + wantChanges []stateChange }{ { // Register the callback for the lifetime of the extension. name: "Register/Lifetime", ext: &testExtension{}, - calls: []profileChange{ + stateCalls: []stateChange{ {Profile: &ipn.LoginProfile{ID: "profile-1"}}, {Profile: &ipn.LoginProfile{ID: "profile-2"}}, {Profile: &ipn.LoginProfile{ID: "profile-3"}}, {Profile: &ipn.LoginProfile{ID: "profile-3"}, SameNode: true}, }, - wantCalls: []profileChange{ // all calls are received by the callback + wantChanges: []stateChange{ // all calls are received by the callback {Profile: &ipn.LoginProfile{ID: "profile-1"}}, {Profile: &ipn.LoginProfile{ID: "profile-2"}}, {Profile: &ipn.LoginProfile{ID: "profile-3"}}, @@ -572,19 +584,19 @@ func TestExtensionHostProfileChangeCallback(t *testing.T) { InitHook: func(e *testExtension) error { var unregister func() handler := func(profile ipn.LoginProfileView, prefs ipn.PrefsView, sameNode bool) { - makeProfileChangeAppender(e)(profile, prefs, sameNode) + makeStateChangeAppender(e)(profile, prefs, sameNode) unregister() } - unregister = e.host.Profiles().RegisterProfileChangeCallback(handler) + unregister = e.host.Profiles().RegisterProfileStateChangeCallback(handler) return nil }, }, - calls: []profileChange{ + stateCalls: []stateChange{ {Profile: &ipn.LoginProfile{ID: "profile-1"}}, {Profile: &ipn.LoginProfile{ID: "profile-2"}}, {Profile: &ipn.LoginProfile{ID: "profile-3"}}, }, - wantCalls: []profileChange{ // only the first call is received by the callback + wantChanges: []stateChange{ // only the first call is received by the callback {Profile: &ipn.LoginProfile{ID: "profile-1"}}, }, }, @@ -592,7 +604,7 @@ func TestExtensionHostProfileChangeCallback(t *testing.T) { // Ensure that ipn.Prefs are passed to the callback. name: "CheckPrefs", ext: &testExtension{}, - calls: []profileChange{{ + stateCalls: []stateChange{{ Profile: &ipn.LoginProfile{ID: "profile-1"}, Prefs: &ipn.Prefs{ WantRunning: true, @@ -603,7 +615,7 @@ func TestExtensionHostProfileChangeCallback(t *testing.T) { }, }, }}, - wantCalls: []profileChange{{ + wantChanges: []stateChange{{ Profile: &ipn.LoginProfile{ID: "profile-1"}, Prefs: &ipn.Prefs{ WantRunning: true, @@ -619,7 +631,7 @@ func TestExtensionHostProfileChangeCallback(t *testing.T) { // Ensure that private keys are stripped from persist.Persist shared with extensions. name: "StripPrivateKeys", ext: &testExtension{}, - calls: []profileChange{{ + stateCalls: []stateChange{{ Profile: &ipn.LoginProfile{ID: "profile-1"}, Prefs: &ipn.Prefs{ Persist: &persist.Persist{ @@ -636,7 +648,7 @@ func TestExtensionHostProfileChangeCallback(t *testing.T) { }, }, }}, - wantCalls: []profileChange{{ + wantChanges: []stateChange{{ Profile: &ipn.LoginProfile{ID: "profile-1"}, Prefs: &ipn.Prefs{ Persist: &persist.Persist{ @@ -654,6 +666,100 @@ func TestExtensionHostProfileChangeCallback(t *testing.T) { }, }}, }, + { + // Ensure that profile state callbacks are also invoked when prefs (rather than profile) change. + name: "PrefsChange", + ext: &testExtension{}, + prefsCalls: []prefsChange{ + { + Profile: &ipn.LoginProfile{ID: "profile-1"}, + Old: &ipn.Prefs{WantRunning: false, LoggedOut: true}, + New: &ipn.Prefs{WantRunning: true, LoggedOut: false}, + }, + { + Profile: &ipn.LoginProfile{ID: "profile-1"}, + Old: &ipn.Prefs{AdvertiseRoutes: []netip.Prefix{netip.MustParsePrefix("192.168.1.0/24")}}, + New: &ipn.Prefs{AdvertiseRoutes: []netip.Prefix{netip.MustParsePrefix("10.10.10.0/24")}}, + }, + }, + wantChanges: []stateChange{ + { + Profile: &ipn.LoginProfile{ID: "profile-1"}, + Prefs: &ipn.Prefs{WantRunning: true, LoggedOut: false}, + SameNode: true, // must be true for prefs changes + }, + { + Profile: &ipn.LoginProfile{ID: "profile-1"}, + Prefs: &ipn.Prefs{AdvertiseRoutes: []netip.Prefix{netip.MustParsePrefix("10.10.10.0/24")}}, + SameNode: true, // must be true for prefs changes + }, + }, + }, + { + // Ensure that private keys are stripped from prefs when state change callback + // is invoked by prefs change. + name: "PrefsChange/StripPrivateKeys", + ext: &testExtension{}, + prefsCalls: []prefsChange{ + { + Profile: &ipn.LoginProfile{ID: "profile-1"}, + Old: &ipn.Prefs{ + WantRunning: false, + LoggedOut: true, + Persist: &persist.Persist{ + NodeID: "12345", + PrivateNodeKey: key.NewNode(), + OldPrivateNodeKey: key.NewNode(), + NetworkLockKey: key.NewNLPrivate(), + UserProfile: tailcfg.UserProfile{ + ID: 12345, + LoginName: "test@example.com", + DisplayName: "Test User", + ProfilePicURL: "https://example.com/profile.png", + }, + }, + }, + New: &ipn.Prefs{ + WantRunning: true, + LoggedOut: false, + Persist: &persist.Persist{ + NodeID: "12345", + PrivateNodeKey: key.NewNode(), + OldPrivateNodeKey: key.NewNode(), + NetworkLockKey: key.NewNLPrivate(), + UserProfile: tailcfg.UserProfile{ + ID: 12345, + LoginName: "test@example.com", + DisplayName: "Test User", + ProfilePicURL: "https://example.com/profile.png", + }, + }, + }, + }, + }, + wantChanges: []stateChange{ + { + Profile: &ipn.LoginProfile{ID: "profile-1"}, + Prefs: &ipn.Prefs{ + WantRunning: true, + LoggedOut: false, + Persist: &persist.Persist{ + NodeID: "12345", + PrivateNodeKey: key.NodePrivate{}, // stripped + OldPrivateNodeKey: key.NodePrivate{}, // stripped + NetworkLockKey: key.NLPrivate{}, // stripped + UserProfile: tailcfg.UserProfile{ + ID: 12345, + LoginName: "test@example.com", + DisplayName: "Test User", + ProfilePicURL: "https://example.com/profile.png", + }, + }, + }, + SameNode: true, // must be true for prefs changes + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -663,26 +769,60 @@ func TestExtensionHostProfileChangeCallback(t *testing.T) { if tt.ext.InitHook == nil { tt.ext.InitHook = func(e *testExtension) error { // Create and register the callback on init. - handler := makeProfileChangeAppender(e) - e.Cleanup(e.host.Profiles().RegisterProfileChangeCallback(handler)) + handler := makeStateChangeAppender(e) + e.Cleanup(e.host.Profiles().RegisterProfileStateChangeCallback(handler)) return nil } } h := newExtensionHostForTest(t, &testBackend{}, true, tt.ext) - for _, call := range tt.calls { + for _, call := range tt.stateCalls { h.NotifyProfileChange(call.Profile.View(), call.Prefs.View(), call.SameNode) } - opts := []deepcmp.Option{ - cmpopts.EquateComparable(key.NodePublic{}, netip.Addr{}, netip.Prefix{}), + for _, call := range tt.prefsCalls { + h.NotifyProfilePrefsChanged(call.Profile.View(), call.Old.View(), call.New.View()) } - if diff := deepcmp.Diff(tt.wantCalls, getProfileChanges(tt.ext), opts...); diff != "" { - t.Errorf("ProfileChange callbacks: (-want +got): %v", diff) + if diff := deepcmp.Diff(tt.wantChanges, getStateChanges(tt.ext), defaultCmpOpts...); diff != "" { + t.Errorf("StateChange callbacks: (-want +got): %v", diff) } }) } } +// TestCurrentProfileState tests that the current profile and prefs are correctly +// initialized and updated when the host is notified of changes. +func TestCurrentProfileState(t *testing.T) { + h := newExtensionHostForTest[ipnext.Extension](t, &testBackend{}, false) + + // The initial profile and prefs should be valid and set to the default values. + gotProfile, gotPrefs := h.Profiles().CurrentProfileState() + checkViewsEqual(t, "Initial profile (from state)", gotProfile, zeroProfile) + checkViewsEqual(t, "Initial prefs (from state)", gotPrefs, defaultPrefs) + gotPrefs = h.Profiles().CurrentPrefs() // same when we only ask for prefs + checkViewsEqual(t, "Initial prefs (direct)", gotPrefs, defaultPrefs) + + // Create a new profile and prefs, and notify the host of the change. + profile := &ipn.LoginProfile{ID: "profile-A"} + prefsV1 := &ipn.Prefs{ProfileName: "Prefs V1", WantRunning: true} + h.NotifyProfileChange(profile.View(), prefsV1.View(), false) + // The current profile and prefs should be updated. + gotProfile, gotPrefs = h.Profiles().CurrentProfileState() + checkViewsEqual(t, "Changed profile (from state)", gotProfile, profile.View()) + checkViewsEqual(t, "New prefs (from state)", gotPrefs, prefsV1.View()) + gotPrefs = h.Profiles().CurrentPrefs() + checkViewsEqual(t, "New prefs (direct)", gotPrefs, prefsV1.View()) + + // Notify the host of a change to the profile's prefs. + prefsV2 := &ipn.Prefs{ProfileName: "Prefs V2", WantRunning: false} + h.NotifyProfilePrefsChanged(profile.View(), prefsV1.View(), prefsV2.View()) + // The current prefs should be updated. + gotProfile, gotPrefs = h.Profiles().CurrentProfileState() + checkViewsEqual(t, "Unchanged profile (from state)", gotProfile, profile.View()) + checkViewsEqual(t, "Changed (from state)", gotPrefs, prefsV2.View()) + gotPrefs = h.Profiles().CurrentPrefs() + checkViewsEqual(t, "Changed prefs (direct)", gotPrefs, prefsV2.View()) +} + // TestBackgroundProfileResolver tests that the background profile resolvers // are correctly registered, unregistered and invoked by the [ExtensionHost]. func TestBackgroundProfileResolver(t *testing.T) { @@ -1231,3 +1371,29 @@ func (b *testBackend) SwitchToBestProfile(reason string) { b.switchToBestProfileHook(reason) } } + +// equatableView is an interface implemented by views +// that can be compared for equality. +type equatableView[T any] interface { + Valid() bool + Equals(other T) bool +} + +// checkViewsEqual checks that the two views are equal +// and fails the test if they are not. The prefix is used +// to format the error message. +func checkViewsEqual[T equatableView[T]](t *testing.T, prefix string, got, want T) { + t.Helper() + switch { + case got.Equals(want): + return + case got.Valid() && want.Valid(): + t.Errorf("%s: got %v; want %v", prefix, got, want) + case got.Valid() && !want.Valid(): + t.Errorf("%s: got %v; want invalid", prefix, got) + case !got.Valid() && want.Valid(): + t.Errorf("%s: got invalid; want %v", prefix, want) + default: + panic("unreachable") + } +} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index e21403fbe..45daefda8 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -530,6 +530,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo if b.extHost, err = NewExtensionHost(logf, sys, b); err != nil { return nil, fmt.Errorf("failed to create extension host: %w", err) } + b.pm.SetExtensionHost(b.extHost) if b.unregisterSysPolicyWatch, err = b.registerSysPolicyWatch(); err != nil { return nil, err @@ -1653,8 +1654,8 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control // Theoretically, a completed login could also result in a switch to a different existing // profile representing a different node (see tailscale/tailscale#8816). // - // Let's check if the current profile has changed, and invoke all registered [ProfileChangeCallback] - // if necessary. + // Let's check if the current profile has changed, and invoke all registered + // [ipnext.ProfileStateChangeCallback] if necessary. if cp := b.pm.CurrentProfile(); *cp.AsStruct() != *profile.AsStruct() { // If the profile ID was empty before SetPrefs, it's a new profile // and the user has just completed a login for the first time. @@ -2408,7 +2409,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { if err != nil { return err } - ccShutdownCbs = b.extHost.NotifyNewControlClient(cc, b.pm.CurrentProfile(), prefs) + ccShutdownCbs = b.extHost.NotifyNewControlClient(cc, b.pm.CurrentProfile()) b.setControlClientLocked(cc) endpoints := b.endpoints diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index 057fe2aae..eb01da705 100644 --- a/ipn/ipnlocal/profiles.go +++ b/ipn/ipnlocal/profiles.go @@ -42,6 +42,19 @@ type profileManager struct { knownProfiles map[ipn.ProfileID]ipn.LoginProfileView // always non-nil currentProfile ipn.LoginProfileView // always Valid. prefs ipn.PrefsView // always Valid. + + // extHost is the bridge between [profileManager] and the registered [ipnext.Extension]s. + // It may be nil in tests. A nil pointer is a valid, no-op host. + extHost *ExtensionHost +} + +// SetExtensionHost sets the [ExtensionHost] for the [profileManager]. +// The specified host will be notified about profile and prefs changes +// and will immediately be notified about the current profile and prefs. +// A nil host is a valid, no-op host. +func (pm *profileManager) SetExtensionHost(host *ExtensionHost) { + pm.extHost = host + host.NotifyProfileChange(pm.currentProfile, pm.prefs, false) } func (pm *profileManager) dlogf(format string, args ...any) { @@ -321,7 +334,6 @@ func (pm *profileManager) SetPrefs(prefsIn ipn.PrefsView, np ipn.NetworkProfile) return err } return pm.setProfileAsUserDefault(cp) - } // setProfilePrefs is like [profileManager.SetPrefs], but sets prefs for the specified [ipn.LoginProfile], @@ -419,7 +431,27 @@ func newUnusedID(knownProfiles map[ipn.ProfileID]ipn.LoginProfileView) (ipn.Prof func (pm *profileManager) setProfilePrefsNoPermCheck(profile ipn.LoginProfileView, clonedPrefs ipn.PrefsView) error { isCurrentProfile := pm.currentProfile == profile if isCurrentProfile { + oldPrefs := pm.prefs pm.prefs = clonedPrefs + + // Sadly, profile prefs can be changed in multiple ways. + // It's pretty chaotic, and in many cases callers use + // unexported methods of the profile manager instead of + // going through [LocalBackend.setPrefsLockedOnEntry] + // or at least using [profileManager.SetPrefs]. + // + // While we should definitely clean this up to improve + // the overall structure of how prefs are set, which would + // also address current and future conflicts, such as + // competing features changing the same prefs, this method + // is currently the central place where we can detect all + // changes to the current profile's prefs. + // + // That said, regardless of the cleanup, we might want + // to keep the profileManager responsible for invoking + // profile- and prefs-related callbacks. + pm.extHost.NotifyProfilePrefsChanged(pm.currentProfile, oldPrefs, clonedPrefs) + pm.updateHealth() } if profile.Key() != "" { @@ -705,6 +737,9 @@ func (pm *profileManager) SwitchToNewProfileForUser(uid ipn.WindowsUserID) { pm.SwitchToProfile(pm.NewProfileForUser(uid)) } +// zeroProfile is a read-only view of a new, empty profile that is not persisted to the store. +var zeroProfile = (&ipn.LoginProfile{}).View() + // NewProfileForUser creates a new profile for the specified user and returns a read-only view of it. // It neither switches to the new profile nor persists it to the store. func (pm *profileManager) NewProfileForUser(uid ipn.WindowsUserID) ipn.LoginProfileView {