rpcperms: allow multiple read-only middleware#10611
rpcperms: allow multiple read-only middleware#10611ziggie1984 merged 3 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello @calvinrzachman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the RPC middleware system by allowing multiple read-only middlewares to be registered concurrently. Previously, the system prevented the registration of more than one middleware if they shared the same (empty) custom caveat name, which was a constraint for read-only middlewares. This change addresses a user-reported issue, providing increased flexibility for users who need to apply multiple non-modifying interceptors to RPC calls without conflict. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements the ability to register multiple read-only RPC middlewares, which was previously not possible due to a check that prevented multiple registrations with the same (empty) caveat name. The logic change in rpcperms/interceptor.go is concise and correct. A new integration test, multipleReadOnlyMiddlewareTest, has been added to verify the new functionality, ensuring that multiple read-only middlewares can be registered and will all receive interception messages. The changes are well-implemented and tested. I have one minor suggestion to improve code clarity by removing an unused parameter in the new test function.
|
@calvinrzachman, remember to re-request review from reviewers when ready |
calvinrzachman
force-pushed
the
multi-readonly-middleware
branch
from
425acda to
392f3fa
Compare
|
In if !middleware.readOnly && resp.replace {
currentMessage = resp.replacement
}We should log a warning here so middleware developers are not left wondering why their replacement has no effect. Something like: if resp.replace {
if middleware.readOnly {
log.Warnf("Read-only middleware %s attempted to replace message, ignoring", middleware.middlewareName)
} else {
currentMessage = resp.replacement
}
} |
|
In for _, middleware := range r.registeredMiddleware {
if middleware.customCaveatName == mw.customCaveatName {
if middleware.readOnly && mw.readOnly {
continue
}
return fmt.Errorf(...)
}
}Since read-only middlewares always have an empty caveat name, we enter the caveat name check only to immediately skip out of it. It would be clearer to check for _, middleware := range r.registeredMiddleware {
if middleware.readOnly && mw.readOnly {
continue
}
if middleware.customCaveatName == mw.customCaveatName {
return fmt.Errorf(...)
}
}This better reflects the intent: for read-only pairs we don't care about caveat names at all. |
| interceptors](https://github.com/lightningnetwork/lnd/pull/10611) to register | ||
| for the same custom caveat name simultaneously. Previously only one middleware | ||
| could register per custom caveat name; this relaxes that restriction for | ||
| read-only middlewares while still ensuring only a single read-write middleware |
There was a problem hiding this comment.
Nit: the release notes are a bit confusing since read-only middlewares cannot specify a caveat name at all (enforced at registration). It would be simpler to just say:
Allow multiple read-only RPC middleware interceptors to be registered simultaneously.
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Nice addition, I had some suggestions
There is validation which requires "read-only" middle ware not specify a caveat name. But when you try to register a second read-only middleware, there's validation which prevents double registration for same caveat (though in this case the caveat name is the empty string). Update the dameon to permit the registration of multiple read-only rpc middleware.
calvinrzachman
force-pushed
the
multi-readonly-middleware
branch
from
392f3fa to
58f5997
Compare
|
@ziggie1984 thanks for the suggestions. I think they all help improve clarity. They should all be addressed. Let me know what you think. |
5 participants
Change Description
There is validation which requires "read-only" middle ware not specify a caveat name. But when you try to register a second read-only middleware, there's validation which prevents double registration for same caveat (though in this case the caveat name is the empty string).
Update the daemon to permit the registration of multiple read-only rpc middleware.
NOTE: This was motivated by request from a user in Slack asking whether lnd can support multiple read-only RPC middleware. The user is getting this error:
Steps to Test
make itest icase=rpc_middleware_interceptorPotential Future Additions