bartificer / xkpasswd-js

The official JavaScript port of the Crypt::HSXKPasswd Perl module.
https://bartificer.github.io/xkpasswd-js/
BSD 2-Clause "Simplified" License
62 stars 11 forks source link

Random Character Separator lets you edit the alphabet but then it throws an error #81

Closed podfeet closed 7 months ago

podfeet commented 8 months ago

The Alphabet field is prefilled with something like -+=.*_| but you can try to edit those characters. But you can't ACTUALLY edit the characters, it throws an error that says "Enter at least 2 characters and no more than 6". Notably that error also hangs around even after you switch to other separator options. You can even get two errors thrown that are conflicting.

Seems it should let you edit or not let you edit.

2 errors - one says enter 1 character, the other says at least 2 and no more than 6
del-leehopper commented 8 months ago

A similar issue is already open here: https://github.com/bartificer/xkpasswd-js/issues/72

podfeet commented 8 months ago

Thanks - you did a good job of documenting it from the code side. I show pretty pictures of the symptoms!

hepabolu commented 7 months ago

Max length of alphabet set to 20

podfeet commented 7 months ago

Shouldn't minimum be 1 if you choose specified character? I can see 2 for random but not specified. Also note it has a green check mark and an error at the same time.

image

hepabolu commented 7 months ago

Grrr, the error is from the (hidden) alphabet

hepabolu commented 7 months ago

I cannot reproduce your issue. When I follow the following steps, the error messages appear/disappear as expected:

Repeat for the padding alphabet. Same behavior.

Does this issue occur in the iphone simulator? If so, could you please check on your phone or in a desktop browser?

podfeet commented 7 months ago

Ah - I did it in an odd order. Select defaultDon't change to specified character Try to delete all the characters but 1Throws error as expected Switch to specified character Green check mark and errorAllisonOn Apr 6, 2024, at 9:27 AM, Helma van der Linden @.***> wrote: I cannot reproduce your issue. When I follow the following steps, the error messages appear/disappear as expected:

select DEFAULT preset (just to have alphabets) remove all characters in the separator alphabet field you will get an error message to add at least 2 characters switch the separator type to 'specified character' the alphabet field is replaced with the 'character' field the alphabet error message disappears and is replaced with an error message to enter exactly 1 character

Repeat for the padding alphabet

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

hepabolu commented 7 months ago

Fixed

podfeet commented 7 months ago

I guess I'll allow this solution 🤪. With one character left and error on random, if I switch to specified it erases the single digit I'd chosen but keeps the error.

hepabolu commented 7 months ago

Are you sure? I followed your exact steps and in my case the error changes from 'at least 2 characters' to 'exactly one character' which is the correct error message for an empty 'specified character'

podfeet commented 7 months ago

I'm saying that it deletes the single character when I switch to specified, so there will be an error. If the single character stayed, then there would be no error. So the steps are:

hepabolu commented 7 months ago

The 'specified character' and the 'alphabet' fields are two separate fields. They are visible or hidden based on the selected type. So there is no 'deletion' of the character, it is simply hidden along with the rest of the field (inputbox and label). This is done to avoid making the list of settings even longer (and also mimics the behavior of the old version).

You could argue that a single character in the alphabet field should be copied to the 'specified character' field, but that algorithm would be very convoluted. Should it be copied when there is only one character? What if there are 2 characters? Should they be copied, or only one, if so which one? What if you already entered a character in the specified character field and you wanted to take a peek at the alphabet and changed your mind and switched back. Do you want your previously entered character to be there or overwritten with the one you just entered in the alphabet field? And if you have multiple characters in the alphabet field, nothing is copied. That is inconsistent behavior which is also confusing.

podfeet commented 7 months ago

This makes my head spin. I never noticed that Random Character used "Alphabet" and Specified Character used "Character". What does that even mean? what's a member of an alphabet and what's a character? Why are they different? Why aren't they both "character"?

I'm sure from a programmatic standpoint this has very distinct meanings but to the casual user I don't know why they're different. It seems to me I'm either going to specify a single character to separate my words and padding, or I'm going to let the program choose from a list of characters randomly. Alphabet implies A-Z, which the random characters don't even use.

As I said, head spinning.

podfeet commented 7 months ago

I've reread what you wrote (3 times) and I think I get what you're saying, so I can live with it. But I still think this alphabet/character thing is very confusing to the user.

I managed to confuse it yet again.

  1. choose default
  2. open settings
  3. Delete all characters in separator (sorry, alphabets?) but one
  4. Throws an error
  5. change to specified character
  6. throws an error (it ALWAYS throws an error btw, before giving a chance to enter a character)
  7. type in a character (I used #)
  8. Green checkmark but red error banner stays.

CleanShot 2024-04-11 at 21 16 37@2x

del-leehopper commented 7 months ago

Can I make a suggestion that will hopefully help everyone:

  1. Change the "frontend" to a single field with a field name of "Random Character" (i.e. you can no-longer select between "Specified Character" and "Random Character").

  2. If the field has one character, use the single character every time (i.e. same functionality as the "Specified Character" field, but using only the "Random Character" field).

  3. If the field has more than one character, it will use a random character from that list (i.e. the current functionality).

  4. To enable backwards compatibility (i.e. if someone uses the "Import Config" function), change this function so that if someone tries to import to the "Specified Character" field, it will map to the "Random Character" field. However, if this config is then Exported, it will be using the "Random Character" field, not the old "Specified Character" field.

I don't think this is a massive change, but I agree with @podfeet the current way is quite confusing for the end user, and only benefits a tiny number of people (of which it will continue to work as it currently does with my simplified suggestion).

podfeet commented 7 months ago

That's a pretty good idea @del-leehopper. It would definitely be less confusing for the new user. For the existing user, it might seem that the specified character has been removed. I'll try to think of an elegant way to put a little "I" that explains to remove all but one character to make it a specified character. I'd sure want that field to be called character, not alphabet if @hepabolu agrees.

bbusschots commented 7 months ago

I really like the approach suggested by @del-leehopper, I think the same concept needs to go up-stream to v2 of the config spec, so created issue #85 to track it