Skip to content

[auth] Add custom header injection to JsonRpcBase (API key support) #828

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

joshuahamsa
Copy link

@joshuahamsa joshuahamsa commented Apr 10, 2025

Summary

This PR contributes unit test coverage for the recently merged custom header support in JsonRpcBase, originally implemented by @Transia-RnD in commit 6c8bab5.

Changes
• Added test_json_rpc_base.py with tests for:
• Global header usage
• Per-request header overrides
• No-header fallback
• 401/403 handling via XRPLAuthenticationException
• Defined XRPLAuthenticationException (if not already present upstream)
• Updated .gitignore to exclude .jrb/ local virtualenv

Context

This test suite helps confirm correctness and stability for consumers using authenticated or rate-limited XRPL infrastructure, and ensures that the new header logic behaves as expected under typical and error scenarios.

Let me know if you’d prefer the tests split out into a separate PR or further generalized to cover WebsocketBase.

joshuahamsa and others added 8 commits October 29, 2024 21:17
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.
•	Replaces api_key with headers: Optional[Dict[str, str]] 	•	Merges global headers from the instance with per-request headers in _request_impl 	•	Keeps the Content-Type: application/json header as the default base
Explicitly suppressed the traceback from the original `JSONDecodeError` by using `from None`, to keeps the error output clean and intentional.
Co-authored-by: Mayukha Vadari <[email protected]>
Co-authored-by: Mayukha Vadari <[email protected]>
…nctionality

This change elevates support for custom HTTP headers to the base `Client` class, allowing all client implementations—HTTP and WebSocket—to access a shared headers attribute for authentication or metadata purposes.

• Updated the `Client` constructor to accept an optional headers argument.
• The headers attribute is now accessible from all subclasses, enabling future support in WebSocket clients.
• Maintains full backward compatibility with existing clients.

⸻

## High Level Overview of Change
This PR introduces shared header support in the top-level `Client` class. The constructor now accepts an optional headers dictionary, which can be used to inject custom HTTP headers into XRPL client requests. This enables more flexible integration with infrastructure providers that require authentication tokens, API keys, or session-based headers.

The change ensures that any future client—including WebSocket-based ones—can leverage the same shared configuration without duplicating logic in subclasses like `JsonRpcBase`.

⸻

## Context of Change
PR [763](XRPLF#763) would add header support only in `JsonRpcBase`, making it unavailable to other clients such as WebSocket-based ones. With XRPL infrastructure increasingly relying on authenticated access, this limitation restricted the usefulness of `xrpl-py` in production environments.

By moving headers to the abstract base class, this change unifies the configuration mechanism and enables consistent authentication handling across all clients.

⸻

## Type of Change
[x] New feature (non-breaking change which adds functionality)

⸻

## Did you update CHANGELOG.md?
[x] Yes

⸻

## Test Plan
The change can be tested by:

• Instantiating JsonRpcBase with a headers dictionary and verifying that headers are properly merged and included in `JSON-RPC` requests.
• Creating a mock WebSocket client subclass that accesses `self.headers` and verifying shared availability.
• Ensuring backward compatibility by initializing clients without the headers argument and observing unchanged behavior.

⸻

## Future Tasks
Future tasks related to this change could include:

• Updating `WebSocketClient` to read from `self.headers` and send them during the handshake if supported.
• Standardizing authentication strategies across all clients (e.g., Bearer tokens, session headers).
• Adding test coverage for header usage across both sync and async clients.
Copy link
Contributor

coderabbitai bot commented Apr 10, 2025

Walkthrough

The Client and JsonRpcBase classes were removed, eliminating their abstract and base implementations for network clients and JSON-RPC requests. A new exception class XRPLAuthenticationException was added for authentication failures. Asynchronous unit tests were introduced to verify HTTP header handling and authentication error raising. The .gitignore file was updated with a new ignore pattern.

Changes

File(s) Change Summary
xrpl/asyncio/clients/client.py
xrpl/asyncio/clients/json_rpc_base.py
Entire files removed, deleting the Client abstract base class and JsonRpcBase class including their methods and request implementations.
xrpl/asyncio/clients/exceptions.py Added new exception class XRPLAuthenticationException inheriting from XRPLRequestFailureException for HTTP 401/403 authentication failures.
tests/unit/asyn/clients/test_json_rpc_base.py Added asynchronous unit tests for verifying global and per-request HTTP header handling and raising XRPLAuthenticationException on 401/403 responses.
.gitignore Added ignore pattern .jrb/ .. to exclude matching files/directories.

Sequence Diagram(s)

(No sequence diagram generated as the changes involve removal of client base classes and addition of tests and exception class without new control flow.)

Poem

I'm a rabbit, hopping with glee,
In code meadows, headers now run free,
With each new line, my eyes do gleam,
Merging values like a perfect dream,
Carrots and bytes—oh, what a spree!
Leaping into change, so joyfully!

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cfbc89e and 3e7c197.

📒 Files selected for processing (2)
  • xrpl/asyncio/clients/client.py (0 hunks)
  • xrpl/asyncio/clients/json_rpc_base.py (0 hunks)
💤 Files with no reviewable changes (2)
  • xrpl/asyncio/clients/json_rpc_base.py
  • xrpl/asyncio/clients/client.py
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 109c413 and a35dfd6.

📒 Files selected for processing (2)
  • xrpl/asyncio/clients/client.py (2 hunks)
  • xrpl/asyncio/clients/json_rpc_base.py (4 hunks)
🔇 Additional comments (7)
xrpl/asyncio/clients/client.py (4)

5-6: Appropriate import addition

The added import for Optional and Dict types is necessary for the new headers parameter type annotation.


27-32: Good use of optional parameter for backward compatibility

The addition of the optional headers parameter with a keyword-only marker (*,) and default value of None ensures backward compatibility with existing code.


38-42: Clear and helpful documentation

The documentation clearly explains the purpose of the headers parameter and provides a useful example of how it can be used for authentication.


44-44: Appropriate default handling for headers

Setting self.headers to headers or {} ensures it's always a dictionary, which is a good defensive programming practice.

xrpl/asyncio/clients/json_rpc_base.py (3)

45-46: Well-designed optional headers parameter

Adding an optional headers parameter to the request implementation allows for per-request customization, which is a flexible design choice.


65-70: Good header merging strategy

The implementation correctly:

  1. Ensures Content-Type is always set
  2. Prioritizes instance-level headers over the default
  3. Allows per-request headers to override instance-level headers

This provides maximum flexibility while maintaining required headers.


76-77: Appropriate use of headers in request

The merged headers are correctly applied to the HTTP request.

@coderabbitai coderabbitai bot mentioned this pull request Apr 10, 2025
Removing the need to assign `self.headers = headers or {}` again in `JsonRpcBase`, because it’s already handled in the parent class now.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@joshuahamsa
Copy link
Author

cc @mvadari @ckeshava @desimmons

Copy link
Collaborator

@ckeshava ckeshava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you prefer to supercede this PR with the current one?

Can you also add the tests for this change? The description of the tests (in the PR abstract) seem useful.

Comment on lines 25 to 29
def __init__(
self: Self,
url: str,
*,
headers: Optional[Dict[str, str]] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a similar update to the WebSocket class's constructor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would updating Client supersede this?

@joshuahamsa
Copy link
Author

Do you prefer to supercede this PR with the current one?

Yes I realize that this would be a better approach than the other.

Can you also add the tests for this change? The description of the tests (in the PR abstract) seem useful.

Will do!

Co-authored-by: Chenna Keshava B S <[email protected]>
@mvadari
Copy link
Collaborator

mvadari commented Apr 11, 2025

Please update the name of whatever PR you choose to go with and close the rest.

@dangell7
Copy link
Contributor

dangell7 commented May 6, 2025

Hey can you close out your other PRs? it was very confusing tracking all these changes.

Here is the implementation for websockets though.

Transia-RnD@6c8bab5

@joshuahamsa
Copy link
Author

Hey can you close out your other PRs? it was very confusing tracking all these changes.

Here is the implementation for websockets though.

Transia-RnD@6c8bab5

Previous PRs closed

@joshuahamsa joshuahamsa changed the title Client api key patch 1 [auth] Add custom header injection to JsonRpcBase (API key support) May 12, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
tests/unit/asyn/clients/test_json_rpc_base.py (1)

66-78: 💡 Verification agent

🧩 Analysis chain

Authentication error test is well-structured

This test verifies that the client raises XRPLAuthenticationException for 401 and 403 HTTP status codes. However, I don't see the implementation in the JsonRpcBase._request_impl method that handles these error codes from the provided snippets.

Please ensure that the JsonRpcBase._request_impl method has been updated to raise XRPLAuthenticationException for 401/403 responses. The relevant snippet provided doesn't show this error handling.


🏁 Script executed:

#!/bin/bash
# Check if JsonRpcBase._request_impl raises XRPLAuthenticationException for 401/403
rg -A 10 -B 5 "await http_client.post" --glob "xrpl/asyncio/clients/json_rpc_base.py"

Length of output: 1243


#!/bin/bash
rg -A30 -B15 "await http_client.post" --glob "xrpl/asyncio/clients/json_rpc_base.py"

#!/bin/bash
# Display the full implementation of _request_impl in JsonRpcBase
rg -A200 -B0 "async def _request_impl" --glob "xrpl/asyncio/clients/json_rpc_base.py"

🏁 Script executed:

#!/bin/bash
# Display the full implementation of _request_impl in JsonRpcBase
rg -A200 -B0 'async def _request_impl' --glob "xrpl/asyncio/clients/json_rpc_base.py"

# Search for XRPLAuthenticationException usage in that file
rg -n 'XRPLAuthenticationException' --glob "xrpl/asyncio/clients/json_rpc_base.py"

Length of output: 3468


Add authentication error handling in JsonRpcBase._request_impl

The _request_impl method in xrpl/asyncio/clients/json_rpc_base.py currently does not check for 401/403 responses and will fall through to JSON parsing (raising XRPLRequestFailureException on decode errors). To satisfy the new test_raises_on_401_403, insert an explicit check immediately after the HTTP POST:

• File: xrpl/asyncio/clients/json_rpc_base.py
• Location: inside _request_impl, right after

response = await http_client.post(
    self.url,
    json=request_to_json_rpc(request),
    headers=merged_headers,
)

• Diff:

 response = await http_client.post(
     self.url,
     json=request_to_json_rpc(request),
     headers=merged_headers,
 )

+    # Raise on authentication errors
+    if response.status_code in (401, 403):
+        raise XRPLAuthenticationException("Authentication failed")

 try:
     return json_to_response(response.json())

This will ensure XRPLAuthenticationException is thrown for 401/403 as expected by the unit test.

🧹 Nitpick comments (1)
tests/unit/asyn/clients/test_json_rpc_base.py (1)

30-48: Test verifies header override functionality, but is missing Content-Type verification

While this test correctly verifies that per-request headers override global headers, it doesn't check that the default Content-Type header is still present, unlike the other header tests.

Consider adding this assertion for consistency with other tests:

        headers_sent = mock_post.call_args.kwargs["headers"]
        assert headers_sent["Authorization"] == "Bearer override"
+       assert headers_sent["Content-Type"] == "application/json"
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d800d78 and cfbc89e.

📒 Files selected for processing (3)
  • .gitignore (1 hunks)
  • tests/unit/asyn/clients/test_json_rpc_base.py (1 hunks)
  • xrpl/asyncio/clients/exceptions.py (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • .gitignore
  • xrpl/asyncio/clients/exceptions.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/asyn/clients/test_json_rpc_base.py (3)
xrpl/asyncio/clients/exceptions.py (1)
  • XRPLAuthenticationException (41-44)
xrpl/asyncio/clients/json_rpc_base.py (1)
  • JsonRpcBase (18-84)
xrpl/models/requests/server_info.py (1)
  • ServerInfo (15-22)
🔇 Additional comments (4)
tests/unit/asyn/clients/test_json_rpc_base.py (4)

1-9: Imports look good and provide all necessary components for testing

The imports include all required modules for testing HTTP header functionality and authentication errors:

  • AsyncMock and patch for mocking async HTTP requests
  • pytest for test decorators and assertions
  • httpx Response for simulating HTTP responses
  • Required XRPL classes including the new XRPLAuthenticationException

11-28: Well-structured test for global headers

This test correctly validates that headers provided during client initialization are properly sent with requests. It confirms both the custom Authorization header and the default Content-Type header are included.


50-64: Good backward compatibility test

This test properly verifies that the client works correctly when no headers are provided, ensuring backward compatibility with existing code. It also validates that the default Content-Type header is still included even when no custom headers are specified.


1-78: Comprehensive test coverage for header injection

Overall, this test file provides good coverage for the new header injection feature. It tests global headers, per-request headers overriding global ones, backward compatibility, and authentication errors.

The tests align well with the PR objectives to add custom header support at the Client class level for authentication and metadata injection.

Copy link
Author

@joshuahamsa joshuahamsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final update to include unit tests for the custom header support introduced in @dangell7's commit.

This PR now contains:
• Test coverage for JsonRpcBase header usage and override logic (per @ckeshava)
• A dedicated check for XRPLAuthenticationException on 401/403 responses
• Minor cleanup only (no attempt to override the upstream implementation)

Please let me know if you’d prefer this test suite in a separate PR or adjusted to also cover WebsocketBase. Thanks again for integrating the core feature!

@joshuahamsa
Copy link
Author

Hey can you close out your other PRs? it was very confusing tracking all these changes.

Here is the implementation for websockets though.

Transia-RnD@6c8bab5

Love to see it! Thanks for making this happen!

@mvadari
Copy link
Collaborator

mvadari commented May 12, 2025

I think you may have messed up your commits - there's only deletions in the source code.

@dangell7
Copy link
Contributor

@joshuahamsa you've gotta cherry pick my commit or add the code yourself. My commit will not be merged. Sorry if that was confusing. What I meant was here is the code, just copy paste it into your PR. :)

git cherry-pick 6c8bab50d04ba1e6d61d81a7dd53009a78f726de

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants