ENH: locking down read access on API key#1158
ENH: locking down read access on API key#1158tech3371 merged 6 commits intoIMAP-Science-Operations-Center:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new read API key scope intended to allow read access while preventing uploads and other write-like operations, addressing increased API traffic by tightening permissions around sensitive endpoints.
Changes:
- Refactors the API key authorizer to centralize scope/path/method authorization logic and adds explicit restrictions for
readscope. - Adds
readscope validation/printing to the key management script and documents scope options in the README. - Enforces upload denial for
readscope in the upload lambda and updates DynamoDB-based authorization tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/infrastructure/test_api_key_dynamodb.py | Updates scope restriction tests to validate the new read behavior (including write/upload denial). |
| sds_data_manager/lambda_code/authorization/manage_api_keys.py | Adds scope documentation + validation for add_key / update_permission and prints scope after changes. |
| sds_data_manager/lambda_code/authorization/lambda_api_key_authorizer.py | Refactors authorization into _is_authorized and adds method/path-based restrictions for read scope. |
| sds_data_manager/lambda_code/SDSCode/api_lambdas/upload_api.py | Denies upload signed-URL generation when authorizer scope is read. |
| sds_data_manager/lambda_code/IAlirtCode/ialirt_data_query_api.py | Treats read as “full read” for HIT field filtering via READ_ONLY_SCOPES. |
| README.md | Documents scope options and updates CLI usage examples to include scope. |
Comments suppressed due to low confidence (1)
sds_data_manager/lambda_code/authorization/manage_api_keys.py:203
- In
update_permission, the variable namekeyis reused to hold the metadata dict (keys[matches[0]]), which is easy to confuse with the API key string itself. Consider renaming it to something likekey_meta/metadatato make the code clearer.
if matches:
key = keys[matches[0]]
table.put_item(
Item={
"api_key": matches[0],
"owner": key["owner"],
"email": key["email"],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Restrict write operations for read-only scope | ||
| if scope == "read" and http_method in ("PUT", "POST", "DELETE", "PATCH"): | ||
| return False | ||
|
|
||
| scope = metadata.get("scope", "") | ||
| path = event.get("rawPath") or event.get("path", "") | ||
| # Restrict write operations (upload) for read-only scope | ||
| if scope == "read" and path.startswith("/api-key/upload"): | ||
| return False |
There was a problem hiding this comment.
The new read-only restrictions only apply when scope == "read". This PR also renames the test scope from read_only to read, which suggests existing DynamoDB records may still have scope="read_only"; those keys would not be restricted from write methods or /api-key/upload by the new logic. Consider treating legacy read_only as equivalent to read (or migrating existing records) so the lock-down actually applies to previously-issued read-only keys.
There was a problem hiding this comment.
we don't have any with read_only. Just read.
sds_data_manager/lambda_code/authorization/lambda_api_key_authorizer.py
Outdated
Show resolved
Hide resolved
274d248 to
57df8cf
Compare
sds_data_manager/lambda_code/authorization/lambda_api_key_authorizer.py
Outdated
Show resolved
Hide resolved
sds_data_manager/lambda_code/authorization/lambda_api_key_authorizer.py
Outdated
Show resolved
Hide resolved
|
I tested it again in dev and read access lock down works and updates and removal of access still works as well. |
Change Summary
closes #1163
Overview
Need to lock down API read access given the amount of API requests coming our way.
File changes
Minor refactoring and added checks for read access for upload API. Others are ok.
Testing