Jblew / firebase-functions-rate-limiter

Js/ts library that allows you to set per-time, per-user or per-anything limits for calling Firebase cloud functions
MIT License
100 stars 15 forks source link

Enhancement: check if quota exceeded without recording usage #2

Closed robertvorthman closed 4 years ago

robertvorthman commented 4 years ago

If I understand correctly, isQuotaExceeded records a usage against the limiter's quota. Is/could there be a feature to check if the quota is exceeded without counting a usage against the quota? I would like to avoid an expensive operation if the quota is already exceeded without incrementing the usage. For example:

const perUserAuthFailsLimiter = FirebaseFunctionsRateLimiter.withRealtimeDbBackend({
    name: "perUserAuthFails_limiter",
    maxCalls: 5,
    periodSeconds: 60 * 60,
}, database);

exports.tryAuth = functions.https.onRequest(async (req, res) => {
    //...

    //aspirational feature enhancement: checkQuotaAlreadyExceeded() or something
    const quotaAlreadyExceeded = await authFailsLimiter.checkQuotaAlreadyExceeded(req.body.username);

    if (quotaAlreadyExceeded) {
        return res.status(429);
        //function execution stops and doesn't continue with unnecessary auth request
    }

    //I would like to avoid hitting the auth server if the quota is already exceeded, but this shouldn't count as an auth failure (since the user's password could have been correct)
    const isAuthenticated = await authenticateUser(req.body.username, req.body.password);
    if (!isAuthenticated) {
        const quotaExceeded = await perUserAuthFailsLimiter.isQuotaExceeded(req.body.username);
        if (quotaExceeded) {
            return res.status(429);
        } else {
            return res.status(200);
        }
    } else {
        return res.status(200);
    }

})
Jblew commented 4 years ago

Hi Robert! You are right, this would be a useful feature. I think, I will implement it today. Stay tuned, I'll mention you here when this is finished.

robertvorthman commented 4 years ago

Thanks. I implemented your library on several of my firebase cloud functions yesterday and it works great.

If you are separating the logic for checking quota and increment usage, it would also be great to have a function that just increments the usage without checking quota. That would reduce firebase reads, right? For example, near the end of my previous code example, if the user authenticates with the wrong password, I should count 1 usage against their quota but there is no need to check if they have exceeded the quota since the cloud function already performed all the work by that point.

Jblew commented 4 years ago

The logic between read and write is coupled on a purpose of atomicity and it is coupled on the level of PersistenceProvider interface. In both backends get-and-update is performed inside a database-provided transaction mechanism.

Also, the data model that is used for storing usages requires a read before update, so there would be no difference between isQuotaExceeded and incrementUsage.

/+ I like the name checkQuotaAlreadyExceeded. I think it is meaningful enough :)

Jblew commented 4 years ago

@robertvorthman — please update to firebase-functions-rate-limiter@3.4.0. Changes:

Please check and tell me if it works as expected. I've already deployed the new version to my functions.

robertvorthman commented 4 years ago

Thanks for the quick work. The new functions are working great. My app UI is more responsive because the rate limited cloud function replies in 60ms if isQuotaAlreadyExceeded == true, instead of doing a bunch of unnecessary work and replying in 600ms.

It would be awesome if the numOfRecentUsages variable in the isQuotaExceeded private method was available so that I could present the user with warnings like "you have 1 attempt remaining..."

I suppose I could just talk directly to Firebase, and I don't need this feature right now, but your library would be more mature with a way to reset usage back to zero for a given identifier.

Jblew commented 4 years ago

It is fairly easy to be done. The easiest way to do this it to replace isQuotaExceededOrRecordCall in GenericRateLimiter class with getNumOfCallsLeftAndRecordCall (resultHolder should hold number instead of boolean) — the value is generated in the runTransactionForAnswer method. Of course tests should be updated and test if the number returned by the getNumOfCallsLeftAndRecordCall is correct. Then the isQuotaExceededOrRecordUsage in the FirebaseFunctionsRateLimiter class should do the check if the number provided by getNumOfCallsLeftAndRecordCall is equal zero, and a separate method that is a facade for getNumOfCallsLeftAndRecordCall should be added.

If you have time you could create a PR, I would guide you and help you with any problems. If not, I will maybe do it in a few days.

Jblew commented 4 years ago

And — please open a separate issue for this :)