Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds password prompt for register and login #424

Merged
merged 4 commits into from
May 24, 2022
Merged

Adds password prompt for register and login #424

merged 4 commits into from
May 24, 2022

Conversation

notsatyarth
Copy link
Contributor

Addresses #344.

I've added a dependency of rpassword since I figured the library would be able to handle the password input better across different platforms compared to something that I would write. I'm also not sure if the Cmd struct for login and register should remain since there are no longer any "options" to use with them.
Do let me know if these changes make sense. Thanks!

@notsatyarth
Copy link
Contributor Author

There are also a few commits since the last PR I raised. I'm not sure why they appear but I think since you squash and merge that should appear on the final tree. Do let me know incase I need to do something else to fix this.

@notsatyarth notsatyarth changed the title Feat/password prompts Adds password prompt for register and login May 23, 2022
@conradludgate
Copy link
Collaborator

Thanks! I'll have to test it to see how rpassword works, but it looks good.

Can you keep keep the original argument mode as well however? At least username and email via flags seems reasonable to me

@notsatyarth
Copy link
Contributor Author

Sure will look into that. I did not do that because I expected it to be confusing to have a partial prompt and options based input. But let me add that back and then decide.

@panekj
Copy link
Contributor

panekj commented May 24, 2022

Please leave --password as well, most sane shells have been capable of secure input via read -s for a long time

@notsatyarth notsatyarth force-pushed the feat/password-prompts branch from 2586b29 to e214cf8 Compare May 24, 2022 07:14
@notsatyarth
Copy link
Contributor Author

So if I were to leave --password then would it not be the same as the existing functionality? Alternatively can you elaborate on how do you expect this command to accept inputs so that I'm sure we are on the same page here.

@conradludgate
Copy link
Collaborator

It would not be the same functionality. Our impl before didn't handle passwords in the prompt safely

@notsatyarth
Copy link
Contributor Author

Okay, so while the password prompt would be handled safely, then option of adding the password with -p would still be plain text. Is that expected?

@conradludgate
Copy link
Collaborator

Yeah I think this is expected

@ellie
Copy link
Member

ellie commented May 24, 2022 via email

@notsatyarth
Copy link
Contributor Author

Thanks, I've updated the PR. Although I'm not happy with the code that I've written and feel that this could be refactored better. Do let me know your suggestions.

@conradludgate
Copy link
Collaborator

I have some ideas for a refactor too. But that can be in a separate PR if you want. I'm testing out the password behaviour locally but the code looks good. Much appreciated

conradludgate
conradludgate approved these changes May 24, 2022
View reviewed changes
Copy link
Collaborator

@conradludgate conradludgate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well. Ty

@conradludgate conradludgate merged commit 119ab9e into atuinsh:main May 24, 2022
@notsatyarth notsatyarth deleted the feat/password-prompts branch May 24, 2022 13:29
ellie added a commit that referenced this pull request Jun 6, 2022
@ellie
Release v0.10.0
7646ed4 
06ac958 Show current version on server index (#436)
706b1af Disable ARM docker builds (#438)
f2abc23 Update README.md
3c2b055 Noyez fix dir hostname utf8 (#430)
3f5350d [feature] Add scroll wheel support to interactive history search (#435)
dcdde22 Fix text outline for dark mode
9ac0c60 Implement cursor (#412)
119ab9e Adds password prompt for register and login (#424)
e5df809 Noyez zsh histdb import (#393)
b08e254 Improve default fish keybindings (#420)
4096c6e Update README.md
cd2a3ab Add fish shell to key binding docs (#418)
b278211 Bump clap_complete from 3.1.3 to 3.1.4 (#397)
ee66c0a Bump axum from 0.5.5 to 0.5.6 (#415)
4297e26 Bump tokio from 1.18.1 to 1.18.2 (#396)
dbd9ca5 Bump clap from 3.1.16 to 3.1.18 (#401)
a7c9d19 Bump tower-http from 0.3.2 to 0.3.3 (#399)
3b79f68 Bump axum from 0.5.4 to 0.5.5 (#402)
f340771 Cleanup dependencies – disable unnecessary or unused features (#407)
ab294cd Don't pollute shell environment - remove 'id' variable (#408)
14b3060 Allow to build atuin server without client (#404)
5e4e8d1 Don't create config dir for server in default location if not needed (#406)
b7946cc Update Chinese version README.md (#403)
e0291f6 Update README.md
301190e Build ARM docker image in GitHub Actions using QEMU (#400)
1d030b9 Importer V3 (#395)
d3a4ff9 Bump clap from 3.1.15 to 3.1.16 (#392)
e9d2ec4 Add ctrl-k and ctrl-j for up and down (#394)
25afb5b Bump serde_json from 1.0.80 to 1.0.81 (#387)
4a839da Adds stats summary  (#384)
7a394b0 Bump serde from 1.0.136 to 1.0.137 (#375)
edd3f81 Bump clap_complete from 3.1.2 to 3.1.3 (#377)
d85d03d Bump log from 0.4.16 to 0.4.17 (#382)
dc3b7ef Bump tokio from 1.18.0 to 1.18.1 (#383)
12440c1 Bump serde_json from 1.0.79 to 1.0.80 (#376)
731042f Bump tower-http from 0.3.1 to 0.3.2 (#378)
82505e6 Bump clap from 3.1.12 to 3.1.15 (#381)
e05c19d Add Chinese documentation translation & Fix spelling mistakes (#373)
6e280e2 Add Russian documentation translation (#365)
40efdd1 Bump http from 0.2.6 to 0.2.7 (#368)
8bc5bec Bump tower-http from 0.3.0 to 0.3.1 (#367)
172ac8d Create FUNDING.yml
7cdd00b Bump tokio from 1.17.0 to 1.18.0 (#357)
9d2e9ea Search: Allow specifiying the limited of returned entries (#364)
93ab4e7 ignore JetBrains IDEs, tidy-up imports (#348)
2cb4cb3 Bump axum from 0.5.3 to 0.5.4 (#355)
796644e Add created_at column to users (#354)
f8233bc SQLx cannot run this migration OK (#353)
d8ef5dd fix db range query (#351)
5926ea6 fix import auto for bash (#352)
43d299f bump tui (#346)
8ac6571 Remove all select * from the server queries (#347)
4030de4 Add btree index on history table (#345)
b692e0c Bump tower-http from 0.2.5 to 0.3.0 (#343)
3680f4a Bump clap from 3.1.11 to 3.1.12 (#342)
7f5310a history list (#340)
@ellie ellie mentioned this pull request Jun 6, 2022
Merged
ellie added a commit that referenced this pull request Jun 6, 2022
@ellie
Release v0.10.0 (#440)
fab8d85 
06ac958 Show current version on server index (#436)
706b1af Disable ARM docker builds (#438)
f2abc23 Update README.md
3c2b055 Noyez fix dir hostname utf8 (#430)
3f5350d [feature] Add scroll wheel support to interactive history search (#435)
dcdde22 Fix text outline for dark mode
9ac0c60 Implement cursor (#412)
119ab9e Adds password prompt for register and login (#424)
e5df809 Noyez zsh histdb import (#393)
b08e254 Improve default fish keybindings (#420)
4096c6e Update README.md
cd2a3ab Add fish shell to key binding docs (#418)
b278211 Bump clap_complete from 3.1.3 to 3.1.4 (#397)
ee66c0a Bump axum from 0.5.5 to 0.5.6 (#415)
4297e26 Bump tokio from 1.18.1 to 1.18.2 (#396)
dbd9ca5 Bump clap from 3.1.16 to 3.1.18 (#401)
a7c9d19 Bump tower-http from 0.3.2 to 0.3.3 (#399)
3b79f68 Bump axum from 0.5.4 to 0.5.5 (#402)
f340771 Cleanup dependencies – disable unnecessary or unused features (#407)
ab294cd Don't pollute shell environment - remove 'id' variable (#408)
14b3060 Allow to build atuin server without client (#404)
5e4e8d1 Don't create config dir for server in default location if not needed (#406)
b7946cc Update Chinese version README.md (#403)
e0291f6 Update README.md
301190e Build ARM docker image in GitHub Actions using QEMU (#400)
1d030b9 Importer V3 (#395)
d3a4ff9 Bump clap from 3.1.15 to 3.1.16 (#392)
e9d2ec4 Add ctrl-k and ctrl-j for up and down (#394)
25afb5b Bump serde_json from 1.0.80 to 1.0.81 (#387)
4a839da Adds stats summary  (#384)
7a394b0 Bump serde from 1.0.136 to 1.0.137 (#375)
edd3f81 Bump clap_complete from 3.1.2 to 3.1.3 (#377)
d85d03d Bump log from 0.4.16 to 0.4.17 (#382)
dc3b7ef Bump tokio from 1.18.0 to 1.18.1 (#383)
12440c1 Bump serde_json from 1.0.79 to 1.0.80 (#376)
731042f Bump tower-http from 0.3.1 to 0.3.2 (#378)
82505e6 Bump clap from 3.1.12 to 3.1.15 (#381)
e05c19d Add Chinese documentation translation & Fix spelling mistakes (#373)
6e280e2 Add Russian documentation translation (#365)
40efdd1 Bump http from 0.2.6 to 0.2.7 (#368)
8bc5bec Bump tower-http from 0.3.0 to 0.3.1 (#367)
172ac8d Create FUNDING.yml
7cdd00b Bump tokio from 1.17.0 to 1.18.0 (#357)
9d2e9ea Search: Allow specifiying the limited of returned entries (#364)
93ab4e7 ignore JetBrains IDEs, tidy-up imports (#348)
2cb4cb3 Bump axum from 0.5.3 to 0.5.4 (#355)
796644e Add created_at column to users (#354)
f8233bc SQLx cannot run this migration OK (#353)
d8ef5dd fix db range query (#351)
5926ea6 fix import auto for bash (#352)
43d299f bump tui (#346)
8ac6571 Remove all select * from the server queries (#347)
4030de4 Add btree index on history table (#345)
b692e0c Bump tower-http from 0.2.5 to 0.3.0 (#343)
3680f4a Bump clap from 3.1.11 to 3.1.12 (#342)
7f5310a history list (#340)
@s0 s0 mentioned this pull request Jun 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@conradludgate conradludgate conradludgate approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@notsatyarth @conradludgate @panekj @ellie