From 37c00a97af4d00c5bb4c0192c28cde877694ae3e Mon Sep 17 00:00:00 2001
From: William Cai <wcai@teramoby.com>
Date: Wed, 24 Jun 2026 15:19:59 +0800
Subject: [PATCH] fix: deny-all permission and discover cleanup goroutine
 recovery

1. calculatePermissions: a deny permission with empty resource, empty
   expression, AND empty actions means 'deny every action on every
   resource'. The old code built an empty (non-nil) action map that never
   matched, so GetAllGrantedPermissions wrongly reported permissions the
   user did not have. Now it returns no permissions, matching IsAllowed's
   matchResourceAction semantics. Added a test case.

2. etcd discover SaveDiscoverRequest: the fire-and-forget DeleteRequests
   cleanup goroutine had no panic recovery; a panic (e.g. nil client during
   shutdown) would crash the process. Added defer/recover.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 pkg/eval/evaluator_test.go  | 34 ++++++++++++++++++++++++++++++++++
 pkg/eval/evaluator_utils.go |  8 ++++++++
 pkg/store/etcd/discover.go  |  5 +++++
 3 files changed, 47 insertions(+)

diff --git a/pkg/eval/evaluator_test.go b/pkg/eval/evaluator_test.go
index a0a84ad8..2c1745d0 100644
--- a/pkg/eval/evaluator_test.go
+++ b/pkg/eval/evaluator_test.go
@@ -303,6 +303,40 @@ func TestGetPermissions(t *testing.T) {
 			]
 		}
 		]
+	}
+			`,
+			request: adsapi.RequestContext{Subject: &alice, ServiceName: "erp", Attributes: map[string]interface{}{}},
+			results: []pms.Permission{},
+		},
+		{
+			// Deny policy with an explicit permission that has empty resource
+			// AND empty actions = deny everything. All granted permissions
+			// must be removed.
+			stream: `
+			{
+		"services": [
+		{
+			"name": "erp",
+			"policies": [
+			{
+				"id": "policy1",
+				"effect": "grant",
+				"permissions": [
+				{ "resource": "/node1", "actions": ["get","create"] }
+				],
+				"principals": [["user:alice"]]
+			},
+			{
+				"id": "policy2",
+				"effect": "deny",
+				"permissions": [
+				{ "resource": "", "actions": [] }
+				],
+				"principals": [["user:alice"]]
+			}
+			]
+		}
+		]
 	}
 			`,
 			request: adsapi.RequestContext{Subject: &alice, ServiceName: "erp", Attributes: map[string]interface{}{}},
diff --git a/pkg/eval/evaluator_utils.go b/pkg/eval/evaluator_utils.go
index f51046b8..c661e4c2 100644
--- a/pkg/eval/evaluator_utils.go
+++ b/pkg/eval/evaluator_utils.go
@@ -161,6 +161,14 @@ func calculatePermissions(grantedPermissions, deniedPermissions []pms.Permission
 	var denyAllActions map[string]bool
 
 	for _, dp := range deniedPermissions {
+		// A denied permission with no resource and no expression matches every
+		// resource. If it additionally has no actions, it denies ALL actions on
+		// ALL resources -> no permission survives. Return empty immediately,
+		// matching IsAllowed's "empty resource + empty actions = deny all"
+		// semantics in matchResourceAction.
+		if len(dp.Resource) == 0 && len(dp.ResourceExpression) == 0 && len(dp.Actions) == 0 {
+			return nil
+		}
 		actions := make(map[string]bool, len(dp.Actions))
 		for _, a := range dp.Actions {
 			actions[a] = true
diff --git a/pkg/store/etcd/discover.go b/pkg/store/etcd/discover.go
index 35fc4588..243c33f7 100644
--- a/pkg/store/etcd/discover.go
+++ b/pkg/store/etcd/discover.go
@@ -65,6 +65,11 @@ func (s *Store) PutRequest(request *ads.RequestContext) (int64, error) {
 					keys = append(keys, string(kv.Key))
 				}
 				go func() {
+				defer func() {
+					if r := recover(); r != nil {
+						log.Errorf("panic in DeleteRequests cleanup goroutine: %v", r)
+					}
+				}()
 				if err := s.DeleteRequests(keys); err != nil {
 					log.Errorf("failed to delete old discover requests: %v", err)
 				}