Make working with encrypted storage RAII

[flub]

This refactors the APIs to work with encrypted storage to follow the Resource Acquisition Is Initialisation principle. Having our structures behave like this is beneficial because it removes a lot of edge-cases that would need to be handled, in the end resulting in a more stable core.

This will break current UIs. In particular it does not attempt to transition as doing so makes things even worse. I think this tradeoff is desirable, and my apologies for not having the time to engage better at the time this was originally introduced.

Using the new FFI

dc_context_new_closed(), dc_context_is_open and dc_context_open are gone, if a context exists it is always open and usable.

dc_context_new_encrypted() can be used to create a new standalone context with encrypted storage.

dc_accounts_add_encrypted_account() creates a new account with encrypted storage when using the account manager.

dc_accounts_get_all() will now only return "loaded" contexts, when creating dc_accounts_t all unencrypted databases are automatically loaded.

dc_accounts_get_encrypted() will return all accounts which need to be decrypted before they can be used, dc_accounts_load_encrypted() does this. Once loaded the encrypted account will show up in dc_accounts_get_all() and no longer appear in dc_accounts_get_encrypted().

dc_accounts_get_selected() has been renamed to dc_accounts_get_selected_id() and now only returns the ID of the account. This lets you use dc_accounts_get_encrypted() and dc_accounts_load_encrypted() if it is an encrypted-not-yet-loaded account. Use the existing dc_accounts_get_account() to get the pointer to the context.

Design notes

Just like with the current implementation this can not distinguish between broken databases and encrypted ones. So if a corrupted database appears it will show up in dc_accounts_get_encrypted() forever.

I have not tried to make the sql module RAII internally and tried to keep the changes there minimal to support the RAII interface. This PR is more than large enough already and this can be done as a followup.

[link2xt]

Some API review, haven't looked at implementation yet.

[link2xt]

Can having a public error type and matching on errors be avoided?

E.g. return anyhow::Result<Option<Self>> from Context::new, with None meaning the key is probably wrong. Then in load_accounts you can use Context::new(...).with_context(|| "failed to create context from file {}", ..)?.

If you don't like Option you can make your own enum type, but errors are something that is either passed upwards with ? or written to logs if there is a fallback to be tried.

[flub]

Hum, going to have to sleep over this. I think with #[error(transparent)] this is a fairly nice option here that allows matching on just what we care about while still letting you propagate errors. I'm not sure if nested types are that much nicer.

[r10s]

if a context exists it is always open and usable.

just a quick feedback: i am a bit skeptical to this approach. as discussed in dev-chat already, that will cause troubles in all UIs as the context is a very basic object, more regarded as "the app" (logging, stockstrings, database) and not only a database. most things currently cannot exist without a context, Activities and ViewController expect them and pass them around etc. sure, changable, but that is effort and noise on all UIs. we should take that into account.

i also do not really like the removed "list all accounts" functionality - now the ui has to glue different lists together, also, doable, but additional effort - all other lists are "well prepared" for ui use. however, this point is probably easy changable. in general, i'd like to think about the ffi more about what the ui "needs".

i am wondering if we cannot leave the ffi as is and do the RAII-thinie rust-internally only. afaik, ffi-context is only a wrapper around rust-context - we we could probably change rust-context on opening, if needed. that would break RAII at some point, but that is a much more well defined point then - and, we can then see in the future to change the ffi.

otherwise, i fear, we will get in timing and stress issues right in march/april, when it comes to M2 ... there are already so many things to do in the UI.

[link2xt]

the context is a very basic object, more regarded as "the app" (logging, stockstrings, database) and not only a database

Logging is an internal thing, all the UI sees is an event emitter.
Stock strings are something you set once whenever an account is opened, this can't be done once at the start of the app only because sometimes new accounts are created.

most things currently cannot exist without a context, Activities and ViewController expect them and pass them around etc. sure, changable, but that is effort and noise on all UIs. we should take that into account.

Then UI will have to do what it does already: if there are no contexts available (because there are none or because they are all locked), create a new context just to display Welcome screen. It's even implemented already.

i also do not really like the removed "list all accounts" functionality - now the ui has to glue different lists together, also, duable, but additional effort - all other lists are "well prepared" for ui use. however, this point is probably easy changable. in general, i'd like to think about the ffi more about what the ui "needs".

Currently locked accounts are useless, you can't even get the display name as it will error out due to database being closed. We need a different UI for locked accounts anyway because they are indistinguishable from each other except for their account ID. Maybe display a counter saying how many locked accounts are there, so when user enters a passphrase we try to open all the locked accounts with it. Locked accounts are really different from the UI point of view. If we decide we want an unified list (which will require storing display name copy in TOML), then it will be not a list of contexts, but a list of new type dc_account_t which has limited number of methods just enough to display a list item and unlock it.

[r10s]

sure, everything is doable, i was just saying that that will be some effort. maybe okay, but after all experience i tend to be more pessimistic in that area :)

i think for the locked account ui: this is just not decided yet. i assume we will dive deeper into that the next weeks. but that is also not directly related to the refactored ui. we could also have a dc_accounts_get_displayname() etc. that would not need much refactoring. but yes, that depdends on what we want at the end.

EDIT: in general, if we agree on that we want these new api changes, maybe before merging this pr, better first have the corresponding desktop/ios/android prs ready, so that we can be sure, things are doable with reasonable effort. maybe, this is no issue, but, tbh, i cannot oversee that at the moment.

[flub]

So I think in general the feedback (apart from r10s' 😉) has been leaning towards the positive side (also in chat), though of course it does incur extra work.

• Regarding doing this entirely without ffi changes: this is doable and I did consider this but also it leaves all these edge-cases open to the UIs so I considered changing the ffi more desirable.
• There are some questions about what the UIs will need regarding handling multiple accounts and how they would present encrypted accounts. This is unsolved in either case, as mentioned here we certainly can change and add things required, but so far doesn't seem fundamentally incompatible.

So, I tentatively suggest we attempt to adopt this? Could we convince a UI to try this out before merging?

[link2xt]

So, I tentatively suggest we attempt to adopt this? Could we convince a UI to try this out before merging?

cc @Hocuri @Jikstra @Simon-Laux for someone who can try to adopt this API for Android or Desktop.

[hpk42]

TBH, from an overall perspective, tackling this PR's approach right now sounds pretty dangerous. Let me recap that for the march release that we have three things pending for enc storage: - automatic migration from unencrypted to encrypted databases. Probably all in core but there are at least out-of-space errors to deal with gracefully. - set a passphrase on export and input a passphrase on import (FFIs plus UI changes) - Desktop implementing system-keyring and encrypted db's (or a master passphrase mechanism) this is quite a bit of things to get ready for end march including tested releases and pushes to app store (also some folks are still travelling). So unless i am missing something about the state of the three tasks above i am rather -1 to introduce breaking changes right now that require work on all UIs before we tackle the above three points. Besides, encrypted storage is not the only thing happening in the next weeks and desktop 1.28 wasn't even released yet at all.

…

On Sun, Feb 20, 2022 at 09:06 -0800, link2xt wrote: > So, I tentatively suggest we attempt to adopt this? Could we convince a UI to try this out before merging? cc @Hocuri @Jikstra @Simon-Laux for someone who can try to adopt this API for Android or Desktop. -- Reply to this email directly or view it on GitHub: #3063 (comment) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***>

[Hocuri]

FTR, I'll copy my older comment from the dev chat here:

I didn't implement this on Android yet, but should be doable I think. What will probably be tricky is allowing the user to set their own password at some point, because our current architecture is so that you need a context for lots of things. And if we only have one account, and it's encrypted so we can't get a Context, it could be tricky to even show a password input dialog - I don't know how tricky, and still possible, but more work than with the current FFI.
Whenever anything of the app is started, the context is assigned to a global variable ApplicationContext.dcContext and needed for lots of things. BTW in the FFI even functions like dc_get_oauth2_url, dc_provider_new_from_email and dc_may_be_valid_addr take a context parameter to do the logging.

All in all, from a technical perspective, I'm +/-0 on which API would be better, and -0.5 on this PR as it's already implemented the other way.

From an overall perspective, @hpk42's concerns sound sensible (I don't know much about what's left to do for the march release though; I think Android would be no problem even if we switched to RAII first, but of course there may always be unforeseen problems, and there are two other platform to worry about; in any case we should not merge this PR before all platforms have PRs ready to switch to the new API because otherwise we might get problems when we want to do the release and one platform hasn't switched yet).

[Hocuri]

I think we can close this, as it seems unrealistic that it will be merged?

[hpk42]

As discussed during irc session we move this to resurrection because there is currently no further work planned on encrypted storage, and desktop uses jsonrpc anyway which is the missing (and most important probably) platform for encrypted storage.