remove default for -method

command/kv_patch.go

@@ -91,9 +91,8 @@ func (c *KVPatchCommand) Flags() *FlagSets {
 	})
 
 	f.StringVar(&StringVar{
-		Name:    "method",
-		Target:  &c.flagMethod,
-		Default: "patch",
+		Name:   "method",
+		Target: &c.flagMethod,
 		Usage: `Specifies which method of patching to use. If set to "patch", then
 		an HTTP PATCH request will be issued. This is the default. If set to "rw",
 		then a read will be performed, then a local update, followed by a remote
@@ -175,7 +174,9 @@ func (c *KVPatchCommand) Run(args []string) int {
 	case "rw":
 		secret, code = c.readThenWrite(client, path, newData)
 	case "patch":
-		secret, code = c.mergePatch(client, path, newData)
+		secret, code = c.mergePatch(client, path, newData, false)
+	case "":
+		secret, code = c.mergePatch(client, path, newData, true)
 	default:
 		c.UI.Error(fmt.Sprintf("Unsupported method provided to -method flag: %s", c.flagMethod))
 		return 2
@@ -269,7 +270,7 @@ func (c *KVPatchCommand) readThenWrite(client *api.Client, path string, newData
 	return secret, 0
 }
 
-func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map[string]interface{}) (*api.Secret, int) {
+func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map[string]interface{}, rwFallback bool) (*api.Secret, int) {
 	data := map[string]interface{}{
 		"data":    newData,
 		"options": map[string]interface{}{},
@@ -282,8 +283,8 @@ func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map
 	secret, err := client.Logical().JSONMergePatch(context.Background(), path, data)
 	if err != nil {
 		// If it's a 403, that probably means they don't have the patch capability in their policy. Fall back to
-		// the old way of doing it.
-		if re, ok := err.(*api.ResponseError); ok && re.StatusCode == 403 {
+		// the old way of doing it if the user didn't specify a -method. If they did, and it was "patch", then just error.
+		if re, ok := err.(*api.ResponseError); ok && re.StatusCode == 403 && rwFallback {
 			c.UI.Warn(fmt.Sprintf("Data was written to %s but we recommend that you add the \"patch\" capability to your ACL policy in order to use HTTP PATCH in the future.", path))
 			return c.readThenWrite(client, path, newData)
 		}

command/kv_test.go

@@ -980,48 +980,75 @@ func TestKVPatchCommand_Methods(t *testing.T) {
 }
 
 func TestKVPatchCommand_403Fallback(t *testing.T) {
-	t.Parallel()
-	client, closer := testVaultServer(t)
-	defer closer()
-
-	if err := client.Sys().Mount("kv/", &api.MountInput{
-		Type: "kv-v2",
-	}); err != nil {
-		t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err)
+	cases := []struct {
+		name     string
+		args     []string
+		expected string
+		code     int
+	}{
+		// if no -method is specified, and patch fails, it should fall back to rw and succeed
+		{
+			"unspecified",
+			[]string{"kv/foo", "bar=quux"},
+			`add the "patch" capability to your ACL policy`,
+			0,
+		},
+		// if -method=patch is specified, and patch fails, it should not fall back, and just error
+		{
+			"specifying patch",
+			[]string{"-method", "patch", "kv/foo", "bar=quux"},
+			"permission denied",
+			2,
+		},
 	}
 
-	// create a policy without patch capability
-	policy := `path "kv/*" { capabilities = ["create", "update", "read"] }`
-	secretAuth, err := createTokenForPolicy(t, client, policy)
-	if err != nil {
-		t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err)
-	}
+	for _, tc := range cases {
+		tc := tc
 
-	kvClient, err := client.Clone()
-	if err != nil {
-		t.Fatal(err)
-	}
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			client, closer := testVaultServer(t)
+			defer closer()
+
+			if err := client.Sys().Mount("kv/", &api.MountInput{
+				Type: "kv-v2",
+			}); err != nil {
+				t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err)
+			}
 
-	kvClient.SetToken(secretAuth.ClientToken)
+			// create a policy without patch capability
+			policy := `path "kv/*" { capabilities = ["create", "update", "read"] }`
+			secretAuth, err := createTokenForPolicy(t, client, policy)
+			if err != nil {
+				t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err)
+			}
 
-	// Write a value then attempt to patch it. It should fail, then fall back to the old method
-	_, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}})
-	if err != nil {
-		t.Fatal(err)
-	}
+			kvClient, err := client.Clone()
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	ui, cmd := testKVPatchCommand(t)
-	cmd.client = kvClient
-	code := cmd.Run([]string{"kv/foo", "bar=quux"})
+			kvClient.SetToken(secretAuth.ClientToken)
 
-	if code != 0 {
-		t.Fatalf("expected code to be 0 but was %d", code)
-	}
+			// Write a value then attempt to patch it
+			_, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}})
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	combined := ui.OutputWriter.String() + ui.ErrorWriter.String()
-	expected := `add the "patch" capability to your ACL policy`
-	if !strings.Contains(combined, expected) {
-		t.Errorf("expected %q to contain %q", combined, expected)
+			ui, cmd := testKVPatchCommand(t)
+			cmd.client = kvClient
+			code := cmd.Run(tc.args)
+
+			if code != tc.code {
+				t.Fatalf("expected code to be %d but was %d", tc.code, code)
+			}
+
+			combined := ui.OutputWriter.String() + ui.ErrorWriter.String()
+			if !strings.Contains(combined, tc.expected) {
+				t.Errorf("expected %q to contain %q", combined, tc.expected)
+			}
+		})
 	}
 }