mizzao / meteor-sharejs

Meteor smart package for transparently adding ShareJS editors to an app
MIT License
225 stars 53 forks source link

User Accounts integration done for mongo users #6

Closed charandas closed 10 years ago

charandas commented 10 years ago

I was able to modularize all of the code to decouple client and server code. Feel free to critique.

Thanks.

charandas commented 10 years ago

@mizzao what do you think?

mizzao commented 10 years ago

Hi @kbdaitch this looks quite comprehensive; give me some time to review it. It's probably about time I also took a look at the ShareJS API and thought about the right way to do things. Did you mention you modified the documents demo to use this as well?

I just got done modifying the library to download and serve CDN files over Meteor directly rather than having the client do it, so I'll need to merge your changes in.

charandas commented 10 years ago

Hi @mizzao. Yes, I did use the documents demo in forming my app. Yeah, it would be great to have the assets come from CDN. Let me know how it goes.

charandas commented 10 years ago

Any updates on this, @mizzao ? Did you get a chance to try it out, or look at the code?

mizzao commented 10 years ago

Hey @kbdaitch, I've been busy but will try and review and merge this over the next few days. I need to keep track of user ids in my docs as well, so we will make sure this is done right. :)

Your changes look quite comprehensive. However, maybe we can have a Skype call or something to discuss them so you can explain what you did. I'm not sure about the owner vs invited-users authentication model as a default, as that doesn't really fit my use case. Let's talk about it.

mizzao commented 10 years ago

Sending Meteor.userId() as the authentication token seems a bit insecure because a user can typically access the userId of other users in the Meteor app. Might we think of something else to use?

charandas commented 10 years ago

A skype call can be great. We can decide on time. Wait a minute, you just merged it?

charandas commented 10 years ago

Publications can be a way to secure user ids. Something like this with additional constraints and removing autopublish should do it.

mizzao commented 10 years ago

Hi @kbdaitch, I merged your code and did some cleanup to simplify things. Please note the slightly different structure of the config file and reorganization of the auth class. Since it doesn't make sense to support something other than Mongo right now, I also took out some of the extra logic for that. Maybe see if the updates still work with your documents-users. (We should probably work out some tests at some point.)

I spent some time trying to see if I could store the Meteor userIds in the ShareJS metadata, which would be very useful with this code. It didn't seem possible to me, though: share/ShareJS#286. Maybe you can help with that?

I'm fine with using the userId for authentication for now, and would happily implement some of the more useful features first before working on security.

Haha - your message popped up just as I was writing this one.

charandas commented 10 years ago

Hi @mizzao, yes, it still works with my documents-demo.

I would definitely love to add tests when the opportunity presents itself. Keep me in loop when you get to that. Thanks for the merge. Hopefully this feature helps someone.

mizzao commented 10 years ago

@kbdaitch note the change in configuration structure, if you were using that.

Never mind...I guess your app doesn't do that. How did you test the authentication stuff?

charandas commented 10 years ago

Thanks for your note, but what exactly changed, is it the added fields:

{
      "db": {
        "type": "mongo",
        "opsCollectionPerDoc": false
      }
}

Its pretty easy to test the authentication stuff with my documents demo. Just checkout the master of meteor-sharejs one directory outside relative to the demo checkout (just like you do with your demo app). The settings.json that is currently checked in has authentication/authorization enabled.

Run the demo with meteor --settings settings.json.

With the current setup:

  1. Authentication only requires that the token supplied be a valid Meteor userId.
  2. The authorization requires the either the document owner matches the token, or is in invitedUsers.

In the browser console:

  1. You can see forbidden failures in authentication to sharejs api if you (deliberately) use a nonsensical setting such "_id": "isnt_equal".
  2. You can see forbidden failures in authorization if you (deliberately) disallow invited users or the owner. "userId": "isnt_equal" or "invitedUsers": "isnt_in_array".
mizzao commented 10 years ago

@kbdaitch I was confused about which repo it was, I thought it was https://github.com/kbdaitch/meteor-documents-demo but it appears to be something else that I don't see as a public repo.

I'm surprised that your app works because I changed the keys of the config and also removed the criteria field because it was basically a singleton. Can you pull from upstream and see if things are still working as intended?

Thanks!

charandas commented 10 years ago

@mizzao thanks for bringing this up. My bad, even though I was trying to test against upstream/master, I forgot to run with mrt meaning I was using the cached version of meteor-sharejs, which didn't have your changes.

I just ran against upstream with the updated settings schema (without criteria and public client-side settings), and it seems to be working as intended. All of the instructions above for testing auth still apply.

Thanks much!

charandas commented 10 years ago

It just occurred to me that an alternative way to test authentication in chrome devtools JS console is:

Session.set('document', <sharejs-document-id-not-owned-or-collaborated_upon-by-you>)

This would show loading... in the editor and forbidden in the console.