Move auth check to the front

[enjoy-binbin]

When requirepass is enabled, we want command calls to return NOAUTH
instead of ERR with the error message.

Previously, these checks were all before the auth check:

• command existence exist
• command arity check
• command protected check

This may expose information such as whether the server supports the
command, whether the configuration item is enabled, etc.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.84%. Comparing base (d00c856) to head (513e02d).
Report is 1 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1475      +/-   ##
============================================
- Coverage     70.86%   70.84%   -0.03%     
============================================
  Files           119      119              
  Lines         64852    64852              
============================================
- Hits          45958    45943      -15     
- Misses        18894    18909      +15     

Files with missing lines	Coverage Δ
src/networking.c	88.35% <ø> (ø)
src/server.c	87.43% <100.00%> (-0.03%)	⬇️

... and 14 files with indirect coverage changes

[madolson]

Wouldn't your argument also extend to these two checks? You can check if the server has passwords enabled by sending both an invalid command or a command that doesn't exist.

127.0.0.1:6379> set Foo
(error) ERR wrong number of arguments for 'set' command
127.0.0.1:6379> set FOO BAR
(error) NOAUTH Authentication required.
127.0.0.1:6379> bah
(error) ERR unknown command 'bah', with args beginning with: 

Seems like we should cover them all.

[enjoy-binbin]

ok, I have thought of this too, and think this check can be put in advance, but it makes sense, I will cover them together.

[enjoy-binbin]

On the other hand, this can let people check whether the server supports xxx command

[PingXie]

I believe AUTH is primarily intended to protect user data. I understand the consistency concern regarding returning the same error message when AUTH is validated, but I don’t see a security issue with revealing a command's presence, arity, or protected status?

BTW, this is a behavior change, though perhaps not a breaking one.

[enjoy-binbin]

I believe AUTH is primarily intended to protect user data

i think it should protect everything according to the existing code.

I understand the consistency concern regarding returning the same error message when AUTH is validated, but I don’t see a security issue with revealing a command's presence, arity, or protected status?

yes, it is not a security issue, i just think it's better this way

BTW, this is a behavior change, though perhaps not a breaking one.

yes, it is a behavior change but i guess it is safe.