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

User Accounts integration done for mongo users #6

Merged
merged 3 commits into from
Jan 23, 2014
Merged

User Accounts integration done for mongo users #6

merged 3 commits into from
Jan 23, 2014

Conversation

charandas
Copy link
Contributor

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

Thanks.

@charandas
Copy link
Contributor Author

@mizzao what do you think?

@mizzao
Copy link
Owner

mizzao commented Dec 3, 2013

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
Copy link
Contributor Author

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 charandas mentioned this pull request Dec 11, 2013
@charandas
Copy link
Contributor Author

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

@mizzao
Copy link
Owner

mizzao commented Jan 23, 2014

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
Copy link
Owner

mizzao commented Jan 23, 2014

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?

@mizzao mizzao merged commit 49f363c into mizzao:master Jan 23, 2014
@charandas
Copy link
Contributor Author

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

@charandas
Copy link
Contributor Author

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

@mizzao
Copy link
Owner

mizzao commented Jan 23, 2014

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: josephg/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
Copy link
Contributor Author

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
Copy link
Owner

mizzao commented Jan 24, 2014

@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
Copy link
Contributor Author

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
Copy link
Owner

mizzao commented Jan 25, 2014

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

This was referenced May 16, 2016
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.

None yet

2 participants