vorta icon indicating copy to clipboard operation
vorta copied to clipboard

Add the change passphrase feature

Open VandalByte opened this issue 8 months ago • 9 comments

Description

This PR intends to add the much requested and discussed functionality to change passphrase in vorta UI.

Related Issue

Closes #303

Motivation and Context

This change adds the ability to change the passphrase directly within the Vorta backup client through a user-friendly interface. Previously, users had to rely on manual borg command-line operations to update their passphrase. This simplifies the process, improving usability by allowing users to easily update their passphrase without leaving the Vorta interface.

How Has This Been Tested?

Manual testing and unit testing.

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] I have read the CONTRIBUTING guide.
  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

VandalByte avatar Mar 31 '25 19:03 VandalByte

This is the design I thought to got with, I have considered a way to logically group this in a new drop down button.

  • This button serves more like a utility button for the selected repo.
  • Currently, the 'Change Passphrase' option is being added, but the dropdown can be easily expanded to include other options in the future (e.g., 'Change Repo Name' feature).
  • I may adjust the icon based on feedback.

I have read the requirements mentioned in #303 by everyone and will try to do this the best way possible. Any suggestions regarding the UI design or functionality are always welcome!

Screenshot 2025-04-01 003733

VandalByte avatar Mar 31 '25 19:03 VandalByte

How about moving more lesser-used features (like unlink) into the submenu?

and I'd use 3 dots as the submenu icon. I feel it's more widely used. The cog icon usually means Settings.

m3nu avatar Apr 03 '25 13:04 m3nu

How about moving more lesser-used features (like unlink) into the submenu?

I can do that.

and I'd use 3 dots as the submenu icon. I feel it's more widely used. The cog icon usually means Settings.

Yeah, so I was actually stuck with these two, I feel like the horizontal one looks a bit better. But if we want to go with vertical one then there's already a 3 dot present ellipse-v.svg we can reuse that icon, and I can't find a horizontal version of that, I'll have to rotate and make an new svg in that case.

Screenshot 2025-04-03 202046

The ellipse-vertical image

Icons used were sourced from Google Material Icons

VandalByte avatar Apr 03 '25 14:04 VandalByte

I have moved the unlink, should I keep the icon or remove it?

image

VandalByte avatar Apr 03 '25 15:04 VandalByte

I would either do a button for both or none.

m3nu avatar Apr 03 '25 23:04 m3nu

@m3nu I'd like your opinion on a few things. The current implementation changes the passphrase for the currently selected repo (same behavior as how unlink works).

  1. At the moment, I'm not validating the current passphrase (i.e., checking whether the old passphrase entered is correct before applying the new passphrase). I'm just directly setting the new one.

    • I felt this might be fine, since from what I saw, vorta when adding an existing repo takes the passphrase from the keyring and applies it, rather than asking the user to enter and confirm it against the keyring value before adding the repo.
  2. For the confirmation message, ie, when passphrase was successfully changed, i have two methods in mind:

    • First, show a message right below the errortext validation, green if successful red if failed.
    • Second, show a dialog with an "OK" button containing the message.

I am thinking of going with the 1st option, to avoid the need for too many dialogs popping up.

This is the screenshot of the currently implemented UI: Screenshot 2025-04-12 110937

VandalByte avatar Apr 12 '25 05:04 VandalByte

I'm just directly setting the new one.

Can you change the passphrase without knowing the existing one? I doubt it.

Still better to run only one command in total for this and throw an error if it fails for any reason (issue with connecting to repo or wrong passphrase or anything else).

Second, show a dialog with an "OK" button containing the message.

This one is probably better because the user isn't left with an open dialog.

m3nu avatar Apr 12 '25 10:04 m3nu

https://github.com/user-attachments/assets/bb40da52-30a6-4bd6-bf61-7629932beb51

VandalByte avatar May 03 '25 04:05 VandalByte

Added passphrase change only for encryption type = repokey, as mentioned in the comment.

VandalByte avatar May 03 '25 07:05 VandalByte

Sorry for the delay in reviewing this. Just ran this locally and think this can be improved in the following ways:

  • It won't update the passphrase in the keychain after changing it. As a result it keeps using the old one and everything fails after changing the passphrase.
Screenshot 2025-06-28 at 07 50 32 Screenshot 2025-06-28 at 07 50 56
  • The space in the dialog could be used more efficiently. And the note at the bottom is cut off:
Screenshot 2025-06-28 at 07 48 45

m3nu avatar Jun 28 '25 06:06 m3nu

The UI part I made some adjustments for the bottom label property and overall widget layout was changed from grid to vertical. The input fields are inside a form layout idk why it's showing up misaligned (labels would be left aligned) in your screenshot. I also noticed you are missing the widget title "Change Passphrase" and the close button at the top. This is how it looks for me (also resizes flawlessly) in MX Linux XFCE.

image

It won't update the passphrase in the keychain after changing it. As a result it keeps using the old one and everything fails after changing the passphrase.

Now about the error, could you tell me how to reproduce this? because I couldn't.

These are the steps I tried:

  • I changed passphrase for a local repo and changed it again for the same repo.
  • I changed the passphrase quit vorta and started vorta again and started a manual backup.

Is this maybe a remote repo specific issue? Could you test it out with a local repo and let me know if it works?

VandalByte avatar Jun 28 '25 12:06 VandalByte

It's not due to the remote repo. I check the macOS keychain and it still had the old password after the change. After changing the password manually, it worked again.

So it might be macOS-specific. Could also be with keychain permissions.

m3nu avatar Jun 29 '25 08:06 m3nu

I might have missed the part to update the keyring since it worked properly in linux during testing (for some reason). I have updated the PR, can you verify? Thanks

VandalByte avatar Jul 01 '25 05:07 VandalByte

Nope. Password is still unchanged in keyring.

Quite possible that our current macos code doesn't allow overwriting an existing item. The method we use is SecKeychainAddGenericPassword.

Does it work on Linux? How did it work on Linux without this code?

I guess we need to add a delete_password() method for all keyring classes to make this work. The only one that works for sure is our internal db.py class, which already uses get_or_create().

m3nu avatar Jul 01 '25 10:07 m3nu

Does it work on Linux? How did it work on Linux without this code?

Yeah sorry, my mistake, I had the keyring code written in the borg/change_passphrase.py, I was looking at the wrong file.

I guess we need to add a delete_password() method for all keyring classes to make this work. The only one that works for sure is our internal db.py class, which already uses get_or_create().

I've added some changes to consider this. It works ok in Linux (I've tested it out). I've added the macOS code as well in darwin.py, it just needs testing.

VandalByte avatar Jul 01 '25 12:07 VandalByte

OK. Will test.

One other thing I had noticed, but forgot to add: The icon for this dialog could be set to type warning or confirmation or something. It may use some default right now. Just whatever we use in other places.

Screenshot 2025-07-01 at 11 18 46

m3nu avatar Jul 02 '25 18:07 m3nu

I've added the appropriate info and warning icon properties to the message boxes. The icons should now display correctly.

VandalByte avatar Jul 04 '25 01:07 VandalByte

I can confirm that the latest commits ensure that the password is updated in the keychain.

For the UI and dialog icon it's unchanged.

Screenshot 2025-07-04 at 17 11 11 Screenshot 2025-07-04 at 17 11 21

m3nu avatar Jul 04 '25 16:07 m3nu

Recent changes include:

  • Made the NOTE label expanding horizontally and added word wrap.
  • Added form property to make the LineEdits expand (FieldGrowthPolicy as ExpandingFieldsGrow).
  • Minor changes like margin and alignment.

The changes has been tested in MX Linux 22 (XFCE).

VandalByte avatar Jul 05 '25 14:07 VandalByte

This new feature works as expected locally and could be merged right away.

I would just add 2 things (which could be in a separate PR, if this one is getting too big or blocking something):

  • At least a simple unit test to make sure this keeps working after future changes.
  • I would show the repo URL in the dialog somewhere so it's always clear which passphrase is updated. (given how important this is to the user)

m3nu avatar Jul 06 '25 20:07 m3nu

All the requested changes have been included. This should be good to go.

Screenshot 2025-07-07 035759

VandalByte avatar Jul 11 '25 14:07 VandalByte

Great! Will test one more time and then merge.

m3nu avatar Jul 11 '25 14:07 m3nu