Refactor and document `eacl` package #526

cthulhu-rider · 2023-10-10T09:21:09Z

No description provided.

List of changes: * SYSTEM role is deprecated. * encoding/decoding/copying methods are provided by the `Table` type only. * `ToV2` method and `NewFromV2` function are replaced by `WriteToV2` and `ReadFromV2` respectively. `ReadFromV2` now check protocol requirements and returns an error. * `Operation` enum is replaced with the `acl.Op` one * Keys to the object filters are exported now. * Protocol string encoding methods of the enums are purged since NeoFS API protocol doesn't define this format (these values are transmitted binary only). * `Validator` type is purged because it is only related to the system nodes' implementation and makes no sense in the SDK. * constructors for the structured types are provided to simplify understanding of required/optional fields. * Documentation is extended/improved. * Code examples are added. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

Previously, test code was needed to randomize `neofscrypto.Signer` via randomizer or manually in order to get random `neofscrypto.PublicKey`. This could increase misunderstanding code (especially examples) that user needs to have signer instance to work with public keys. Add `RandomPublicKey` function returning random public key instances. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

smallhive

In my opinion, there are too many panics in code. Part of them can be replaced with regular errors.

smallhive · 2023-10-11T04:52:37Z

eacl/filter.go

+// See also [NewFilter].
+func NewFilterObjectAttribute(key string, matcher Matcher, value string) Filter {
+	if strings.HasPrefix(key, acl.ObjectFilterPrefix) {
+		panic(fmt.Sprintf("reserved prefix in '%s'", key))


'%s' -> %q?

smallhive · 2023-10-11T04:57:39Z

eacl/table.go

+// resulting Table. All records MUST be correctly constructed.
+//
+// See also [NewForContainer].
+func New(records []Record) Table {


Does this function make sense? Anyway, the table must be linked to the container. NewForContainer is a perfect candidate for a single constructor because it covers this requirement. And rename NewForContainer to New

agree. not sure a table without a container could be used, so LimitByContainer is useless also to me

smallhive · 2023-10-11T05:02:30Z

eacl/table.go

+func (t Table) Container() (cid.ID, bool) {
+	if t.cid == nil {
+		var zero cid.ID
+		return zero, false
+	}

-// SetVersion sets version of eACL format.
-func (t *Table) SetVersion(version version.Version) {
-	t.version = version
+	return *t.cid, true


t.cid is a pointer here, we may use this fact to remove the second bool returning value. And check the result, it is nil or not. Pros and cons?

smallhive · 2023-10-11T05:07:40Z

eacl/target.go

+// The argument MUST be non-empty.
+func (t *Target) readFromV2(ms []v2acl.Target) error {
+	if len(ms) == 0 {
+		panic("empty slice of targets")


Panic? may be just an error like here? We already have error returning value

smallhive · 2023-10-11T05:15:36Z

eacl/filter.go

-func (u u64Stringer) EncodeToString() string {
-	return strconv.FormatUint(uint64(u), 10)
+// returns message about docs violation or zero if everything is OK.
+func (f Filter) validate() string {


I would suggest to replace string with error

smallhive · 2023-10-11T05:20:05Z

eacl/target.go

+// returns message about docs violation or zero if everything is OK.
+func (t Target) validate() string {
+	if len(t.roles)+len(t.keys) == 0 {
+		return "neither roles nor keys are presented"


I suggest here and below in the function to replace the string with an error

smallhive · 2023-10-11T05:20:42Z

eacl/target.go

+	for i := range t.roles {
+		switch t.roles[i] {
+		default:
+			panic(fmt.Sprintf("invalid role #%d: forbidden value %v of enum %T", i, t.roles[i], t.roles[i]))


And here we may return the error instead of panic

smallhive · 2023-10-11T05:26:36Z

eacl/target.go

-			return false
-		}
+	if withKeys {
+		ms[len(ms)-1].SetKeys(t.keys)


Right now, we have all the checks to be sure this row works appropriately. But one may change some of these checks, and in one moment ms can't be zero length => we have runtime panic.
Could we add a simple check ms > 0?

roman-khimov

A single patch changing whole eacl is bad. It shouldn't be this way, it should be a series of patches.
Many of the conceptual fixes are nice (validators/system/multi role)
But renamings are better be avoided at this stage. We're at RC, there has to be a reason to change something.
New* constructors are nice and look fine in related PRs (less code bloat).

roman-khimov · 2023-11-14T13:14:46Z

eacl/enums.go

+//
+// Note that type conversion from- and to numerical types is not recommended:
+// use enum names only.
+type Action v2acl.Action


So it doesn't need to tov2/fromv2, it's v2 already?

roman-khimov · 2023-11-14T13:15:08Z

eacl/enums.go

-	MatchStringNotEqual
+	_           Action = iota
+	ActionAllow        // allows something
+	ActionDeny         // denies something


But these are new enums that are not synchronized with v2 in any way.

roman-khimov · 2023-11-14T15:14:58Z

eacl/filter.go

+// payload's homomorphic checksum of the objects.
+//
+// See also [NewFilter].
+func NewFilterObjectPayloadHomomorphicChecksum(matcher Matcher, cs [tz.Size]byte) Filter {


Suppose I have a MatchIntegerLessThan matcher, will it work here (and in other cases)?

roman-khimov · 2023-11-14T15:22:43Z

eacl/table.go

+// implementation only and is not expected to be directly used by applications.
+//
+// See also [Table.ReadFromV2].
+func (t Table) WriteToV2(m *acl.Table) {


On one hand, it's unification. On the other, we still have ToV2() for other structures. So I'm not sure it's an improvement at all.

roman-khimov · 2023-11-14T15:24:31Z

eacl/table.go

 }

-// SetCID sets identifier of the container that should use given access control rules.
-func (t *Table) SetCID(cid cid.ID) {


Doesn't add a lot of value at RC11 stage.

btw, i do not agree that Container is somehow better than CID. less explicit, more things to keep in mind, struct context is required for code reading, and so on

roman-khimov · 2023-11-14T15:41:02Z

eacl/table.go

-}
-
-// EqualTables compares Table with each other.
-func EqualTables(t1, t2 Table) bool {


No replacement for it?

roman-khimov · 2023-11-14T16:11:55Z

eacl/test/benchmark_test.go

-	"github.com/stretchr/testify/require"
-)
-
-func baseBenchmarkTableBinaryComparison(b *testing.B, factor int) {


We no longer care?

roman-khimov · 2023-11-14T16:14:40Z

eacl/example_test.go

 // eACL provides ability to determine target subjects of access rules, in
 // particular, distribute/restrict access on an exclusive basis.
 func ExampleTable_exclusiveRights() {
-	friendPubKey := randomPublicKey()
+	friendPubKey := test.RandomPublicKey()


NACK. It's an example. It must not import any test packages, it should be something people can copy/paste. Sure, randomPublicKey() will need to be replaced, but at least one can see what's in there, unlike when it's an imported package.

carpawell

Linter died.

carpawell · 2023-11-15T17:41:51Z

eacl/test/benchmark_test.go

@@ -1,116 +0,0 @@
-package eacltest


carpawell · 2023-11-15T17:47:10Z

eacl/test/generate.go

@@ -1,50 +1,44 @@
 package eacltest


this is literally not the eacl package that is mentioned as being changed in the commit and no words about this change in the body. a separate commit?

carpawell · 2023-11-15T18:05:03Z

eacl/filter.go

+// Key MUST be non-empty.
+//
+// See also other helper constructors.
+func NewFilter(typ HeaderType, key string, matcher Matcher, value string) Filter {


why so generic name for a header-typed filter?

carpawell · 2023-11-15T18:06:02Z

eacl/filter.go

+		value:   value,
+	}
+	if msg := f.validate(); msg != "" {
+		panic(msg)


panic? i think we are getting rid of them in sdk (and least in constructors)

carpawell · 2023-11-15T18:09:17Z

eacl/record.go

+// Return value MUST NOT be mutated, make a copy first if needed.
+//
+// See also [Record.IsForKey].
+func (r Record) TargetBinaryKeys() [][]byte {


is it ok to have bytes here but neofscrypto.PublicKey in the func a few lines below?

carpawell · 2023-11-15T18:13:01Z

eacl/table.go

 	cid     *cid.ID
 	records []Record
 }

+// New constructs eACL from the given list of access rules. Being applied as
+// part of access control to the NeoFS resources, the rules are matched
+// according to the first hit: if a rule with a certain index is applicable,


do we want this behavior? i would say that looking for every applicable rule and choosing the most restrictive looks better

carpawell · 2023-11-15T18:14:09Z

eacl/table.go

+// resulting Table. All records MUST be correctly constructed.
+//
+// See also [NewForContainer].
+func New(records []Record) Table {


agree. not sure a table without a container could be used, so LimitByContainer is useless also to me

carpawell · 2023-11-15T18:15:55Z

eacl/table.go

 }

-// SetCID sets identifier of the container that should use given access control rules.
-func (t *Table) SetCID(cid cid.ID) {


btw, i do not agree that Container is somehow better than CID. less explicit, more things to keep in mind, struct context is required for code reading, and so on

carpawell · 2023-11-15T18:18:35Z

eacl/target.go

-	return t.keys
-}
+// See also other helper constructors.
+func NewTarget(roles []Role, publicKeys []neofscrypto.PublicKey) Target {


again, why some version of a generic struct is more default than others? why keys are more default New than roles?

carpawell · 2023-11-15T18:21:51Z

eacl/validator.go

-//
-// If no matching table entry is found or some filters are missing,
-// ActionAllow is returned and the second return value is false.
-func (v *Validator) CalculateAction(unit *ValidationUnit) (Action, bool) {


we are so get used to moving code from one repo to another and many other types of useless work. i am not a conservative but can we at least explain why we moving smth in and out again?

roman-khimov

Feedback must be addressed.

cthulhu-rider · 2024-09-16T08:13:20Z

overlapped by #605

cthulhu-rider requested review from roman-khimov, smallhive and carpawell as code owners October 10, 2023 09:21

cthulhu-rider force-pushed the avoid-redundant-pointers branch 2 times, most recently from 5641760 to b22e4f6 Compare October 10, 2023 09:36

cthulhu-rider mentioned this pull request Oct 10, 2023

Upgrade NeoFS SDK with eacl changes nspcc-dev/neofs-s3-gw#840

Closed

cthulhu-rider added 2 commits October 10, 2023 15:08

cthulhu-rider force-pushed the avoid-redundant-pointers branch from b22e4f6 to c2f3f5e Compare October 10, 2023 11:08

cthulhu-rider mentioned this pull request Oct 10, 2023

Upgrade NeoFS SDK with eacl changes nspcc-dev/neofs-node#2618

Closed

smallhive reviewed Oct 11, 2023

View reviewed changes

roman-khimov reviewed Nov 14, 2023

View reviewed changes

carpawell reviewed Nov 15, 2023

View reviewed changes

carpawell mentioned this pull request Jan 31, 2024

Drop grpc/profobuff dependencies #551

Open

roman-khimov requested changes Jan 31, 2024

View reviewed changes

cthulhu-rider closed this Sep 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and document `eacl` package #526

Refactor and document `eacl` package #526

cthulhu-rider commented Oct 10, 2023

smallhive left a comment

smallhive Oct 11, 2023

smallhive Oct 11, 2023

carpawell Nov 15, 2023

smallhive Oct 11, 2023

smallhive Oct 11, 2023

smallhive Oct 11, 2023

smallhive Oct 11, 2023

smallhive Oct 11, 2023

smallhive Oct 11, 2023

roman-khimov left a comment

roman-khimov Nov 14, 2023

roman-khimov Nov 14, 2023

roman-khimov Nov 14, 2023

roman-khimov Nov 14, 2023

roman-khimov Nov 14, 2023

carpawell Nov 15, 2023

roman-khimov Nov 14, 2023

roman-khimov Nov 14, 2023

roman-khimov Nov 14, 2023

carpawell left a comment

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023

carpawell Nov 15, 2023 •

edited

Loading

roman-khimov left a comment

cthulhu-rider commented Sep 16, 2024

Refactor and document eacl package #526

Refactor and document eacl package #526

Conversation

cthulhu-rider commented Oct 10, 2023

smallhive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

cthulhu-rider commented Sep 16, 2024

Refactor and document `eacl` package #526

Refactor and document `eacl` package #526

carpawell Nov 15, 2023 •

edited

Loading