mbg / wai-rate-limit

Rate limiting for Servant and as WAI middleware
MIT License
12 stars 3 forks source link

Ergonomics (accessing environment in rate limit instances) #6

Closed hasufell closed 2 years ago

hasufell commented 2 years ago

I'm trying to access application environment specifically in the HasRateLimitStrategy instance (but also possibly in the HasRateLimitPolicy) one.

To access application environment there are two ways in servant afais:

  1. your custom monad
  2. the Context which is specifically for the combinators

I tried to shoehorn both into the instances, but I think it's impossible. The 3rd possibility (which is what sadly the README even suggests) is using unsafePerformIO.

In the end I went with overlapping the HasServer (RateLimit foo bar :> api) ctx instance, copy pasting and adjusting the internal code, then adding my own HasContextEntry constraint on top. This works ok-ish, but overall... this is rather unergonomic and hard to figure out.

If you have better suggestions, please share. Otherwise I suggest to add a more complicated example to the readme that does this.

mbg commented 2 years ago

Hi @hasufell, thanks for the feedback about this! I can see how being able to access the Servant context would be useful and it is a pretty easy change.

I have pushed 81a3fd53e763b02f3465c9aaae2cbb1230c12b93 and 8dd878b6da62fa51a591644eafd43bb16fb9c48f which implement this for HasRateLimitPolicy and HasRateLimitStrategy respectively.

I have also extended the README and the tests with an example that consumes a value from the context in a HasRateLimitPolicy policy.

Let me know if these changes work for you.

hasufell commented 2 years ago

@mbg I tried something similar I think and I got stuck with that because now in order to make useful use of the Context, I need to specialize the key in HasRateLimitStrategy. But it doesn't have knowledge of the key type.

hasufell commented 2 years ago

Maybe I should expand a little more:

  1. I use JWT auth
  2. Rate limiting information is embedded in the json token
  3. strategyValue is supposed to pattern match on the verified token and then decide about rate limiting
mbg commented 2 years ago

The key type is essentially dictated by the Backend in use. Assuming that you are using wai-rate-limit-redis for example, the type of the backend returned by redisBackend is Backend ByteString. This corresponds to the type of key used by the backend to associate with the usage. So in that case you would always want type RateLimitPolicyKey ctx YourPolicy = ByteString and then convert your more concrete type (e.g. ApiKey in the case of my example) into a ByteString that the backend can then store.

In your case, it seems that you wouldn't really need a Backend at all, since the rate limiting information is stored in the JWT? In that case, I suspect that a workaround here might be to just build a dummy Backend that is parameterised over whatever more concrete type you want to use.

Instances of the HasRateLimitStrategy class aren't really intended to do anything key-specific, other than pass the Request -> IO key function from the Backend on to the underlying rate-limiting strategy.

hasufell commented 2 years ago

Yes, my key type is ByteString and I'm using redis. Redis will store this key normally.

What I mean is that my Strategy building function needs knowledge of the key. strategyValue calls that function, but that doesn't typecheck, because one doesn't know about the key, while the other does.

hasufell commented 2 years ago

In your case, it seems that you wouldn't really need a Backend at all, since the rate limiting information is stored in the JWT?

I do need a backend. There's just additional logic that will return False in strategyOnRequest on certain tokens.

The only workaround I see is moving that logic into the Backend itself and abusing BackendError for "rate limited"... but that feels wrong (and requires me to overwrite the existing redis backend).

strategyOnRequest is really the function that decides what is rate limited and what isn't. So it should be able to inspect the key as well.

mbg commented 2 years ago

It's a bit tricky for me to visualise what exactly your code looks like at the moment, sorry. My current understanding is that you have something along the lines of this:

instance HasContextEntry ctx JWTData => HasRateLimitStrategy ctx CustomStrategy where
    strategyValue ctx backend getKey = case getContextEntry ctx of
        JWTDataCtr0 k0 -> ...
        JWTDataCtr1 k1 -> ...

Where k0 :: key0 and k1 :: key1 for some key0 and key1? And then you want to use some Strategy-building function in each of the branches involving different types of key, but also relating to the key type variable, which then doesn't work because key is universally quantified while key0 and key1 are (potentially) other / more concrete types?

Would parameterising HasRateLimitStrategy over key help you?

I would definitely try to avoid moving things into a Backend, especially since that type will change soon due #7.

hasufell commented 2 years ago

Yes, this does work:

class HasRateLimitStrategy (ctx :: [Type]) strategy key where
    strategyValue ::
        Context ctx -> Backend key -> (Request -> IO key) -> Strategy
hasufell commented 2 years ago

Thanks, I guess this needs a major PVP version bump, because we broken the class API.

mbg commented 2 years ago

Yes indeed. I am hoping to include the changes to Backend in the next release, which can then combine a bunch of breaking changes.