Read several passwords from reader

[dvermd]

For testing purpose, I'm migrating calls from prompt_password_stdout to read_password_with_reader. I'm facing a problem with the signature

pub fn read_password_with_reader<T>(source: Option<T>) -> ::std::io::Result<String>
    where T: ::std::io::BufRead {

which "consumes" the BufRead and doesn't make it possible to be called several times with the same reader.

Would you consider changing the signature to

fn read_password_with_reader<T>(source: Option<&mut T>) -> ::std::io::Result<String>
where
    T: ::std::io::BufRead,

instead ?

[conradkleinespel]

@dvermd Thanks for your suggestion! We could totally do this, although it would require a bit of trickery to not break code without proper upgrade path. So before we get into that, I have a question.

In your tests, what is the reason for re-using the same reader instead of creating a brand new one for each test? Is it because you call read_password_with_reader several times from the same function?

[dvermd]

The function I'd like to test is something like

fn get_keypair(keys: &mut KeyPair) -> Result<(), Error> {
    let password1 = rpassword.prompt_password_stdout("password1: ")?;
    let password2 = rpassword.prompt_password_stdout("password2: ")?;
    keys = do_some_crypto_computation(password1, password2)?;
    Ok(())
}

And I was trying to change it to something like

fn get_keypair(mut reader: BufRead) -> Result<KeyPair, Error> {
    print!("password1: ");
    let password1 = rpassword.read_password_with_reader(Some(reader))?;
    print!("\npassword2: ");
    let password2 = rpassword.read_password_with_reader(Some(reader))?;
    let keys = do_some_crypto_computation(password1, password2)?;
    Ok(keys)
}

which does not compile.

I'm working on a testing solution that will work without changing rpassword code.

trait Bufferize<'a>: Read {
    type Buffered: BufRead + 'a;
    fn bufferize(&'a self) -> Self::Buffered;
}

impl<'a> Bufferize<'a> for Stdin {
    type Buffered = StdinLock<'a>;
    fn bufferize(&'a self) -> Self::Buffered {
        self.lock()
    }
}

fn get_keypair<'a, R>(reader: &'a R) -> Result<KeyPair, Error>
where
    R: Bufferize<'a>
{
    print!("password1: ");
    let salt = rpassword::read_password_with_reader(Some(reader.bufferize()))?;
    print!("\npassword2: ");
    let password = rpassword::read_password_with_reader(Some(reader.bufferize()))?;
    let keys = do_some_crypto_computation(password1, password2)?;
    Ok(keys)
}

I don't yet wrote the struct for testing purpose, it'll be a struct implementing the Bufferize trait

We may hold this issue for some time until I come up with a working solution. Then either change the rpassword code or add an example.

[conradkleinespel]

@dvermd Thanks for explaining in such detail :+1:

I'm all for integrating the changes you suggested in the first place. That'll make testing way easier. So if you make a PR for that, I'll be happy to merge. Here's how we'd go about integrating those changes all the while minimizing upgrade effort for existing users:

1. Add a new function read_password_with_reader_mut<T>(source: Option<&mut T>), mark read_password_with_reader as deprecated ("Should use read_password_with_reader_mut instead of read_password_with_reader") and bump the minor version to 4.1.0
2. Rename read_password_with_reader_mut to read_password_with_reader and bump the major version to 5.0.0

This will allow people to have the following simple upgrade path:

1. upgrade to 4.1
2. fix deprecations
3. upgrade to 5.0

This means we need 2 PRs:

• one that adds the new function and bumps to 4.1
• one that renames the new function to the name of the old one and bumps to 5.0

Would that be OK with you ?

[conradkleinespel]

@dvermd Another option would be to simply add a new function to fit your needs, I'm not sure if that makes a lot of sense though. From your explanation, I'm not sure how the current signature allows for much testing, even though we added this function specifically to enable easier testing, from what I remember.

[dvermd]

@conradkleinespel The upgrate path you describe seems fine to me. Users would have to update their code twice but they'll have time to manage thess updates.

Another way to add this change is to add a feature or a cfg flag. This way existing users would continue to use the current API and new ones or those willing to add mock to their code can enable the feature. English not being my main language, I can't come up with a meaningful feature name besides enhanced_mock or extended_mock. Anything you pick might be better than that.

Once we're OK on the way to add this, I'll give a try at implementing it.

[dvermd]

Here is a draft PR with the feature option to discuss about the principle. It may get completely reworked if it's not the option you choose.

[dvermd]

@conradkleinespel I'm facing a problem with read_password_with_reader that when the reader is stdin the password is displayed on the console because of keyboard echoing.

There's another way to add tests without changing rpassword API by adding an indirection in the caller's code. There are some examples in this branch.

With these examples, there's another way to close this issue.

[conradkleinespel]

The changes have been merged :heavy_check_mark: