Repository: avenga/couper
Pull request #484 (2022-04-13 13:33:37)

Permissions mixed config
Check for required_permission "mixed config"

---
<details>
    <summary>Reviewer checklist</summary>
    <ul>
        <li>Read PR description: a summary about the changes is required</li>
        <li>Changelog updated</li>
        <li>Documentation: docs/{Reference, Cli, ...}, Docker and cli help/usage</li>
        <li>Pulled branch, manually tested</li>
        <li>Verified requirements are met</li>
        <li>Reviewed the code</li>
        <li>Reviewed the related tests</li>
    </ul>
</details>



Patch:
diff --git a/config/configload/load.go b/config/configload/load.go
index bb54a767..55e83a34 100644
--- a/config/configload/load.go
+++ b/config/configload/load.go
@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/hcl/v2/gohcl"
 	"github.com/hashicorp/hcl/v2/hclparse"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
+	"github.com/zclconf/go-cty/cty"
 
 	"github.com/avenga/couper/config"
 	"github.com/avenga/couper/config/configload/collect"
@@ -363,6 +364,11 @@ func LoadConfig(body hcl.Body, src []byte, filename, dirPath string) (*config.Co
 				}
 			}
 
+			err = checkPermissionMixedConfig(apiConfig)
+			if err != nil {
+				return nil, err
+			}
+
 			err = refineEndpoints(helper, apiConfig.Endpoints, true)
 			if err != nil {
 				return nil, err
@@ -392,3 +398,44 @@ func LoadConfig(body hcl.Body, src []byte, filename, dirPath string) (*config.Co
 
 	return helper.config, nil
 }
+
+// checkPermissionMixedConfig checks whether, for api blocks with at least two endpoints,
+// all endpoints in api have either
+// a) no required permission set or
+// b) required permission or disable_access_control set
+func checkPermissionMixedConfig(apiConfig *config.API) error {
+	if apiConfig.RequiredPermission != cty.NilVal {
+		// default for required permission: no mixed config
+		return nil
+	}
+
+	l := len(apiConfig.Endpoints)
+	if l < 2 {
+		// too few endpoints: no mixed config
+		return nil
+	}
+
+	countEpsWithPermission := 0
+	countEpsWithPermissionOrDisableAC := 0
+	for _, e := range apiConfig.Endpoints {
+		if e.RequiredPermission != cty.NilVal {
+			// endpoint has required permission attribute set
+			countEpsWithPermission++
+			countEpsWithPermissionOrDisableAC++
+		} else if e.DisableAccessControl != nil {
+			// endpoint has didable AC attribute set
+			countEpsWithPermissionOrDisableAC++
+		}
+	}
+
+	if countEpsWithPermission == 0 {
+		// no endpoints with required permission: no mixed config
+		return nil
+	}
+
+	if l > countEpsWithPermissionOrDisableAC {
+		return errors.Configuration.Messagef("api with label %q has endpoint without required permission", apiConfig.Name)
+	}
+
+	return nil
+}
diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md
index 5e888f3b..93b25633 100644
--- a/docs/REFERENCE.md
+++ b/docs/REFERENCE.md
@@ -140,7 +140,7 @@ produce an explicit or implicit client response.
 | `path`                  | string           | -       | Changeable part of the upstream URL. Changes the path suffix of the outgoing request.                             | -                                                                                                                                                                                                                                                                                                                                                                                                                                               | -                                                                    |
 | `access_control`        | list             | -       | Sets predefined [Access Control](#access-control) for `endpoint` block context.                                   | -                                                                                                                                                                                                                                                                                                                                                                                                                                               | `access_control = ["foo"]`                                           |
 | `allowed_methods`       | tuple of string  | `["*"]` == `["GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"]` | Sets allowed methods _overriding_ a default set in the containing `api` block. Requests with a method that is not allowed result in an error response with a `405 Method Not Allowed` status. | The default value `*` can be combined with additional methods. Methods are matched case-insensitively. `Access-Control-Allow-Methods` is only sent in response to a [CORS](#cors-block) preflight request, if the method requested by `Access-Control-Request-Method` is an allowed method. | `allowed_methods = ["GET", "POST"]` or `allowed_methods = ["*", "BREW"]` |
-| `beta_required_permission` | string or object | -    | Permission required to use this endpoint (see [error type](ERRORS.md#error-types) `beta_insufficient_permissions`).| Overrides `beta_required_permission` in a containing `api` block. If the value is a string, the same permission applies to all request methods. If there are different permissions for different request methods, use an object with the request methods as keys and string values. Methods not specified in this object are not permitted. `"*"` is the key for "all other standard methods". Methods other than `GET`, `HEAD`, `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS` must be specified explicitly. A value `""` means "no permission required". | `beta_required_permission = "read"` or `beta_required_permission = { post = "write", "*" = "" }` |
+| `beta_required_permission` | string or object | -    | Permission required to use this endpoint (see [error type](ERRORS.md#error-types) `beta_insufficient_permissions`).| Overrides `beta_required_permission` in a containing `api` block. If the value is a string, the same permission applies to all request methods. If there are different permissions for different request methods, use an object with the request methods as keys and string values. Methods not specified in this object are not permitted. `"*"` is the key for "all other standard methods". Methods other than `GET`, `HEAD`, `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS` must be specified explicitly. A value `""` means "no permission required". For `api` blocks with at least two `endpoint`s, all endpoints must have either a) no `beta_required_permission` set or b) either `beta_required_permission` or `disable_access_control` set. Otherwise, a configuration error is thrown. | `beta_required_permission = "read"` or `beta_required_permission = { post = "write", "*" = "" }` |
 | `custom_log_fields`     | map              | -       | Defines log fields for [Custom Logging](LOGS.md#custom-logging).                                                  | &#9888; Inherited by nested blocks.                                                                                                                                                                                                                                                                                                                                                                                                             | -                                                                    |
 | [Modifiers](#modifiers) | -                | -       | -                                                                                                                 | -                                                                                                                                                                                                                                                                                                                                                                                                                                               | -                                                                    |
 


Test patch:
diff --git a/config/configload/validate_test.go b/config/configload/validate_test.go
index 3108b34d..f5fda3cf 100644
--- a/config/configload/validate_test.go
+++ b/config/configload/validate_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/avenga/couper/cache"
 	"github.com/avenga/couper/config/runtime"
+	"github.com/avenga/couper/errors"
 	logrustest "github.com/sirupsen/logrus/hooks/test"
 )
 
@@ -441,3 +442,117 @@ defaults {
 		})
 	}
 }
+
+func TestPermissionMixed(t *testing.T) {
+	tests := []struct {
+		name  string
+		hcl   string
+		error string
+	}{
+		{
+			"mixed config: error",
+			`server {
+  api "foo" {
+    endpoint "/a" {
+      beta_required_permission = "a"
+      response {}
+    }
+    endpoint "/b" {
+      response {}
+    }
+  }
+}`,
+			"configuration error: api with label \"foo\" has endpoint without required permission",
+		},
+		{
+			"no mix: all endpoints with permission set",
+			`server {
+  api "foo" {
+    endpoint "/a" {
+      beta_required_permission = "a"
+      response {}
+    }
+    endpoint "/b" {
+      beta_required_permission = ""
+      response {}
+    }
+  }
+}`,
+			"",
+		},
+		{
+			"no mix: permission set by api",
+			`server {
+  api "foo" {
+    beta_required_permission = "api"
+    endpoint "/a" {
+      beta_required_permission = "a"
+      response {}
+    }
+    endpoint "/b" {
+      response {}
+    }
+  }
+}`,
+			"",
+		},
+		{
+			"no mix: disable_access_control",
+			`server {
+  api "foo" {
+    endpoint "/a" {
+      beta_required_permission = "a"
+      response {}
+    }
+    endpoint "/b" {
+      disable_access_control = ["foo"]
+      response {}
+    }
+  }
+}`,
+			"",
+		},
+		{
+			"no mix: separate apis",
+			`server {
+  api "foo" {
+    endpoint "/a" {
+      beta_required_permission = "a"
+      response {}
+    }
+  }
+  api "bar" {
+    endpoint "/b" {
+      response {}
+    }
+  }
+}`,
+			"",
+		},
+	}
+
+	logger, _ := logrustest.NewNullLogger()
+	log := logger.WithContext(context.TODO())
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(subT *testing.T) {
+			conf, err := LoadBytes([]byte(tt.hcl), "couper.hcl")
+			if conf != nil {
+				tmpStoreCh := make(chan struct{})
+				defer close(tmpStoreCh)
+
+				_, err = runtime.NewServerConfiguration(conf, log, cache.New(log, tmpStoreCh))
+			}
+
+			var errMsg string
+			if err != nil {
+				gErr, _ := err.(errors.GoError)
+				errMsg = gErr.LogError()
+			}
+
+			if tt.error != errMsg {
+				subT.Errorf("%q: Unexpected configuration error:\n\tWant: %q\n\tGot:  %q", tt.name, tt.error, errMsg)
+			}
+		})
+	}
+}
diff --git a/server/testdata/integration/config/11_couper.hcl b/server/testdata/integration/config/11_couper.hcl
index c3c02e5e..830ebff3 100644
--- a/server/testdata/integration/config/11_couper.hcl
+++ b/server/testdata/integration/config/11_couper.hcl
@@ -59,6 +59,7 @@ server {
 
     endpoint "/wildcard" {
       allowed_methods = ["*"]
+      beta_required_permission = ""
 
       response {
         body = "a"
@@ -76,6 +77,7 @@ server {
 
     endpoint "/blocked" {
       allowed_methods = []
+      disable_access_control = ["token"]
 
       response {
         body = "a"
diff --git a/server/testdata/integration/logs/01_couper.hcl b/server/testdata/integration/logs/01_couper.hcl
index a7d0e5de..f5c1a875 100644
--- a/server/testdata/integration/logs/01_couper.hcl
+++ b/server/testdata/integration/logs/01_couper.hcl
@@ -150,7 +150,9 @@ server "logs" {
         status = 204
       }
     }
+  }
 
+  api {
     endpoint "/error-handler/endpoint" {
       access_control = ["JWT"]