Fix ACL category for SELECT, WAIT, ROLE, LASTSAVE, READONLY, READWRITE, ASKING by oranagra · Pull Request #9208 · redis/redis

oranagra · 2021-07-06T18:38:40Z

SELECT and WAIT don't read or write from the keyspace (unlike DEL, EXISTS, EXPIRE, DBSIZE, KEYS, etc).
they're more similar to AUTH and HELLO (and maybe PING and COMMAND).
they only affect the current connection, not the server state, so they should be @connection, not @keyspace
ROLE, like LASTSAVE is @admin (and @dangerous like INFO)
ASKING, READONLY, READWRITE are @connection too (not @keyspace)
Additionally, i'm now documenting the exact meaning of each ACL category so it's clearer which commands belong where.

oranagra · 2021-07-06T18:39:47Z

@redis/core-team please approve or share your thoughts.
brought up by #9205

madolson · 2021-07-06T19:05:27Z

I would also add readonly, readwrite, and asking to @connection (and remove them from @keyspace), since they modify the client as opposed to having anything to do with the keyspace.

oranagra · 2021-07-06T19:17:26Z

i keep wondering if maybe we don't understand correctly what's was the intended meaning of @keyspace and @connection.
the fact is (as i mentioned in #6860) that the meaning of the various ACL categories is not documented anywhere (not even in the code)

madolson

I think we should document them given this. I also agree, I don't really understand what @keyspace is supposed to be, like why is dbsize in it?

huangzhw · 2021-07-07T09:32:31Z

I also have questions. Why LASTSAVE ROLE are dangerous.

oranagra · 2021-07-07T09:56:46Z

don't know.. i don't even know why we have these commands... LASTSAVE and ROLE are part of INFO, i guess backwards compatibility from a time redis was half baked.
on the other hand, since they're not really needed for anything, and since they're read-only, i don't care much about their ACL category.
anyone has any reason not to modify them? (should have the came category as INFO)

zuiderkwast · 2021-07-08T11:46:23Z

INFO is also @dangerous. (Besides, INFO is @slow while LASTSAVE and ROLE are @fast, some of them are also @read and LASTSAVE is also @admin.) Maybe we can try to catch the essence of each category and describe them on https://redis.io/topics/acl#playings-with-command-categories instead of the ACL CAT example which just lists their names?

oranagra · 2021-07-08T12:06:56Z

certainly.. we wanna document them.. just not sure we understand what each of them meant to be..

zuiderkwast · 2021-07-08T13:24:15Z

Maybe a generated overview of all categories and commands can help: redis/redis-doc#1598

oranagra · 2021-07-18T18:37:16Z

@redis/core-team i made a few more changes in the command flags and ACL categories (top comment is updated)
I also documented the exact purpose of each category, this documentation should maybe be copied / moved to redis.conf, and also maybe copied to redis.io/topics/acl
Note this is a breaking change, should only be released in 7.0.
please have another look and approve.

src/acl.c

enhanched description of fast, and blocking cagegories. other changes are just newline, parenthesis and indentation.

redis.conf

Co-authored-by: sundb <[email protected]>

redis.conf

Co-authored-by: sundb <[email protected]>

madolson

Minor comments, but generally still LGTM.

redis.conf

Co-authored-by: Madelyn Olson <[email protected]>

madolson

go go go

soloestoy · 2021-07-20T06:02:37Z

Why we flag TOUCH as write command? it's just like GET command would alter the last access time of keys.

oranagra · 2021-07-20T06:53:25Z

@soloestoy you're right. not sure what i was thinking.. reverting.

…E, ASKING (redis#9208) - SELECT and WAIT don't read or write from the keyspace (unlike DEL, EXISTS, EXPIRE, DBSIZE, KEYS, etc). they're more similar to AUTH and HELLO (and maybe PING and COMMAND). they only affect the current connection, not the server state, so they should be `@connection`, not `@keyspace` - ROLE, like LASTSAVE is `@admin` (and `@dangerous` like INFO) - ASKING, READONLY, READWRITE are `@connection` too (not `@keyspace`) - Additionally, i'm now documenting the exact meaning of each ACL category so it's clearer which commands belong where.

ACL category for SELECT and WAIT should be connection, not keyspace

0cd29ee

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Jul 6, 2021

also modify READONLY, READWRITE, and ASKING

f17cc22

madolson previously approved these changes Jul 6, 2021

View reviewed changes

oranagra added the approval-needed Waiting for core team approval to be merged label Jul 7, 2021

oranagra mentioned this pull request Jul 8, 2021

Document ACL categories redis/redis-doc#1598

Closed

yossigo previously approved these changes Jul 18, 2021

View reviewed changes

oranagra dismissed stale reviews from yossigo and madolson via 8757468 July 18, 2021 18:21

oranagra changed the title ~~ACL category for SELECT and WAIT should be connection, not keyspace~~ Fix ACL category for SELECT, TOUCH, WAIT, ROLE, LASTSAVE, READONLY, READWRITE, ASKING Jul 18, 2021

ROLE add admin, TOUCH is read instead of write, add desc for ACL cats

64fc716

zuiderkwast reviewed Jul 18, 2021

View reviewed changes

src/acl.c Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

huangzhw reviewed Jul 19, 2021

View reviewed changes

src/acl.c Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

oranagra added 2 commits July 19, 2021 09:06

CR comments and typos

e3d8a8a

moved ACL category docs to redis.conf

5f15b24

enhanched description of fast, and blocking cagegories. other changes are just newline, parenthesis and indentation.

sundb reviewed Jul 19, 2021

View reviewed changes

redis.conf Outdated Show resolved Hide resolved

Update redis.conf

b6fe77d

Co-authored-by: sundb <[email protected]>

sundb reviewed Jul 19, 2021

View reviewed changes

redis.conf Outdated Show resolved Hide resolved

Update redis.conf

bf1678e

Co-authored-by: sundb <[email protected]>

madolson previously approved these changes Jul 19, 2021

View reviewed changes

redis.conf Outdated Show resolved Hide resolved

redis.conf Outdated Show resolved Hide resolved

itamarhaber previously approved these changes Jul 19, 2021

View reviewed changes

oranagra dismissed stale reviews from itamarhaber and madolson via 374bee1 July 20, 2021 05:07

Apply suggestions from code review

374bee1

Co-authored-by: Madelyn Olson <[email protected]>

oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Jul 20, 2021

madolson previously approved these changes Jul 20, 2021

View reviewed changes

oranagra changed the title ~~Fix ACL category for SELECT, TOUCH, WAIT, ROLE, LASTSAVE, READONLY, READWRITE, ASKING~~ Fix ACL category for SELECT, WAIT, ROLE, LASTSAVE, READONLY, READWRITE, ASKING Jul 20, 2021

revert change to touch command flags.

e51fe8c

oranagra dismissed madolson’s stale review via e51fe8c July 20, 2021 06:54

soloestoy approved these changes Jul 20, 2021

View reviewed changes

oranagra added state:needs-doc-pr requires a PR to redis-doc repository breaking-change This change can potentially break existing application labels Jul 20, 2021

yossigo approved these changes Jul 20, 2021

View reviewed changes

oranagra merged commit 32e61ee into redis:unstable Jul 20, 2021

oranagra mentioned this pull request Jan 23, 2023

Module commands to have ACL categories. #11708

Merged

Conversation

oranagra commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jul 6, 2021

Uh oh!

madolson commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jul 6, 2021

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

huangzhw commented Jul 7, 2021

Uh oh!

oranagra commented Jul 7, 2021

Uh oh!

zuiderkwast commented Jul 8, 2021

Uh oh!

oranagra commented Jul 8, 2021

Uh oh!

zuiderkwast commented Jul 8, 2021

Uh oh!

oranagra commented Jul 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

soloestoy commented Jul 20, 2021

Uh oh!

oranagra commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

oranagra commented Jul 6, 2021 •

edited

Loading

madolson commented Jul 6, 2021 •

edited

Loading

oranagra commented Jul 18, 2021 •

edited

Loading