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

Plugin system with authentication and userlist plugin. #35

Merged
merged 5 commits into from
Jul 4, 2015

Conversation

timschwartz
Copy link
Member

No description provided.

Moved authentication and user logging to plugin system.

Implemented 'users_online' server request.
@sirkitree
Copy link
Contributor

Wow, this is really impressive @timschwartz! I haven't tested it out yet, but gave the code a peruse this morning. I won't be around much today to test most likely but I'll try as soon as possible.

  • I really like the Plugin architecture you've come up with. I'm wondering how you see people installing plugins. It would be good to have some instructions around that in the near future. For instance if someone writes a totally separate plugin that you can install with npm, how would that work?
  • I'd like to tag the current version and start semantic versioning. Then this is a large enough architecture change that we would tag a new major version since it's not entirely backwards compatible. It mostly is, but we have no automatic cleanup if someone has existing tables and such, so I'd term this a breaking change. But perhaps we merge this and tag as the first major version and write up some instructions on how to upgrade from a previous version. Thoughts?
  • There is some indenting that could be fixed. Each line within a function() should be indented.
  • Lastly, I'd love to get @source83's review of this since it touches much of what he wrote, moving it out of the core and into a plugin. @source83 if you don't have time, that's fine, just let us know.

@timschwartz timschwartz mentioned this pull request Jul 3, 2015
@timschwartz
Copy link
Member Author

For instance if someone writes a totally separate plugin that you can install with npm, how would that work?

We could include the node_modules/ directory in the plugin search path.

Actually, now that I think about it, require() already loads stuff from node_modules/.

So making an npm package "should" just work with a slight edit to the plugin loading function.

@timschwartz timschwartz closed this Jul 3, 2015
@timschwartz
Copy link
Member Author

I'm going to make a few changes and repush this.

@sirkitree
Copy link
Contributor

Was just about to reply when you closed this... you can actually just push to the branch to make changes.

Changed 'mysql-userlist' to npm package 'janus-mysql-userlist'.
Removed plugins/ directory. Plugins install via package.json now.
@timschwartz
Copy link
Member Author

OK, I've created packages janus-mysql-auth and janus-mysql-userlist on npm and adjusted janus-server to use them.

@timschwartz timschwartz reopened this Jul 3, 2015
@sirkitree
Copy link
Contributor

Dude, you're amazing! :) I should be able to give this a try within the hour 👍

@sirkitree
Copy link
Contributor

Tested locally and I'm still able to connect. npm install the plugins and they run just fine as far as I can tell. the users table was created locally, already had access_statistics.

The php script that is currently in the lobby displaying the user online is reading from the online_users table which was installed as a result of the previous code. This table only ever held user names as they signed in and removed them on log out. What would be the new method of detecting online users since this seems to hold users in the new users table indefinitely?

@timschwartz
Copy link
Member Author

You should widen the ip column of access_statistics to 40 characters so it will fit IPv6 addresses.

Use this query to see currently online users:

"SELECT userId, roomId, ip, note, client_version FROM users WHERE updated_at > DATE_SUB(NOW() , INTERVAL 10 SECOND)"

@sirkitree
Copy link
Contributor

Ah, so that table is updated on an interval. Ok I didn't realize that. Thank you.

Another question: If I wanted to authenticate against the VR Sites user base, I'd need to create my own authentication module, similar to janus-mysql-auth, and use that one instead, correct?

@timschwartz
Copy link
Member Author

Right, you would just need to modify the queries a bit.

@sirkitree
Copy link
Contributor

Ok, cool. I think this is okay to merge. Really great work @timschwartz! Thank you so much 😀

It'll take me a bit to deploy on VR Sites and get that php script changed up. I think once this and the config changes are merged, I'm going to cut our first release: v0.1.0

timschwartz added a commit that referenced this pull request Jul 4, 2015
Plugin system with authentication and userlist plugin.
@timschwartz timschwartz merged commit 31f4d56 into janusvr:master Jul 4, 2015
@timschwartz
Copy link
Member Author

Cool.

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.

2 participants