Account based eacl targets #592

smallhive · 2024-07-04T09:56:55Z

Refs nspcc-dev/neofs-api#278

cthulhu-rider

correct under the hood, but i propose to extend/change API

eacl/target.go

carpawell

Why not API repo changes first?

cthulhu-rider · 2024-07-04T18:17:16Z

Why not API repo changes first?

there will be one comment in docs, no binary protocol changes. But we wanna review the concept in practice. In total, yes, we'll have to merge this and API simultaneously @smallhive

codecov · 2024-07-05T07:12:49Z

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.

Project coverage is 53.69%. Comparing base (0f82848) to head (c30d691).
Report is 1 commits behind head on master.

Files	Patch %	Lines
user/id.go	0.00%	2 Missing ⚠️
eacl/target.go	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #592      +/-   ##
==========================================
+ Coverage   53.62%   53.69%   +0.06%     
==========================================
  Files         164      164              
  Lines       19078    19113      +35     
==========================================
+ Hits        10231    10262      +31     
- Misses       8443     8447       +4     
  Partials      404      404

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

user/id.go

eacl/target.go

eacl/validator.go

eacl/target.go

eacl/validator_test.go

smallhive · 2024-07-22T10:57:42Z

Updated to user.ID account meaning with 25 byte length

eacl/validator.go

eacl/validator_test.go

user/id_test.go

user/id.go

eacl/target.go

eacl/types.go

carpawell · 2024-07-25T21:02:31Z

Why not API repo changes first?

But still.

eacl/target.go

carpawell · 2024-07-29T22:13:01Z

eacl/target.go

+// SetAccounts sets list of accounts to identify target subject.
+//
+// Each element of the accounts parameter is a slice of bytes of [user.ID].
+func (t *Target) SetAccounts(accounts [][]byte) {


why not just user.ID then? it is even documented like this

SetBinaryKeys is deprecated now and changing SetAccounts signature eliminates possibility to pass public keys

do not see problems or do not understand. if we do not want users to use SetBinaryKeys why we want them to be able to pass keys via SetAccounts (a func that is even documented to be used with user.ID slices only)?

i bet the reason is that we cannot keep getter and setter symmetry while having Set([]user.ID). Field is [][]byte anyway, so it's pretty normal to accept/return value of this type. At the same time, we may provide functional helpers for those who desires to set []user.ID (#592 (comment))

Field is [][]byte anyway

the field should be kept as long as we have such eALCs set in public networks. but new eACLs should be filled with SetAccounts and no keys should be accepted. at least that is how i see it

new eACLs should be filled with SetAccounts and no keys should be accepted

it's questionable which ones are new. For example, if i need to add one rule to my existing table w/o touching previously set records - i dont expect the system to deny such table. It worked with N records, i expect it to work with N+1 ones. Yeah, i can refill they whole table, but this is inconvenient for me being a client

the field should be kept as long as we have such eALCs set in public networks

weve been through this before - its impossible to track this in general. We need to clearly define the moment after which support will be cut. This should be either a system timestamp or next major protocol version

why we deprecate if we gonna support it? to me, that should work this way: 0.43.0 release (node and ir) supports both ways, 0.44.0 supports only accounts. "supports" means accepting new tables, old ones should always work until we understand there are no such tables in the contract

anyway, what is your understanding of public keys deprecation? how long are we gonna support it? till 1.0.0 SDK release? what definition will it have? SetAccounts(accounts [][]byte) too?

Keys will be supported for as long as needed.

As for the method interface --- I'd really love to see more specific type here. It can be done symmetrically, the only problem is keys in existing eacls. Keys can be converted into standard accounts, but that can be misleading. We can treat them as separate entities though. We already have Role concept in the Target that is independent of Keys. So we can also have Accounts that are independent of them, you get keys (size-checked!) in BinaryKeys() and you get accounts from Accounts(), they can technically be mixed in the same eacl rule.

all cases can be supported by functions around the most generic methods accepting/returning [][]byte, so currently proposed approach is the most preferable to me cuz it is the closest to the protocol

eacl/target.go

eacl/types.go

user/id.go

roman-khimov · 2024-08-05T13:27:22Z

eacl/target.go

+// SetAccounts sets list of accounts to identify target subject.
+//
+// Each element of the accounts parameter is a slice of bytes of [user.ID].
+func (t *Target) SetAccounts(accounts [][]byte) {


As for the method interface --- I'd really love to see more specific type here. It can be done symmetrically, the only problem is keys in existing eacls. Keys can be converted into standard accounts, but that can be misleading. We can treat them as separate entities though. We already have Role concept in the Target that is independent of Keys. So we can also have Accounts that are independent of them, you get keys (size-checked!) in BinaryKeys() and you get accounts from Accounts(), they can technically be mixed in the same eacl rule.

roman-khimov · 2024-08-05T13:28:40Z

user/id.go

@@ -82,6 +82,11 @@ func (x ID) WriteToV2(m *refs.OwnerID) {
 // Deprecated: use [NewFromScriptHash] instead.
 func (x *ID) SetScriptHash(scriptHash util.Uint160) { *x = NewFromScriptHash(scriptHash) }

+// GetScriptHash gets scripthash from user ID.
+func (x ID) GetScriptHash() util.Uint160 {


Can just be ScriptHash(), no need to Get.

eacl/target.go

carpawell · 2024-08-07T10:38:52Z

eacl/target.go

@@ -45,7 +47,26 @@ func (t Target) CopyTo(dst *Target) {
 //
 // The value returned shares memory with the structure itself, so changing it can lead to data corruption.
 // Make a copy if you need to change it.
+// Deprecated: use [Target.Accounts] instead.


sounds a little strange to me: if i am using binary keys, using Accounts does not solve my "deprication-message-from-linter" problem; can be like "using raw binary keys is not encouraged and will be dropped eventually, use Accoun-based targets instead". however, it is a minor problem

eacl/target.go

eacl/types.go

Refs #483. Signed-off-by: Evgenii Baidakov <[email protected]>

This is just a helper for scripthash extraction from user.ID. We can create user.ID from scripthash but don't have the option to do it vice versa. Signed-off-by: Evgenii Baidakov <[email protected]>

Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive requested review from roman-khimov, cthulhu-rider and carpawell as code owners July 4, 2024 09:56

cthulhu-rider reviewed Jul 4, 2024

View reviewed changes

eacl/target.go Outdated Show resolved Hide resolved

eacl/target.go Outdated Show resolved Hide resolved

eacl/target.go Outdated Show resolved Hide resolved

cthulhu-rider mentioned this pull request Jul 4, 2024

Check the account alongside the public key nspcc-dev/neofs-node#2883

Merged

carpawell reviewed Jul 4, 2024

View reviewed changes

smallhive force-pushed the account-based-eacl-targets branch from 724f847 to e3543cb Compare July 5, 2024 07:06

smallhive requested a review from cthulhu-rider July 5, 2024 07:09

cthulhu-rider reviewed Jul 5, 2024

View reviewed changes

eacl/validator_test.go Outdated Show resolved Hide resolved

cthulhu-rider reviewed Jul 5, 2024

View reviewed changes

eacl/validator_test.go Outdated Show resolved Hide resolved

smallhive force-pushed the account-based-eacl-targets branch from e3543cb to 8b59b64 Compare July 22, 2024 10:56

smallhive requested review from carpawell and cthulhu-rider July 22, 2024 10:57

roman-khimov reviewed Jul 22, 2024

View reviewed changes

eacl/validator.go Outdated Show resolved Hide resolved

cthulhu-rider reviewed Jul 24, 2024

View reviewed changes

smallhive force-pushed the account-based-eacl-targets branch from 8b59b64 to 2c4e319 Compare July 25, 2024 09:54

smallhive requested review from roman-khimov and cthulhu-rider July 25, 2024 09:54

cthulhu-rider reviewed Jul 26, 2024

View reviewed changes

eacl/target.go Outdated Show resolved Hide resolved

smallhive force-pushed the account-based-eacl-targets branch from 2c4e319 to bd1d702 Compare July 29, 2024 05:26

carpawell reviewed Jul 29, 2024

View reviewed changes

smallhive force-pushed the account-based-eacl-targets branch from bd1d702 to 74b1113 Compare July 30, 2024 04:55

roman-khimov reviewed Aug 5, 2024

View reviewed changes

cthulhu-rider requested changes Aug 6, 2024

View reviewed changes

eacl/target.go Show resolved Hide resolved

smallhive force-pushed the account-based-eacl-targets branch from 74b1113 to c024d0e Compare August 6, 2024 10:15

smallhive requested review from roman-khimov, cthulhu-rider and carpawell August 6, 2024 10:22

smallhive force-pushed the account-based-eacl-targets branch from c024d0e to de87053 Compare August 7, 2024 09:15

cthulhu-rider approved these changes Aug 7, 2024

View reviewed changes

eacl/target.go Outdated Show resolved Hide resolved

cthulhu-rider reviewed Aug 7, 2024

View reviewed changes

eacl/target.go Outdated Show resolved Hide resolved

smallhive force-pushed the account-based-eacl-targets branch from de87053 to 1cecfe2 Compare August 7, 2024 09:54

carpawell reviewed Aug 7, 2024

View reviewed changes

smallhive force-pushed the account-based-eacl-targets branch from 1cecfe2 to 6b22f9d Compare August 7, 2024 11:25

smallhive added 3 commits August 7, 2024 15:26

eacl: Allow to set accounts as target

6794d14

Refs #483. Signed-off-by: Evgenii Baidakov <[email protected]>

user: Add ScriptHash func

af47b79

This is just a helper for scripthash extraction from user.ID. We can create user.ID from scripthash but don't have the option to do it vice versa. Signed-off-by: Evgenii Baidakov <[email protected]>

eacl: Fix comment

c30d691

Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive force-pushed the account-based-eacl-targets branch from 6b22f9d to c30d691 Compare August 7, 2024 11:26

smallhive requested a review from carpawell August 7, 2024 11:27

carpawell approved these changes Aug 7, 2024

View reviewed changes

roman-khimov approved these changes Aug 7, 2024

View reviewed changes

roman-khimov merged commit 3528eb5 into master Aug 7, 2024
12 checks passed

roman-khimov deleted the account-based-eacl-targets branch August 7, 2024 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account based eacl targets #592

Account based eacl targets #592

smallhive commented Jul 4, 2024

cthulhu-rider left a comment

carpawell left a comment

cthulhu-rider commented Jul 4, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading

smallhive commented Jul 22, 2024

carpawell commented Jul 25, 2024

carpawell Jul 29, 2024

smallhive Jul 30, 2024

carpawell Jul 30, 2024

cthulhu-rider Jul 30, 2024

carpawell Jul 30, 2024

cthulhu-rider Jul 31, 2024

carpawell Jul 31, 2024

roman-khimov Aug 5, 2024

roman-khimov Aug 5, 2024

cthulhu-rider Aug 6, 2024

roman-khimov Aug 5, 2024

roman-khimov Aug 5, 2024

carpawell Aug 7, 2024

Account based eacl targets #592

Account based eacl targets #592

Conversation

smallhive commented Jul 4, 2024

cthulhu-rider left a comment

Choose a reason for hiding this comment

carpawell left a comment

Choose a reason for hiding this comment

cthulhu-rider commented Jul 4, 2024

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

smallhive commented Jul 22, 2024

carpawell commented Jul 25, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 5, 2024 •

edited

Loading