From e8a83bb46341ab3f78fea3fb5b4eefc9266f72f9 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 9 Aug 2017 15:06:57 -0700 Subject: [PATCH] =?UTF-8?q?Revert=20"Return=20403=20rather=20than=20a=2040?= =?UTF-8?q?4=20when=20acls=20cause=20all=20results=20to=20be=20filter?= =?UTF-8?q?=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- agent/consul/filter.go | 24 ++++++------------------ agent/consul/filter_test.go | 28 +++++----------------------- agent/consul/kvs_endpoint.go | 13 ++++--------- agent/consul/kvs_endpoint_test.go | 10 ++++++++-- agent/consul/txn_endpoint.go | 10 ++-------- 5 files changed, 25 insertions(+), 60 deletions(-) diff --git a/agent/consul/filter.go b/agent/consul/filter.go index 040aa8ef60..0edad0f269 100644 --- a/agent/consul/filter.go +++ b/agent/consul/filter.go @@ -22,13 +22,9 @@ func (d *dirEntFilter) Move(dst, src, span int) { // FilterDirEnt is used to filter a list of directory entries // by applying an ACL policy -func FilterDirEnt(acl acl.ACL, ent structs.DirEntries) (structs.DirEntries, error) { +func FilterDirEnt(acl acl.ACL, ent structs.DirEntries) structs.DirEntries { df := dirEntFilter{acl: acl, ent: ent} - filtered := ent[:FilterEntries(&df)] - if len(filtered) == 0 { - return nil, errPermissionDenied - } - return filtered, nil + return ent[:FilterEntries(&df)] } type keyFilter struct { @@ -49,13 +45,9 @@ func (k *keyFilter) Move(dst, src, span int) { // FilterKeys is used to filter a list of keys by // applying an ACL policy -func FilterKeys(acl acl.ACL, keys []string) ([]string, error) { +func FilterKeys(acl acl.ACL, keys []string) []string { kf := keyFilter{acl: acl, keys: keys} - filteredKeys := keys[:FilterEntries(&kf)] - if len(filteredKeys) == 0 { - return nil, errPermissionDenied - } - return filteredKeys, nil + return keys[:FilterEntries(&kf)] } type txnResultsFilter struct { @@ -81,13 +73,9 @@ func (t *txnResultsFilter) Move(dst, src, span int) { // FilterTxnResults is used to filter a list of transaction results by // applying an ACL policy. -func FilterTxnResults(acl acl.ACL, results structs.TxnResults) (structs.TxnResults, error) { +func FilterTxnResults(acl acl.ACL, results structs.TxnResults) structs.TxnResults { rf := txnResultsFilter{acl: acl, results: results} - filtered := results[:FilterEntries(&rf)] - if len(filtered) == 0 { - return nil, errPermissionDenied - } - return filtered, nil + return results[:FilterEntries(&rf)] } // Filter interface is used with FilterEntries to do an diff --git a/agent/consul/filter_test.go b/agent/consul/filter_test.go index ac8fcff9f1..52b40e0ad9 100644 --- a/agent/consul/filter_test.go +++ b/agent/consul/filter_test.go @@ -16,7 +16,6 @@ func TestFilter_DirEnt(t *testing.T) { type tcase struct { in []string out []string - err error } cases := []tcase{ tcase{ @@ -26,7 +25,6 @@ func TestFilter_DirEnt(t *testing.T) { tcase{ in: []string{"abe", "lincoln"}, out: nil, - err: errPermissionDenied, }, tcase{ in: []string{"abe", "foo/1", "foo/2", "foo/3", "nope"}, @@ -40,10 +38,7 @@ func TestFilter_DirEnt(t *testing.T) { ents = append(ents, &structs.DirEntry{Key: in}) } - ents, err := FilterDirEnt(aclR, ents) - if err != tc.err { - t.Fatalf("Unexpected error, got %v, wanted %v", err, tc.err) - } + ents = FilterDirEnt(aclR, ents) var outL []string for _, e := range ents { outL = append(outL, e.Key) @@ -63,7 +58,6 @@ func TestFilter_Keys(t *testing.T) { type tcase struct { in []string out []string - err error } cases := []tcase{ tcase{ @@ -72,7 +66,7 @@ func TestFilter_Keys(t *testing.T) { }, tcase{ in: []string{"abe", "lincoln"}, - err: errPermissionDenied, + out: []string{}, }, tcase{ in: []string{"abe", "foo/1", "foo/2", "foo/3", "nope"}, @@ -81,14 +75,10 @@ func TestFilter_Keys(t *testing.T) { } for _, tc := range cases { - out, err := FilterKeys(aclR, tc.in) - if tc.err != err { - t.Fatalf("Unexpected error, got %v, wanted %v", err, tc.err) - } + out := FilterKeys(aclR, tc.in) if !reflect.DeepEqual(out, tc.out) { t.Fatalf("bad: %#v %#v", out, tc.out) } - } } @@ -100,7 +90,6 @@ func TestFilter_TxnResults(t *testing.T) { type tcase struct { in []string out []string - err error } cases := []tcase{ tcase{ @@ -110,7 +99,6 @@ func TestFilter_TxnResults(t *testing.T) { tcase{ in: []string{"abe", "lincoln"}, out: nil, - err: errPermissionDenied, }, tcase{ in: []string{"abe", "foo/1", "foo/2", "foo/3", "nope"}, @@ -124,10 +112,7 @@ func TestFilter_TxnResults(t *testing.T) { results = append(results, &structs.TxnResult{KV: &structs.DirEntry{Key: in}}) } - results, err := FilterTxnResults(aclR, results) - if tc.err != err { - t.Fatalf("Unexpected error, got %v, wanted %v", err, tc.err) - } + results = FilterTxnResults(aclR, results) var outL []string for _, r := range results { outL = append(outL, r.KV.Key) @@ -141,10 +126,7 @@ func TestFilter_TxnResults(t *testing.T) { // Run a non-KV result. results := structs.TxnResults{} results = append(results, &structs.TxnResult{}) - results, err := FilterTxnResults(aclR, results) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + results = FilterTxnResults(aclR, results) if len(results) != 1 { t.Fatalf("should not have filtered non-KV result") } diff --git a/agent/consul/kvs_endpoint.go b/agent/consul/kvs_endpoint.go index ab1d04844c..fd3947738e 100644 --- a/agent/consul/kvs_endpoint.go +++ b/agent/consul/kvs_endpoint.go @@ -129,7 +129,7 @@ func (k *KVS) Get(args *structs.KeyRequest, reply *structs.IndexedDirEntries) er return err } if acl != nil && !acl.KeyRead(args.Key) { - return errPermissionDenied + ent = nil } if ent == nil { // Must provide non-zero index to prevent blocking @@ -168,11 +168,9 @@ func (k *KVS) List(args *structs.KeyRequest, reply *structs.IndexedDirEntries) e return err } if acl != nil { - ent, err = FilterDirEnt(acl, ent) - if err != nil { - return err - } + ent = FilterDirEnt(acl, ent) } + if len(ent) == 0 { // Must provide non-zero index to prevent blocking // Index 1 is impossible anyways (due to Raft internals) @@ -219,10 +217,7 @@ func (k *KVS) ListKeys(args *structs.KeyListRequest, reply *structs.IndexedKeyLi } if acl != nil { - keys, err = FilterKeys(acl, keys) - if err != nil { - return err - } + keys = FilterKeys(acl, keys) } reply.Keys = keys return nil diff --git a/agent/consul/kvs_endpoint_test.go b/agent/consul/kvs_endpoint_test.go index de668e3168..271866c256 100644 --- a/agent/consul/kvs_endpoint_test.go +++ b/agent/consul/kvs_endpoint_test.go @@ -206,7 +206,7 @@ func TestKVS_Get_ACLDeny(t *testing.T) { } var out bool if err := msgpackrpc.CallWithCodec(codec, "KVS.Apply", &arg, &out); err != nil { - t.Fatalf("Unexpected err: %v", err) + t.Fatalf("err: %v", err) } getR := structs.KeyRequest{ @@ -214,10 +214,16 @@ func TestKVS_Get_ACLDeny(t *testing.T) { Key: "zip", } var dirent structs.IndexedDirEntries - if err := msgpackrpc.CallWithCodec(codec, "KVS.Get", &getR, &dirent); err.Error() != errPermissionDenied.Error() { + if err := msgpackrpc.CallWithCodec(codec, "KVS.Get", &getR, &dirent); err != nil { t.Fatalf("err: %v", err) } + if dirent.Index == 0 { + t.Fatalf("Bad: %v", dirent) + } + if len(dirent.Entries) != 0 { + t.Fatalf("Bad: %v", dirent) + } } func TestKVSEndpoint_List(t *testing.T) { diff --git a/agent/consul/txn_endpoint.go b/agent/consul/txn_endpoint.go index 9800bd5335..fc2862e6ca 100644 --- a/agent/consul/txn_endpoint.go +++ b/agent/consul/txn_endpoint.go @@ -72,10 +72,7 @@ func (t *Txn) Apply(args *structs.TxnRequest, reply *structs.TxnResponse) error // just taking the two slices. if txnResp, ok := resp.(structs.TxnResponse); ok { if acl != nil { - txnResp.Results, err = FilterTxnResults(acl, txnResp.Results) - if err != nil { - return err - } + txnResp.Results = FilterTxnResults(acl, txnResp.Results) } *reply = txnResp } else { @@ -116,10 +113,7 @@ func (t *Txn) Read(args *structs.TxnReadRequest, reply *structs.TxnReadResponse) state := t.srv.fsm.State() reply.Results, reply.Errors = state.TxnRO(args.Ops) if acl != nil { - reply.Results, err = FilterTxnResults(acl, reply.Results) - if err != nil { - return err - } + reply.Results = FilterTxnResults(acl, reply.Results) } return nil }