Add API key support for JSON-RPC requests #763
Conversation
This change allows `JsonRpcBase` to include an API key in headers when connecting to XRPL nodes that require authentication. The `__init__` method now accepts an `api_key` argument, which is stored as an attribute and used in `_request_impl`. - Updated the `_request_impl` method to add the `Authorization` header with the API key when provided. - Added an `api_key` parameter to the `__init__` method for easy instantiation with an API key. - This change was tested by connecting to a private XRPL server with required API authentication.
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
xrpl/asyncio/clients/json_rpc_base.py (2)
25-34: Enhance API key parameter documentation.
The implementation looks good, but consider expanding the API key documentation to include:
- Expected format (Bearer token)
- Usage implications
- Whether it's required for private servers
Consider updating the docstring:
"""
Initializes a new JsonRpcBase client.
Arguments:
url: The URL of the XRPL node to connect to.
- api_key: Optional API key for connecting to a private XRPL server.
+ api_key: Optional API key for authenticating with private XRPL servers.
+ When provided, it will be sent as a Bearer token in the Authorization
+ header. Required for private servers and recommended for public nodes
+ to avoid rate limiting.
"""Line range hint 68-76: Consider enhancing error handling for authentication failures.
The current error handling doesn't distinguish between authentication failures and other HTTP errors. Consider:
- Adding specific handling for 401/403 status codes
- Creating a dedicated exception type for authentication failures
Example enhancement:
if response.status_code in (401, 403):
raise XRPLAuthenticationException(
f"Authentication failed: {response.text}"
)📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- xrpl/asyncio/clients/json_rpc_base.py (3 hunks)
🔇 Additional comments (3)
xrpl/asyncio/clients/json_rpc_base.py (3)
6-6: LGTM: Import added for Optional type.
The addition of Optional import is appropriate for the new api_key parameter type hint.
65-67: LGTM: Headers properly included in request.
The headers dictionary is correctly passed to the post request, maintaining the existing functionality while adding API key support.
55-61: Verify Authorization header format with XRPL documentation.
The implementation looks solid, but there are two considerations:
- The comment "Adjust key name if necessary" suggests uncertainty about the header format
- There's no validation of the API key format
Consider:
- Validating the API key format in the constructor
- Making the header name configurable if different XRPL nodes use different header formats
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand:
- How Authorization headers are used across the codebase
- How API keys are handled
- If there's any documentation about authentication requirements
The search revealed this is the first implementation of API key support in the codebase. Let's check the documentation and tests to understand if there's any guidance on the expected authentication format.
Bearer token format is appropriate for XRPL API authentication
The implementation is correct as is. Here's why:
- This is the first implementation of API key support in the codebase, introducing a standard way to authenticate with XRPL nodes
- The Bearer token format is the industry standard for API authentication
- The comment "Adjust key name if necessary" can be removed as the Authorization header with Bearer scheme is the correct approach
The uncertainty expressed in the review comment is unwarranted. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing Authorization header usage in the codebase
# to ensure consistency
rg -l 'Authorization.*Bearer'
Length of output: 31
Script:
#!/bin/bash
# Let's search more broadly for any Authorization header usage
rg -i "authorization.*header" -A 3
# Also search for any API key related code
rg -i "api.?key" -A 3
# Check if there's any documentation about authentication
fd -g "*.md" -x grep -l -i "auth\|api.?key" {}
Length of output: 2026
Script:
#!/bin/bash
# Let's check the README.md content for any authentication documentation
cat README.md
# Let's also check if there are any tests that might show the expected usage
rg -i "test.*api.?key" -A 5
Length of output: 16803
|
To make this a bit more general/future-proof, I would write this to allow any headers in the init (and also in the request method, in case you want to include some headers just for some requests). |
|
@joshuahamsa Is the |
Headers aren't included in |
I understand that, but The API keys appear to be required at the POST request construction. Alternatively, it would be appropriate to make these changes at the top-most |
That wouldn't be very developer-friendly. Why should headers be limited to just the
Agreed, but adding it to just JSONRPC clients would be a good start. |
|
Hmm, I assumed that this was a use case for admin-methods. rippled operators could provide a wrapper with API-keys for their users to access admin methods. But I agree with you, other |
This change allows
JsonRpcBaseto include an API key in headers when connecting to XRPL nodes that require authentication. The__init__method now accepts anapi_keyargument, which is stored as an attribute and used in_request_impl._request_implmethod to add theAuthorizationheader with the API key when provided.api_keyparameter to the__init__method for easy instantiation with an API key.High Level Overview of Change
This PR introduces support for API key authentication in the
JsonRpcBaseclass. An__init__method has been added to include an optionalapi_keyparameter, which, if provided, is used to add anAuthorizationheader with each JSON-RPC request. This enables users to connect to private XRPL nodes that require API key-based authentication while maintaining compatibility with public nodes.Context of Change
This change is driven by the growing use of API-key-protected XRPL nodes, often necessary to avoid rate-limiting on public nodes. Previously,
JsonRpcBasecould not include an API key in request headers, limiting its ability to connect to private nodes that require authentication. By adding this feature,xrpl-pycan now be used more flexibly in production environments and with private infrastructure.In considering alternatives, modifying
json_rpc_base.pywas selected as the most efficient approach, as this base class is shared across JSON-RPC implementations. Adding theapi_keyattribute at this level ensures that all inheriting classes have access to the feature, allowing for consistent usage without impacting users who do not require an API key.Type of Change
Did you update CHANGELOG.md?
Test Plan
The change could be tested by:
api_keyparameter, confirming that the change does not introduce any issues when using unauthenticated nodes.api_keyparameter to ensure existing users can useJsonRpcBasewithout changes to their code.Future Tasks
Future tasks related to this PR could include:
Summary by CodeRabbit
api_keyparameter for theJsonRpcBaseclass, allowing users to authenticate requests to private XRPL servers.These updates improve user experience by enabling secure connections to private XRPL nodes.