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

qml: rework auth #8382

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

Conversation

accumulator
Copy link
Member

now handles keystore-only password and defines a new set of auth methods.

@accumulator accumulator force-pushed the auth_rework branch 2 times, most recently from 6abce44 to 78f0122 Compare May 3, 2023 19:12
@accumulator accumulator marked this pull request as ready for review May 3, 2023 19:50
@ecdsa
Copy link
Member

ecdsa commented May 4, 2023

This looks complicated, and I don't have time to review it today.
I think we can consider it for the next major release, but not as a bug fix to be deployed quickly.
To fix #8374, why not simply ask for the password unconditionally on startup?

@accumulator
Copy link
Member Author

This looks complicated, and I don't have time to review it today. I think we can consider it for the next major release, but not as a bug fix to be deployed quickly. To fix #8374, why not simply ask for the password unconditionally on startup?

This is actually less impact than the alternative, which would need a refactor of the wallet loading logic in relation to qewalletdb, expand its scope beyond just wallet db stuff (qewalletdb is used to trigger whether a password is needed before opening the wallet, amongst other things).
Also it is IMO an improvement because it allows the auth layer to pass along (transient) passwords or other secrets without the decorated objects having to handle/assume passwords themselves. This could also be used for HWW unlock, passphrases etc.

This PR stays much closer to what e.g. the desktop client does, doesn't need refactoring of the qml wallet loading logic, does not need a password at startup for keystore-only passwords (might even be a feature instead of a bug?)

Finally, I don't think I can fully implement and test the alternative today, so that would mean a delay until after the weekend

SomberNight added a commit to SomberNight/electrum that referenced this pull request May 4, 2023
… wallets

This is a hugely hackish -- it uses the kivy approach, which uses this same hack...
I am not really content with it but it should be relatively easy to review,
and if ok, should hotfix the linked issue.

fixes spesmilo#8374
related spesmilo#8382
return False

try:
self.wallet.keystore.check_password(password)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this always behaves correctly, e.g. for a multisig wallet that has multiple keystores. This only checks the first keystore. I would prefer if we could just use wallet.check_password() directly, which has this logic encapsulated. None of the other GUIs call keystore.check_password() AFAICT.

Copy link
Member Author

@accumulator accumulator May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, but wallet.check_password() directly calls wallet.keystore.check_password() so I'm not sure why this would be problematic

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

Successfully merging this pull request may close these issues.

3 participants