feat(wallets): restrict wallet creation with allow_registrations flag
#2203
Conversation
KDF startup throws an error if the `allow_registrations` config value is false and the seed is unknown (no name provided) or if a name is provided and it has not been registered before. This is used to emulate the sign-in vs register behaviour a typical auth service would provide.
CharlVS
changed the title
allow_registrations flag
CharlVS
changed the title
allow_registrations flag
|
@takenagain can you please assist with writing tests for this while I focus on other GUI tasks? |
Co-authored-by: Samuel Onoja <[email protected]>
|
@takenagain please also review borngraced's feedback about redundant comments and snip them. |
| // Importing an encrypted passphrase without a wallet name is not supported since it's not possible to save the passphrase. | ||
| (None, Some(Passphrase::Decrypted(passphrase))) => { | ||
| if ctx.allow_registrations() { | ||
| Ok(Some(passphrase)) |
There was a problem hiding this comment.
I want to understand the need of allow_registrations here, this is the legacy approach where we pass a passphrase without a wallet name, so no wallet is created from KDF side here.
I understand the need for it in other places, although I think it's redundant, it can have benefits from GUI side for clear paths between registrations and login but GUI should never allow it's users to hit an error from KDF side such as WalletInitError::WalletCreationNotAllowed if I understand correctly. So, I don't think I also understand it's need in other places :)
I will take over this PR and it's fixes once I understand the need for this.
There was a problem hiding this comment.
@shamardy I'm fine leaving this part out because it won't be used in our GUI, but it will mean we have to entirely deprecate non-name wallet logins in the KDF SDK I'm working on. However, leaving this in will allow both register and sign-in for non-name wallets, giving devs more flexibility to use the SDK as they wish. From what I can tell, there is no downside for us.
Either way, I'll work with whatever your call is. It's worth noting that this slightly akin to the anonymous login offered by conventional auth services like Firebase Auth and Supabase Auth.
There was a problem hiding this comment.
but it will mean we have to entirely deprecate non-name wallet logins in the KDF SDK I'm working on
non-name wallet logins still work the same way as before (before adding seed/mnemonic management in KDF) by just providing a passphrase with no wallet_name or wallet_password.
There was a problem hiding this comment.
KDF startup throws an error if the allow_registrations config value is false and the seed is unknown (no name provided) or if a name is provided and it has not been registered before. Defaults to true (current dev behaviour)
When did we add allow_registrations to the config file? I can't recall this field and it doesn't exist anywhere in the project. 🤔
| // Check if the wallet file already exists | ||
| if !wallet_path.exists() { | ||
| // If it doesn't exist and registrations are not allowed, return an error | ||
| if !ctx.allow_registrations() { |
There was a problem hiding this comment.
We can avoid the nested scope with:
| // Check if the wallet file already exists | |
| if !wallet_path.exists() { | |
| // If it doesn't exist and registrations are not allowed, return an error | |
| if !ctx.allow_registrations() { | |
| // Handle the case when the wallet file does not exist, and registrations are not allowed. | |
| if !wallet_path.exists() && !ctx.allow_registrations() { |
| // Check if the wallet already exists | ||
| let existing_wallet = table | ||
| .get_item_by_unique_index("wallet_name", wallet_name.to_string()) | ||
| .await?; | ||
|
|
||
| if existing_wallet.is_none() && !ctx.allow_registrations() { | ||
| return Err(MmError::new(WalletsDBError::Internal( | ||
| "Wallet creation is not allowed. Registrations are disabled.".to_string(), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
I would guard this logic with if !ctx.allow_registrations() to query the wallet table only when it's necessary.
KDF startup throws an error if the
allow_registrationsconfig value is false and the seed is unknown (no name provided) or if a name is provided and it has not been registered before. Defaults totrue(currentdevbehaviour)This is used to emulate the sign-in vs register behaviour a typical auth service would provide.
TODO: Tests