Tests leave key in keyring #1639

ganeshrevadi · 2023-03-09T16:12:15Z

Adding remove password abstract method in abc.py and implementing in secretstorage.py.
@real-yfprojects

real-yfprojects

Looking good! Next up would be KWallet. We use the dbus daemon exposed by KWallet. Refer to the Qt DBus docs how to use dbus via qt. You will have to do some research to find a documentation of set kwallet daemon.

src/vorta/keyring/secretstorage.py

real-yfprojects

Great, it works. Where did you find the removeEntry dbus method? Next up would be MacOS support which is quite hard without access to a Mac. We use the PyObj-C bridge allowing access to MacOS native libraries. You can refer to the apple developer docs.

src/vorta/keyring/kwallet.py

m3nu · 2023-03-12T08:17:51Z

I'll try macOS today.

m3nu · 2023-03-12T12:25:05Z

Added for macOS:

In [1]: from vorta.keyring import darwin

In [2]: k = darwin.VortaDarwinKeyring()

In [3]: k.get_password('vorta', 'google.com')

In [4]: k.set_password('vorta', 'google.com', 'asdf')

In [5]: k.get_password('vorta', 'google.com')
Out[5]: 'asdf'

In [6]: k.remove_password('vorta', 'google.com')

In [7]: k.get_password('vorta', 'google.com')

In [8]:

real-yfprojects · 2023-03-12T15:55:17Z

That's bad you just removed the commits containing MacOS support.

ganeshrevadi · 2023-03-12T16:03:01Z

That's bad you just removed the commits containing MacOS support.

Hey I'm really sorry how can this be fixed now?

m3nu · 2023-03-12T19:24:54Z

Rebased and pushed it again.

Please be careful with force-push and use it only when really needed.

real-yfprojects · 2023-03-12T19:48:51Z

And when you do use --force-with-lease.

…revadi/vorta into Test-leave-key-in-keyring

ganeshrevadi · 2023-03-13T03:09:55Z

And when you do use --force-with-lease.

when you update a branch changed by other people

ganeshrevadi · 2023-03-13T03:11:33Z

should remove_password next implemented in db.py?

src/vorta/keyring/db.py

m3nu · 2023-03-13T15:30:10Z

For macOS-related stuff it's needed because some libraries aren't available on linux (pyobjc).

In this case it could be due to circular dependencies. Needs to be reviewed. If no issues, put it at the start of the file, where it belongs.

ganeshrevadi · 2023-03-15T15:58:01Z

For macOS-related stuff it's needed because some libraries aren't available on linux (pyobjc).

In this case it could be due to circular dependencies. Needs to be reviewed. If no issues, put it at the start of the file, where it belongs.

I tried adding it above but its failing the pre-commit run!

real-yfprojects · 2023-03-15T16:22:19Z

I tried adding it above but its failing the pre-commit run!

As always, nobody can help you if you just say that it failed. You will have to provide more details. The error/complaint message is a minimum.

ganeshrevadi · 2023-03-16T14:49:01Z

I have imported the RepoPassword at beginning of the file and it works fine and sorry for the lack of info in above comment.

…revadi/vorta into Test-leave-key-in-keyring

ganeshrevadi · 2023-03-25T13:41:53Z

Not quite.

The keyring instance you create in the fixture isn't used in the test and Vorta will always make its own.

You wouldn't need to remove the password inside tests, if the fixture was working. keyring.remove_password('vorta-repo', test_repo_url)

Your fixture runs at the module level. So all tests in one file would use the same password and remove it??

So what you need is this:

Fixture that runs again for each test

Fixture that returns some random repo URL and random password for testing, as we do in some places: test_repo_url = f'vorta-test-repo.{uuid.uuid4()}.com:repo' # Random repo URL to avoid macOS keychain and password = str(uuid.uuid4())

The repo URL and password are then used during testing

After each test function the password for the test repo is removed again.

This should be more clear and deduplicates some more code (creating a random repo URL and password)

One more thing: vorta-test-repo.{uuid.uuid4()}.com:repo would give a SFTP-style URL that's not supported in the future. Best if you change it to ssh:// format.

This means that the repo url and password that are generated in the test has to be removed and only to be generated in the fixture and then used in multiple tests . Same question for remove_password ?

m3nu · 2023-03-25T14:51:14Z

Repo URL and password are generated by the fixture. The password manager entry is also removed by the fixture during cleanup.

ganeshrevadi · 2023-03-25T18:04:38Z

@m3nu I've tried implementing pytest fixture as per your suggestion , but the tests are failing. Need some help here!

ganeshrevadi · 2023-03-25T18:05:39Z

tests/test_repo.py

@@ -116,8 +110,6 @@ def test_repo_add_success(qapp, qtbot, mocker, borg_json_output, keyring_fixture
    keyring = VortaKeyring.get_keyring()
    assert keyring.get_password("vorta-repo", RepoModel.get(id=2).url) == LONG_PASSWORD


gives me a assertion error here.

In that case you should debug the test and see why it fails. Looking at the compared values would be a first good step.

stale · 2023-07-19T06:17:49Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

real-yfprojects · 2023-07-21T14:44:33Z

Are you still working on this @ganeshrevadi? This would be a big quality of life improvement for developers.

ganeshrevadi · 2023-07-21T14:51:38Z

@real-yfprojects Hey I'm currently not working on this!

Adding remove password

abb3ec8

ganeshrevadi changed the title ~~Adding remove password~~ Tests leave key in keyring Mar 9, 2023

real-yfprojects reviewed Mar 9, 2023

View reviewed changes

src/vorta/keyring/secretstorage.py Outdated Show resolved Hide resolved

ganeshrevadi added 2 commits March 10, 2023 23:29

remove pass method

527797c

Kwallet

6afdbbc

ganeshrevadi requested a review from real-yfprojects March 11, 2023 08:55

real-yfprojects reviewed Mar 11, 2023

View reviewed changes

src/vorta/keyring/kwallet.py Outdated Show resolved Hide resolved

Implement remove password for kwallet

cfd0ebf

ganeshrevadi force-pushed the Test-leave-key-in-keyring branch from fd26575 to cfd0ebf Compare March 12, 2023 15:51

ganeshrevadi force-pushed the Test-leave-key-in-keyring branch from cfd0ebf to 6afdbbc Compare March 12, 2023 16:01

ganeshrevadi and others added 2 commits March 12, 2023 21:33

Merge branch 'master' into Test-leave-key-in-keyring

1e17d31

Implement remove_password() for darwin

31f1048

Merge branch 'Test-leave-key-in-keyring' of https://github.com/ganesh…

f5a5a0b

…revadi/vorta into Test-leave-key-in-keyring

Remove password for db

3d27600

ganeshrevadi requested a review from real-yfprojects March 13, 2023 13:47

real-yfprojects reviewed Mar 13, 2023

View reviewed changes

src/vorta/keyring/db.py Outdated Show resolved Hide resolved

Remove password for db

923b410

ganeshrevadi requested a review from real-yfprojects March 15, 2023 16:45

ganeshrevadi added 5 commits March 25, 2023 13:12

remove_password

37c76e4

fixes

020973e

fixes2

a4c11b8

pytest-fixtures

9177435

pytest-fixtures2

ac42825

ganeshrevadi force-pushed the Test-leave-key-in-keyring branch from 55e6de6 to ac42825 Compare March 25, 2023 07:42

Merge branch 'Test-leave-key-in-keyring' of https://github.com/ganesh…

9abe861

…revadi/vorta into Test-leave-key-in-keyring

ganeshrevadi added 3 commits March 25, 2023 23:01

fixtures

f1cc8f4

fixtures-2

c910d61

fixtures-3

eb2e878

ganeshrevadi commented Mar 25, 2023

View reviewed changes

ganeshrevadi added 4 commits April 2, 2023 20:02

Merge branch 'master' into Test-leave-key-in-keyring

30201b1

fixing

099e9e4

fix2

1c21dd9

fix3

b6aad7a

m3nu mentioned this pull request Apr 17, 2023

PyQt6 Upgrade #1685

Merged

9 tasks

real-yfprojects mentioned this pull request May 24, 2023

fix "Tests leave key in keyring", #1215 #1584

Closed

real-yfprojects linked an issue Jun 27, 2023 that may be closed by this pull request

Tests leave key in keyring. #1215

Open

real-yfprojects mentioned this pull request Jun 27, 2023

Updated README.md #1739

Merged

9 tasks

stale bot added the wontfix label Jul 19, 2023

real-yfprojects added good first issue Simple change to start learning code base help wanted This issue is available, comment if you want to fix it and removed wontfix good first issue Simple change to start learning code base labels Jul 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests leave key in keyring #1639

Tests leave key in keyring #1639

ganeshrevadi commented Mar 9, 2023 •

edited

Loading

real-yfprojects left a comment

real-yfprojects left a comment

m3nu commented Mar 12, 2023

m3nu commented Mar 12, 2023

real-yfprojects commented Mar 12, 2023

ganeshrevadi commented Mar 12, 2023

m3nu commented Mar 12, 2023

real-yfprojects commented Mar 12, 2023

ganeshrevadi commented Mar 13, 2023

ganeshrevadi commented Mar 13, 2023 •

edited

Loading

m3nu commented Mar 13, 2023

ganeshrevadi commented Mar 15, 2023

real-yfprojects commented Mar 15, 2023

ganeshrevadi commented Mar 16, 2023

ganeshrevadi commented Mar 25, 2023

m3nu commented Mar 25, 2023

ganeshrevadi commented Mar 25, 2023

ganeshrevadi Mar 25, 2023

m3nu Mar 26, 2023

stale bot commented Jul 19, 2023

real-yfprojects commented Jul 21, 2023

ganeshrevadi commented Jul 21, 2023

		@@ -116,8 +110,6 @@ def test_repo_add_success(qapp, qtbot, mocker, borg_json_output, keyring_fixture
		keyring = VortaKeyring.get_keyring()
		assert keyring.get_password("vorta-repo", RepoModel.get(id=2).url) == LONG_PASSWORD

Tests leave key in keyring #1639

Are you sure you want to change the base?

Tests leave key in keyring #1639

Conversation

ganeshrevadi commented Mar 9, 2023 • edited Loading

real-yfprojects left a comment

Choose a reason for hiding this comment

real-yfprojects left a comment

Choose a reason for hiding this comment

m3nu commented Mar 12, 2023

m3nu commented Mar 12, 2023

real-yfprojects commented Mar 12, 2023

ganeshrevadi commented Mar 12, 2023

m3nu commented Mar 12, 2023

real-yfprojects commented Mar 12, 2023

ganeshrevadi commented Mar 13, 2023

ganeshrevadi commented Mar 13, 2023 • edited Loading

m3nu commented Mar 13, 2023

ganeshrevadi commented Mar 15, 2023

real-yfprojects commented Mar 15, 2023

ganeshrevadi commented Mar 16, 2023

ganeshrevadi commented Mar 25, 2023

m3nu commented Mar 25, 2023

ganeshrevadi commented Mar 25, 2023

ganeshrevadi Mar 25, 2023

Choose a reason for hiding this comment

m3nu Mar 26, 2023

Choose a reason for hiding this comment

stale bot commented Jul 19, 2023

real-yfprojects commented Jul 21, 2023

ganeshrevadi commented Jul 21, 2023

ganeshrevadi commented Mar 9, 2023 •

edited

Loading

ganeshrevadi commented Mar 13, 2023 •

edited

Loading