Conversation
c19a8df to
2105678
Compare
758239d to
719ed0c
Compare
b5b3818 to
4bb05c5
Compare
for more information, see https://pre-commit.ci
warunawickramasingha
left a comment
There was a problem hiding this comment.
Proposed a few suggestions to improve the code, style and better handle exceptions + rate limiting rule at nginx level. Please check and apply as necessary.
| def verify_token(request) -> bool: | ||
| token = get_bearer_token(request) | ||
| secret = environ.get("QUERY_SECRET_KEY", "") | ||
| return token == secret |
There was a problem hiding this comment.
Not a huge concern in this context. But for secret validations, it is recommended to use something like compare_digest instead of == to avoid timing attacks.
| return token == secret | |
| from hmac import compare_digest | |
| return compare_digest(token, secret) |
| # Create your views here. | ||
| from django.views.decorators.cache import cache_page | ||
| from services.models import Message, Usage, FeatureUsage, Location | ||
| from rest_framework import response, viewsets |
There was a problem hiding this comment.
| from rest_framework import response, viewsets, status |
| proxy_connect_timeout 60s; | ||
| proxy_send_timeout 60s; | ||
| proxy_read_timeout 60s; |
There was a problem hiding this comment.
I guess these numbers are quite generous
| proxy_connect_timeout 60s; | |
| proxy_send_timeout 60s; | |
| proxy_read_timeout 60s; | |
| proxy_connect_timeout 30s; | |
| proxy_send_timeout 30s; | |
| proxy_read_timeout 30s; |
| @api_view(("POST",)) | ||
| def query(request, format=None): | ||
| if not verify_token(request): | ||
| return response.Response(status=401, data="UNAUTHORIZED") |
There was a problem hiding this comment.
For improved readability
| return response.Response(status=401, data="UNAUTHORIZED") | |
| return response.Response(status=status.HTTP_401_UNAUTHORIZED, data="UNAUTHORIZED") |
| if sql_err: | ||
| return response.Response(status=400, data=f"Invalid Parameters: {sql_err}") | ||
| conn = connections["readonly"] | ||
| with conn.cursor() as cur: | ||
| cur.execute(sql) | ||
| res = cur.fetchall() | ||
| return response.Response(res) |
There was a problem hiding this comment.
Exception handling for db operations
| if sql_err: | |
| return response.Response(status=400, data=f"Invalid Parameters: {sql_err}") | |
| conn = connections["readonly"] | |
| with conn.cursor() as cur: | |
| cur.execute(sql) | |
| res = cur.fetchall() | |
| return response.Response(res) | |
| if sql_err: | |
| return response.Response(status=status.HTTP_400_BAD_REQUEST, data=f"Invalid Parameters: {sql_err}") | |
| try: | |
| conn = connections["readonly"] | |
| with conn.cursor() as cur: | |
| cur.execute(sql) | |
| res = cur.fetchall() | |
| except Exception: | |
| return Response({"error": "Query failed"}, status=status.HTTP_400_BAD_REQUEST) | |
| return response.Response(res) |
| err = "" | ||
| if not val: | ||
| err = f"No {param} parameter provided" | ||
| return err, val |
There was a problem hiding this comment.
To work better with --data-urlencode "sql="
| return err, val | |
| def get_parameter(request, param): | |
| val = request.POST.get(param) | |
| if val is None or val.strip() == "": | |
| return f"No {param} parameter provided", None | |
| return None, val |
| @@ -1 +1 @@ | |||
| server { | |||
There was a problem hiding this comment.
Define a rate limiting zone with 5 requests per second to be used at /api/query location to avoid any spikes due to usage errors
| http { | |
| limit_req_zone $binary_remote_addr zone=query_limit:10m rate=5r/s; | |
| server { |
| # allow ISIS VPN traffic | ||
| allow 130.246.0.0/16; | ||
| deny all; | ||
|
|
There was a problem hiding this comment.
Apply the above defined query_limit rate limiting allowing short bursts with delays to be queued further
| # Rate limiting | |
| limit_req zone=query_limit burst=10; |
| proxy_send_timeout 60s; | ||
| proxy_read_timeout 60s; | ||
| } | ||
| } |
There was a problem hiding this comment.
add extra closing } for the new http block
| } | |
| } | |
| } |
This PR adds an endpoint that allows trusted users to query the report database.
To test (contact me for the token, it's on keeper):
Error on write:
Read action:
Closes #95