Support multiple password stores

lverns commented 7 years ago

(This is already in the TODO section of the README.)

In my case, I have a password store for my personal accounts and also use a password store shared with other people. I would be nice to be able to access both stores on my phone without having to combine them in any way.

mddeff commented 6 years ago

I too think this would be really helpful. I use pass both for my personal passwords at home as well as with a team at work, and the ability to have multiple accounts would be huge!

amc90 commented 5 years ago

Multiple repositories would be a very useful feature. I've got 3 different repositories (two different teams, and a personal one), since Gopass makes it so easy to manage them.

lsowen commented 5 years ago

Is anyone interested in working on this? I would be open to funding the development of this feature.

dllud commented 5 years ago

Hi everyone,

This is a feature that I have been wishing for since I first started using Android Password Store (APS) back in 2015. As no one else stepped in for the last 2 weeks, I decided to team up with @GrimKriegor and take @lsowen's proposal to fund the development effort.

We created a tasks list (pasted bellow) that shows how we plan to approach the development of this feature. We would be very grateful if the maintainers (@zeapo @MSF-Jarvis @zidhuss) could take a look at it and let us know if these features (and proposed implementations) are acceptable and likely to get merged.

Adding support for multiple password stores, in a clean and maintainable way, will require us to refactor a good portion of APS's codebase, for instance the way it handles data persistence, and some parts of the User Interface (UI). Therefore, hearing the maintainers opinion would be much helpful. Please take this as a Request for Comments (RFC). Nothing is written in stone, and we can change our approach if someone reaches out with better solutions.

Onto it: we plan to split our implementation into 3 major tasks, each targeted to be an independent pull request (PR). These tasks are detailed bellow along with more detailed steps.

PR 1: Multiple git repositories (SharedPreferences -> SQLite DB)

Each password store is a git repository. In order to support multiple stores, APS will have to handle multiple git repositories.

APS is using Android's SharedPreferences to store all its data. This current solution isn't fit for the structured data we will need for each password store. As such, we must refactor APS's code to have it using an SQLite database to store its data. Application-wide preferences, such as the OpenPGP key provider, will be kept on the SharedPreferences.

Steps

List all data that will be needed. Divide it into a set of Entities for storage using the Room persistence library.
Refactor APS's code to use the Room persistence library instead of SharedPreferences, while also adding support for multiple git repos.
Regression tests and bug fixes. App should behave as before.
Migration (optional): migration code to allow a seamless upgrade to the new data storage and git repo filesystem structure.

PR 2: Multiple PGP keys (honour .gpg-id)

When decrypting passwords APS calls OpenKeyChain that automatically finds which private key in its keyring is able to decrypt the password. When creating and encrypting passwords things work another way. APS must provide a list with the keys it wishes to encrypt to. Currently APS handles the "encrypt to" list in an exquisite way: there is a single global configuration, under Select OpenPGP Key id in settings, where the user selects the "encrypt to" keys. This approach isn't compatible with multiple stores, as users may wish to encrypt to different PGP keys for each store. Furthermore, it is also incompatible with zx2c4 pass, that uses per directory .gpg-id files to list the "encrypt to" keys.

Our solution is thus to honour the .gpg-id files from zx2c4 pass, which will both support multiple store and close a long standing issue in APS (#159).

Steps

Create a method that traverses the git repository directories upwards to look for a .gpg-id file.
Create a method on PgpActivity class that asks the OpenPGP provider for keys that match the IDs/email addresses/names on .gpg-id.
Refactor PgpActivity and all classes that use it to have them using the keys fetched from .gpg-id.
Write unit tests for the new methods.

PR 3: User Interface

APS's User Interface (UI) has several issues: lack of a synchronize button in the action menu, too many items in the overflow menu (kebab menu), too many settings in the single page of the settings activity, etc. Although we would love to address all these issues we will constrain our actions just into what's necessary to have an UI for multiple stores. Our mail goal is to keep the current follow if only a single store is used, thus making the multiple stores feature almost unnoticeable for those that use do not use it. We took inspiration from apps such as Nextcloud that support multiple accounts in a single app, and came up with the sketches shown bellow.

Sketches and interaction

When the user first opens APS she will get a screen that is much like previous versions of APS, except for one extra button and the top left corner.
This extra button opens a hamburger menu. This menu contains entries for the creation and management of stores.
As new stores are created they appear on the hamburger menu.
When a store is selected, the hamburger menu is hidden and the store contents appear on the main activity.
When the app is open it will always show the previously used store.

Steps

Add a hamburger menu, most probably with a suitable library.
Create the hamburger menu items (add/manage store and stores' list).
Create an activity, views and methods for the add store action.
Create an activity, views and methods for the manage stores action.
Create methods that allow switching between stores.

msfjarvis commented 5 years ago

Apologies for the delay in responding.

As far the RFC is concerned, the UI is compelling for now. When we deploy this and gather feedback, I will be willing to make improvements as our users request.

APS is using Android's SharedPreferences to store all its data. This current solution isn't fit for the structured data we will need for each password store. As such, we must refactor APS's code to have it using an SQLite database to store its data. Application-wide preferences, such as the OpenPGP key provider, will be kept on the SharedPreferences.

I was looking to do this for a good while, :+1:

PR 2: Multiple PGP keys (honour .gpg-id) [..]

This will require serious regression testing, APS unit test coverage is pretty abysmal as of now so I'm willing to participate in a full refactor effort to make unit testing easier.

msfjarvis commented 5 years ago

I've pinned the issue for now so people can see where the current effort is headed and hopefully find even more people with the skill to contribute to it.

dllud commented 5 years ago

Thanks for the review. We will start working on this soon. We will post our status as we go along.

PR 2: Multiple PGP keys (honour .gpg-id) This will require serious regression testing

Indeed.

I'm willing to participate in a full refactor effort to make unit testing easier.

Thanks! That will be very helpful.

dllud commented 5 years ago

Here follows a list of the data elements that Android Password Store (APS) currently saves to persistent memory. This list tries to be exhaustive. It will serve as base for the database design. Please let me know if there is any missing or misinterpreted element.

All data elements are currently kept in SharedPreferences with the single exception of the SSH private and public keys, which are stored as files.

All preferences are stored on the default SharedPreferences file, except the autofill preferences for each app and website, which are stored in files named autofill and autofill_web respectively.

git

git_server_info

Key	Type	Example content
git_remote_protocol	String	ssh:// or https://
git_remote_username	String	bob
git_remote_server	String	homeserver.com
git_remote_port	String	22
git_remote_location	String	~/pwd-store.git
git_remote_auth	String	ssh-key or OpenKeychain or username/password

git_config

Key	Type	Example content
git_config_user_name	String	Bobby Tables
git_config_user_email	String	bob@homeserver.com

other git configs

Key	Type	Example content
repository_initialized	Boolean	false
repo_changed	Boolean	true
git_external_repo	String	/sdcard/pass.git

ssh

Key	Type	Example content
use_generated_key	Boolean	true
ssh_key_passphrase	String	myLongPassword

The SSH private and public keys are stored as files named .ssh_key and .ssh_key.pub respectively.

hotp

Key	Type	Example content
hotp_remember_check	Boolean	false
hotp_remember_choice	Boolean	false

crypto

Key	Type	Example content
openpgp_provider_list	String	org.sufficientlysecure.keychain
openpgp_key_ids_set	StringSet	0xab123a11;0x1ed123

general

Key	Type	Example content
general_show_time	String	45
show_password	Boolean	true
show_extra_content	Boolean	true
copy_on_decrypt	Boolean	true
clear_after_copy	Boolean	true
filter_recursively	Boolean	true
sort_order	String	FOLDER_FIRST or FILE_FIRST or INDEPENDENT

autofill

Key	Type	Example content
autofill_enable	Boolean	true
autofill_default	Boolean	true
autofill_always	Boolean	false

Per app/website preferences

APS creates two independent SharedPreferences files to store the autofill configurations of apps and websites, named autofill and autofill_web respectively. Inside them there is one entry per app/website, identified by its package name or URL, that contains a String with the autofill behaviour.

Key	Type	Example content
packageName or URL	String	empty string or /first or /never or a passwords list

misc

Key	Type	Example content
clear_clipboard_20x	Boolean	false
use_android_file_picker	Boolean	false

msfjarvis commented 5 years ago

Schema looks complete to me. Anything I can do upstream in the meantime to make things easier for you?

dllud commented 5 years ago

Thanks for looking into it. Right now I cannot think about any specific roadblocks. I'll let you know if something comes up as I go along.

What would be generally useful is more unit tests. We will need to do extensive regression testing, as we will be moving a good chunk of the data into the DB. Though it may be hard to write tests that are agnostic to the data storage method. For instance, DecryptTest.kt:97 depends on the current way of storing data (SharedPreferences).

dllud commented 5 years ago

I need some input. I was going about the database design and took a good look at what JGit provides. I noticed that a lot of what's currently stored using SharedPreferences (e.g. most entries under git_server_info and git_config) can be easily retrieved using JGit's API. Actually, some code in GitActivity.java is just there to keep the synchronization between what's stored in SharedPreferences and JGit. Is there a specific reason to keep this data duplicated?

If not, we would be better just keeping it within JGit. Whenever there are UI elements to be filled, we can just query JGit (for instance, there's an splitURI() method already implemented). This would simplify the DB design and avoid the common mistakes that arise whenever there's duplicated data.

Bellow is a partial ER diagram of the DB. It is far from complete, I was still in the beginning. The red strikethrough is the duplicated data. erd

zeapo commented 5 years ago

No reason at all (I don't recall why in fact...). Feel free to use jgit's api. Just bear in mind that you'll be stuck in version 3.7.

On Sun, Apr 28, 2019 at 6:51 PM dllud notifications@github.com wrote:

I need some input. I was going about the database design and took a good look at what JGit provides. I noticed that a lot of what's currently stored using SharedPreferences (e.g. most entries under git_server_info and git_config) can be easily retrieved using JGit's API http://download.eclipse.org/jgit/site/5.3.0.201903130848-r/apidocs/index.html . Actually, some code in GitActivity.java https://github.com/zeapo/Android-Password-Store/blob/00e2cf352fe08252853c4d271711d96b9456ef26/app/src/main/java/com/zeapo/pwdstore/git/GitActivity.java#L462 is just there to keep the synchronization between what's stored in SharedPreferences and JGit. Is there a specific reason to keep this data duplicated?

If not, we would be better just keeping it within JGit. Whenever there are UI elements to be filled, we can just query JGit (for instance, there's an splitURI() method already implemented). This would simplify the DB design and avoid the common mistakes that arise whenever there's duplicated data.

Bellow is a partial ER diagram of the DB. It is far from complete, I was still in the beginning. The red strikethrough is the duplicated data. [image: erd] https://user-images.githubusercontent.com/1716156/56867463-55d19c00-69dd-11e9-81be-70ce98d2bb58.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zeapo/Android-Password-Store/issues/260#issuecomment-487396194, or mute the thread https://github.com/notifications/unsubscribe-auth/AACCABQTVFQ6UWNDRMUHIFLPSXIYJANCNFSM4C3EPSWA .

-- Mohamed Zenadi

dllud commented 5 years ago

Thanks for the prompt reply.

you'll be stuck in version 3.7

Ohh I see, 3.7.1.201504261725-r. Can you let us know why it is not possible to use a later version?

zeapo commented 5 years ago

Versions 4 and up use java.nio.Files which was not supported until Android API 26+.

That means that we'll be stuck until we decide to drop all compatibility to versions prior to 26 :)

On Sun, Apr 28, 2019 at 7:14 PM dllud notifications@github.com wrote:

Thanks for the prompt reply.

you'll be stuck in version 3.7

Ohh I see, 3.7.1.201504261725-r. Can you let us know why it is not possible to use a later version?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zeapo/Android-Password-Store/issues/260#issuecomment-487398443, or mute the thread https://github.com/notifications/unsubscribe-auth/AACCABTZBLFO74KTPTA742TPSXLQBANCNFSM4C3EPSWA .

-- Mohamed Zenadi

dllud commented 5 years ago

Thanks!

Just one more (unrelated) question: Should the preferences for HOTP (hotp_remember_check and hotp_remember_choice) be replicated for each store or kept as global configs?

zeapo commented 5 years ago

Why would a user want a different behavior for HOTP for two different stores?

What do you think @MSF-Jarvis ?

msfjarvis commented 5 years ago

Why would a user want a different behavior for HOTP for two different stores?

What do you think @MSF-Jarvis ?

I'd rather they be global settings.

Versions 4 and up use java.nio.Files which was not supported until Android API 26+.

About java.nio.Files, there does seem to exist a backport of them here so I think a fork of JGit that leverages it can be deployed for Android users. I can probably maintain it myself and publish an -android artifact everytime there's a JGit release. Would that make things easier for you @dllud ?

dllud commented 5 years ago

Sorry for the delay on my reply. So far there's no need to keep a JGit fork. I checked version 3.7 and it has all the methods needed retrieve the git configurations. Thanks anyway!

dllud commented 5 years ago

Here's the plan for the new storage location of all data elements that need persistence.

git

git_server_info

Key	Storage
git_remote_protocol	JGit
git_remote_username	JGit
git_remote_server	JGit
git_remote_port	JGit
git_remote_location	JGit
git_remote_auth	DB

All attributes from JGit can be retrieved using org.eclipse.jgit.transport.Transport.getURI() or org.eclipse.jgit.transport.RemoteConfig.getURIs().

git_config

Key	Storage	Retrieval
git_config_user_name	JGit	`org.eclipse.jgit.lib.UserConfig.getAuthorName()`
git_config_user_email	JGit	`org.eclipse.jgit.lib.UserConfig.getAuthorEmail()`

other git configs

Key	Storage
repository_initialized	DB
repo_changed	DB
git_external_repo	DB

ssh

Key	Storage
use_generated_key	DB
ssh_key_passphrase	DB

The paths to the SSH private and public keys will also be stored on the DB.

hotp

Key	Storage
hotp_remember_check	SharedPreferences
hotp_remember_choice	SharedPreferences

crypto

Key	Storage
openpgp_provider_list	SharedPreferences
openpgp_key_ids_set	DB (later: `.gpg-id` files)

general

Key	Storage
general_show_time	SharedPreferences
show_password	SharedPreferences
show_extra_content	SharedPreferences
copy_on_decrypt	SharedPreferences
clear_after_copy	SharedPreferences
filter_recursively	SharedPreferences
sort_order	SharedPreferences

autofill

Key	Storage
autofill_enable	SharedPreferences
autofill_default	SharedPreferences
autofill_always	SharedPreferences

Per app/website preferences

Key	Storage
packageName or URL	SharedPreferences

Whenever a passwords list is used it must now include not only the password path but also the store name.

misc

Key	Storage
clear_clipboard_20x	SharedPreferences
use_android_file_picker	SharedPreferences

And this is how the DB will look like: erd

dllud commented 5 years ago

Would you like to have (in the future) multiple git remotes for each store? If not I will simply add git_remote as a nested object of store. Otherwise I will write it as an independent entity. There seems to be no one asking for multiple remotes here on the GitHub issues.

zeapo commented 5 years ago

Some people have monorepos. That means that a single remote, single checkout behave as multiple repository.

Le mer. 1 mai 2019 à 14:11, dllud notifications@github.com a écrit :

Would you like to have (in the future) multiple git remotes for each store? If not I will simply add git_remote as a nested object https://developer.android.com/training/data-storage/room/defining-data.html#nested-objects of store. Otherwise I will write it as an independent entity. There seems to be no one asking for multiple remotes here on the GitHub issues.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zeapo/Android-Password-Store/issues/260#issuecomment-488267820, or mute the thread https://github.com/notifications/unsubscribe-auth/AACCABUT47WTKAMCHCQMPMTPTGCIXANCNFSM4C3EPSWA .

zeapo commented 5 years ago

Some addition. That means that sometimes, users want a sub folder to behave as a password store.

Le mer. 1 mai 2019 à 14:15, Mohamed Zenadi mohamed.zenadi@gmail.com a écrit :

Some people have monorepos. That means that a single remote, single checkout behave as multiple repository.

Le mer. 1 mai 2019 à 14:11, dllud notifications@github.com a écrit :

Would you like to have (in the future) multiple git remotes for each store? If not I will simply add git_remote as a nested object https://developer.android.com/training/data-storage/room/defining-data.html#nested-objects of store. Otherwise I will write it as an independent entity. There seems to be no one asking for multiple remotes here on the GitHub issues.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zeapo/Android-Password-Store/issues/260#issuecomment-488267820, or mute the thread https://github.com/notifications/unsubscribe-auth/AACCABUT47WTKAMCHCQMPMTPTGCIXANCNFSM4C3EPSWA .

dllud commented 5 years ago

Thanks for the input.

Is that something you would like to support right from the start? Adding that option, that elevates a sub-folder to the status of store, would be cumbersome. We would need to add support in the UI besides the DB. This is easy to do on the command line (just export PASSWORD_STORE_DIR=~/repo/sub-folder/) but requires a dedicated UI on the smartphone.

This behaviour will be partially supported once our PR 2 (honour .gpg-id) is done. It will make APS behave just as zx2c4 pass does. Once in a sub-folder it will use the PGP keys that are assigned to it.

dllud commented 5 years ago

As there is no current use for multiple git remotes we embedded both git_remote and ssh_key tables into store. Since we are using Room library this may be easily reverted in the future, whenever a use case for multiple remotes arises.

In practice, this flattens the DB schema, making it look like this: merged

The pgp_key table will be removed once we get into PR 2. The end result will be a database with a single table, but still with 3 Entity classes in Room for a better separation of concerns: Store, GitRemote and SshKey.

zeapo commented 5 years ago

That seems reasonable.

Le jeu. 2 mai 2019 à 02:01, dllud notifications@github.com a écrit :

As there is no current use for multiple git remotes we embedded https://github.com/drpout/Android-Password-Store/blob/7fa041f1e4503e7ddb933f10c6795956ad4fbfea/app/src/main/java/com/zeapo/pwdstore/db/entity/StoreEntity.kt#L13 both git_remote and ssh_key tables into store. Since we are using Room library this may be easily reverted in the future, whenever a use case for multiple remotes arises.

In practice, this flattens the DB schema, making it look like this: [image: merged] https://user-images.githubusercontent.com/1716156/57050519-de1d9e80-6c74-11e9-8cf8-8b3fca905166.png

The pgp_key table will be removed once we get into PR 2. The end result will be a database with a single table, but still with 3 Entity classes in Room for a better separation of concerns: Store, GitRemote and SshKey https://github.com/drpout/Android-Password-Store/tree/database/app/src/main/java/com/zeapo/pwdstore/db/entity .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zeapo/Android-Password-Store/issues/260#issuecomment-488506337, or mute the thread https://github.com/notifications/unsubscribe-auth/AACCABST5F67IK2QGZGLQQ3PTIVM3ANCNFSM4C3EPSWA .

msfjarvis commented 5 years ago

@dllud anywhere we can track progress?

dllud commented 5 years ago

Here: https://github.com/drpout/Android-Password-Store/tree/database

Progress has been slow though, mostly because in the past few days we took what was supposed to be little digression, to have a vim config ready to autocomplete both Kotlin and Java for Android.

Untangling code paths

Also, we spent a good measure of some time studying the main code paths and wondering what do to about them. The code on PasswordStore.java is too extensive and highly convoluted. It needs a serious refactoring. This main class is an Android Activity, that should roughly correspond to a view in a MVC architecture. It should therefore implement View related logic. However, it lacks separation of concerns and implements lots of Controller and even some Model logic.

Furthermore, it has:

Identical code blocks repeated throughout the class. E.g.: switch case on initRepository(), dialog to select PGP keys on initializeRepositoryInfo() and createPassword().
Repeated checks of the current store state between methods where the state should be well-known and determinate. E.g.: initRepository() and initializeRepositoryInfo() interaction.
Inconsistent behaviour. E.g.: the dialog to select PGP keys is shown when a store is created from scratch, but not when a new store is cloned.
Poor method naming, making it hard to read and interpret the code. E.g.: checkLocalRepository(File localDir) which does not check for anything but rather launches the appropriate Fragment according to the app state.

Bellow are two diagrams that I have drawn to help us understand what happens on App startup and Store creation. (Note: these do not attempt to be proper UML activity diagrams. They mix in several concepts that do not belong to activity diagrams. They are just an attempt to help us better understand the code flow.)

startup store-creation

How to fit in DB queries

Notice how on App startup, there is no explicit check for the existence of a store. Rather, it directly queries actual properties of the store (external, initialized, changed, etc.) throughout different methods. On store creation there is a similar pattern. The store isn't created on a single method, but rather throughout many methods, each independently changing store properties. This poses no issues when using SharedPreferences, but it makes things tricky when trying to replace it for DB queries.

As detailed earlier, our tentative approach for this first PR aims to keep the app logic as is, and only replace SharedPreferences access for Room lib calls. As all app logic assumes there is a single store, for now we will use a single store on the DB named default.

On first App startup this default store is not present. Retrieving it from the DB would leave us with a null reference. We thus imagined 3 ways forward:

Add null checks on all ifs that query store properties. Cons: ugly, ugly, ugly!
Create the default store on startup with dummy values and repo_initialized = false. Cons: wrong logic that will bite us later on. No store should be created without user input. Forces us to replace the dummy values later on. Pros: compatible with existing logic.
Check if a store exists and, if not, show the ToCloneOrNot fragment. Pros: the proper way to do it, as a store would only be created when we have the required user inputs. Cons: requires refactoring of the method that launches fragments, checkLocalRepository(File localDir), and a long cascade of all those methods that use it.

We are going for approach 2 right now, in order to avoid breaking the existing logic and having to deal with all bugs that would come along. Later on, we will go for 3, probably before submitting PR1.

The proper way: LiveData

Notice that we are allowing DB queries on the main thread. This is a known bad practice that may stall the UI during long queries. However it allows us to keep the current app logic without much refactoring. Otherwise we would be forced to add even more code to PasswordStore.java to cater for AsyncTasks. Luckily our queries are quite simple and the performance impact should be negligible.

Anyway, in the event of a future refactoring effort, we believe that APS's UI should be built around the Observer pattern and leverage Android's LiveData which integrates graciously with Room lib.

djagoo commented 4 years ago

Is this feature still in active development? For the time being I'm stuck with using work profiles for different pass stores.

Skrilltrax commented 4 years ago

Yep, however it is a part of v2 so it'll take some time.

dllud commented 4 years ago

Nice to see work being done on this @Skrilltrax ! I hope you can find some time to address the issues mentioned in my previous comment. That would make the implementation of this feature much easier.

I had to put this on hiatus due to too much work landing on my shoulders on the second half of 2019. I should be able to give this a push in April.

Skrilltrax commented 4 years ago

@dllud current code with the god activity pattern is very difficult to maintain so we are re-writing the whole app again for v2, that should fix most of the problems you mentioned and we will also be able to make better decisions regarding the app architecture.

djagoo commented 4 years ago

@Skrilltrax thats very good to hear. Thanks and keep up the good work.

msfjarvis commented 2 years ago

Closing as part of issue tracker cleanup.

android-password-store / Android-Password-Store