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

Replace plaintext prompt with a password prompt #17

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ternite
Copy link

@ternite ternite commented Jul 27, 2022

This approach replaces the functionverifyDecrypt (which uses plaintext prompt) with an asynchronous solution that inserts an input field with a type of value password. Such an input field supports masking of the input value, while JavaScript's prompt() does not.

While merging the code, please consider the following additional information:

  • the old function verifyDecrypt(ctext,lock,key) is no longer used in the new context. It's commented out and probably should be deleted!
  • Apparently, there is a lot of dead code in script.js. My assumption is, that all of the following code can be deleted:
    • verifyDecrypt is now called in only one function: decryptBlock(..).
    • This function is only called in decryptMixedText(..),
    • while that function is only called in decryptEditForm(..).
    • Finally, this function is added as an event handler in the function decryptEditSetup(..), which itself is called only in line 7, which is commented out.

resolves #4

@syntaxseed
Copy link
Owner

Hi. Thanks for the contribution! I will look these PRs over when I return mid Aug.

@syntaxseed
Copy link
Owner

Hello,

Tested this and the new password overlay does work when decrypting on a page. However when editing a page the DecryptSecret button doesn't work - throws an verifyDecrypt is not defined error.

Also creating a new <SECRET> block and saving the page actually wipes the page (or section) you are editing completely and you have to restore from a previous revision.

@ternite
Copy link
Author

ternite commented Aug 8, 2022

I'll have a look at this and will get back in touch. :)

@syntaxseed
Copy link
Owner

Also... needs rebase onto latest version that incorporates the copy to clipboard feature.

@ternite
Copy link
Author

ternite commented Aug 8, 2022

Also... needs rebase onto latest version that incorporates the copy to clipboard feature.

I'll get that on track, too. Can't promise it will still be this week, but I'll try. ;)

@syntaxseed
Copy link
Owner

Also... needs rebase onto latest version that incorporates the copy to clipboard feature.

I'll get that on track, too. Can't promise it will still be this week, but I'll try. ;)

No rush... Just got back from vacay so I'm drowning in work. lol

ternite and others added 14 commits August 9, 2022 07:55
PR syntaxseed#16 - settings to copy contents to clipboard on decrypt.
PR syntaxseed#16 - settings to copy contents to clipboard on decrypt.
this new callback is should be specific to the need at hand and will insert the decrypted text into the correct location in the DOM tree
Asking for passwords now don't use a synchronous plaintext prompt, but an HTML input field that supports hiding password characters, but needs to be implemented asynchronously.

There are still some issues with this code, like:
* Using the enter key while entering an encryption password is passed through to the password verification input.
* Trying to decrypt a password by clicking on 'Decrypt Encrypted Text' fails to do the decryption.
@ternite
Copy link
Author

ternite commented Aug 15, 2022

Hello syntaxseed,

I made a complete overhaul of script.js. I had to convert the original code (100% synchronous event handling) into a program flow that supports asynchronous handling.

Most of the work went into one big bulk commit: 2473cb6

I had a lot of hassle with the sheer size of script.js, so I decided to move all encryption related code into two files:

  • crypto_high-level.js - this one contains code that is more or less specific to dokucrypt2, but is not related to event handling. All functions there can be used in a synchronous context.
  • crypto_low-level.js - this one contains functions that provide the encryption algorithms. Also with a synchronous workflow.

Unfortunately, this decision probably causes this pull request to be harder to merge. Additional information for the merge review: crypto_low-level.js should contain code from the original script.js that was moved there 100% copy'n'paste. Also, most of the code in crypto_high-level.js was not changed at all, but I believe there were some minor changes.

I got a bit lost with the "lock" that is to be used to differentiate between several password spheres (at least that's what I suppose). After my rework, this functionality is still coded, but probably non-functional. I'm not sure if there ever was a working user interface to set another lock than 'default'. I'm pretty sure that my code version will not work properly if lock is manually set to anything else but "default".

@syntaxseed
Copy link
Owner

Thanks @ternite for all this work. I plan to review this fully and possibly just overwrite the entire plugin. It was always a mess anyway.

It will take me a whilte to get a block of time to look on this, but I will get to it.

@syntaxseed syntaxseed self-assigned this Aug 15, 2022
@syntaxseed
Copy link
Owner

So.... I have a big question for you @ternite .... what are your feelings about turning your new changes into a Fork, say 'DokuCrypt3' and making it the successor of this project?

I have to be quite honest.... I forked the original in order to patch a few bugs and keep it working for my own use. But I never really took the time to fully understand the code completely. Encryption is not an area that I have significant expertise in, and personally, I've now migrated the content I was originally using dokuCrypt for, into a password manager. So I don't have a lot of stake nor interest in continuing to maintain the plugin. I think a maintainer who actively still uses the plugin is a good thing.

For me to merge your changes, I'd be making a lot of assumptions and am not confident I could 'put my name to it' so to speak. If you wanted to create a fork, I would archive this repo, document your fork as the new official successor and also update the Dokuwiki plugin page. You'd become the new maintainer of Dokucrypt.

Let me know your thoughts. I have unfortunately not had time to look at this PR yet.

@ternite
Copy link
Author

ternite commented Oct 25, 2022

Still thinking about it. Reason is the same as yours: "I forked the original in order to patch a few bugs and keep it working for my own use. But I never really took the time to fully understand the code completely."

I tend to keep using the plugin, so the plugin might actually be better off under my control. But I'm not eager to have another project to care for (not talking about github projects).

So I repeat: Still thinking about it.

@ternite
Copy link
Author

ternite commented Nov 10, 2022

@syntaxseed

ok, I guess I could do it. There's one or two things I wanted to do earlier, but refrained from because of the integration effort. I'd feel more free to touch such things when the code is under my control.

Now a technical aspect: I already created a fork - GitHub won't let me create another one. At least the name should probably contain the term "dokucrypt3". Any ideas? Is it possible to delete a fork, then create a new one?

@syntaxseed
Copy link
Owner

@syntaxseed

ok, I guess I could do it. There's one or two things I wanted to do earlier, but refrained from because of the integration effort. I'd feel more free to touch such things when the code is under my control.

Now a technical aspect: I already created a fork - GitHub won't let me create another one. At least the name should probably contain the term "dokucrypt3". Any ideas? Is it possible to delete a fork, then create a new one?

@ternite
Yes, you should be able to delete a fork. But yes I agree that the name Dokucrypt3 is a good idea.

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

Successfully merging this pull request may close these issues.

Mask/hide the password prompt box.
2 participants