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

[NEW] Do not rate limit bots on createDirectMessage #7326

Conversation

jangmarker
Copy link
Contributor

@jangmarker jangmarker commented Jun 24, 2017

@RocketChat/core

Closes #7302

The code is adapted from similar code in sendMessage.js, including the fix of #7325.

@CLAassistant
Copy link

CLAassistant commented Jun 24, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@geekgonecrazy geekgonecrazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off thanks for contributing! Second, i'd like to suggest doing based on permission instead of on role.

connectionId() {
return true;
userId(userId) {
const user = RocketChat.models.Users.findOneById(userId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use RocketChat.authz.hasPermission(userId, permissionNode) instead of finding a user by id and checking the role.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@jangmarker jangmarker force-pushed the dont-rate-limit-bots-on-createDirectMessage branch from af31639 to 6f64e37 Compare July 17, 2017 04:36
@geekgonecrazy geekgonecrazy dismissed graywolf336’s stale review July 17, 2017 16:50

Changes addressed

@graywolf336
Copy link
Contributor

Follow up question: does the bot role have send-many-messages applied to it by default?

@geekgonecrazy
Copy link
Contributor

geekgonecrazy commented Jul 17, 2017

@graywolf336
Copy link
Contributor

Was a migration created to apply this to existing bots? If not, then it's kind of pointless to use that permission for previous systems and will only apply to new bots.

@jangmarker
Copy link
Contributor Author

@graywolf336 No, I didn't add a migration, didn't think of it. I can add this in a follow-up PR, if you like?

@graywolf336
Copy link
Contributor

@jangmarker That would be great. Mark it as a [FIX] since that supposedly has made it in a release already and should be fixed.

@jangmarker
Copy link
Contributor Author

@graywolf336

I might be mistaken here, but I understand the permission+roles system to work as follows:

  1. a user has a role
  2. a role has permissions

So as long as all bots have the bot role they now automatically have the permission send-many-messages via the bot role.

Is that correct?
In this case, no migration is required, from what I understand.

@geekgonecrazy
Copy link
Contributor

@jangmarker looks like you're correct. If you modify a permission a migration is needed but looks like if a new one is created no migration is needed: https://github.com/RocketChat/Rocket.Chat/blob/develop/packages/rocketchat-authorization/server/startup.js#L69

@geekgonecrazy geekgonecrazy added this to the 0.58.0 milestone Jul 18, 2017
@jangmarker
Copy link
Contributor Author

Thanks for confirming :-).

Then I think nothing is left for this PR except waiting for it be merged. Thanks for all your help and patience.

@rodrigok rodrigok merged commit 320145a into RocketChat:develop Jul 25, 2017
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.

Lift rate limit on createDirectMessage for bots
5 participants