TrueCloudLab/frostfs-node
20
1
Fork 28
Code Issues 114 Pull requests 4 Projects Releases 2 Activity Actions

Fix error for APE check #1524

Merged
fyrchik merged 2 commits from aarifullin/frostfs-node:fix/ape/error into master 2024-11-29 10:48:17 +00:00
Conversation 0 Commits 2 Files changed 2 +12 -10
2 changed files with 12 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
pkg/services/common/ape/checker.go
View file

@ -11,7 +11,6 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/netmap"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/ape"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer"
apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user"
apechain "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
@ -104,14 +103,7 @@ func (c *checkerCoreImpl) CheckAPE(prm CheckPrm) error {
if found && status == apechain.Allow {
return nil
}
err = fmt.Errorf("access to operation %s is denied by access policy engine: %s", prm.Request.Operation(), status.String())
return apeErr(err)
}
func apeErr(err error) error {
errAccessDenied := &apistatus.ObjectAccessDenied{}
errAccessDenied.WriteReason(err.Error())
return errAccessDenied
return fmt.Errorf("access to operation %s is denied by access policy engine: %s", prm.Request.Operation(), status.String())
fyrchik commented 2024-11-28 05:49:29 +00:00
Review
Copy link

This file has not been touched by the recent refactoring.
Why change it instead of fixing what has changed there?

This file has not been touched by the recent refactoring. Why change it instead of fixing what has changed there?
aarifullin commented 2024-11-28 08:30:56 +00:00
Review
Copy link

It seems in PR #1362 I unnecessarily moved apeErr to common package because I was thinking rather about tree service than object service. It's better if the common package just returns error without statuses and each service wraps into status on its own

Why change it instead of fixing what has changed there?

Let's have a look at the ape middleware of object service. Any failure is turned to object access denied by toStatusErr invocation

func toStatusErr(err error) error {
	errAccessDenied := &apistatus.ObjectAccessDenied{}
	errAccessDenied.WriteReason("ape denied request: " + err.Error())
	return errAccessDenied
}

So, I suppose the middleware wraps an error on its own and I don't need to check statuses within

It seems in PR #1362 I unnecessarily [moved](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1362/files#diff-96aa001a23f215fa6c13e999754461bfa7dbd606) `apeErr` to common package because I was thinking rather about `tree` service than `object` service. It's better if the common package just returns error without statuses and each service wraps into status on its own > Why change it instead of fixing what has changed there? Let's have a look at the ape middleware of object service. Any failure is turned to `object access denied` by `toStatusErr` invocation ```go func toStatusErr(err error) error { errAccessDenied := &apistatus.ObjectAccessDenied{} errAccessDenied.WriteReason("ape denied request: " + err.Error()) return errAccessDenied } ``` So, I suppose the middleware wraps an error on its own and I don't need to check statuses within
}
// isValidBearer checks whether bearer token was correctly signed by authorized

12
pkg/services/tree/signature.go
View file

@ -11,6 +11,7 @@ import (
core "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/container"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/api/refs"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer"
apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/acl"
cidSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
frostfscrypto "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/crypto"
@ -62,7 +63,16 @@ func (s *Service) verifyClient(ctx context.Context, req message, cid cidSDK.ID,
return fmt.Errorf("can't get request role: %w", err)
}
return s.checkAPE(ctx, bt, cnr, cid, op, role, pubKey)
if err = s.checkAPE(ctx, bt, cnr, cid, op, role, pubKey); err != nil {
return apeErr(err)
}
return nil
}
func apeErr(err error) error {
errAccessDenied := &apistatus.ObjectAccessDenied{}
errAccessDenied.WriteReason(err.Error())
return errAccessDenied
}
// Returns true iff the operation is read-only and request was signed