From 095d3edd3316b459de27033f69b246001d515290 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Fri, 14 Apr 2023 16:13:06 -0700 Subject: [PATCH] ipn/ipnlocal: reenable profile tests on Windows This fix does not seem ideal, but the test infrastructure using a local goos doesn't seem to avoid all of the associated challenges, but is somewhat deeply tied to the setup. The core issue this addresses for now is that when run on Windows there can be no code paths that attempt to use an invalid UID string, which on Windows is described in [1]. For the goos="linux" tests, we now explicitly skip the affected migration code if runtime.GOOS=="windows", and for the Windows test we explicitly use the running users uid, rather than just the string "user1". We also now make the case where a profile exists and has already been migrated a non-error condition toward the outer API. Updates #7876 [1] https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers Signed-off-by: James Tucker --- ipn/ipnlocal/profiles.go | 15 ++++++++++++--- ipn/ipnlocal/profiles_test.go | 32 ++++++++++++-------------------- ipn/ipnlocal/profiles_windows.go | 6 ++++-- 3 files changed, 28 insertions(+), 25 deletions(-) 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 }