Do not ask for password if not needed. #69
Conversation
In certain circumstances we do not need to ask for a password because we already have previously authenticated to sudo and can just reuse the credentials. Since there is no way to check if a sudo timestamp has timed out we run a simple command and check the result. Using an existing sudo timestamp from within nvim seems to work only if the timestamp_type is set to 'global' in the sudo configuration file. The assesment of the risk of setting the security relevant setting timestamp_type to 'global' needs to be done by the user.
WalkthroughThis update enhances the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| " Let's try running a command non-interactively. If it works, we have a sudo | ||
| " timestamp that has not timed out yet. In this case there is no need to ask | ||
| " for a password. | ||
| " This only works if the timestamp_type is set to 'global' in the sudo | ||
| " configuation file. It does not work with 'ppid', 'kernel' or 'tty'. | ||
| let cmd = printf('%s -n true', g:suda#executable) | ||
| let result = system(cmd) | ||
| if v:shell_error == 0 | ||
| let cmd = printf('%s %s', g:suda#executable, a:cmd) | ||
| else | ||
| try | ||
| call inputsave() | ||
| redraw | let password = inputsecret(g:suda#prompt) | ||
| finally | ||
| call inputrestore() | ||
| endtry | ||
| let cmd = printf('%s -p '''' -S %s', g:suda#executable, a:cmd) | ||
| endif |
There was a problem hiding this comment.
The implementation of the non-interactive command execution to check for an active sudo timestamp before prompting for a password is a clever solution to improve user experience. However, there are a few points to consider for refinement:
-
Security Consideration Comment: It's good practice to include a comment about the security implications of setting the sudo timestamp type to 'global', as mentioned in the PR description. This helps ensure that users are aware of the potential risks involved.
-
Error Handling: The current implementation assumes that if
v:shell_erroris not 0 after the non-interactive command execution, it's safe to proceed with password prompting. However, it might be beneficial to consider specific error codes that indicate a lack of sudo timestamp versus other potential errors that could occur during command execution. -
Password Variable Scope: The
passwordvariable is used only within theelseblock but is appended to the command outside of this block. This could lead to a situation where thepasswordvariable is not defined. Consider initializing thepasswordvariable before theifstatement or restructuring the logic to ensure that it's always defined when needed. -
Duplication of Command Construction: The command construction logic (
let cmd = printf('%s %s', g:suda#executable, a:cmd)) is repeated before and after the non-interactive check. This could be refactored to avoid duplication and enhance maintainability. -
Documentation: Adding documentation for the new logic within the
suda#systemfunction would be helpful for future maintainers. This documentation could explain the rationale behind the non-interactive command execution and the conditions under which it is expected to work.
Consider addressing these points to refine the implementation and ensure clarity and robustness in the code.
| " This only works if the timestamp_type is set to 'global' in the sudo | ||
| " configuation file. It does not work with 'ppid', 'kernel' or 'tty'. |
There was a problem hiding this comment.
Please explain this behavior and limitation in doc/suda.txt as well.
There was a problem hiding this comment.
Done. Please let me know if you are ok with an extra commit like that or you want me to squash it.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- doc/suda.txt (1 hunks)
Additional comments: 1
doc/suda.txt (1)
- 40-50: The documentation now includes a section explaining how users can configure sudo with
Defaults timestamp_type = globalto avoid unnecessary password prompts when the sudo session is still valid. This is a valuable addition for users who have sudo version 1.8.21 or higher and prefer not to enter their password multiple times during a session. However, it's crucial to emphasize the security implications of this configuration, as it could potentially increase the risk if the user's session is compromised. Users should be encouraged to carefully consider their security requirements before enabling this feature.Consider adding a note or warning that explicitly outlines the potential security risks associated with enabling global timestamps for sudo authentication. This will help users make an informed decision based on their security needs and the trustworthiness of their environment.
| > | ||
| : echo executable('sudo') | ||
| < | ||
| *suda* will ask for a password each time sudo is used for reading or writing. | ||
| However, you can set global timestamps in your sudoers configuration, if you | ||
| have sudo version 1.8.21 or higher. This will enable *suda* to reuse an | ||
| existing sudo authentication token. In this case, it will not ask for a | ||
| password if not needed. To enable, configure sudo with this option: | ||
| > | ||
| Defaults timestamp_type = global | ||
| < | ||
| The other types 'ppid', 'kernel' or 'tty' will not allow *suda* to use sudo | ||
| credential caching. Please make sure this is in line with your security | ||
| requirements. | ||
|
|
||
| ============================================================================= | ||
| USAGE *suda-usage* |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-29]
In the sentence "This plugin is strongly inspired by sudo.vim but the interfaces was aggressively modified for modern Vim script," consider adding a comma before "but" and correcting the verb agreement to "were" to improve readability and grammatical correctness.
- This plugin is strongly inspired by sudo.vim but the interfaces was aggressively modified for modern Vim script.
+ This plugin is strongly inspired by sudo.vim, but the interfaces were aggressively modified for modern Vim script.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [102-102]
The verb "switch" does not agree with the subject in the sentence "When |g:suda_smart_edit| is true, suda automatically switch a buffer name when the target file is not readable or writable." Consider changing "switch" to "switches" for correct subject-verb agreement.
- When |g:suda_smart_edit| is true, suda automatically switch a buffer name when the target file is not readable or writable.
+ When |g:suda_smart_edit| is true, suda automatically switches a buffer name when the target file is not readable or writable.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [192-192]
There's a typo in "openning" within the sentence "so you shouldn't feel any slowdown when openning buffers." It should be corrected to "opening" for proper spelling.
- so you shouldn't feel any slowdown when openning buffers.
+ so you shouldn't feel any slowdown when opening buffers.
In certain circumstances we do not need to ask for a password because we already have previously authenticated to sudo and can just reuse the credentials.
Since there is no way to check if a sudo timestamp has timed out we run a simple command and check the result.
Using an existing sudo timestamp from within nvim seems to work only if the timestamp_type is set to 'global' in the sudo configuration file.
The assesment of the risk of setting the security relevant setting timestamp_type to 'global' needs to be done by the user.
Summary by CodeRabbit
sudaplugin to improve user experience by reducing unnecessary password prompts.sudafunctionality documentation to include support for global timestamps for sudo authentication.