-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix:Fixed a test for a NOAUTH error which is Arbitrary command gives an error when AUTH is required #1680
fix:Fixed a test for a NOAUTH error which is Arbitrary command gives an error when AUTH is required #1680
Conversation
… is required in auth.tcl Fixes: OpenAtomFoundation#1676 Signed-off-by: hqh-cell <2112206118@e.gzhu.edu.cn>
…hen auth is required in auth.tcl" This reverts commit 78de65e82e8266f8309884e537e961e497216121.
…h is required in auth.tcl Fixes: OpenAtomFoundation#1676 Signed-off-by: hqh-cell <2112206118@e.gzhu.edu.cn>
@@ -201,7 +201,7 @@ proc start_server {options {code undefined}} { | |||
if {$directive == "port"} { | |||
puts -nonewline $fp "$directive : " | |||
puts $fp [dict get $config $directive] | |||
} elseif {$directive == "requirepass"} { | |||
} elseif {$directive == "requirepass" || $directive == "userpass"} { | |||
if {[dict get $config $directive] eq ":"} { | |||
puts $fp "$directive: " | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the given code patch, there is a change in the start_server
procedure. It appears that the condition for checking the directive "requirepass" has been modified to also include "userpass" as a valid directive.
Here are some suggestions for improvement and potential bug risks:
-
Commenting: Adding comments to the code can help improve code readability and make it easier for other developers to understand its functionality.
-
Error Handling: Consider adding error handling for scenarios where the
$config
dictionary doesn't contain the specified directive. This will prevent possible errors or unexpected behavior. -
Magic Values: Replace magic values like ":" in the code with named constants or variables to make the intention and purpose of the code clearer.
-
Testing: Ensure comprehensive tests are in place to cover different scenarios and edge cases to verify the correctness of the code changes. Test both positive and negative cases.
Without a complete understanding of the context and the rest of the codebase, it's difficult to identify all potential issues. Reviewing the surrounding code and considering how this patch fits into the larger context would be valuable for a more thorough review.
test {Arbitrary command gives an error when AUTH is required} { | ||
catch {r set foo bar} err | ||
set _ $err | ||
} {NOAUTH*} | ||
} {ERR*NOAUTH*} | ||
|
||
test {AUTH succeeds when the right password is given} { | ||
r auth foobar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the code patch provided, here are some observations and suggestions:
- It seems that a test case is missing for the scenario where the AUTH command is not required.
- In the first
test {AUTH succeeds when the right password is given}
, you can remove the lineset _ $err
since it is not being used anywhere. - In the second
test {AUTH fails when a wrong password is given}
, you might want to specify that catching the error is expected by usingcatch
to capture the error message instead of assigning it to_
. For example:catch {r auth wrong!} errMsg set _ $errMsg
Overall, there don't appear to be any significant bug risks, but the review would be more thorough with the complete codebase.
…an error when AUTH is required (OpenAtomFoundation#1680) * fix: fixed a test error of arbitrary command gives an error when auth is required in auth.tcl Fixes: OpenAtomFoundation#1676 Signed-off-by: hqh-cell <2112206118@e.gzhu.edu.cn> * Revert "fix: fixed a test error of arbitrary command gives an error when auth is required in auth.tcl" This reverts commit 78de65e82e8266f8309884e537e961e497216121. * fix: fixed a test error of arbitrary command gives an error when auth is required in auth.tcl Fixes: OpenAtomFoundation#1676 Signed-off-by: hqh-cell <2112206118@e.gzhu.edu.cn> --------- Signed-off-by: hqh-cell <2112206118@e.gzhu.edu.cn>
In
pika/src/pika_client_conn.cc
Line 285 in f8da20c
Found that when the userpass is not empty,the code will give a stat is kNoAuth.So when the auth.tcl start a server ,it should overload a userpass,not require pass.
So i add a situation that server starting with a userpass.And move the NoAuth test to it.