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

Add support for WebSocket #229

Merged
merged 30 commits into from
Jun 10, 2016

Conversation

rodrigoprimo
Copy link
Contributor

This PR is a working proof of concept of adding WebSocket support to the Liveblog plugin (issue #211). It uses socket.io, Redis and socket.io-php-emitter (responsible for sending messages from PHP to the socket.io server via Redis). I'm opening it to validate the softwares that I chose and the initial implementation before continuing. It is important to note that this is my first experience with Socket.io and Redis.

I chose to use Socket.io and Redis for this proof of concept because of Guillermo Rauch and Tony Kovanen talks about the subject on WordCamp San Francisco 2014 and WordCamp Europe 2015. I wasn't able to find any WordPress plugin that supported WebSocket to use as reference or to check for alternatives to Socket.io. I still intend to do a little more research to see if I find anything worth checking. Please let me know if you think I should look at another WebSocket library.

In this PR the plugin will use WebSocket instead of AJAX polling to refresh the list of Liveblog entries in the connected clients whenever a entry is added, deleted or updated. The plugin is still using AJAX when the user create, delete or update a Liveblog entry since I wasn't able to find a compelling reason to switch that part to WebSocket.

How to test

To test this PR it is necessary to have both Redis and Node.js installed and then, from the Liveblog plugin directory, run the following commands:

composer install     # to install socket.io-php-emitter
cd nodeapp
npm install     # to install socket.io and the adapter socket.io-redis
node app.js     # to start the Node app that starts the socket.io server

Finally you should add the constant LIVEBLOG_USE_SOCKETIO to wp-config.php and set it to true.

After following the steps above, if you open a Liveblog post in two different browser windows, it is possible to see that whenever a user changes the list of entries in one window, the list in the other window is refreshed in "real time".

Next steps

After researching a bit more for alternatives to Socket.io and hearing feedback from others, if a decision is made to use the architecture proposed in this proof of concept, the next steps will be:

  • Fix known issues
  • Better organize the code created for this PR
  • Write documentation

Known issues

This is a list of known issues that will be fixed if a decision is made to proceed with the proposed architecture.

  • Users without permission will see new or updated entries with both the edit and delete button.
  • For now it is only possible to use Redis and Socket.io running on the same server as WordPress and in their default ports.
  • WordPress supports PHP 5.2 but socket.io-php-emitter works only with PHP >= 5.3 (it uses namespaces) and that is why the Travis build is failing.
  • Handle errors when it is not possible to connect to the Socket.io or Redis server

This commit adds:

- a Node.js app that starts the socket.io server with the Redis adapter to a new directory called "nodeapp"
- "rase/socket.io-emitter", the package responsible for emiting messages from PHP to the socket.io server using Redis, as a new dependency to composer.json
- the socket.io client to the list JSs enqueued by the plugin
When a constant called "LIVEBLOG_USE_SOCKETIO" is set to true, the plugin will use a webscoket instead of AJAX requests to refresh the list of Liveblog entries.
@mdawaffe
Copy link
Member

This looks great. I'm pleasantly surprised at how simple the basics are.

@rodrigoprimo
Copy link
Contributor Author

I was pleasantly surprised as well. It took me longer to figure out a dependency problem in an example I followed to test the technology (https://github.com/rase-/wceu-2015-presentation-code/pull/1) than to create this PR.

I did a little more research and I think that Socket.io, Redis and socket.io-php-emitter are indeed a good option to add websocket support to the plugin. This trio seems the natural choice for WordPress and I couldn't find any alternative with an outstanding advantage. Although it is important to note that I could find only presentations and examples on the subject. I couldn't find any project being used in production with websocket and WordPress. Anyone knows a project using those technologies that I could use as a reference?

To proceed I need help with the following. socket.io-php-emitter requires PHP >= 5.3 but WordPress and Liveblog plugin supports PHP 5.2. Considering that less than 10% of all WordPress installs are running PHP <= 5.2 (https://wordpress.org/about/stats/) and that to use websocket the user will probably have the means to update PHP (as it will be be necessary to install both Node.js and Redis), I think it is easier to require PHP 5.3 than to take the time to find or create a PHP 5.2 compatible solution. Anyone against requiring PHP >= 5.3 for the websocket support?

Anything else to add before I continue the implementation?

@mdawaffe
Copy link
Member

I think bumping the requirements for this piece is fine especially since, as you mentioned, we're already bumping the Node and Redis requirements :)

I pointed out to you privately another Automattic project that uses websockets, though I don't think it will be too useful for this (different requirements, different capabilities).

The approach looks good to me.

All the PHP code related with Socket.io was moved from liveblog.php to its own class called WPCOM_Liveblog_Socketio to better organize the code.
@rodrigoprimo
Copy link
Contributor Author

Ok, thanks for your feedback. I will proceed with the implementation.

This commit checks the PHP version and load WebSocket support only if server is running PHP >= 5.3.0 (which is required by socket.io-php-emitter). If an older version is found, a message is added to the admin asking the user to update PHP or to remove LIVEBLOG_USE_SOCKETIO from wp-config.php.
@rodrigoprimo
Copy link
Contributor Author

There is an issue that I hadn't considered before so I'm writing to explain it and tell how I'm planning to solve it. Please let me know if you have any comments.

By default any client can connect to the WebSocket server and I can't find an easy way to associate WordPress users with Socket.io clients in the Socket.io server (anyone aware of a way to achieve this?). This means that anyone can listen to any message emitted by WP via Socket.io and that there is no way to tell in the Socket.io server what WP content a given user has permission to see. Two problems come to my mind because of that:

  • It is easy for anyone to connect to the Socket.io server and to listen to messages that he is not supposed to. In the case of the Liveblog plugin, this means that anyone could listen to new entries created for a private Liveblog post.
  • When a new Liveblog post entry is created, PHP sends a message to Socket.io via Redis with the HTML that should be displayed to the user. Socket.io then sends this message to all connected clients. The problem is that, since the message was generated in PHP by a user with permission to add, edit and delete entries, the HTML contains the "edit" and "delete" buttons and there is no way to tell which clients should receive just the entry text and which clients should receive the entry text and the action buttons.

It seems to me that the ideal solution would be to somehow share the information about the users between WordPress and the Socket.io server making it possible for the Socket.io server to know which clients have permission to receive a particular message. But I couldn't find an easy way to do that so I'm considering an alternative approach that doesn't include authenticating WP users in the Socket.io server hoping that it will be good enough.

About the first problem, I think it is fair to consider that most of the Liveblog posts are public and maybe only a few are private. So my idea, given that the Socket.io server is not aware of WordPress users and their capabilities, is to add WebSocket support only for public Liveblog posts, at least for now. This way the problem will be solved simply by using Socket.io only for messages that any client can receive and I'm hoping that this will still benefit most of the users of the plugin.

I'm planning to address the second problem by hiding the action buttons with JavaScript when the client (the browser) receives the message. An attacker could then see those buttons but as far as I can see they won't work so it is ok.

This change is necessary since for now there is no way to check in the socket.io server if a connected client has permission to view a Liveblog post.
…e permission to edit or delete entries

Since the message with the new entry is always emitted in PHP by a user with permission to edit entries and in the Socket.io server there is no way to tell which clients have permission to edit entries, we have to remove the action buttons when necessary in the JS before displaying them to the users.
… is used for updated and deleted entries as well

The message name was "new liveblog entry $post_id" and now it is "liveblog entry $post_id"
…e URL used by the Socket.io client to connect to the Socket.io server
…et users configure the host and port used to connect to the Redis server
…in the Socket.io server via command-line arguments

This commit adds a Node module called "commander" to the Node app used to start the Socket.io server to let users configure Socket.io server port, Redis host and Redis port using the following syntax:

node app.js --socketio-port [port] --redis-host [host] --redis-port [port]
The update nag bar was not being used when Socket.io support was enabled as liveblog.display_entry() was being called directly. This commit separates the logic to use the update nag bar from the logic that handles the AJAX response by creating a new method called liveblog.maybe_display_entries() and uses it when the Socket.io client receives a new message.
… support

Only enable WebSocket support if socket.io-php-emitter is installed otherwise a message is displayed in the admin and AJAX is used.
…server

This commit adds an error message using the same style as the update nag bar when the Socket.io client is unable to connect to the Socket.io server. If the connection is reestablished the message is removed.
It was necessary to change the Redis client from TinyRedisClient to Predis since the former didn't provided a way to check if the connection with the Redis server was established.
…ethods into one

The method WPCOM_Liveblog_Socketio_Loader::is_socketio_enabled() was called only once inside WPCOM_Liveblog_Socketio_Loader::should_use_socketio() so it was removed and its functionality was moved to WPCOM_Liveblog_Socketio_Loader::should_use_socketio().

The method WPCOM_Liveblog_Socketio_Loader::should_use_socketio() was then renamed to WPCOM_Liveblog_Socketio_Loader::is_enabled()
The same structure used in other JS files of this plugin was used. Now Socket.io client is loaded only when the event 'after-init' is triggered in liveblog.init().
@rodrigoprimo
Copy link
Contributor Author

I've finished implementing WebSocket support using the approach discussed in the comments above and I also wrote a first version of the documentation in the README.md file.

For now, as discussed, WebSocket support will only work for public Liveblog posts (see #229 (comment)) since I couldn't think of an easy way to prevent someone without authorization to receive messages for a private Liveblog post (I was assuming that it would be necessary to find a way to verify WordPress users in the Socket.io server).

But now it occurred to me that it would be possible to generate an unique key for each Liveblog post and send this key from WordPress only to the clients with permission to see the post. The client would then send the same key to the Socket.io server when establishing a new connection. This way, when there is a change in a entry, WordPress could send a message to the Socket.io server with the entry content and the Liveblog post key. The server would forward this message only to the clients that connected providing the same key. With this setup, someone without permission to see the Liveblog post would still be able to connect to the Socket.io server but wouldn't receive any message related with the private post.

I still have to check if it is possible to make the Socket.io server send a message only to the clients that connected providinng the right key but I think so. I'm sharing this idea here anyway in case anyone has any suggestion or can see something that I'm missing. It is also necessary to consider if it is worth to support private Liveblog posts since it seems safe to assume that the majority of the Liveblog posts are public. Maybe we could leave this for another moment if there is demand for this feature.

Please let me know as well if you have any comments regarding what I have done so far.

*
* @return string
*/
public static function get_post_key() {
Copy link
Member

Choose a reason for hiding this comment

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

This seems fragile. Do we know that the correct post will be always be in that global?

@mdawaffe
Copy link
Member

mdawaffe commented Feb 4, 2016

It looks like if wan make ::get_post_key() more robust, we'll be able to do two things:

  1. Remove the post_id from the "liveblog entry $post_id" socket message.
  2. Start allowing liveblogs for private posts. We use the key to specify the Socket.IO room. Can you look into if rooms are secret from one another (from the clients' perspective).

public static function get_post_key() {
global $post;

return wp_hash( $post->ID );
Copy link
Member

Choose a reason for hiding this comment

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

wp_hash() has an optional second parameter. I think we should use it: wp_hash( $post->ID, 'liveblog-socket' ) or something.

Simplify the path of the two JS files enqueued to add WebSocket support by removing the redundant '/classes/../'
@rodrigoprimo
Copy link
Contributor Author

I just realized that I made a mistake and unwillingly committed an unfinished test I was doing to check how to support private posts in a commit that was created to change only a detail in the path of the included JS files.

I will fix this by leaving in the commit above only the path fix and I will start working on a new commit to support private posts based on my test and on @mdawaffe suggestions.

…s as well

Before this commit WebSocket was used to update the list of entries only for public Liveblog posts. Now it works for private Liveblog posts as well.

To achieve this WordPress generates a unique key for each post based on the post ID and its status and share this key only with the clients who have permission to see the post. The clients forward this key to the Socket.io server. The Socket.io server use the key to add the clients to a room named after the post key. WordPress emits a new message with a Liveblog post entry only to the clients that are in the post room. This way only users who can see the post will receive the messages.
… key to be generated

When WebSocket is enabled, the plugin uses an unique key for each post based on its ID and status to control who can receive messages emitted by the Socket.io server. This should be enough for most cases but it has its limitations. Since the key for a given post is the same for all users and it doesn't change after a period of time, once a user receives the key it can have access to the messages emitted by the Socket.io server forever (assuming he saved the key somewhere) even if his account is removed or he loses access to the post.

This is not a problem for public Liveblog posts but it can be for private Liveblog posts with sensitive data. So the filter 'liveblog_socketio_post_key' was created to allow sites to implement their own criteria to generate the key. For example, a site could implement a random post key that can be manually invalidated by an editor whenever necessary.
@rodrigoprimo
Copy link
Contributor Author

It looks like if wan make ::get_post_key() more robust, we'll be able to do two things:

Regarding making ::get_post_key() more robust I've replaced the post global with a call to WPCOM_Liveblog::get_post_id() (a method that I just created to return the Liveblog post id). I've also added the post status to the data used to generate the post key. This way the key will change if a post changes from public to private. You have something else in mind?

  1. Remove the post_id from the "liveblog entry $post_id" socket message.

Done

  1. Start allowing liveblogs for private posts. We use the key to specify the Socket.IO room. Can you look into if rooms are secret from one another (from the clients' perspective).

Rooms are secret from one another from the clients' perspective in the sense that rooms can be joined only on the server side and that room messages are send only to the clients that are in a given room. So without the post key it is not possible to receive the messages for a given post.

But there is still one problem with Liveblog for private posts and I'm struggling to find a solution that is simple to implement (I'm still assuming that private Liveblog posts are rare so it is worth to support it in this first version only if we find an easy solution). Once a user receives the post key, if he saves it somewhere, he will be able to receive messages from the Socket.io server for that particular post even if for some reason he loses access to the post (for example if the user is removed from WordPress).

In an attempt to find a middle ground between not supporting private Liveblog posts and fully supporting it dealing with the issue described above, I have added a filter to ::get_post_key(). This way a site that wants to use private Liveblog posts with sensitive data could implement its own criteria to generate the key. It could, for example, implement a random post key that can be manually invalidated by an editor whenever necessary. I considered using this approach by default instead of the filter by creating a random post key and adding a button to the Liveblog metabox to invalidate the key and create a new one. But I think this will be confusing for most users and used only by a few.

Do you think the filter solution is good enough (we could add a note in the documentation about the limitations of the support for private Liveblog posts)? Can you see an easy way to prevent someone from receiving messages from the Socket.io server if his user is removed from WordPress or if he loses access to the post?

@mdawaffe
Copy link
Member

Regarding making ::get_post_key() more robust I've replaced the post global with a call to WPCOM_Liveblog::get_post_id() (a method that I just created to return the Liveblog post id). I've also added the post status to the data used to generate the post key. This way the key will change if a post changes from public to private. You have something else in mind?

That's great - thanks. I was thinking the method should accept a post ID parameter: ::get_post_key( $post_id = null ).

Rooms are secret from one another from the clients' perspective

So there's no way for a client to retrieve a list of rooms from the server?

Can you see an easy way to prevent someone from receiving messages from the Socket.io server if his user is removed from WordPress or if he loses access to the post?

Thanks for thinking through these edge cases.

Changing the key is hard - you have to pass the new key to all valid clients, have them connect to the new room, probably send the info out to both rooms for a period to avoid lost data, have the clients merge those duplicates (possibly by going back to a "liveblog entry $post_id" socket message :)), etc. Seems like a mess.

I think documenting the limitations is sufficient. As you mentioned, the use case here is not common.

@rodrigoprimo
Copy link
Contributor Author

That's great - thanks. I was thinking the method should accept a post ID parameter: ::get_post_key( $post_id = null ).

Done

So there's no way for a client to retrieve a list of rooms from the server?

No, a client can't retrieve the list of rooms from the server (unless a method is implement on the server to provide it). So, as far as I can see, using the post key as the room name won't be a problem.

I think documenting the limitations is sufficient. As you mentioned, the use case here is not common.

Ok, I have added a section in the README.md file documenting the limitation.

@philipjohn
Copy link
Contributor

Thanks for being so thorough, @rodrigoprimo .

I decided to give this a spin using VVV. Your clear instructions made that a breeze, and the functionality worked perfectly. It's wonderful to see the Liveblog update so quickly! Nice job.

The code is clear and well-documented, too, making the functionality easy to follow.

@mdawaffe We'll need to disable this on wpcom - do you think setting the LIVEBLOG_USE_SOCKETIO define in wpcom-helper.php to false will be early enough, or should we add a filter on that to the plugin?

@mdawaffe
Copy link
Member

Setting LIVEBLOG_USE_SOCKETIO to false should be enough (the default behavior is disabled, but being explicit is a good idea).

@rodrigoprimo
Copy link
Contributor Author

Thanks for taking the time to check this PR @philipjohn. I'm glad to hear that the functionality worked well.

… that private Liveblog posts is supported

I forgot to update this method documentation when support to private Liveblogs posts was added.
@mdawaffe
Copy link
Member

@philipjohn, pending any tests on WordPress.com, this seems ready to merge to me.

@philipjohn
Copy link
Contributor

Yep, this should be ready to go. Having some trouble with my sandbox so once that's sorted I'll test on WordPress.com and merge

@philipjohn philipjohn added this to the 1.6 milestone Jun 10, 2016
@philipjohn philipjohn merged commit d1a54a0 into Automattic:develop Jun 10, 2016
@philipjohn philipjohn removed this from the 1.6 milestone Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants