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

Fix sscpac/chat-locker#4. Integrate MAC restrictions (backend) into chat-locker #12

Merged
merged 4 commits into from
Jul 29, 2015

Conversation

rwakida
Copy link

@rwakida rwakida commented Jul 23, 2015

  • Create canAccessResource Meteor method that checks if user(s) can access
    a resource based on a set of permissions
  • add MAC checks when creating a direct message or private group.
  • add permissions argument to createDirectMessage and createPrivateGroup
    Meteor methods. permissions apply to the Room being created.
  • add beforeSaveMessage hook that appends accessPermissions and
    legacySecurityLabel from parent room to newly created message. Only
    applies to direct message and private group message because other room
    types are not MAC restricted (i.e. are public)

chat-locker

- Create canAccessResource Meteor method that checks if user(s) can access
  a resource based on a set of permissions
- add MAC checks when creating a direct message or private group.
- add permissions argument to createDirectMessage and createPrivateGroup
  Meteor methods.  permissions apply to the Room being created.
- add beforeSaveMessage hook that appends accessPermissions and
  legacySecurityLabel from parent room to newly created message.  Only
  applies to direct message and private group message because other room
  types are not MAC restricted (i.e. are public)
@bbrockman bbrockman modified the milestone: Product Backlog Jul 28, 2015
@douglasrapp
Copy link

Would it make sense to combine the new "canAccessResource" method with Rocket's existing "canAccessRoom" method? They seem similar, but maybe they are different enough to warrant separation?

@bbrockman
Copy link

I think the main issue with that is that it will make it hard to merge their code with our (upstream merges) if we are overriding their specific methods. What do you think?

@rwakida
Copy link
Author

rwakida commented Jul 28, 2015

Good idea. I thought about adding our code to it at first, but decided not to because:

  1. Like Brent said, merging upstream changes would be harder. We'd have to double check any code that calls the canAccessRoom method.
  2. canAccessRoom only is checking a 'room', but our method can be used to check a more generic "resource" like messages
  3. RocketChat uses canAccessRoom as security check when publishing ChatSubscriptions and sending messages. It checks that the user belongs to the room and assumes if the current user is part of the room, then they have access. The logic is simple which is good because it's run for every sent message. Adding our logic "may" hurt performance.
  4. Our logic safeguards creating the association between a user and a room and depends on the canAccessRoom to enforce that user access to rooms (e.g sending message, seeing rooms, etc)

In regards to #3 and #4 above, is that enough? Should we double check with every message that's sent?

@bbrockman
Copy link

I think it is enough to get them access to the room, but we would also do the checks for each messages to check their accesses right?

@rwakida
Copy link
Author

rwakida commented Jul 28, 2015

we only check access when associating a user to a room. we currently DO NOT check with each message. The only check is that the user belongs to the room (done by RocketChat's canAccessRoom).

We're like the bouncer. We check their access at the door. Once they're in, no access checks are done.

If you want to add more layers of security, we can check for access when publishing Chat subscriptions. That's what is used to build the side nav with the room list. If you really want then we can check each message as well, but I'm not sure about the overhead.

@berkich
Copy link

berkich commented Jul 29, 2015

I would merge it Reid, I looked it over but not in great detail. I would like do some testing with code in issue 3

rwakida added a commit that referenced this pull request Jul 29, 2015
Fix #4.  Integrate MAC restrictions (backend) into chat-locker
@rwakida rwakida merged commit 6deba1c into master Jul 29, 2015
@bbrockman bbrockman modified the milestone: Product Backlog Aug 3, 2015
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.

4 participants