I agree we probably need this command. I couldn't think of a good alternative that respects the keyspecs without adding a lot of complexity.

@redis/core-team please approve.

@yossigo i was thinking that many people won't use ACL with key patterns limit.
and even the ones who do, will only grant each key-pattern either read-only or read+write permissions.
The write-only use case seems rare, so only these will have to revert to using SET_WO.

Reverting the GET argument we added in 6.2 would require to really disable this mechanism, not just deprecate it.

Another option we can take maybe, is handle it the same way we're gonna handle the BY and GET arguments of SORT.
i.e. at runtime.
This means to avoid flagging the SET command as a read one (flagging the command as a write-only one),
And then, inside setGenericCommand test the user access rights and if we see they used the GET argument, and they have insufficient right, reject the command before it did any modification.

I suppose this may be a valid way for ACL, and would be less valid from the perspective of exposing the right key-spec flags to clients (who knows what they're gonna do with them).
But since at the moment we don't know if anyone is gonna use it and for what, maybe that's a better approach.

@madolson WDYT?

@oranagra There's also an option to leave the key-specs as they are and a specific hack in ACL handling to ignore that for SET and only treat it as read if it really is.

I think either one of those alternatives, or even the third one (breaking SET GET completely), is better than SET_WO. If we accept SET_WO as a solution, we basically say that using SET without GET is wrong and should be replaced with SET_WO - which is unreasonable. It also means that while technically write-only ACL is supported, in practice it will not be usable for most apps unless they are modified, which also makes little sense to me.

ok. i agree about SET_WO (we don't wanna go there).
i don't think we can simple delete all the code behind the GET argument in 7.0.

so the two alternatives are:

1. avoid flagging the SET command as read.
2. flag the SET command as read, but add some hack in ACL to ignore that.

and then on top of one of the above, add and explicit ACL check inside setGenericCommand when the GET argument is used.

i prefer [2]

we basically say that using SET without GET is wrong and should be replaced with SET_WO - which is unreasonable

yes... yossigo is right. breaking the SET GET is a better one than SET_WO.

if this up to me. i might want to break SET GET and add a new command to replace it (like setget?), and we can flag it with anything we want

flag the SET command as read, but add some hack in ACL to ignore that.

or this, we may write some ugly / hack code?

I think it boils down to how much we officially want to support the GET parameter. What's more important: promoting the GET parameter or avoiding flagging SET as read?

1. avoid flagging the SET command as read.

This is the alternative of choice if we want to deprecate the GET parameter and add a hack to make the GET parameter work and require read permissions when it's used, to be backward compatible with 6.2.

2. flag the SET command as read, but add some hack in ACL to ignore that.

This is a good alternative if we don't want to deprecate the GET parameter.

(3. Add SET_WO. If we go this way, we can steer users in the right direction using documentation. The first sentence in SET_WO can be "A write-only variant of SET, which should only be used if you have write but not read permission to the key". Most developers try to follow best practices after all.)

The same can be asked about SORT's STORE parameter: Is it important enough to flag SORT as a write command? If we had added an ACL hack, we wouldn't have needed to add SORT_RO. Perhaps we can add an ACL hack for STORE and then there's no need for SORT_RO anymore...?

note that SORT_RO (and EVAL_RO) was not added for ACL, it was added for clients to be able to send this command to replicas.

I vote for 2.
i.e.

• don't add SET_WO.
• keep the ACCESS flag for SET
• add some ACL hack to only block it if GET argument is used and the user doesn't have read permission.

discussed this with @yossigo and we agree on my above proposal.

make a quick hack (b979fb7c6306eacb54cd1dab746abdaa273b6641)

SET RW

127.0.0.1:6379> ACL SETUSER key-permission-RW-selector on nopass "(%R~write* +@all)" "(%W~write* +@all)"
OK
127.0.0.1:6379> auth key-permission-RW-selector password
OK
127.0.0.1:6379> set write_string redis
OK
127.0.0.1:6379> set write_string redis
OK
127.0.0.1:6379> set write_string redis2 GET
"redis"

SET only write

127.0.0.1:6379> ACL SETUSER key-permission-W-selector on nopass "(%W~write* +@all)"
OK
127.0.0.1:6379> auth key-permission-W-selector password
OK
127.0.0.1:6379> set write_string redis
OK
127.0.0.1:6379> set write_string redis GET
(error) NOPERM this user has no permissions to access one of the keys used as arguments
127.0.0.1:6379> set write_string redis2 GET
(error) NOPERM this user has no permissions to access one of the keys used as arguments
127.0.0.1:6379> set write_string redis2
OK

before:

127.0.0.1:6379> ACL SETUSER key-permission-RW-selector on nopass "(%R~write* +@all)" "(%W~write* +@all)"
OK
127.0.0.1:6379> auth key-permission-RW-selector password
OK
127.0.0.1:6379> set write_string redis
(error) NOPERM this user has no permissions to access one of the keys used as arguments
127.0.0.1:6379> set write_string redis GET
(error) NOPERM this user has no permissions to access one of the keys used as arguments


127.0.0.1:6379> ACL SETUSER key-permission-W-selector on nopass "(%W~write* +@all)"
OK
127.0.0.1:6379> auth key-permission-W-selector password
OK
127.0.0.1:6379> set write_string redis
(error) NOPERM this user has no permissions to access one of the keys used as arguments
127.0.0.1:6379> set write_string redis GET
(error) NOPERM this user has no permissions to access one of the keys used as arguments

thanks.
I don't like the approach of ACLSelectorCheckKeyForSet, (iterating on argc to match them with "GET").
if for some reason one of the arguments has a value (like "EX" has the TTL as a value), and if that value is a free string, it can be the string "get".

I rather have acl.c explicitly remove one of READ flag for setCommand, and then add some code to setCommand to check ACL on it's own (we'll need similar check to be done by SORT for the BY and GET arguments, see #10106)

note that SORT_RO (and EVAL_RO) was not added for ACL, it was added for clients to be able to send this command to replicas.

@oranagra Sure, but a similar hack would be possible when checking if it's allowed in replicas.

@zuiderkwast check where? these commands were basically added so that their command flags are different, in order to affect client libraries, so adding such a hack for that concern will need to be done on the client libraries, not reids.

you can argue that SET_WO is required for the same concern, but i don't think it's likely.
the read-only flag is used by clients. but so far the only use case i see for write-only is for ACL (and even that one is probably not popular)

another ugly / attempt hack... Note the key pattern check become O(2n) ffc90e4

# SET write and read
127.0.0.1:6379> ACL SETUSER key-permission-RW-selector on nopass "(%R~write* +@all)" "(%W~write* +@all)"
OK
127.0.0.1:6379> auth key-permission-RW-selector password
OK
127.0.0.1:6379> set write_string redis
OK
127.0.0.1:6379> set write_string redis GET
"redis"

# SET write only
127.0.0.1:6379> ACL SETUSER key-permission-W-selector on nopass "(%W~write* +@all)"
OK
127.0.0.1:6379> auth key-permission-W-selector password
OK
127.0.0.1:6379> set write_string redis
OK
127.0.0.1:6379> set write_string redis GET
(error) NOPERM this user has no permissions to access one of the keys used as arguments

# acl log
127.0.0.1:6379> acl log 1
1)  1) "count"
    2) (integer) 1
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "write_string"
    9) "username"
   10) "key-permission-W-selector"
   11) "age-seconds"
   12) "139.69399999999999"
   13) "client-info"
   14) "id=3 addr=127.0.0.1:33168 laddr=127.0.0.1:6379 fd=8 name= age=103 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=52 qbuf-free=16332 argv-mem=23 multi-mem=0 obl=0 oll=0 omem=0 tot-mem=33517 events=r cmd=set user=key-permission-W-selector redir=-1 resp=2"

@enjoy-binbin i think the approach of your last message is right.
added some comments on the code.
i don't think the O(2N) is problematic, we don't expect many selectors.
let's see what @madolson thinks of this approach (or at all of the solution to handle the GET argument as an exception, not via key-spec flags)

I'm ok with skipping over SET_WO, I never liked it much anyways. I don't think there is a clear winner here though.

I'm not sure how I feel about deprecating the GET argument, although I think it's the best long term outcome. The GET argument also causes the SET command to return a different type (integer vs string) which I don't think we should be doing as well as the ACL issue that has been raised. It might be useful longterm to be able to rewrite some commands for compatibility but execute them as different commands, in this case we could rewrite SET + GET into a GETSET command (and presumably implement the needed flags). We're no doubt going to make more mistakes, maybe it's worth building some engineering to help us unwind them.

It sounds like a lot of work, so maybe the most Redis thing is just to implement a simple hack. I don't think there is that much to lose in just parsing the arguments twice and implementing a custom getKeys function. Most clients shouldn't care, since key positions are the same. madolson@e22e6c2. If we find more later we can add this hack there as well.

@madolson i like your approach (implementing a getkeys_proc to control the flags).
.. why didn't i think of that?

few minor notes:

1. we probably want to set VARIABLE_FLAGS to a bunch of other commands (for COMMAND command completeness), like SUNIONSTORE etc. this will mean we also need to add flags support for their getkeys_proc, which is no big deal)
2. the flag should probably be one word "variable"

p.s.
i'm a little uncomfortable about the location of the setGetKeys function, maybe it belongs in db.c with the rest of get getkeys functions.
this would mean parseExtendedStringArguments and the OBJ_SET_ flags needs to be declared in server.h, or instead, like the getkeys functions, it can implement argument parsing from scratch.
maybe we should some day move all these function to sit next to the commands they serve, not sure if that's today.

@oranagra

1. Which flags change with SUNIONSTORE? I'm not aware of what you're talking about. BITFIELD is the only one that immediately came to my mind that also changes flags.
2. I didn't like the single word "variable" since users might be confused, "maybe it's storing something", but I didn't have a better word and I was just putting out the idea.
3. I'm fine with either position of setGetKeys, it was just less work when testing the idea. I don't like putting so much stuff in server.h, but such is our current architecture.

i am done here, i am not sure the title (should we mention BITFIELD?)
and maybe we can ask @madolson take a look too, and see what she think (I think she should be up soon)

yes, no harm in waiting a few more hours.
please update the title and top comment (with other changes you made, like missing since and optional, etc)