nikolaposa / rate-limit

🚔 General purpose rate limiter implementation.
MIT License
271 stars 47 forks source link

Use IP or user ID as identity, also check LBs to provided array #14

Closed poursal closed 4 years ago

nikolaposa commented 6 years ago

Doesn't seem very generic on the first glance and as something that should be part of the library itself. Also, without unit tests, I won't merge it.

poursal commented 6 years ago

Hello Nikola,

The provided IpAddressIdentityResolver has a security flow. You should trust and use the HTTP_CLIENT_IP and HTTP_X_FORWARDED_FOR headers only if the REMOTE_ADDR contains an IP that you have predefined as a trusted load balancer or proxy.

Also using the IP as key without a rate-limiter prefix could cause conflicts in a shared redis database.

Finally, I added the authenticated user attribute to have different identities based on the status of the user. So use the IP address on non-autheticated or the user ID on authenticated users for limiting the API.

Review the functionality and if you are willing to include this file I would be happy to put down some unit tests.

nikolaposa commented 6 years ago

Ideal solution to this problem would be to:

  1. fix flaws and issues you've pointed out in the existing IpAddressIdentityResolver itself (prefixes, trusted proxies)
  2. add new AuthIdentityResolver (need to come up with a better name) - your implementation that inspects authKeyName in request attributes
  3. add new IdentityResolverAggregate that accepts multiple resolvers through its constructor: public function __construct(IdentityResolverInterface ...$identityResolvers) and returns first non-empty string identity. With the help of this resolver, one can put resolving process this way:
    new IdentityResolverAggregate(
    new AuthIdentityResolver(), //will resolve user ID if available
    new IpAddressIdentityResolver() //fallback to IP address
    );

@poursal What do you think about this idea? Do you think you will be able to pull this off? If you think third step is to complicated I'll handle it. Idea is that, instead of having one, bloated Identity Resolver implementation, their functionality should be split across multiple classes so that they all adhere to a single responsibility principle

poursal commented 6 years ago

@nikolaposa Very clean solution. No problem, I will make the commits.

nikolaposa commented 4 years ago

Closing PR due to staleness. I did a complete rewrite for v2.0, and some classes these changes depend on do not exist anymore.

Consider creating a new PR for the current codebase.