Added visual feedback while hiding secret information

[cogutvalera]

Just little improvements. As discussed here bitshares/bitshares-core#1382
(as commented here also bitshares/bitshares-core#1382 (comment))

Please check this improvement. Thanks !

[troglobit]

Are you sure you want to output the exact number of spaces as characters are input in this mode? There are many examples of password input dialogs that output 1-3 * instead as input is read.

[cogutvalera]

Are you sure you want to output the exact number of spaces as characters are input in this mode? There are many examples of password input dialogs that output 1-3 * instead as input is read.

IMHO this solution with cursor representation works well enough as visual feedback by cursor.

Thanks for your time @troglobit ! A bit later when I will exactly know if BitShares cli_wallet requires this solution or another then I can answer to your question exactly (I have asked @pmconrad). Do you think current solution is not good enough ?

[troglobit]

@cogutvalera I have no opinion really, it was just an observation. You guys are the only users of this API atm, so you're free to define the initial implementation. I can merge the PR as-is, but let's await a reply from @pmconrad

[cogutvalera]

@troglobit Thank you very much ! Really appreciate your time and efforts ! Just wanted to know what do you think about current solution with visual feedback by cursor :)

[pmconrad]

IMO the behaviour is still strange and unexpected.

Simple usage is as expected, but if you move the cursor back and forth you can "erase" the visible part of the input line as well. I. e. you make it invisible although it's there.

You can also move the cursor back into the regexp-matching part and change it, so that it no longer matches. This will make the entire line visible again (which is OK), but if you change it back so that it matches, the stuff that should be invisible is still there. Until you move the cursor over it again, which will overwrite it with blanks.

[cogutvalera]

Thank you very much @pmconrad ! I will check it and improve soon.

[cogutvalera]

Simple usage is as expected, but if you move the cursor back and forth you can "erase" the visible part of the input line as well. I. e. you make it invisible although it's there.

@pmconrad sorry I cannot understand just this, what do you mean here ?

Thanks !

[pmconrad]

1. In BitShares cli_wallet, type unlock abc - display will be unlock
2. Press cursor left, type x - display will be unlock
3. Press cursor left 6 times, type x - display will be unloxck abxc
4. Press backspace - display will be unloxck abxc
5. Press cursor right - display will be unlo ck abxc (but line buffer contains unlock abxc)

[cogutvalera]

Thanks @pmconrad this I've understood ! I cannot understand what did you mean by:

move the cursor back and forth you can "erase" the visible par

If I move cursor back and forth nothing heppens, that's why I could NOT understand only this 1 point.

Thanks ! Now I understood what did you mean !

[troglobit]

I've looked into this now and I agree with @pmconrad, this is messy and I'm not sure the current implementation can be improved. So I'm leaning towards reverting it from editline.

@cogutvalera Have you looked into implementing something else for 'unlock' using the existing el_no_echo? I'd like to suggest adding another mode of operation for your CLI, e.g.; type a keyword to enter password-entry mode, change prompt and read password, allow user to input text (erasing with backspace or Ctrl-U), return to regular mode when user presses enter. Something like:

standard prompt> unlock
pwd>                    <-- set el_no_echo=1 before calling readline("pwd> ");
standard prompt>        <-- set el_no_echo=0 and call readline("standard prompt> ");

[cogutvalera]

Thank you very much for your idea ! I'm too thinking about another solution and have also some other ideas based on current merged code. Please do not revert it now, a bit later let's decide ... hope soon to finish with my solution or maybe with yours ... let's see soon ...

Thanks !

[troglobit]

OK, I'll hold off on the revert for now.

[cogutvalera]

Thanks a lot ! I'm working on it and hope soon to finish ...

[cogutvalera]

new commit is ready, check it please, added secret mode, I've tested and for me now it works well enough, hope you guys @troglobit and @pmconrad will like it too ;-)

[troglobit]

@cogutvalera This is quite an extensive change. Would you care to describe what it does and how this improves the usability of editline more than "new commit, please check"? For one, the commit no longer seems to match the original PR description, at all.

Also, the malloc() in the code, is it ever free()'d anywhere, or is it handled by the regular code? If so, that should probably be mentioned in a comment close to that malloc. And why the malloc + memcpy instead of strdup?

[cogutvalera]

@troglobit Thanks ! I've described code by comments in new commit, also now using strdup it is really simplify code and added forgotten free memory code, thanks !

[cogutvalera]

@troglobit is it better now ? do you have still any questions or miss-understandings maybe ?
Thanks for your time and efforts to review it ! Really appreciate it !

[troglobit]

No it's not. Maybe I wasn't clear, but I didn't want you to put a C++ comment on every line of this C project (no other part of the code uses C++ comments and I'd rather not spend more time cleaning up obvious coding style offenses). I simply wanted you to describe, in plain English, what your commits aims to achieve. I can read the code, I'm familiar with C, so commenting obvious code lines is not of interest to me.

What I am, and every other maintainer of an Open Source project is, interested in is what you want. There may be other ways to achieve what you want, there may be misunderstandings. Things that are easily explained by talking about it. Also, I questioned your reuse of this pull request, which initially was about moving the cursor on secret data, we've clearly moved beyond that by now, so maybe it's time to close this PR and restart afresh -- I'd really not like to pollute the commit history of this project with your experimental ideas. Please finish them and clean them up to easily reviewable changes s on your own end before submitting a new PR.

[cogutvalera]

ok sure I can rewrite commit history in this PR so it would be clean and easily reviewable changes or do you wish new PR only without rewriting and clean commit history here ?

[cogutvalera]

I've rewrited commit history so now it is clean and without comments.

The main idea of this PR is to add visual feedback for hiding secret information by entering security mode as you suggested earlier:

standard prompt> unlock
Enter secret: 
standard prompt> 

[cogutvalera]

IMHO it is pretty simple idea enough and very effective at the same time, works nice for me

[troglobit]

This last commit sure is better, codewise, no doubt. However, I'm not convinced any of it belongs as part of the editline library. So I'm leaning towards declining this PR, and also revert the previous code, please follow my reasoning below for an explanation:

1. editline is supposed to be small and provide only the bare necessities for reading line based input, like GNU readline (but without the requirement for the full ncurses stack)
2. These PRs looks a lot like application specific callbacks triggered by a specific code word, something I'd prefer application writers implement in their applications instead of in editline

Here is a modified version of main() from examples/cli.c which, as far as I can see, achieves the same thing your code extensions does, yet keeps the logic in the application. From all the PRs you've done, I've not seen one motivation from you why the following construct should not be sufficient:

int main(void)
{
    char *line;
    char *prompt = "cli> ";
    char *passwd = "Enter password: ";

    /* Setup callbacks */
    rl_set_complete_func(&my_rl_complete);
    rl_set_list_possib_func(&my_rl_list_possib);

    el_bind_key('?', list_possible);
    el_bind_key(CTL('C'), do_break);
    el_bind_key(CTL('D'), do_exit);
    el_bind_key(CTL('Z'), do_suspend);
    read_history(HISTORY);

    while ((line = readline(prompt))) {
	int next = 0;

	if (!strncmp(line, "unlock", 6)) {
	    el_no_echo = 1;
	    while ((line = readline(passwd))) {
		if (strncmp(line, "secret", 6)) {
		    printf("\nWrong password, please try again, it's secret.\n");
		    continue;
		}

		el_no_echo = 0;

		printf("\nAchievement unlocked!\n");
		next = 1;
		break;
	    }
	}

	if (next)
	    continue;

	printf("\t\t\t|%s|\n", line);
	free(line);
    }

    return 0;
}

The only constructs necessary, IMHO, are el_no_echo and calling readline() with a different prompt. If you disagree, then please provide motivation and reasoning to what value you perceive your code adds to editline as a library.

[cogutvalera]

It can break BitShares Core stuff that is used already by different 3rd parties, so I need to check your idea ...

Thanks !

[cogutvalera]

I've checked your main code that you've suggested and it doesn't work as required.
Thinking how it can be improved to work as required.
As for now in your code we need to press ENTER after typing unlock command, but it will break 3rd parties that are based on commands without ENTER key, my solution does it automatically without breaking 3rd parties.

[troglobit]

I see, well editline is a line based library. Adding magic to trigger el_no_echo = 1 based on keywords entered inside a line that has not yet been completed is really hard to make watertight without adding checks almost everywhere. I would strongly advise against it and try to change your CLI application instead.

In all honesty, I cannot justify that complexity in editline just for the sake of one user (application) of the library.

[cogutvalera]

ok sure, as you wish, Thanks !

[troglobit]

I'm sorry for denying your pull request(s) so late, considering all the work you've put in. Hope you can find a good solution for the application!

[cogutvalera]

this is a good solution, we just will fork this project with my changes, because we don't need to break our 3rd parties apps and services. No worries ! Thank you !