From d1db407c720183632750eeae806882e39a3970ee Mon Sep 17 00:00:00 2001
From: Airat Arifullin <aarifullin@yadro.com>
Date: Fri, 21 Feb 2025 16:47:33 +0300
Subject: [PATCH] [#83] router: Ignore local overrides with `Allow` status

* If a target owner sets some private rules into morph, then the
  access may be intentionally/unintentionally violated by setting
  a local override that allows any operation;
* The same happens if no morph rule is found;
* Fix unit-test `TestInmemory_MultipleTargets`;
* Add unit-test `TestLocalOverrideAllowRule`.

Signed-off-by: Airat Arifullin <aarifullin@yadro.com>
---
 pkg/engine/chain_router.go           |  6 ++-
 pkg/engine/inmemory/inmemory_test.go | 75 +++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/pkg/engine/chain_router.go b/pkg/engine/chain_router.go
index 3eced4a..5e55782 100644
--- a/pkg/engine/chain_router.go
+++ b/pkg/engine/chain_router.go
@@ -28,9 +28,13 @@ func (dr *defaultChainRouter) IsAllowed(name chain.Name, rt RequestTarget, r res
 	status, ruleFound, err = dr.checkLocal(name, rt, r)
 	if err != nil {
 		return chain.NoRuleFound, false, err
-	} else if ruleFound {
+	} else if ruleFound && status != chain.Allow {
 		// The local overrides have the highest priority and thus
 		// morph rules are not considered if a local one is found.
+		//
+		// Any override with the "Allow" status is ignored in order
+		// to prevent access violations by bypassing the denying rules
+		// set for the target.
 		return
 	}
 
diff --git a/pkg/engine/inmemory/inmemory_test.go b/pkg/engine/inmemory/inmemory_test.go
index 30bd7c3..6eebdee 100644
--- a/pkg/engine/inmemory/inmemory_test.go
+++ b/pkg/engine/inmemory/inmemory_test.go
@@ -7,6 +7,7 @@ import (
 	"git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
 	"git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine"
 	resourcetest "git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource/testutil"
+	nativeschema "git.frostfs.info/TrueCloudLab/policy-engine/schema/native"
 	"github.com/nspcc-dev/neo-go/pkg/vm/stackitem"
 	"github.com/stretchr/testify/require"
 )
@@ -31,6 +32,76 @@ func TestAddRootOverrides(t *testing.T) {
 	require.Equal(t, string(id), string(res[0].ID))
 }
 
+func TestLocalOverrideAllowRule(t *testing.T) {
+	const (
+		namespace = "root"
+		object    = "native:object//73tQMTYyUkTgmvPR1HWib6pndbhSoBovbnMF7Pws8Rcy/BzQw5HH3feoxFDD5tCT87Y1726qzgLfxEE7wgtoRzB3R"
+		actor1    = "owner1"
+	)
+
+	s := NewInMemoryLocalOverrides()
+
+	res := resourcetest.NewResource(object, map[string]string{"FromS3": "true"})
+
+	reqBadIP := resourcetest.NewRequest(nativeschema.MethodPutObject, res, map[string]string{
+		"SourceIP": "10.2.1.12",
+		"Actor":    actor1,
+	})
+
+	s.LocalStorage().AddOverride(chain.Ingress, engine.NamespaceTarget(namespace), &chain.Chain{
+		Rules: []chain.Rule{
+			{ // This allowing rule shouldn't override denying rules from morph storage.
+				Status:    chain.Allow,
+				Actions:   chain.Actions{Names: []string{nativeschema.MethodPutObject}},
+				Resources: chain.Resources{Names: []string{nativeschema.ResourceFormatRootObjects}},
+				Any:       true,
+				Condition: []chain.Condition{
+					{
+						Op:    chain.CondStringNotLike,
+						Kind:  chain.KindRequest,
+						Key:   "SourceIP",
+						Value: "10.1.1.*",
+					},
+				},
+			},
+		},
+	})
+
+	s.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(namespace), &chain.Chain{
+		Rules: []chain.Rule{
+			{
+				Status:    chain.AccessDenied,
+				Actions:   chain.Actions{Names: []string{nativeschema.MethodDeleteObject}},
+				Resources: chain.Resources{Names: []string{nativeschema.ResourceFormatRootObjects}},
+			},
+			{
+				Status:    chain.AccessDenied,
+				Actions:   chain.Actions{Names: []string{nativeschema.MethodPutObject}},
+				Resources: chain.Resources{Names: []string{nativeschema.ResourceFormatRootObjects}},
+				Any:       true,
+				Condition: []chain.Condition{
+					{
+						Op:    chain.CondStringNotLike,
+						Kind:  chain.KindRequest,
+						Key:   "SourceIP",
+						Value: "10.1.1.*",
+					},
+					{
+						Op:    chain.CondStringNotEquals,
+						Kind:  chain.KindRequest,
+						Key:   "Actor",
+						Value: actor1,
+					},
+				},
+			},
+		},
+	})
+
+	status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadIP)
+	require.Equal(t, chain.AccessDenied, status)
+	require.True(t, ok)
+}
+
 func TestInmemory_MultipleTargets(t *testing.T) {
 	const op = "ape::test::op"
 
@@ -68,8 +139,8 @@ func TestInmemory_MultipleTargets(t *testing.T) {
 
 			status, found, err := s.IsAllowed(chain.Ingress, target, req)
 			require.NoError(t, err)
-			require.True(t, found)
-			require.Equal(t, chain.Allow, status)
+			require.False(t, found)
+			require.Equal(t, chain.NoRuleFound, status)
 		})
 	}
 }