Repository: cerbos/cerbos
Pull request #1256 (2022-09-30 09:07:03)

fix: Treat non-boolean expression results as `false`
When evaluating `any`, `all` or `none` blocks, if one of the expressions
returns a non-boolean result, the whole block is marked as having
failed. We can be a bit more lenient here and treat non-boolean values
as `false`. Note that this treats _all_ values as `false` and does not
make the distinction of treating certain values as `true`. Users are
responsible for converting any non-boolean values into boolean values
themselves.

Signed-off-by: Charith Ellawala <charith@cerbos.dev>


codecov[bot]: # [Codecov](https://codecov.io/gh/cerbos/cerbos/pull/1256?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos) Report
> Merging [#1256](https://codecov.io/gh/cerbos/cerbos/pull/1256?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos) (44d610c) into [main](https://codecov.io/gh/cerbos/cerbos/commit/8565255dbd51b8b26c61fc25fa3be73eb1e714d0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos) (8565255) will **increase** coverage by `5.83%`.
> The diff coverage is `100.00%`.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://codecov.io/gh/cerbos/cerbos/pull/1256/graphs/tree.svg?width=650&height=150&src=pr&token=tGaxiUZUzL&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos)](https://codecov.io/gh/cerbos/cerbos/pull/1256?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos)

```diff
@@            Coverage Diff             @@
##             main    #1256      +/-   ##
==========================================
+ Coverage   55.98%   61.81%   +5.83%     
==========================================
  Files         109       32      -77     
  Lines       12375     5385    -6990     
==========================================
- Hits         6928     3329    -3599     
+ Misses       4885     1780    -3105     
+ Partials      562      276     -286     
```


| [Impacted Files](https://codecov.io/gh/cerbos/cerbos/pull/1256?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos) | Coverage Δ | |
|---|---|---|
| [internal/engine/evaluator.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-aW50ZXJuYWwvZW5naW5lL2V2YWx1YXRvci5nbw==) | `79.15% <100.00%> (-0.66%)` | :arrow_down: |
| [cmd/cerbosctl/get/internal/policy/sort.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-Y21kL2NlcmJvc2N0bC9nZXQvaW50ZXJuYWwvcG9saWN5L3NvcnQuZ28=) | | |
| [internal/storage/blob/store.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-aW50ZXJuYWwvc3RvcmFnZS9ibG9iL3N0b3JlLmdv) | | |
| [internal/config/config.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-aW50ZXJuYWwvY29uZmlnL2NvbmZpZy5nbw==) | | |
| [internal/storage/db/internal/connpool.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-aW50ZXJuYWwvc3RvcmFnZS9kYi9pbnRlcm5hbC9jb25ucG9vbC5nbw==) | | |
| [internal/storage/git/conf.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-aW50ZXJuYWwvc3RvcmFnZS9naXQvY29uZi5nbw==) | | |
| [internal/verify/verify.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-aW50ZXJuYWwvdmVyaWZ5L3ZlcmlmeS5nbw==) | | |
| [internal/storage/blob/fs.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-aW50ZXJuYWwvc3RvcmFnZS9ibG9iL2ZzLmdv) | | |
| [cmd/cerbos/repl/internal/eval.go](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos#diff-Y21kL2NlcmJvcy9yZXBsL2ludGVybmFsL2V2YWwuZ28=) | | |
| ... and [70 more](https://codecov.io/gh/cerbos/cerbos/pull/1256/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cerbos) | |

</details>

Patch:
diff --git a/cmd/cerbos/run/run.go b/cmd/cerbos/run/run.go
index 92ea3dd0..bc498460 100644
--- a/cmd/cerbos/run/run.go
+++ b/cmd/cerbos/run/run.go
@@ -75,7 +75,10 @@ type Cmd struct {
 }
 
 func (c *Cmd) Run(k *kong.Kong) error {
-	logging.InitLogging(c.LogLevel)
+	notifyCtx, stopFunc := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	defer stopFunc()
+
+	logging.InitLogging(notifyCtx, c.LogLevel)
 	defer zap.L().Sync() //nolint:errcheck
 
 	log := zap.S().Named("run")
@@ -83,9 +86,6 @@ func (c *Cmd) Run(k *kong.Kong) error {
 	undo, _ := maxprocs.Set(maxprocs.Logger(log.Infof))
 	defer undo()
 
-	notifyCtx, stopFunc := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
-	defer stopFunc()
-
 	if err := c.loadConfig(); err != nil {
 		log.Errorw("Failed to load configuration", "error", err)
 		return err
diff --git a/cmd/cerbos/server/server.go b/cmd/cerbos/server/server.go
index 1a36094e..56ad8fb9 100644
--- a/cmd/cerbos/server/server.go
+++ b/cmd/cerbos/server/server.go
@@ -55,7 +55,10 @@ type Cmd struct {
 }
 
 func (c *Cmd) Run() error {
-	logging.InitLogging(string(c.LogLevel))
+	ctx, stopFunc := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	defer stopFunc()
+
+	logging.InitLogging(ctx, string(c.LogLevel))
 	defer zap.L().Sync() //nolint:errcheck
 
 	log := zap.S().Named("server")
@@ -72,9 +75,6 @@ func (c *Cmd) Run() error {
 		defer agent.Close()
 	}
 
-	ctx, stopFunc := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
-	defer stopFunc()
-
 	// load any config overrides
 	confOverrides := map[string]any{}
 	for _, override := range c.Set {
diff --git a/internal/engine/evaluator.go b/internal/engine/evaluator.go
index 94e31407..2b7d0e38 100644
--- a/internal/engine/evaluator.go
+++ b/internal/engine/evaluator.go
@@ -362,7 +362,7 @@ func (ep evalParams) evaluateBoolCELExpr(expr *exprpb.CheckedExpr, variables map
 
 	boolVal, ok := val.(bool)
 	if !ok {
-		return false, fmt.Errorf("unexpected result: wanted bool, got %T", val)
+		return false, nil
 	}
 
 	return boolVal, nil
diff --git a/internal/observability/logging/logging.go b/internal/observability/logging/logging.go
index 10530cc7..1b625299 100644
--- a/internal/observability/logging/logging.go
+++ b/internal/observability/logging/logging.go
@@ -6,7 +6,11 @@ package logging
 import (
 	"context"
 	"os"
+	"os/signal"
 	"strings"
+	"sync/atomic"
+	"syscall"
+	"time"
 
 	grpc_zap "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap"
 	"github.com/mattn/go-isatty"
@@ -17,29 +21,26 @@ import (
 	"github.com/cerbos/cerbos/internal/util"
 )
 
+const tmpLogLevelDuration = 10 * time.Minute
+
 type ctxLog struct{}
 
 var ctxLogKey = &ctxLog{}
 
 // InitLogging initializes the global logger.
-func InitLogging(level string) {
+func InitLogging(ctx context.Context, level string) {
 	if envLevel := os.Getenv("CERBOS_LOG_LEVEL"); envLevel != "" {
-		doInitLogging(envLevel)
+		doInitLogging(ctx, envLevel)
 		return
 	}
 
-	doInitLogging(level)
+	doInitLogging(ctx, level)
 }
 
-func doInitLogging(level string) {
+func doInitLogging(ctx context.Context, level string) {
 	var logger *zap.Logger
 
-	errorPriority := zap.LevelEnablerFunc(func(lvl zapcore.Level) bool {
-		return lvl >= zapcore.ErrorLevel
-	})
-
 	minLogLevel := zapcore.InfoLevel
-
 	switch strings.ToUpper(level) {
 	case "DEBUG":
 		minLogLevel = zapcore.DebugLevel
@@ -51,13 +52,6 @@ func doInitLogging(level string) {
 		minLogLevel = zapcore.ErrorLevel
 	}
 
-	infoPriority := zap.LevelEnablerFunc(func(lvl zapcore.Level) bool {
-		return lvl < zapcore.ErrorLevel && lvl >= minLogLevel
-	})
-
-	consoleErrors := zapcore.Lock(os.Stderr)
-	consoleInfo := zapcore.Lock(os.Stdout)
-
 	encoderConf := ecszap.NewDefaultEncoderConfig().ToZapCoreEncoderConfig()
 	var consoleEncoder zapcore.Encoder
 
@@ -68,6 +62,18 @@ func doInitLogging(level string) {
 		consoleEncoder = zapcore.NewConsoleEncoder(encoderConf)
 	}
 
+	consoleErrors := zapcore.Lock(os.Stderr)
+	consoleInfo := zapcore.Lock(os.Stdout)
+	atomicLevel := zap.NewAtomicLevelAt(minLogLevel)
+
+	errorPriority := zap.LevelEnablerFunc(func(lvl zapcore.Level) bool {
+		return atomicLevel.Enabled(lvl) && lvl >= zapcore.ErrorLevel
+	})
+
+	infoPriority := zap.LevelEnablerFunc(func(lvl zapcore.Level) bool {
+		return atomicLevel.Enabled(lvl) && lvl < zapcore.ErrorLevel
+	})
+
 	core := zapcore.NewTee(
 		zapcore.NewCore(consoleEncoder, consoleErrors, errorPriority),
 		zapcore.NewCore(consoleEncoder, consoleInfo, infoPriority),
@@ -86,6 +92,44 @@ func doInitLogging(level string) {
 			return lvl > zapcore.ErrorLevel
 		}))),
 	)
+
+	handleUSR1Signal(ctx, minLogLevel, &atomicLevel)
+}
+
+// setLevelForDuration sets the global log level to the given level for a given duration. Reverts to the original
+// level after the duration.
+func setLevelForDuration(level, originalLevel zapcore.Level, duration time.Duration, inProgress *atomic.Bool, atomicLevel *zap.AtomicLevel) {
+	log := zap.S().Named("logging")
+
+	log.Infof("Temporarily setting global log level to %s for %s", level, duration)
+	atomicLevel.SetLevel(level)
+
+	time.AfterFunc(duration, func() {
+		log.Infof("Reverting global log level to %s", originalLevel)
+		atomicLevel.SetLevel(originalLevel)
+		inProgress.Store(false)
+	})
+}
+
+// handleUSR1Signal sets the log level to zapcore.DebugLevel for some duration in case syscall.SIGUSR1 received.
+func handleUSR1Signal(ctx context.Context, originalLevel zapcore.Level, atomicLevel *zap.AtomicLevel) {
+	sigusr1 := make(chan os.Signal, 1)
+	signal.Notify(sigusr1, syscall.SIGUSR1)
+
+	go func() {
+		inProgress := atomic.Bool{}
+		for {
+			select {
+			case <-ctx.Done():
+				return
+			case <-sigusr1:
+				if !inProgress.Load() {
+					inProgress.Store(true)
+					setLevelForDuration(zapcore.DebugLevel, originalLevel, tmpLogLevelDuration, &inProgress, atomicLevel)
+				}
+			}
+		}
+	}()
 }
 
 // FromContext returns the logger from the context if one exists. Otherwise it returns a new logger.


Test patch:
diff --git a/internal/test/test.go b/internal/test/test.go
index 960be9bf..b584507d 100644
--- a/internal/test/test.go
+++ b/internal/test/test.go
@@ -28,7 +28,7 @@ import (
 
 func init() {
 	if logLevel := os.Getenv("CERBOS_TEST_LOG_LEVEL"); logLevel != "" {
-		logging.InitLogging(logLevel)
+		logging.InitLogging(context.Background(), logLevel)
 	}
 }
 
diff --git a/internal/test/testdata/cel_eval/null_attr_allof.yaml b/internal/test/testdata/cel_eval/null_attr_allof.yaml
new file mode 100644
index 00000000..4dc6f1bc
--- /dev/null
+++ b/internal/test/testdata/cel_eval/null_attr_allof.yaml
@@ -0,0 +1,21 @@
+---
+condition:
+  all:
+    of:
+      - expr: P.attr.x
+      - expr: R.attr.department == "marketing"
+input: {
+  "requestId": "test",
+  "actions": ["*"],
+  "principal": {
+    "id": "john",
+    "roles": ["employee"]
+  },
+  "resource": {
+    "kind": "leave_request",
+    "attr": {
+      "department": "marketing"
+    }
+  }
+}
+want: false
diff --git a/internal/test/testdata/cel_eval/null_attr_anyof.yaml b/internal/test/testdata/cel_eval/null_attr_anyof.yaml
new file mode 100644
index 00000000..df6d8673
--- /dev/null
+++ b/internal/test/testdata/cel_eval/null_attr_anyof.yaml
@@ -0,0 +1,21 @@
+---
+condition:
+  any:
+    of:
+      - expr: P.attr.x
+      - expr: R.attr.department == "marketing"
+input: {
+  "requestId": "test",
+  "actions": ["*"],
+  "principal": {
+    "id": "john",
+    "roles": ["employee"]
+  },
+  "resource": {
+    "kind": "leave_request",
+    "attr": {
+      "department": "marketing"
+    }
+  }
+}
+want: true
diff --git a/internal/test/testdata/cel_eval/null_attr_noneof.yaml b/internal/test/testdata/cel_eval/null_attr_noneof.yaml
new file mode 100644
index 00000000..41e71579
--- /dev/null
+++ b/internal/test/testdata/cel_eval/null_attr_noneof.yaml
@@ -0,0 +1,21 @@
+---
+condition:
+  none:
+    of:
+      - expr: P.attr.x
+      - expr: R.attr.department == "engineering"
+input: {
+  "requestId": "test",
+  "actions": ["*"],
+  "principal": {
+    "id": "john",
+    "roles": ["employee"]
+  },
+  "resource": {
+    "kind": "leave_request",
+    "attr": {
+      "department": "marketing"
+    }
+  }
+}
+want: true