Announcement Announcement Module
Collapse
No announcement yet.
No clear way to disconnect a single connection from provider Page Title Module
Move Remove Collapse
X
Conversation Detail Module
Collapse
  • Filter
  • Time
  • Show
Clear All
new posts

  • No clear way to disconnect a single connection from provider

    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

  • #2
    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

    Comment


    • #3
      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

      Comment


      • #4
        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

        Comment


        • #5
          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.

          Comment


          • #6
            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

            Comment


            • #7
              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
              Last edited by sfussenegger; Mar 17th, 2011, 11:16 AM.

              Comment


              • #8
                Originally posted by Keith Donald View Post
                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).

                Comment


                • #9
                  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

                  Comment


                  • #10
                    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?

                    Comment


                    • #11
                      You are correct in that a Connection is a link between a local account and a provider account. However, I was not comfortable forcing capture of the providerAccountId at the level of all AbstractServiceProvider implementations, given it's not needed by the ServiceProvider framework to obtain a Service API.

                      So we've removed that in the meantime while we think through how we want to handle the case where additional provider profile information is needed to support user stories such as provider sign-in, rendering of connected profiles, etc. This discussion on the forum has been quite helpful in that regard.

                      Keith

                      Comment


                      • #12
                        Is there any possibility to follow that process of "thinking through" or contribute to it?

                        I've made pretty good progress with my spring-security integration but got stuck on exactly this question. As soon as this is decided, I could finish the integration and put everything into production.

                        If this takes too long though, I'd have to find a solution on my own to get something working. So any decisions, hints, promises, etc that would ease my mind while waiting are very well appreciated

                        Comment


                        • #13
                          Well, I'm exploring, post-connection, using the Service API returned by a ServiceProvider to insert a ConnectedProfile into a repository that could be retrieved later. So this wouldn't be something ServiceProvider implementations would be required to do, rather it would be a separate (and optional) responsibility.

                          So I could imagine...
                          Connect to provider XYZ
                          Post-connect, store a ConnectedProfile record e.g. profileRepo.insert(serviceApi.getProfile());

                          On provider sign-in, you'd then query the repo to determine if a connected profile exists by the providerAccountId returned from the authentication dance; if so, you'd sign the associated local user in, otherwise you'd require them to sign-up or implicitly create a local account. To handle this more elegantly, I think we need a OAuth1/2ServiceProvider operation that can simply return a Service API instance without saving a connection. Also, I think OAuth1/2Template should add a new operation that can be used to get a general-purpose RestTemplate for accessing protected resources the ServiceProvider implementations could call.

                          There is also the opportunity to have a normalized interface for a ConnectedProfile that could be implemented across Service APIs.

                          These are just some things I've been thinking. I need to actually prove this out in code to see how it looks. If you think your spring-social-security module is ready for review/to integrate some of these ideas, I'd be happy to work with you on this.

                          Keith
                          Last edited by Keith Donald; Mar 24th, 2011, 08:47 AM.

                          Comment


                          • #14
                            sound pretty good to me. please let me know of any (even experimental) code you make available (github branch I guess). I'll give feedback whether it plays nicely with the spring-security integration.

                            Comment


                            • #15
                              I've created a "connect-m3" branch that contains an initial commit of the refactoring work to address the issues brought up in this thread. This branch is being actively developed, and I hope to have it complete and ready to merge by the end of the week.

                              Here's the link:
                              https://github.com/SpringSource/spri...ree/connect-m3

                              Keith

                              Comment

                              Working...
                              X