No clear way to disconnect a single connection from provider

[garyj]

Hi All

I am working against the latest source from github and I can't seem to find a clear way to disconnect a single connection from a provider. My app allows for multiple Twitter connections, and it seems I can either disconnect all the Twitter connections or none.

Looking into the code in ConnectController disconnecting all the connections is exactly what it does.

This has even been made harder since some of the recent changes to the repository code where the providerAccoundId saving has been removed since it's not used by ServiceProvider framework. Where before I could use a custom AccountService to delete connections based on providerAccountId (eg Twitter Screen Names) now even this is unavailable.

In fact working with a single connection within an array of multiple connections for a given provider is quite cumbersome I find, perhaps the ServiceProvider should define a method:

ServiceProviderConnection<S> getConnection(Serializable accountId, String providerAccountId);

and

boolean isConnected(Serializable accountId, String providerAccountId);

Just to simplify the process of obtaining a single connection and working with it direct.

Moreover the JavaDoc for the List<ServiceProviderConnection<S>> getConnections(Serializable accountId); says "The first connection in the list is the "primary" connection between the account and this service provider." I am not certain whether in the Twitter example and a user having multiple accounts one is considered a "primary account" they are all of equal value and are not linked to each other.

Is there something I am missing in the ServiceProvider/Connection framework?

Would be great to hear your thoughts.

garyj

[Keith Donald]

Gary,

I'd consider this is a gap, and something we should address for M3. Each connection is assigned an internal identifier, and this could be used to remove individual connections. I think I'd prefer to use this over providerAccountId, since for some providers that account id could potentially change (Twitter lets you rename your screen name, for example). How does that sound?

Would you mind elaborating a bit on your requirement to look up an individual connection? How/when is such a look-up done exactly? Would look-up by a public providerAccountId be preferred to an internal connection identifier in any way?

Keith

[sfussenegger]

Twitter accounts have IDs too, not only screen names, see http://dev.twitter.com/doc/get/statuses/show/:id

As I don't know of any service provider that hasn't such a never-changing ID, I'd assume that providerId and providerAccountId make a pretty sane unique key for connections, don't they?

Cheers, Stefan

[Keith Donald]

Stefan,

That's a good point. It seems there is a case for the unique key of a connection to be the 1. local user account id + 2. provider id + 3. provider account id.

Currently, we treat the local user account id + provider id + provider-assigned access token as the connection key, which, as has been pointed out on this forum, may not be ideal as access tokens can change (due to expiration or re-generation, for example).

So applied to the provider sign-in flow, such a change would require the authentication filter/controller to execute the OAuth dance with the provider and use the returned access token to fetch the provider account id (though some providers, such as Twitter, return the account id in the access token response). Once obtained, we can lookup matching connections by provider id + provider account id (and update connection properties if necessary, if the access token has changed, for example). In the case of multiple matches (a corner case, but still possible given you can link one to many local accounts to the same provider account), we could either ask the user which account they want to sign-in with or just sign-in with the first match (this is what we do in M2).

Thoughts?

Keith

[sfussenegger]

Personally, I wouldn't use the local user account id as part of the unique key. But as you mentioned there might be use cases where this is desired. Hence I think it would be great to make this somehow configurable. Simply because I'd prefer the framework to disallow n:m connections, probably even 1:n connections.

Getting rid of the OAuth access token as part of the unique id seems important since it may change at any time. And while we are at it: I believe that access tokens should be stored along with their expiry timestamp. Given that an access token may expire, one could even consider moving access information out of the connection class altogether.

[Keith Donald]

Are you saying you wish to limit one connection to a given provider account? Can you elaborate on why this is desirable for your case? Also, when you say "probably also 1:n connections", can you elaborate on what you mean there in terms of being more restrictive? Just want to make sure I fully understand the behavior you're after and the rationale.

To your last comment, we need to store the access token as a property of the connection--otherwise there's no way to get an service API instance. I agree we need to be able to detect if the access token has expired, and if so, get a new one. As Craig has mentioned, the refresh token can be used for this in OAuth2.

Keith

[sfussenegger]

I'll stick to Facebook for my example since this is what I'm currently working on

Up to now, we've been using 1:1 connections from our accounts to FB account, i.e. 1 user may have no more than 1 FB account, one FB account may have no more than 1 local account. That's the simplest case which I think is used by most FB Connect implementations. If a user clicks the Connect button, there will always be the same local user logged in. If the user than wants to publish something, it's always published to one user. As simple as that.

If you are going n:m, say each FB account may be connected to any number of local accounts and any local account may be connected to any number of FB accounts, it gets complicated. What happens when a user clicks on Connect, will he have to select a local account to work with? What if the user want's to publish something, what account will it be published to? To avoid answering this questions, it's best to stick to 1:1 connections.

The question now is how to enforce this restriction while keeping maximum flexibility. I'd argue this should be part of the framework, as otherwise people will start writing the same code over and over again. Therefore, there should be two simple configurable checks:

boolean allow = allowMultipleConnectionsPerLocalAccount || !isConnected(accountId, providerId);
allow = allow && allowMultipleConnectionsPerProviderAccount || !isConnected(providerAccountId, providerId);
if (!allow) throw ...

depending on the configuration, these tuples may or may not be unique: {accountId, providerId}, {providerAccountId, providerId}, {accountId, providerAccountId, providerId}. The last one must always be unique, that's what you suggested. So long story short: yes, this might be the best identifier

[sfussenegger]

To your last comment, we need to store the access token as a property of the connection--otherwise there's no way to get an service API instance.
Keith

I didn't suggest to remove it completely. I tried to suggest to move access token and expiry into their own class which would be a property of Connection. So it would simply change from connection.getAccessToken() to ((OAuth2ServiceAuthentication)connection.getServic eAuthentication()).getAccessToken() where ServiceAuthentication is an interface or abstract class. This way, one could easily add non-OAuth connections (some custom scheme for instance) or even collections of tokens (if this would be required for some weird authentication scheme).

[Keith Donald]

Well, Connection returned by a ConnectionRepository is just designed as a dumb data holder ... so I'm not sure we want to be abstracting much at that layer... the authorization specifics are part of the domain layer in the ServiceProvider implementations.
Keith

[sfussenegger]

Now that I've seen the latest version of Connection from GitHub rather than the M2 vresion, it becomes apparent that my understanding of Connection was different. I've thought of it as the link between a providerAccountId and a local accountId. Now that providerAccountId was removed, it looks as if a new class containing accountId, providerAccountId and a connection (or set of connections?) would be what I though of. Nevertheless, I wonder why providerAccountId was removed.

Considering the usecase of a returning user, authenticating against a yet unknown, temporary OAuth token (see "Handling of temporary OAuth tokens"), there has to be a way to get the accountId from providerId and providerAccountId. The currently used findAccountIdByConnectionAccessToken(..) won't work here.

imho, this would be best addressed by adding findAccountIdsByProviderAccountId(..) back to ConnectionRepository and add providerAccountId back to connection - or even this:

AccountConnection
- id
- accountId
- providerId
- providerAccountId
- connections (Set<IConnection>)

OAuthConnection implements IConnection
- id
- accessToken
- refreshToken
- secret
- expires

CredentialsConnections implements IConnection
- id
- providerUsername
- providerPassword // storing passwords? argh ...

This would add the possibility to add sources using other schemes than OAuth. It probably would have to be postponed to some later release, as it would certainly make things a bit more complicated, especially at the DB level.

Your thoughts?