RocketChat / Rocket.Chat

The communications platform that puts data protection first.
https://rocket.chat/
Other
40.09k stars 10.35k forks source link

Update usename with RateLimiter even for admin user #11436

Open uncledent opened 6 years ago

uncledent commented 6 years ago

Description:

If you update a username of a user logged in as a admin user, then it is still delimited to 1 per 60 second.

Steps to reproduce:

Make multiple REST calls to user.update with a username set.

Expected behavior:

It should not be delimited for admin user

Actual behavior:

It is delimited

Server Setup Information:

0.66.3

Additional context

The problem is here:

https://github.com/RocketChat/Rocket.Chat/blob/34e607c4326ccb8896c3eb9519fc5c92895a1a2e/packages/rocketchat-lib/server/functions/setUsername.js

RocketChat.setUsername = RocketChat.RateLimiter.limitFunction(RocketChat._setUsername, 1, 1000, {
    [0](userId) {
        return !userId || !RocketChat.authz.hasPermission(userId, 'edit-other-user-info');
    }
});

!RocketChat.authz.hasPermission(userId, 'edit-other-user-info') checks if the user which is being changed has this permission, but not if the user who performs the change has it.

That means that admin can change usernames without delimiter of admins only.

Relevant logs:

You must wait 60 seconds before trying again. [error-too-many-requests] at EventEmitter.setUsername (/app/bundle/programs/server/packages/rocketchat_lib.js:2902:15) at EventEmitter.RocketChat.saveUser (/app/bundle/programs/server/packages/rocketchat_lib.js:5354:18) at DDPCommon.MethodInvocation.Meteor.runAsUser (/app/bundle/programs/server/packages/rocketchat_api.js:5721:52) at packages/dispatch_run-as-user.js:211:14 at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:12) at Object.Meteor.runAsUser (packages/dispatch_run-as-user.js:210:33) at Object.post (/app/bundle/programs/server/packages/rocketchat_api.js:5721:12) at Object._internalRouteActionHandler [as action] (/app/bundle/programs/server/packages/rocketchat_api.js:197:37) at Route.share.Route.Route._callEndpoint (/app/bundle/programs/server/packages/nimble_restivus.js:347:32) at /app/bundle/programs/server/packages/nimble_restivus.js:236:33 at packages/simple_json-routes.js:98:9

Galatoni commented 5 years ago

I based on the reports of others and issues i'm seeing when attempting to update users details en-masse via scripts; this issue is affecting pretty much anything from the api users endpoint. This includes, setting avatars and updating other details for users, even when done by an admin user.

The communication to these endpoints by anyone with the admin role shouldn't be limited IMO.

winterstefan commented 5 years ago

Hey guys,

I can confirm this issue from my side. I'm using an admin account for updating some user's names. On a regular basis, that can happen more frequently than 1 every 60s (current rate limit).

In my use case, Rocket.Chat is just a secondary data source, where the primary data source syncs the username automatically (so editing a username in system A will trigger an update in Rocket.Chat). This sync tends to run out-of-sync as soon as the rate limit is reached.

Is there any plan in opening up this limit for admins?

mbrodala commented 4 years ago

I noticed the same in a similar setup but with e.g. groups.invite and groups.kick if called quickly and repeatedly.

In theory users with the role bot or admin should be able to bypass the API rate limit:

https://github.com/RocketChat/Rocket.Chat/blob/5bbd7d772a67aa1e675d6f3da99e47a3cc049519/app/authorization/server/startup.js#L21 https://github.com/RocketChat/Rocket.Chat/blob/6ef7275eb7cd0b8964328516a2742d135d6c379c/app/api/server/api.js#L186

However, I still see the mentioned rate limit applied no matter what. Is there a way to debug the API condition I just mentioned?

In any case for now I have disabled the API_Enable_Rate_Limiter setting and the calls are working as expected. Given that API access requires a token the risk is rather low but still I'd like to know if and how this is supposed to work.

stylefish commented 3 years ago

i notced the same when calling users.setStatus with a user that has the api-bypass-rate-limit gets "please slow down messages"