Client: Different accounts may use same keychain entry

[ckamm]

Currently the client saves and looks up keychain entries by user + server url. This leads to problems when there are several accounts with the same identifier.

Specifics:

• Deleting one of the accounts will wipe the keychain entries, making the second one unusable.
• If the two accounts use different auth mechanisms (oauth2 vs http), the keychain entries get overwritten with different data.

An account should generate an additional unique identifier to disambiguate the different accounts. Care needs to be taken to make it backwards compatible.

[ogoffart]

If the two accounts use different auth mechanisms (oauth2 vs http), the keychain entries get overwritten with different data.

That's not possible, the client uses oauth2 always if the server supports it. Even if the account was configured with basic http auth before, it will then switch to oauth2 on the next connection.

[SamuAlfageme]

Related behavior for the cookies on log out/account removal: https://github.com/owncloud/client/pull/5383#issuecomment-267327203

[ckamm]

@ogoffart Then there's some other bug lurking there, because I set up a new oauth2 account. Restarted the client and one of the two accounts pointing to the same server was signed out for no apparent reason. I guessed that the http-account's persist() overwrote some oauth keychain data...

[guruz]

An account should generate an additional unique identifier to disambiguate the different accounts. Care needs to be taken to make it backwards compatible.

Remember that this might be a quite exotic setup, so don't introduce too much complexity for this.
Maybe WONTFIX? Or whatever @ogoffart says?

[SamuAlfageme]

An account should generate an additional unique identifier to disambiguate the different accounts.

Couldn't we simply use the id from the .cfg file also to reference the accounts on the keychain, etc... (i.e. the N in N\config_key=config_value)

[Accounts]
0\Folders\1\ignoreHiddenFiles=true
0\Folders\1\journalPath=._sync_fd41dc4a0d48.db
0\Folders\1\localPath=/Users/ownCloud/
0\Folders\1\paused=false
0\Folders\1\targetPath=/
0\authType=http
0\http_oauth=false
0\http_user=admin
0\serverVersion=10.0.2.1
0\url=https://demo.owncloud.com
0\user=admin
1\Folders\2\ignoreHiddenFiles=true
1\Folders\2\journalPath=._sync_8da9d25ef3ff.db
1\Folders\2\localPath=/Users/ownCloud2/
1\Folders\2\paused=false
1\Folders\2\targetPath=/
1\authType=http
1\http_oauth=false
1\http_user=demo
1\serverVersion=10.0.2.1
1\url=https://demo.owncloud.com
1\user=demo
version=2

Remember that this might be a quite exotic setup

Can be exotic yup, but if allowed (https://github.com/owncloud/client/issues/5305) I think it should be done with unique account id's since otherwise it may have collateral effects for the people that use the client in that way (having the same account multiple times) 🤔

[ckamm]

@guruz I think that if it's a supported feature we should care about the related bugs (but not with high priority since few people see it). If we don't want to support these setups altogether we should remove the ability to create multiple accounts for the same server in the first place.

[phil-davis]

the ability to create multiple accounts for the same server in the first place

Just checking that here you mean some sort of "weird" setup where someone adds the same account on the same server multiple times.

Having multiple different accounts set up on the client that are all on the same server works fine, and is a reasonable thing to do (well, I do it)

[ckamm]

@phil-davis Yes, thanks for the clarification. I meant several accounts pointing to the same user on the same server.

[ogoffart]

This might actually be a problem for oauth where the refresh_token can only be used once.

[guruz]

Upgraded to p2.
Gold ticket?

[ogoffart]

What should happen (not tested, because the oauth2 app currently does not allow several connections) is that one of the account connects and the other re open the browser. Not too bad.
If we change the way we store in the keychain then we need an upgrade path.
And does having twice the same account make any sense?

[guruz]

because the oauth2 app currently does not allow several connections

but it will..

If we change the way we store in the keychain then we need an upgrade path.

ack

And does having twice the same account make any sense?

Yes, we had agreed that this shall be possible because users are doing things we don't imagine (e.g. they sync the same account to both their HD and a USB drive)
I'm also doing this for testing scenarios.

[ogoffart]

Tested with https://github.com/owncloud/oauth2/pull/65 and everything works perfectly as we can re-use the refresh_token several time.

[SamuAlfageme]

Tested with owncloud/oauth2#65 and everything works perfectly as we can re-use the refresh_token several time.

Pointed out in owncloud/oauth2#70 since that should be solved in the future and we might need to adapt to get a deterministic behavior.

[SamuAlfageme]

Another issue with current implementation: after https://github.com/owncloud/client/issues/5752 removing an account could have the collateral of login out the other one when the client is restarted.

[ckamm]

It's a bit painful due to migration, but I can take care of it.

[SamuAlfageme]

Tested with different upgrade and multiaccount scenarios and all of them ran perfectly smooth. 🎉

Closin' here 👍 Very nice!