cmd/tailscale: make up respect explicitly empty --operator= value

soypete · Soypete · commit 412c4c55e2d6 · 2022-06-27T13:59:45.000-06:00
Fixes tailscale#3808 Signed-off-by: soypete <miriah@tailscale.com>
diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go
@@ -493,14 +493,16 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			applyImplicitPrefs(newPrefs, tt.curPrefs, tt.curUser)
-			var got string
-			if err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upCheckEnv{
+			upEnv := upCheckEnv{
 				goos:          goos,
 				flagSet:       flagSet,
 				curExitNodeIP: tt.curExitNodeIP,
 				distro:        tt.distro,
-			}); err != nil {
+				user:          tt.curUser,
+			}
+			applyImplicitPrefs(newPrefs, tt.curPrefs, upEnv)
+			var got string
+			if err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upEnv); err != nil {
 				got = err.Error()
 			}
 			if strings.TrimSpace(got) != tt.want {
@@ -784,6 +786,10 @@ func TestUpdatePrefs(t *testing.T) {
 		curPrefs *ipn.Prefs
 		env      upCheckEnv // empty goos means "linux"
 
+		// checkUpdatePrefsMutations, if non-nil, is run with the new prefs after
+		// updatePrefs might've mutated them (from applyImplicitPrefs).
+		checkUpdatePrefsMutations func(t *testing.T, newPrefs *ipn.Prefs)
+
 		wantSimpleUp   bool
 		wantJustEditMP *ipn.MaskedPrefs
 		wantErrSubtr   string
@@ -885,6 +891,28 @@ func TestUpdatePrefs(t *testing.T) {
 			},
 			env: upCheckEnv{backendState: "Running"},
 		},
+		{
+			// Issue 3808: explicitly empty --operator= should clear value.
+			name:  "explicit_empty_operator",
+			flags: []string{"--operator="},
+			curPrefs: &ipn.Prefs{
+				ControlURL:       "https://login.tailscale.com",
+				CorpDNS:          true,
+				AllowSingleHosts: true,
+				NetfilterMode:    preftype.NetfilterOn,
+				OperatorUser:     "somebody",
+			},
+			env: upCheckEnv{user: "somebody", backendState: "Running"},
+			wantJustEditMP: &ipn.MaskedPrefs{
+				OperatorUserSet: true,
+				WantRunningSet:  true,
+			},
+			checkUpdatePrefsMutations: func(t *testing.T, prefs *ipn.Prefs) {
+				if prefs.OperatorUser != "" {
+					t.Errorf("operator sent to backend should be empty; got %q", prefs.OperatorUser)
+				}
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -909,6 +937,9 @@ func TestUpdatePrefs(t *testing.T) {
 				}
 				t.Fatal(err)
 			}
+			if tt.checkUpdatePrefsMutations != nil {
+				tt.checkUpdatePrefsMutations(t, newPrefs)
+			}
 			if simpleUp != tt.wantSimpleUp {
 				t.Fatalf("simpleUp=%v, want %v", simpleUp, tt.wantSimpleUp)
 			}
diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go
@@ -362,7 +362,7 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo
 // without changing any settings.
 func updatePrefs(prefs, curPrefs *ipn.Prefs, env upCheckEnv) (simpleUp bool, justEditMP *ipn.MaskedPrefs, err error) {
 	if !env.upArgs.reset {
-		applyImplicitPrefs(prefs, curPrefs, env.user)
+		applyImplicitPrefs(prefs, curPrefs, env)
 
 		if err := checkForAccidentalSettingReverts(prefs, curPrefs, env); err != nil {
 			return false, nil, err
@@ -881,13 +881,20 @@ func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheck
 	return errors.New(sb.String())
 }
 
-// applyImplicitPrefs mutates prefs to add implicit preferences. Currently
-// this is just the operator user, which only needs to be set if it doesn't
+// applyImplicitPrefs mutates prefs to add implicit preferences for the user operator.
+// If the operator flag is passed no action is taken, otherwise this only needs to be set if it doesn't
 // match the current user.
 //
 // curUser is os.Getenv("USER"). It's pulled out for testability.
-func applyImplicitPrefs(prefs, oldPrefs *ipn.Prefs, curUser string) {
-	if prefs.OperatorUser == "" && oldPrefs.OperatorUser == curUser {
+func applyImplicitPrefs(prefs, oldPrefs *ipn.Prefs, env upCheckEnv) {
+	explicitOperator := false
+	env.flagSet.Visit(func(f *flag.Flag) {
+		if f.Name == "operator" {
+			explicitOperator = true
+		}
+	})
+
+	if prefs.OperatorUser == "" && oldPrefs.OperatorUser == env.user && !explicitOperator {
 		prefs.OperatorUser = oldPrefs.OperatorUser
 	}
 }
