finos / symphony-bdk-java

The Symphony BDK (Bot Developer Kit) for Java helps you to create production-grade Chat Bots and Extension Applications on top of the Symphony REST APIs.
https://symphony-bdk-java.finos.org
Apache License 2.0
23 stars 69 forks source link

ConnectionListener returns wrong user for ConnectionRequested/Accepted #68

Closed rikoe closed 5 years ago

rikoe commented 5 years ago

In DatafeedEventsService.java I see the following code:

case "CONNECTIONREQUESTED":
    for (ConnectionListener listener : connectionListeners) {
        listener.onConnectionRequested(event.getInitiator().getUser());
    }
    break;

event.getInitiator() returns the user who received the event, i.e. the bot account, not the user who has requested the connection. This can easily be seen from the code where it compares the initiator of the event message to the current bot user.

What I would expect to see instead is:

case "CONNECTIONREQUESTED":
    for (ConnectionListener listener : connectionListeners) {
        User toUser = event.getPayload().getConnectedRequested().getToUser();
        listener.onConnectionRequested(toUser);
    }
    break;
ystxn commented 5 years ago

Unfortunately, the event payload is counter-intuitive. For example, if the human John Smith requests a connection to the bot GigaBot, this payload would appear when GigaBot reads its datafeed:

[
    {
        "id": "r7dup1",
        "messageId": "__AjtfFS534ggqfjVy24Ur3___pNFJmTYbw",
        "timestamp": 1565781499239,
        "type": "CONNECTIONREQUESTED",
        "initiator": {
            "user": {
                "userId": 291876051431612,
                "firstName": "John",
                "lastName": "Smith",
                "displayName": "John Smith",
                "email": "john@smith.com",
                "username": "john.smith"
            }
        },
        "payload": {
            "connectionRequested": {
                "toUser": {
                    "userId": 74402672223674928,
                    "displayName": "GigaBot"
                }
            }
        }
    }
]

So the "initiator" in this case is John Smith and the "toUser" is GigaBot. From the bot's perspective, the point of this event handler is to accept the connection request from John Smith, so supplying GigaBot as the user object wouldn't be of much use.

The code was actually as you mentioned a while ago and the current state is a deliberate fix.

rikoe commented 5 years ago

Thanks @ystan - we are definitely seeing the bot user coming through in the event, so maybe we are using the old version of the Java client, will check.

I was looking at the symphonyoss java client implementation, as it pulls the connection details out of the event payload, not the initiator. But maybe theirs is the wrong one after the fix you mention.

See: https://github.com/symphonyoss/symphony-java-client/blob/d091ebee847e18e323e4c73e37acd3e4aa134f23/symphony-client/src/main/java/org/symphonyoss/client/services/MessageService.java#L433

rikoe commented 5 years ago

Incidentally, we have now decided to move away from the connection requested event as a way to handle connections, because it seems we could miss connection requests that happened while the bot was not running.

We are now looking at polling the connections API (i.e. using the ConnectionsClient) so that we can always retrieve pending connections.

The other challenge we are facing is that despite what is indicated in the REST API documentation we always get only the user ID and display name for pending connections, no extra information, so then we have to use the user lookup API (UsersClient) to find information about the user who requested the connection. Even when we do that we cannot see e.g. their email address in the returned payload, so that we can decide based on domain whether to accept the connection.

The lack of documentation and examples showing how to write a bot that needs to respond to external connections is very frustrating.

ystxn commented 5 years ago

That's a good point. The current principle behind user lookup is that you only get complete information about an external user if you are connected to that user. Email is considered a sensitive field so it's hidden by default. So this principle should include pending connections since it is provided in the datafeed event anyway. I shall file a request to the platform team - can I find out which organisation/customer you represent so I can tag the request accordingly?

Meanwhile, a slightly hackish workaround would be to just accept all incoming connection requests, after which you have full access to the user lookup and can retrospectively remove the connection if the domain is not in the desired list.

rikoe commented 5 years ago

Thanks @ystan, we did indeed consider this is an option, but the user automatically gets a notification of we accept the connection and remove it, which we would like to avoid.

We are going to use the company name for now, but once the email is available, that would be really useful.

I will PM you the details about the implementation.

ystxn commented 5 years ago

Closing this issue as the discussion has moved onto Symphony