TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 107 Pull requests 9 Projects Releases 2 Activity Actions

tree: Use delete verb instead put for Remove #1443

Merged
fyrchik merged 2 commits from aarifullin/frostfs-node:fix/tree_remove_ape into master 2024-10-29 08:04:24 +00:00
Conversation 2 Commits 2 Files changed 2 +208 -1
2 changed files with 208 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

207
pkg/services/tree/ape_test.go Normal file
View file

@ -0,0 +1,207 @@
package tree
import (
"context"
"encoding/hex"
"fmt"
"testing"
"git.frostfs.info/TrueCloudLab/frostfs-contract/frostfsid/client"
core "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/container"
frostfsidcore "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/frostfsid"
checkercore "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/common/ape"
containerSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/acl"
cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine"
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine/inmemory"
nativeschema "git.frostfs.info/TrueCloudLab/policy-engine/schema/native"
"github.com/nspcc-dev/neo-go/pkg/crypto/keys"
"github.com/nspcc-dev/neo-go/pkg/util"
"github.com/stretchr/testify/require"
)
var (
containerID = "73tQMTYyUkTgmvPR1HWib6pndbhSoBovbnMF7Pws8Rcy"
senderPrivateKey, _ = keys.NewPrivateKey()
senderKey = hex.EncodeToString(senderPrivateKey.PublicKey().Bytes())
rootCnr = &core.Container{Value: containerSDK.Container{}}
)
type frostfsIDProviderMock struct {
subjects map[util.Uint160]*client.Subject
subjectsExtended map[util.Uint160]*client.SubjectExtended
}
func (f *frostfsIDProviderMock) GetSubject(key util.Uint160) (*client.Subject, error) {
v, ok := f.subjects[key]
if !ok {
return nil, fmt.Errorf("%s", frostfsidcore.SubjectNotFoundErrorMessage)
}
return v, nil
}
func (f *frostfsIDProviderMock) GetSubjectExtended(key util.Uint160) (*client.SubjectExtended, error) {
v, ok := f.subjectsExtended[key]
if !ok {
return nil, fmt.Errorf("%s", frostfsidcore.SubjectNotFoundErrorMessage)
}
return v, nil
}
var _ frostfsidcore.SubjectProvider = (*frostfsIDProviderMock)(nil)
func newFrostfsIDProviderMock(t *testing.T) *frostfsIDProviderMock {
return &frostfsIDProviderMock{
subjects: map[util.Uint160]*client.Subject{
scriptHashFromSenderKey(t, senderKey): {
Namespace: "testnamespace",
Name: "test",
KV: map[string]string{
"tag-attr1": "value1",
"tag-attr2": "value2",
},
},
},
subjectsExtended: map[util.Uint160]*client.SubjectExtended{
scriptHashFromSenderKey(t, senderKey): {
Namespace: "testnamespace",
Name: "test",
KV: map[string]string{
"tag-attr1": "value1",
"tag-attr2": "value2",
},
Groups: []*client.Group{
{
ID: 1,
Name: "test",
Namespace: "testnamespace",
KV: map[string]string{
"attr1": "value1",
"attr2": "value2",
},
},
},
},
},
}
}
func scriptHashFromSenderKey(t *testing.T, senderKey string) util.Uint160 {
pk, err := keys.NewPublicKeyFromString(senderKey)
require.NoError(t, err)
return pk.GetScriptHash()
}
type stMock struct{}
func (m *stMock) CurrentEpoch() uint64 {
return 8
}
func TestCheckAPE(t *testing.T) {
cid := cid.ID{}
_ = cid.DecodeString(containerID)
t.Run("put non-tombstone rule won't affect tree remove", func(t *testing.T) {
los := inmemory.NewInmemoryLocalStorage()
mcs := inmemory.NewInmemoryMorphRuleChainStorage()
fid := newFrostfsIDProviderMock(t)
s := Service{
cfg: cfg{
frostfsidSubjectProvider: fid,
},
apeChecker: checkercore.New(los, mcs, fid, &stMock{}),
}
los.AddOverride(chain.Ingress, engine.ContainerTarget(containerID), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
Actions: chain.Actions{Names: []string{nativeschema.MethodPutObject}},
Resources: chain.Resources{
Names: []string{fmt.Sprintf(nativeschema.ResourceFormatRootContainerObjects, containerID)},
},
Condition: []chain.Condition{
{
Op: chain.CondStringNotEquals,
Kind: chain.KindResource,
Key: nativeschema.PropertyKeyObjectType,
Value: "TOMBSTONE",
},
},
},
},
MatchType: chain.MatchTypeFirstMatch,
})
mcs.AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(containerID), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.Allow,
Actions: chain.Actions{Names: []string{nativeschema.MethodDeleteObject}},
Resources: chain.Resources{
Names: []string{fmt.Sprintf(nativeschema.ResourceFormatRootContainerObjects, containerID)},
},
},
},
MatchType: chain.MatchTypeFirstMatch,
})
err := s.checkAPE(context.Background(), nil, rootCnr, cid, acl.OpObjectDelete, acl.RoleOwner, senderPrivateKey.PublicKey())
require.NoError(t, err)
})
t.Run("delete rule won't affect tree add", func(t *testing.T) {
los := inmemory.NewInmemoryLocalStorage()
mcs := inmemory.NewInmemoryMorphRuleChainStorage()
fid := newFrostfsIDProviderMock(t)
s := Service{
cfg: cfg{
frostfsidSubjectProvider: fid,
},
apeChecker: checkercore.New(los, mcs, fid, &stMock{}),
}
los.AddOverride(chain.Ingress, engine.ContainerTarget(containerID), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
Actions: chain.Actions{Names: []string{nativeschema.MethodDeleteObject}},
Resources: chain.Resources{
Names: []string{fmt.Sprintf(nativeschema.ResourceFormatRootContainerObjects, containerID)},
},
},
},
MatchType: chain.MatchTypeFirstMatch,
})
mcs.AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(containerID), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.Allow,
Actions: chain.Actions{Names: []string{nativeschema.MethodPutObject}},
Resources: chain.Resources{
Names: []string{fmt.Sprintf(nativeschema.ResourceFormatRootContainerObjects, containerID)},
},
Condition: []chain.Condition{
{
Op: chain.CondStringNotEquals,
Kind: chain.KindResource,
Key: nativeschema.PropertyKeyObjectType,
Value: "TOMBSTONE",
},
},
},
},
MatchType: chain.MatchTypeFirstMatch,
})
err := s.checkAPE(context.Background(), nil, rootCnr, cid, acl.OpObjectPut, acl.RoleOwner, senderPrivateKey.PublicKey())
require.NoError(t, err)
})
}

2
pkg/services/tree/service.go
View file

@ -228,7 +228,7 @@ func (s *Service) Remove(ctx context.Context, req *RemoveRequest) (*RemoveRespon
return nil, err
}
err := s.verifyClient(ctx, req, cid, b.GetBearerToken(), acl.OpObjectPut)
err := s.verifyClient(ctx, req, cid, b.GetBearerToken(), acl.OpObjectDelete)
aarifullin commented 2024-10-22 13:06:42 +00:00
Review
Copy link

An alternative: pass OpObjectPut as well but verifyClient must specify the context of invocation to set "object type" property to APE-resource that seems unnecessary

An alternative: pass `OpObjectPut` as well but `verifyClient` must specify the context of invocation to set "object type" property to APE-resource that seems unnecessary
dkirillov commented 2024-10-24 12:46:03 +00:00
Review
Copy link

I suppose it's ok to use just acl.OpObjectDelete here (in tree service). But for the same check in object service it seems we need add object type property. Otherwise, if put tombstone request goes to node that is not in container we will get the same problem (because check fails). Or do I miss something?

I suppose it's ok to use just `acl.OpObjectDelete` here (in tree service). But for the same check in object service it seems we need add object type property. Otherwise, if `put tombstone` request goes to node that is not in container we will get the same problem (because check fails). Or do I miss something?
aarifullin commented 2024-10-24 12:53:07 +00:00
Review
Copy link

The object service correctly sets resource properties and checks are performed correctly. We've got no problems so far

The tree service should have set the property on its own - like the object service does. But that's unnecessary if we use Delete op instead Put

The object service correctly sets resource properties and checks are performed correctly. We've got no problems so far The tree service should have set the property on its own - like the object service does. But that's unnecessary if we use Delete op instead Put
if err != nil {
return nil, err
}