Skip to content

Refactor authentication and account management code

akwizgran requested to merge 1341-account-manager-refactoring into master

This branch refactors the authentication and account management code into a new AccountManager, with a portable implementation in bramble-core and an Android-specific implementation in bramble-android. Bramble-based apps that don't password-protect the database key don't have to use an AccountManager.

The AccountManager is responsible for checking and changing the password, and for managing all account state on disk, with the following exceptions:

  • The DatabaseComponent is responsible for creating the database
  • Android components that use SharedPreferences may update the preferences on disk without going through the AccountManager

These exceptions can potentially happen concurrently with changes managed by the AccountManager, such as deleting the account, in which case files that aren't managed by the AccountManager may not be deleted. Unfortunately, preventing this would require creating our own SharedPreferences API and making a lot of components depend on the AccountManager. On balance I don't think it's worth it.

Concurrent changes to the state managed by the AccountManager (such as creating and deleting the account at the same time) are prevented by a lock.

We had planned to add an AccountState enum, as in !855 (closed), but after looking at the ways the AccountManager interface is used, I'm no longer sure that would be the best approach. If the AccountManager had a getAccountState() method, most of the callers would want to check whether the returned state was in some set of states indicating a particular condition - for example, SIGNED_IN or CHANGING_PASSWORD, if the caller wanted to know whether the user was signed in. When a new state was added, all callers would need to be checked to decide how they should handle the new state. So I think it may be better to stick with methods that expose the conditions directly, such as hasDatabaseKey() and accountExists() (and in future isLocked()).

I've added some TODOs for changes that aren't part of this refactoring.

Closes #1341 (closed)

Edited by akwizgran

Merge request reports