Closed albertisfu closed 3 weeks ago
An alternative is to use a random function to check about 1 in 50 requests,
Since this will have some performance impact, a random number does seem better. Otherwise, every X seconds all API requests will be impacted.
The rest sounds right to me. Thank you Alberto!
Since this will have some performance impact, a random number does seem better. Otherwise, every X seconds all API requests will be impacted.
Yeah, that's the downside of the time approach.
I've changed it to the random approach. So ~1 in 50
are checked.
def check_request() -> bool:
# Check 1 in 50 requests.
if random.randint(1, 50) == 1:
return True
return False
@mlissner Just a heads up: Before we merge this PR, let's run the following script:
from cl.lib.redis_utils import get_redis_interface
r = get_redis_interface("STATS")
v3_stats_members = r.zrange("api:v3.user.counts", 0, -1)
r.sadd("v3-users-list", *v3_stats_members)
Once that's done, we can safely flip the BLOCK_NEW_V3_USERS
flag to true.
Wonderful. Thank you both. I ran the code and set the variable to True before merging, so I think we'll be good to go once deployed.
💀 API V3 💀
As outlined in #4489, this PR introduces a permission class to block anonymous users and new users from accessing V3 of the API.
Additionally, this PR introduces stats logging in Redis for V4, so once merged, the logs will be independent from
V3
, usingv4
as a prefix. The stats logged will be exactly the same as forV3
.I also noticed that the Stat API Events, which log total API request milestones and API user milestones, are tied to API stats in Redis. As a result, when we start logging
V4
stats in Redis, we will have duplicate milestones sinceV4
is starting from scratch.To differentiate events between V3 and V4, I tweaked the description so future milestones for V3 are logged as:
API v3 has logged 10,000 total requests.
User 'username' has placed their 1st API v3 request.
For V4:
API v4 has logged 1 total request.
User 'username' has placed their 1st API v4 request.
Does this seem good to you?
One issue I encountered was how to detect if a user is a current V3 user. Initially, I planned to use
v3.user.counts
in Redis to check if the user existed there, but since we'll only check some requests, when a new user request is allowed, they will become a V3 user and will never be blocked in the future. The solution is to create an initialv3-users-list
, which must be set up before enabling the V3 block feature.The permission process works according to the following checks, in order of priority:
BLOCK_NEW_V3_USERS
is disabled, the request is allowed.AnonymousUser
, it is blocked, returning a 403 status code with this message:Anonymous users don't have permission to access V3 of the API. Please use V4 instead.
v3-blocked-users-list
, a list where "new" users are added so that all their future requests are blocked.v3-users-list
. If the user is not found there, the request is blocked, and the user is added to thev3-blocked-users-list
. The response is a 403 status code with this message:As a new user, you don't have permission to access V3 of the API. Please use V4 instead.
So I implemented this method:
It checks requests every 10 seconds (we can adjust this value as needed), meaning all requests happening within one second every 10 seconds will be checked. Does this sound good to you? An alternative is to use a random function to check about 1 in 50 requests, but that wouldn't be fully stateless.
BLOCK_NEW_V3_USERS
is a setting that defaults toFalse
, so the V3 block feature is disabled during testing (to avoid conflicts) and for developers by default. This setting should be set toTrue
in production after creating thev3-users-list
. To create it, we can use the following command:This command will copy all the current users in
api:v3.user.counts
to thev3-users-list
.Finally, I added the new
V3APIPermission
class to all API endpoints, except for:MembershipWebhookViewSet
: This is not available to users and is only used for Neon, so it's probably not worth performing theV3APIPermission
checks here.I also didn't add the
V3APIPermission
to the following RECAP endpoints, which currently use the following permission classes:PacerProcessingQueueViewSet
:RECAPUploaders
EmailProcessingQueueViewSet
:EmailProcessingQueueAPIUsers
PacerDocIdLookupViewSet
:RECAPUsersReadOnly
My thinking is that these endpoints are mostly for internal use with the RECAPExtension and RECAPEmail, so it's probably not worth performing the
V3APIPermission
checks here.For all the other API endpoints that already had a permission class, the new
V3APIPermission
was added at the end of the list so that it prioritizes other permissions as before.Let me know what you think.