diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index 1c16ee30b..8cdedad7c 100644 --- a/ipn/ipnlocal/profiles.go +++ b/ipn/ipnlocal/profiles.go @@ -22,6 +22,8 @@ "tailscale.com/util/winutil" ) +var errAlreadyMigrated = errors.New("profile migration already completed") + // profileManager is a wrapper around a StateStore that manages // multiple profiles and the current profile. type profileManager struct { @@ -66,7 +68,7 @@ func (pm *profileManager) SetCurrentUserID(uid ipn.WindowsUserID) error { b, err := pm.store.ReadState(ipn.CurrentProfileKey(string(uid))) if err == ipn.ErrStateNotExist || len(b) == 0 { if runtime.GOOS == "windows" { - if err := pm.migrateFromLegacyPrefs(); err != nil { + if err := pm.migrateFromLegacyPrefs(); err != nil && !errors.Is(err, errAlreadyMigrated) { return err } } else { @@ -544,7 +546,14 @@ func newProfileManagerWithGOOS(store ipn.StateStore, logf logger.Logf, goos stri if err := pm.setPrefsLocked(prefs); err != nil { return nil, err } - } else if len(knownProfiles) == 0 && goos != "windows" { + // Most platform behavior is controlled by the goos parameter, however + // some behavior is implied by build tag and fails when run on Windows, + // so we explicitly avoid that behavior when running on Windows. + // Specifically this reaches down into legacy preference loading that is + // specialized by profiles_windows.go and fails in tests on an invalid + // uid passed in from the unix tests. The uid's used for Windows tests + // and runtime must be valid Windows security identifier structures. + } else if len(knownProfiles) == 0 && goos != "windows" && runtime.GOOS != "windows" { // No known profiles, try a migration. if err := pm.migrateFromLegacyPrefs(); err != nil { return nil, err @@ -562,7 +571,7 @@ func (pm *profileManager) migrateFromLegacyPrefs() error { sentinel, prefs, err := pm.loadLegacyPrefs() if err != nil { metricMigrationError.Add(1) - return err + return fmt.Errorf("load legacy prefs: %w", err) } if err := pm.SetPrefs(prefs); err != nil { metricMigrationError.Add(1) diff --git a/ipn/ipnlocal/profiles_test.go b/ipn/ipnlocal/profiles_test.go index f3027c3a8..7090b8a0f 100644 --- a/ipn/ipnlocal/profiles_test.go +++ b/ipn/ipnlocal/profiles_test.go @@ -5,7 +5,7 @@ import ( "fmt" - "runtime" + "os/user" "strconv" "testing" @@ -18,9 +18,6 @@ ) func TestProfileCurrentUserSwitch(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("TODO(#7876): test regressed on windows while CI was broken") - } store := new(mem.Store) pm, err := newProfileManagerWithGOOS(store, logger.Discard, "linux") @@ -77,9 +74,6 @@ func TestProfileCurrentUserSwitch(t *testing.T) { } func TestProfileList(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("TODO(#7876): test regressed on windows while CI was broken") - } store := new(mem.Store) pm, err := newProfileManagerWithGOOS(store, logger.Discard, "linux") @@ -158,9 +152,6 @@ func TestProfileList(t *testing.T) { // TestProfileManagement tests creating, loading, and switching profiles. func TestProfileManagement(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("TODO(#7876): test regressed on windows while CI was broken") - } store := new(mem.Store) pm, err := newProfileManagerWithGOOS(store, logger.Discard, "linux") @@ -312,10 +303,11 @@ func TestProfileManagement(t *testing.T) { // TestProfileManagementWindows tests going into and out of Unattended mode on // Windows. func TestProfileManagementWindows(t *testing.T) { - - if runtime.GOOS == "windows" { - t.Skip("TODO(#7876): test regressed on windows while CI was broken") + u, err := user.Current() + if err != nil { + t.Fatal(err) } + uid := ipn.WindowsUserID(u.Uid) store := new(mem.Store) @@ -365,8 +357,8 @@ func TestProfileManagementWindows(t *testing.T) { { t.Logf("Set user1 as logged in user") - if err := pm.SetCurrentUserID("user1"); err != nil { - t.Fatal(err) + if err := pm.SetCurrentUserID(uid); err != nil { + t.Fatalf("can't set user id: %s", err) } checkProfiles(t) t.Logf("Save prefs for user1") @@ -401,7 +393,7 @@ func TestProfileManagementWindows(t *testing.T) { { t.Logf("Set user1 as current user") - if err := pm.SetCurrentUserID("user1"); err != nil { + if err := pm.SetCurrentUserID(uid); err != nil { t.Fatal(err) } wantCurProfile = "test" @@ -411,8 +403,8 @@ func TestProfileManagementWindows(t *testing.T) { t.Logf("set unattended mode") wantProfiles["test"] = setPrefs(t, "test", true) } - if pm.CurrentUserID() != "user1" { - t.Fatalf("CurrentUserID = %q; want %q", pm.CurrentUserID(), "user1") + if pm.CurrentUserID() != uid { + t.Fatalf("CurrentUserID = %q; want %q", pm.CurrentUserID(), uid) } // Recreate the profile manager to ensure that it starts with test profile. @@ -421,7 +413,7 @@ func TestProfileManagementWindows(t *testing.T) { t.Fatal(err) } checkProfiles(t) - if pm.CurrentUserID() != "user1" { - t.Fatalf("CurrentUserID = %q; want %q", pm.CurrentUserID(), "user1") + if pm.CurrentUserID() != uid { + t.Fatalf("CurrentUserID = %q; want %q", pm.CurrentUserID(), uid) } } diff --git a/ipn/ipnlocal/profiles_windows.go b/ipn/ipnlocal/profiles_windows.go index bc47e154e..1661c08d3 100644 --- a/ipn/ipnlocal/profiles_windows.go +++ b/ipn/ipnlocal/profiles_windows.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" + "io/fs" "os" "os/user" "path/filepath" @@ -21,8 +22,6 @@ legacyPrefsExt = ".conf" ) -var errAlreadyMigrated = errors.New("profile migration already completed") - func legacyPrefsDir(uid ipn.WindowsUserID) (string, error) { // TODO(aaron): Ideally we'd have the impersonation token for the pipe's // client and use it to call SHGetKnownFolderPath, thus yielding the correct @@ -56,6 +55,9 @@ func (pm *profileManager) loadLegacyPrefs() (string, ipn.PrefsView, error) { prefsPath := filepath.Join(userLegacyPrefsDir, legacyPrefsFile+legacyPrefsExt) prefs, err := ipn.LoadPrefs(prefsPath) + if errors.Is(err, fs.ErrNotExist) { + return "", ipn.PrefsView{}, errAlreadyMigrated + } if err != nil { return "", ipn.PrefsView{}, err }