-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add Account Deletion and Change Password to CLI #983
Conversation
WalkthroughThe changes introduce two new methods, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Client
participant Server
User->>CLI: invoke change password
CLI->>Client: ChangePassword(username, currentPassword, newPassword)
Client->>Server: request password change
Server-->>Client: success
Client-->>CLI: confirmation of change
CLI-->>User: display success message
User->>CLI: invoke delete account
CLI->>Client: DeleteAccount(username, password)
Client->>Server: request account deletion
Server-->>Client: success
Client-->>CLI: confirmation of deletion
CLI-->>User: display success message
Assessment against linked issues
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Thank you for your contribution.
Overall, your code looks good. How about referring to our other CLI command codes for the code consistency?
Also, I think you need to lint the code, it is not passing our CI lint.
You can lint your code by make lint
command in your CLI.
For your question:
I'm wondering where the printout code is in the code for the terminal. I'm thinking about using the 'golang.org/x/crypto/ssh/terminal' package to cover it up when a user enters a password.
We are using Cobra CLI library for our CLI implementation.
And there will be confirmation prompt feature in Cobra CLI library. Could you check their repository for more information?
Also, I think we need to refactor our user-related files to new user
folder in the cmd
command later. This may change CLI to yorkie user ...
. How about this approach?
cc. @hackerwins
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #983 +/- ##
=======================================
Coverage 51.06% 51.07%
=======================================
Files 73 73
Lines 10782 10784 +2
=======================================
+ Hits 5506 5508 +2
Misses 4725 4725
Partials 551 551 ☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
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.
Below are just my thoughts related to CLIs.
1. Using go prompt library for prompting.
Current prompting method looks somewhat raw. If we use go prompt library like go-prompt or promptui, it will make our CLI prompt more interactive.
2. Considering refactor of all the CLI related codes.
We can consider refactoring our CLI related codes for better code readability. Especially by refactoring out code into several functions that can explain what that code block does.
For example, for change-password.go
file, we can refactor into:
RunE: func(cmd *cobra.Command, args []string) error {
promptPassword()
changePassword()
deleteAuthSession()
}
What's your thought about this? @hackerwins
We might want to perform above action items after this PR merges.
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.
Leaving just one small comment related to my previous request changes.
Besides that, looks good to me :)
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cmd/yorkie/user/delete-account.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cmd/yorkie/user/delete-account.go
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.
Well... I forgot to mention that the filename change-password.go
and delete-account.go
should be change_password.go
and delete_account.go
, just like we use in pkg/document/internal_document.go
.
Sorry for dragging this PR :(
No, it's an opportunity for me to grow if you review it in detail! |
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.
Thank you for your contribution.
After this PR merges, we might need to consider refactoring the whole cmd
package. I think current code is enough for now.
@sigmaith Having pair programming with contributors will help you understand overall code style & flow.
Added functionality allowing users to delete accounts and change passwords through the CLI to support recent development of admin ChangePassword and DeleteAccount APIs on the server side.
Added functionality allowing users to delete accounts and change passwords through the CLI to support recent development of admin ChangePassword and DeleteAccount APIs on the server side.
What this PR does / why we need it:
Added functionality allowing users to delete accounts, change passwords, etc., through the CLI. Additionally, grouped related commands such as login, logout, account deletion, and password change under the same user folder for better organization.
Which issue(s) this PR fixes:
fixes #982
Special notes for your reviewer:
I've been trying to do all the security implementations. If I've missed anything, please let me know. Plus, I'm wondering where the printout code is in the code for the terminal. I'm thinking about using the
golang.org/x/crypto/ssh/terminal
package to cover it up when a user enters a password. (checked)Replaced the deprecated
golang.org/x/crypto/ssh/terminal
withgolang.org/x/term
to securely handle password input in the terminal.Does this PR introduce a user-facing change?:
Users need to start with
yorkie user ~
when they want to perform actions like login, logout, account deletion, and password change via the CLI.Additional documentation:
example)
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
These changes empower users with greater control over their accounts while ensuring secure and intentional actions are taken.