mizzao / meteor-sharejs

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

Unclear how to require that only authenticated users can edit #31

Closed mitar closed 8 years ago

mitar commented 9 years ago

Examples who how to limit access only to a predefined set of users. But how could I allow everyone to see the content, but only authenticated users could edit documents?

mizzao commented 9 years ago

So, some history on this: I wrote this for a quick-and-dirty way to get concurrent editing into Meteor apps. @kbdaitch added some integration with the ShareJS authentication, which basically handles authorization and authentication in two separate functions.

It's possible the current mode of operation doesn't support what you want to do, but it would basically happen by denying certain operations for unauthenticated users. You would probably need to write some custom code there.

Overall though, I think this is just a band-aid package until ShareJS 0.7 is stable or we get some better OT support in Meteor.

A simpler way to achieve what you are doing would be to use a method call to get the document snapshot from ShareJS for unauthenticated users, and use the actual editor for authenticated users. You could also just connect to ShareJS and disable the editor on the client side, but technically that's a security risk I suppose.

mitar commented 9 years ago

Hm, now that I am looking at this, this looks insecure. How it is currently coded client side sends Meteor.userId() to the server side and server side simply trusts this and decides then with all this complicated rules code if it allows or disallows a change. But of course, anyone can modify client side code to send a static ID and impersonate any user (userIds are not really a secret in Meteor).

I know that this is a dirty patch to get something in, and I am also using it as a dirty patch, but it works pretty well, so I am looking into how far does it go. :-)

mizzao commented 9 years ago

Hi @mitar - yes, good point, I remember this issue now; I had forgotten about that. I actually did this originally because I wanted to associate ShareJS ops with userIds, and I think this was the shortest path to victory or something.

Something more secure would be to send along the userId and a one-time key that only the user knows. I'm not really much of a security person; any thoughts on how to do this while minimizing the overhead for each ShareJS connection?

mitar commented 9 years ago

I think this should go through Meteor methods instead of using ShareJS transport. Then there would be an easy access to Meteor userId on the server.

So I think we should find a way to access Meteor userId on the server, instead of implementing some random tokens and stuff.

mitar commented 9 years ago

Probably we should use meteor-streams for ShareJS transport.

mizzao commented 9 years ago

Implementing our own ShareJS transport seems like the "right" way to go; however I'm not sure it's worth implementing on the legacy ShareJS 0.6.x. I think I looked into this earlier and got a pretty negative response: share/ShareJS#277.

Perhaps we should try again with ShareJS 0.7. Keep in mind, though, that some of the "security" right now comes from not publishing document IDs to users who shouldn't be able to see them - so even if they could impersonate a user, they won't be able to open the right documents.

However, the creation of new documents is not limited at all right now. All in all, a big mess, and something I would like to fix...eventually.

mitar commented 9 years ago

I read the ticket you referenced and I think they answered to you and this is also what I am reading from their docs: ShareJS is modular enough to implement what we want.

I was reading the current master documentation when I referenced above their README, so we could do the 0.7, if I understand correctly. If you want, create a new branch, pull in a new version, 0.7, and don't make ACE default, just plain text, (ACE should be then additional plugin one can add), and I can look into making a Meteor-based transport. So you do the rest, I will connect the transport.

mizzao commented 9 years ago

I think you could actually make a ShareJS transport over DDP pub-sub but I don't think the client and server functions are exposed enough to do this easily; that's what I was complaining about in that issue.

Right now they are providing the communication code; there is no abstraction there. So you would be doing a lot of work to put in our own transport.

I'm going to split the editor modules as in #27 and then make a 0.7 branch. Things are busy here so it won't be instant, but let's see what we can do.

I think perhaps we should jump on a call and look at the 0.7 code together, to see where we can replace the current transport with DDP stuff.

mitar commented 9 years ago

I think you could actually make a ShareJS transport over DDP pub-sub

We can, or we can just use Meteor streams, which provide a nice abstraction already over the same thing. So it is even easier.

but I don't think the client and server functions are exposed enough to do this easily

In documentation they are claiming that they do.

I think perhaps we should jump on a call and look at the 0.7 code together, to see where we can replace the current transport with DDP stuff.

We can do that as well. Do you have any proposal for technology for looking at the code together?

mitar commented 9 years ago

But see example. It seems doable. So instead of this browserChannel, we use Meteor pub/sub or meteor streams. And then we call stream.push data and stuff.

mitar commented 9 years ago

And on the client side, we just need an object, which simulates websocket API.

zephraph commented 9 years ago

I'm working on a project that needs this as well. Have either of you made any progress since this conversation?

DavidSichau commented 9 years ago

I also need a more secure solution than the already implemented.

The main problem is, that the ShareJS model callbacks have no access to Meteor.userId() on the server, therefore this approach fails. Meteor.userId() is only available in Meteor methods. This makes it insecure, because one need to trust the client code.

So far I thought about a meteor method, which is called before an shareJS connection is generated. This method should check the access rights of the user and returns a token, which is stored in a server site collection. I addition to the token the userId and the docId should be stored.

On the client side this token is send with each shareJS operation. Then this token is checked on the Server side if it is available in the collection and if it corresponds to the correct document. It is then also possible to get the user and make further validation checks.

I am willing to implement this kind of authorisation, if there is an general interest. However I am unsure how to add it to this package. Especially the auth function, which is executed on the server. What would be the best approach to let users of the package implement their own auth functions?