Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store custom client ID on Agent/Connection ID #224

Closed
wants to merge 3 commits into from
Closed

Store custom client ID on Agent/Connection ID #224

wants to merge 3 commits into from

Conversation

alecgibson
Copy link
Collaborator

It can be useful to identify which connection submitted what operation.
This is already possible to some extent by tracking the ID randomly
generated when instantiating Agent.

However, this ID obviously changes on every connection, and bears no
relation to the actual client that is connecting.

This change allows the client to specify some ID when connecting, which
will be concatenated on to the agent's ID like this:

<randomId>:<clientId>

Keeping the randomId ensures that the IDs remain unique, and
concatenating the clientId allows the consumer to later determine
which operations were submitted by the client that matches that ID, by
checking the src field stored on an operation.

It can be useful to identify which connection submitted what operation.
This is already possible to some extent by tracking the ID randomly
generated when instantiating `Agent`.

However, this ID obviously changes on every connection, and bears no
relation to the actual client that is connecting.

This change allows the client to specify some ID when connecting, which
will be concatenated on to the agent's ID like this:

`<randomId>:<clientId>`

Keeping the `randomId` ensures that the IDs remain unique, and
concatenating the `clientId` allows the consumer to later determine
which operations were submitted by the client that matches that ID, by
checking the `src` field stored on an operation.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 96.494% when pulling feeb831 on alecgibson:custom-agent-id into 762e05d on share:master.

@coveralls
Copy link

coveralls commented Jul 16, 2018

Coverage Status

Coverage increased (+0.002%) to 96.494% when pulling 849e615 on alecgibson:custom-agent-id into 762e05d on share:master.

Copy link
Contributor

@gkubisa gkubisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Could you update README.md before I merge (into https://github.com/teamwork/sharedb), please?

Alec Gibson added 2 commits July 16, 2018 13:37
If we prepend the custom client ID to the agent ID rather than prepend,
then we gain the advantage of being able to easily group and fetch ops
made by the same client (provided the underlying data structure has an
appropriate index).
@alecgibson
Copy link
Collaborator Author

@gkubisa I've added a bit more documentation for Backend.listen. I've also inverted the order of the ID. The custom ID is now prepended instead of appended, so that if the underlying datastore has an index on the src field, you can easily group and fetch ops by a single client.

@gkubisa
Copy link
Contributor

gkubisa commented Jul 16, 2018

Nice, thanks @alecgibson 👍 It's merged and published as @teamwork/[email protected].

@gkubisa
Copy link
Contributor

gkubisa commented Jul 16, 2018

It would be great, if the remote src was reported in create, op and del events... currently we always get false. How do you access the clientId on the client side, @alecgibson ?

In my opinion the above events should be emitted with an Event object, rather than individual params for op, src, etc. This would be much more flexible, however, it would require a major version bump.

@alecgibson
Copy link
Collaborator Author

@gkubisa this isn't to be confused with the (other) source on Doc events. I've never been very sure of what the purpose of that one is apart from figuring out which events have been fired from which clients.

The intended use case for this src modification is to get some client-identifying tag into the src field stored in the database (which is currently just a random Agent/Connection ID).

@gkubisa
Copy link
Contributor

gkubisa commented Jul 16, 2018

Grand, I might look into exposing it in the client API at some point.

@alecgibson
Copy link
Collaborator Author

Yes, that would make sense. My use-case is really only about dumping them into the database for retrieval via Mongo (outside of ShareDb), but they should be pretty trivial to extract again, too at the other end. I thought I wouldn't implement that end, because I don't really know what would be required.

@alecgibson
Copy link
Collaborator Author

As we discussed in the PR meeting, I'll close this in favour of just storing this on metadata in the middleware.

If anyone does want this sort of functionality in the future, we discussed that the best option is just to let consumers pass in a function that generates an ID, to give more control to them.

@alecgibson alecgibson closed this Jul 18, 2018
@curran
Copy link
Contributor

curran commented Jul 19, 2018

How would the equivalent middleware code look?

Might it be possible to convert this PR into a reusable middleware package?

It would be a shame to see this work go to waste, especially if it address a common problem.

@alecgibson
Copy link
Collaborator Author

@curran I need to implement this, so once I've done it I'll try to share a gist or something.

@alecgibson
Copy link
Collaborator Author

alecgibson commented Jul 19, 2018

@curran for reference, this is roughly what the middleware looks like:

share.use('connect', (request, callback) => {
  // Clone request to avoid mutation after connection
  const requestJson = JSON.stringify(request.req || {});
  const requestData = JSON.parse(requestJson);

  Object.assign(request.agent.custom, requestData);
  callback();
});

share.use('submit', (request, callback) => {
  request.op.m.userId = request.agent.custom.userId;
  callback();
});

I think it's probably too small to bother putting into a library.

@alecgibson alecgibson deleted the custom-agent-id branch July 19, 2018 09:22
@curran
Copy link
Contributor

curran commented Jul 20, 2018

Awesome. Thanks!

@houshuang
Copy link

@alecgibson Hi, this looks like exactly what I need, but I need a bit more context to use it. My use case is that I want to identify a userid when logging in from the client, however for the life of me I cannot figure out how to pass the custom userId when connecting from a client? Your help would be much appreciated! I tried

  const ReconnectingWebSocket = require('reconnectingwebsocket');
  const sharedbClient = require('sharedb/lib/client');

  socket = new ReconnectingWebSocket(shareDbUrl);
  _connection = new sharedbClient.Connection(socket, { userId: 'stian' });

but when console.logging the request in the middleware, it was undefined. And anyway I don't see any code in the client library which would pass any information along to the server?

@alecgibson
Copy link
Collaborator Author

@houshuang you need to pass the information in from the server side, so wherever it is in your backend code that you call share.connect or share.listen, you need to pass the user Id in there (eg by retrieving it through an authentication cookie or similar).

@houshuang
Copy link

houshuang commented Oct 1, 2018 via email

@alecgibson
Copy link
Collaborator Author

@houshuang on your server you should some code that looks something like this (from the README):

var WebSocketJSONStream = require('websocket-json-stream');

// 'ws' is a websocket server connection, as passed into
// new (require('ws').Server).on('connection', ...)
var stream = new WebSocketJSONStream(ws);
share.listen(stream);

Each websocket will be unique to the connecting client, which you are hopefully somehow authenticating when they connect. With that authentication information, you should have a handle on their user ID on the server, so you should be able to adapt the above to something like:

var wss = new WebSocket.Server();
wss.on('connection', function(ws, req) {
  // If you're using Express you'll need to use cookie-parser.
  const authenticationToken = req.cookies.token;
  // Then you'll presumably need to do something with your token
  // to check what the user's ID is (eg db lookup, or parse JWT, etc.)
  const userId = getUserIdFrom(token);

  const stream = new WebSocketJSONStream(ws);   
  backend.listen(stream, { userId: userId });
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants