search

FightPandemics / FightPandemics

This is not the first and last pandemic. Currently, the information about the pandemic is highly fragmented, especially at the local level. Whereas one can find global information and trackers, it is difficult to find information that is relevant to your community. Furthermore, it is difficult for organizations, funders, companies, volunteers and local leaders to coordinate responses. This puts humanity and local communities at risk. FightPandemics was born to make communities more resilient to pandemics by facilitating access to information and coordinated responses.
MIT License
110 stars 141 forks source link

Log In Rate Limit on Backend Server #1660

Open jessica-lau opened 4 years ago

jessica-lau commented 4 years ago

After running the login backend test continuously 10+ times, the two tests in the screenshot failed because they're expecting a 429 status error (too many requests) instead of a 401 status error (unauthorized). The message we expect the server to return is 401 unauthorized user. Apparently, the 429 error appears due to rate limits being set to protect the server from intentional abuse. X-RateLimit-* headers can be set with information about the limits.

Firstly, is it OK to set a rate limit? If so, how can we set this and what limits can we use?

https://blog.bearer.sh/error-429-too-many-requests-what-to-do-when-youve-been-rate-limited/

image

jessica-lau commented 4 years ago

@galeza FYI

ddveloper commented 4 years ago

here is the rate-limit definition on Auth0 page, add @mannykary to double confirm https://auth0.com/docs/policies/rate-limit-policy/management-api-endpoint-rate-limits

mannykary commented 4 years ago

I think this is working as intended. Auth0 rate limits and sends back a 429 error code if there are too many requests, and we check for that status code and relay it back to the frontend. This change was made as part of issue #1287, and was fixed in PR #1318.

For mocha tests, I think we should mock Auth0. I believe @joshmorel was starting to work on this in #1495.

joshmorel commented 4 years ago

Haha yes @mannykary. Maybe this week I hope.

@jessica-lau after a given email & IP is blocked it needs to be manually unblocked.

Now when we do implement the Auth0 mock, this test below will fail unless we implement the same kind of rate limiting in front of Auth0 or alternate auth provider. I don't think we should fully duplicate their service but I wonder should we have some sort of rate limiting for our API in general? Thoughts @mannykary? Through AWS, do we have measures to protect against DDoS? And are there other API abuse we should be considered about?

    describe('429 - maximum sign in attempts', function () {

        it('Too Many Requests error - when maximum sign in attempts are exceeded', async function () {
            let response ;
            for (var i = 0; i < 11; i++) {
                response = await apiHelper.sendPOSTRequest(APP_URL, apiEndPoint, userCredentialsWithRandomEmail);
        };
            validator.validateStatusCodeErrorAndMessage(response, httpStatus.TOO_MANY_REQUESTS, 'Too Many Requests', 'Maximum number of sign in attempts exceeded.');
        });   
    });
mannykary commented 4 years ago

If we implement rate limiting in the mock, and use the mock in the mocha test, wouldn't the mocha test just be testing the mock? I'm not really seeing the point of the test.

We created issues #997 (short term NGINX implementation) and #1211 (long term AWS implementation). @git-satya started working on #997 but then realized that we don't really have a good baseline or a good idea of what "normal" is.

joshmorel commented 4 years ago

@mannykary (cc @jessica-lau) I have no plan to implement rate limiting in the mock.

To be honest, I don't think it makes sense to have automated tests for any of the /api/auth/ endpoints because:

  1. Now we are relying on an external service
  2. When the mock is done it will be just a mock

The Cypress e2e tests make sense for our React code.

Unit tests might also make sense when we decouple some logic from Auth0 (first for the mock, later for silkey), however.

Was there any more plans related to mocha/ @jessica-lau tests? Because, it's more things like /api/posts that make sense to test first. Later once we have auth mocked, /api/users & /api/organisations

galeza commented 4 years ago

Hi @joshmorel We were thinking about testing/verifying error messages when incorrect data is provided with /api/auth/.

joshmorel commented 4 years ago

Hi @joshmorel We were thinking about testing/verifying error messages when incorrect data is provided with /api/auth/.

In this case, we do need develop an interface that all the auth providers (mock, Silkey, Auth0) must implement, otherwise you'd just be testing the mock (not very helpful...).

But in such a case unit tests would be equally effective as integration tests. In any case when I get a PR we can review & discuss.

I would wait on doing any more work with mocha for /api/auth. Maybe /api/posts would be a better focus?

galeza commented 4 years ago

@joshmorel

Ok, we will focus on /api/posts.

jessica-lau commented 4 years ago

Okay, thanks @joshmorel