nspcc-dev / neofs-sdk-go

Go implementation of NeoFS SDK
Apache License 2.0
6 stars 14 forks source link

Client key for read operations #429

Closed roman-khimov closed 1 year ago

roman-khimov commented 1 year ago

Many simple wide-open metadata read operations rely on Client's key. But this key is optional. But there is no way to pass a key explicitly into client for these operations (like BalanceGet). This situation effectively makes the key mandatory, but we tried to avoid it and in many cases people really don't care. Our options:

  1. Make the key (Signer, cough) mandatory, let users create new ones as they want. Kinda OK, but requires some dances user-side when the only thing required is just getting some container metadata.
  2. Add SetSigner() to all of the respective optional method parameters. Seems like too much hassle for these simple things that do not require any real authentication.
  3. Add an ephemeral key for the client if there is none passed. Keeps the code simple for user, but adds a bit of magic in the process, some requests will work (BalanceGet), while some will fail (ContainerPut, if we're to restrict ephemeral key use) or, maybe even worse, succeed (if we're to treat it as normal key).

Opinions, suggestions? Refs. #422.

cthulhu-rider commented 1 year ago

i'd free user from providing credentials where they dont affect request payload:

less hidden logic => less unexpected stuff

carpawell commented 1 year ago

so signer is mandatory prm which must be explicitly set with this approach client constructor doesn't accept Signer prm, so it becomes easier

That would make Client less convenient IMO. I would say that a Client that uses more than one key is smth unusual (and we support it with an optional ContainerPut's SetSigner call anyway).

cthulhu-rider commented 1 year ago

my proposal is

// no external signer at all, it's not needed, info is public. For transport, Client can use random key.
func (*Client) ContainerGet(context.Context, cid.ID) (container.Container, error)

// signer must be explicitly specified cuz it affect accounting and ownership, so user cares about
// it on each call. 
func (*Client) ContainerPut(context.Context, container.Container, neofscrypto.Signer)  error

and zero default signers affecting the system behavior (only cosmetic one for transport)'

@carpawell imo if application desires to use fixed single Signer everywhere, it can wrap our client into thin utility with static signer. Or not - let it decide.

smallhive commented 1 year ago

I support @cthulhu-rider suggestion to pass Signer explicitly in each required method. Also I suggest creating a default random signer in Client constructor. It will help us to handle other methods, like balanceGet, etc

cthulhu-rider commented 1 year ago

final approach:

this covers all use-cases (simple monitoring utility, data tool) and forces users to provide credentials explicitly when (and only when) they affect access control and money transfers.

With this approach, SetDefaultSigner becomes obsolete. For monitoring ops we can generate signer (random key) internally and use it