Adds complete ACL coverage for /v1/internal/ui/node endpoints.

consul/acl.go

@@ -445,26 +445,34 @@ func (f *aclFilter) filterNodeDump(dump *structs.NodeDump) {
 	for i := 0; i < len(nd); i++ {
 		info := nd[i]
 
+		// Filter nodes
+		if node := info.Node; !f.allowNode(node) {
+			f.logger.Printf("[DEBUG] consul: dropping node %q from result due to ACLs", node)
+			nd = append(nd[:i], nd[i+1:]...)
+			i--
+			continue
+		}
+
 		// Filter services
-		for i := 0; i < len(info.Services); i++ {
-			svc := info.Services[i].Service
+		for j := 0; j < len(info.Services); j++ {
+			svc := info.Services[j].Service
 			if f.allowService(svc) {
 				continue
 			}
 			f.logger.Printf("[DEBUG] consul: dropping service %q from result due to ACLs", svc)
-			info.Services = append(info.Services[:i], info.Services[i+1:]...)
-			i--
+			info.Services = append(info.Services[:j], info.Services[j+1:]...)
+			j--
 		}
 
 		// Filter checks
-		for i := 0; i < len(info.Checks); i++ {
-			chk := info.Checks[i]
+		for j := 0; j < len(info.Checks); j++ {
+			chk := info.Checks[j]
 			if f.allowService(chk.ServiceName) {
 				continue
 			}
 			f.logger.Printf("[DEBUG] consul: dropping check %q from result due to ACLs", chk.CheckID)
-			info.Checks = append(info.Checks[:i], info.Checks[i+1:]...)
-			i--
+			info.Checks = append(info.Checks[:j], info.Checks[j+1:]...)
+			j--
 		}
 	}
 	*dump = nd

consul/acl_test.go

@@ -1107,7 +1107,7 @@ node "node1" {
 	// Now it should go through.
 	{
 		services := fill()
-		filt := newAclFilter(perms, nil, false)
+		filt := newAclFilter(perms, nil, true)
 		filt.filterNodeServices(&services)
 		if len((*services).Services) != 1 {
 			t.Fatalf("bad: %#v", (*services).Services)
@@ -1262,50 +1262,128 @@ func TestACL_filterCoordinates(t *testing.T) {
 }
 
 func TestACL_filterNodeDump(t *testing.T) {
-	// Create a node dump
-	dump := structs.NodeDump{
-		&structs.NodeInfo{
-			Node: "node1",
-			Services: []*structs.NodeService{
-				&structs.NodeService{
-					ID:      "foo",
-					Service: "foo",
+	// Create a node dump.
+	fill := func() structs.NodeDump {
+		return structs.NodeDump{
+			&structs.NodeInfo{
+				Node: "node1",
+				Services: []*structs.NodeService{
+					&structs.NodeService{
+						ID:      "foo",
+						Service: "foo",
+					},
 				},
-			},
-			Checks: []*structs.HealthCheck{
-				&structs.HealthCheck{
-					Node:        "node1",
-					CheckID:     "check1",
-					ServiceName: "foo",
+				Checks: []*structs.HealthCheck{
+					&structs.HealthCheck{
+						Node:        "node1",
+						CheckID:     "check1",
+						ServiceName: "foo",
+					},
 				},
 			},
-		},
+		}
 	}
 
-	// Try permissive filtering
-	filt := newAclFilter(acl.AllowAll(), nil, false)
-	filt.filterNodeDump(&dump)
-	if len(dump) != 1 {
-		t.Fatalf("bad: %#v", dump)
+	// Try permissive filtering.
+	{
+		dump := fill()
+		filt := newAclFilter(acl.AllowAll(), nil, false)
+		filt.filterNodeDump(&dump)
+		if len(dump) != 1 {
+			t.Fatalf("bad: %#v", dump)
+		}
+		if len(dump[0].Services) != 1 {
+			t.Fatalf("bad: %#v", dump[0].Services)
+		}
+		if len(dump[0].Checks) != 1 {
+			t.Fatalf("bad: %#v", dump[0].Checks)
+		}
+	}
+
+	// Try restrictive filtering.
+	{
+		dump := fill()
+		filt := newAclFilter(acl.DenyAll(), nil, false)
+		filt.filterNodeDump(&dump)
+		if len(dump) != 1 {
+			t.Fatalf("bad: %#v", dump)
+		}
+		if len(dump[0].Services) != 0 {
+			t.Fatalf("bad: %#v", dump[0].Services)
+		}
+		if len(dump[0].Checks) != 0 {
+			t.Fatalf("bad: %#v", dump[0].Checks)
+		}
+	}
+
+	// Allowed to see the service but not the node.
+	policy, err := acl.Parse(`
+service "foo" {
+  policy = "read"
+}
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
 	}
-	if len(dump[0].Services) != 1 {
-		t.Fatalf("bad: %#v", dump[0].Services)
+	perms, err := acl.New(acl.DenyAll(), policy)
+	if err != nil {
+		t.Fatalf("err: %v", err)
 	}
-	if len(dump[0].Checks) != 1 {
-		t.Fatalf("bad: %#v", dump[0].Checks)
+
+	// This will work because version 8 ACLs aren't being enforced.
+	{
+		dump := fill()
+		filt := newAclFilter(perms, nil, false)
+		filt.filterNodeDump(&dump)
+		if len(dump) != 1 {
+			t.Fatalf("bad: %#v", dump)
+		}
+		if len(dump[0].Services) != 1 {
+			t.Fatalf("bad: %#v", dump[0].Services)
+		}
+		if len(dump[0].Checks) != 1 {
+			t.Fatalf("bad: %#v", dump[0].Checks)
+		}
 	}
 
-	// Try restrictive filtering
-	filt = newAclFilter(acl.DenyAll(), nil, false)
-	filt.filterNodeDump(&dump)
-	if len(dump) != 1 {
-		t.Fatalf("bad: %#v", dump)
+	// But with version 8 the node will block it.
+	{
+		dump := fill()
+		filt := newAclFilter(perms, nil, true)
+		filt.filterNodeDump(&dump)
+		if len(dump) != 0 {
+			t.Fatalf("bad: %#v", dump)
+		}
+	}
+
+	// Chain on access to the node.
+	policy, err = acl.Parse(`
+node "node1" {
+  policy = "read"
+}
+`)
+	if err != nil {
+		t.Fatalf("err %v", err)
 	}
-	if len(dump[0].Services) != 0 {
-		t.Fatalf("bad: %#v", dump[0].Services)
+	perms, err = acl.New(perms, policy)
+	if err != nil {
+		t.Fatalf("err: %v", err)
 	}
-	if len(dump[0].Checks) != 0 {
-		t.Fatalf("bad: %#v", dump[0].Checks)
+
+	// Now it should go through.
+	{
+		dump := fill()
+		filt := newAclFilter(perms, nil, true)
+		filt.filterNodeDump(&dump)
+		if len(dump) != 1 {
+			t.Fatalf("bad: %#v", dump)
+		}
+		if len(dump[0].Services) != 1 {
+			t.Fatalf("bad: %#v", dump[0].Services)
+		}
+		if len(dump[0].Checks) != 1 {
+			t.Fatalf("bad: %#v", dump[0].Checks)
+		}
 	}
 }
 

consul/internal_endpoint_test.go

@@ -284,6 +284,12 @@ func TestInternal_NodeInfo_FilterACL(t *testing.T) {
 			t.Fatalf("bad: %#v", info.Services)
 		}
 	}
+
+	// We've already proven that we call the ACL filtering function so we
+	// test node filtering down in acl.go for node cases. This also proves
+	// that we respect the version 8 ACL flag, since the test server sets
+	// that to false (the regression value of *not* changing this is better
+	// for now until we change the sense of the version 8 ACL flag).
 }
 
 func TestInternal_NodeDump_FilterACL(t *testing.T) {
@@ -327,6 +333,12 @@ func TestInternal_NodeDump_FilterACL(t *testing.T) {
 			t.Fatalf("bad: %#v", info.Services)
 		}
 	}
+
+	// We've already proven that we call the ACL filtering function so we
+	// test node filtering down in acl.go for node cases. This also proves
+	// that we respect the version 8 ACL flag, since the test server sets
+	// that to false (the regression value of *not* changing this is better
+	// for now until we change the sense of the version 8 ACL flag).
 }
 
 func TestInternal_EventFire_Token(t *testing.T) {