search

nextcloud / Android-SingleSignOn

Single sign-on for Nextcloud (Android Library Project)
GNU General Public License v3.0
70 stars 30 forks source link

Question: Drop use of androids build in AccountManager #49

Open David-Development opened 5 years ago

David-Development commented 5 years ago

@tobiasKaminsky Just throwing in some thoughts here. As we had a bunch of issues already when trying to figure out which accounts exist, I was thinking about if we can drop the need to use the Android "AccountManager" at all. As of Android 9 getting accounts from another app is even more difficult. Therefore dropping it makes things easier for all involved parties.

Advantages:

Disadvantages:

So basically what I'm proposing is:

This also might be interesting for @desperateCoder @stefan-niedermann

tobiasKaminsky commented 5 years ago

Hm. I am not sure if I understand it. Do you want to get rid of the account handling in files app? And implement our own one? Or only within SSO? E.g. https://github.com/nextcloud/Android-SingleSignOn/blob/79df148a8fee888187d75edcaf2ab907de04c8ba/src/main/java/com/nextcloud/android/sso/AccountImporter.java#L92-L104

David-Development commented 5 years ago

@tobiasKaminsky The method you mentioned uses the internal Android AccountManager. the call to getAccounts() will only return accounts, that the app currently has permission to access. So if we reinstall the files app, the permission is revoked and therefore the user has to request that permission again. Right now the only purpose of using the Android build in Account Manager is to figure out which accounts are installed and to select one of the accounts. The whole part with requesting / exchanging tokens is handled elsewhere.

That's why I was wondering.. maybe we can throw the Android Account Manager out so we don't have to work around those permission handling stuff? What do you think?

Reference: https://developer.android.com/about/versions/oreo/android-8.0-changes.html#aaad

mario commented 5 years ago

Personally I find using my own account management/storage compared to Android Account Manager significantly better, even if Android Account Manager has a tighter integration into the OS (which I don't really use or care about).

tobiasKaminsky commented 5 years ago

So if we reinstall the files app, the permission is revoked and therefore the user has to request that permission again.

Even if we use our own account management system, after a reinstallation these infos will be gone, or?

AndyScherzinger commented 5 years ago

So if we don't rely on Android's account manager but rather our very own we wouldn't have all the issues we currently face with the login loops (my guess). So shouldn't we try to drop Android's account manager anyways?

David-Development commented 5 years ago

@tobiasKaminsky Not necessarily. Right now, it's required that if you want to authenticate an account, you have to send the account object. (See: https://github.com/nextcloud/Android-SingleSignOn/blob/master/src/main/java/com/nextcloud/android/sso/AccountImporter.java#L259)

The problem is, that if you reinstall the files app, you won't be able to "find" that account anymore as you don't have the permission to access it anymore (on Android 8 and above). What we could do (using the Android Account Manager or our custom Account Manager) is to only send the account name (instead of the whole account object). This way we can work around the permission issue when reinstalling the app as we know the account name and we don't have to "recieve" the account object. But that has some serious side effects later on (if you want to list all accounts available, no accounts will be shown as you don't have the permission anymore.. etc..)

If we create our own solution, we need to provide some mechanism to return a list of available accounts to the sso library (e.g. if you plan on using multiple accounts) -> something similar to what the AccountManager provides right now (https://github.com/nextcloud/Android-SingleSignOn/blob/master/src/main/java/com/nextcloud/android/sso/AccountImporter.java#L96)

@AndyScherzinger yes, there are definitely some advantages to it.

My only concern right now is: How do we provide a list of all accounts to the client app?

tobiasKaminsky commented 5 years ago

Is this urgent? I would love to talk about this on conf/hackweek, as I think this is a fundamental change and we should think really well about this…

David-Development commented 5 years ago

Would like to share some ideas here:

3PA = Third Party App (such as news / deck / ...)

Provide a getAccounts() api that returns all nextcloud accounts

  1. 3PA -> SSO -> Nextcloud Files App -> getAccounts()
  2. 3PA -> SSO -> Show Account List (we can combine dev/normal files app accounts) or
  3. 3PA -> SSO -> Nextcloud Files App -> Show Account List (only show accounts of dev or normal app)
  4. 3PA -> SSO -> Nextcloud Files App -> requestAccountPermission

This works because the Nextcloud Files App owns all accounts and can list them. Basically we're creating a workaround for the new android security features (in favor of better usability). We can also integrate multi-account support pretty well this way.

3PA -> SSO -> Nextcloud Files App -> Show Account List 3PA -> SSO -> Nextcloud Files App -> requestAccountPermission

This one provides more security as we don't show any accounts to third party apps (Kind of similar how it is on Android 26+ now).

I personally prefer option one as it is more flexible. I think providing the "account list" to all apps isn't too bad as it doesn't contain much sensitive information. (Apps need to use the custom sso api to get this information). So if an apps really wants to spy on you.. they have to use the sso api and even then they will only be able to read the account names (username and url).

What do you guys think?

tobiasKaminsky commented 5 years ago

they have to use the sso api and even then they will only be able to read the account names (username and url).

Currently this is shown in account list anyway. Also we would have to expose this infos on account chooser, as otherwise people could not distinguish same username on different users, or?

David-Development commented 3 years ago

@tobiasKaminsky

Currently this is shown in account list anyway.

That is true - but only if apps have permission to list all accounts, right?

Just to clarify:

    • SSO -> Nextcloud Files App -> getAccounts()
    • SSO -> Show Account List (we can combine dev/normal files app accounts)

or

2.

Both apps (nextcloud files app dev and prod) are signed with the same key, right? So the main nextcloud files app (prod) would be able to request accounts from the dev app as well, right?

Trying to wrap my head around it if it would work if we always use the production / or beta version of the nextcloud files app to show the new account chooser dialog. Because it would need to pass on the information to the sso library to tell which app it needs to connect to when making requests..

tobiasKaminsky commented 3 years ago

let us move this to a brainstorming session on conf …

David-Development commented 3 years ago

Okay, sounds good 👍