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.

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

allow request for new key to confirm entered pass #702

Merged
merged 1 commit into from
Jun 25, 2023

Conversation

Aderemi-Adesada
Copy link
Contributor

allow request for new key to prompt user to confirm the password they entered

@istudyatuni
Copy link
Contributor

Maybe move these closures to separate function?

@Aderemi-Adesada
Copy link
Contributor Author

What is the best file to add the function so that both key.rs and init.rs can access

@istudyatuni
Copy link
Contributor

crates/rustic_core/src/password.rs, but I'm not sure

@aawsome
Copy link
Member

aawsome commented Jun 23, 2023

First, this PR IMO doesn't allow to confirm the password, but enforces it.
I personally wouldn't need that, but if you think this is a worthwhile addition, I'm open for it ;-)

About the place: Please not in rustic_core. This crate provides core functionality to access repositories and perform operations on it. All UI topics should stay outside - and this is for sure just a UI topic!

@Aderemi-Adesada
Copy link
Contributor Author

@istudyatuni @aawsome what do you think about having a function called prompt_password_twice in rustic/src/helpers that both init.rs and key.rs can call

@istudyatuni
Copy link
Contributor

I'm afraid to make those decisions in this project 😁

@aawsome
Copy link
Member

aawsome commented Jun 24, 2023

As discussed @Aderemi-Adesada on discord, I also think that the best idea is to replace rpassword with dialoguer which already implements this functionality.

@Aderemi-Adesada
Copy link
Contributor Author

rpassword has been replaced with dialoguer in init.rs and key.rs when prompting for new key

Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

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

Beside my remarks, I was wondering if we can remove the dependency on rpassword completely. Then I remembered some newline handling which needed to be dealt with.
Checking the code I realized that that fix already removed rpassword when reading from a file - but obviously I have overseen this in src/command/key.rs. Can you please have a look at read_password_from_reader in crates/rustic_core/src/repository.rs and use this in src/command/key.rs - this should allow to remove rpassword completely.

Moreover, can you please add an entry in changelog/new.txt and after your changes squash all commits into 1 commit? Thanks a lot!

src/commands/init.rs Outdated Show resolved Hide resolved
src/commands/init.rs Outdated Show resolved Hide resolved
@Aderemi-Adesada
Copy link
Contributor Author

rpassword have been removed completely and src/command/key.rs now uses read_password_from_reader as used in crates/rustic_core/src/repository.rs

@aawsome
Copy link
Member

aawsome commented Jun 25, 2023

@Aderemi-Adesada there are syntax errors in lib.rs - can you have a look at it?

@Aderemi-Adesada
Copy link
Contributor Author

syntax error

I will fix that right now

@Aderemi-Adesada
Copy link
Contributor Author

Aderemi-Adesada commented Jun 25, 2023

@aawsome syntax error have been fixed

@aawsome
Copy link
Member

aawsome commented Jun 25, 2023

Can you please rebase to main and squash all commits instead of re-merging master back into your branch? That will keep commit history clean. Thanks a lot!

@Aderemi-Adesada Aderemi-Adesada force-pushed the prompt_confirm_password branch 2 times, most recently from c89f198 to cdbe671 Compare June 25, 2023 19:22
@aawsome aawsome merged commit 4250bb2 into rustic-rs:main Jun 25, 2023
@Aderemi-Adesada Aderemi-Adesada deleted the prompt_confirm_password branch June 25, 2023 19:57
@aawsome
Copy link
Member

aawsome commented Jun 25, 2023

Oh, clicked wrong - actually wanted to run the tests.
Anyway, we'll see in the CI. Thanks a lot @Aderemi-Adesada - looks good to me!

@Aderemi-Adesada
Copy link
Contributor Author

You are welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants