-
Couldn't load subscription status.
- Fork 138
Improve MacaroonWhitelist Structure and Permission Granularity
#1841
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
Conversation
Refactor the function by introducing an internal helper to improve readability.
Refactor MacaroonWhitelist to handle public universe proof courier permissions independently. This clarifies the logic and avoids coupling courier access with other universe server permissions.
Remove the defaultMacaroonWhitelist map and inline its entries directly into the conditional logic within MacaroonWhitelist. This ensures that access to previously always-available endpoints is now governed by explicit user configuration (read/write/courier), improving permission control and aligning with expected access restrictions.
Summary of ChangesHello @ffranr, 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 primarily focuses on improving the security and clarity of the Macaroon permissioning model, enhancing the robustness of database migrations, and extending the functionality of Universe RPC endpoints to provide more detailed blockchain information. It refactors the Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great improvement to the macaroon permissioning system. The refactoring makes the code much clearer and enhances security by removing the default whitelist, ensuring all endpoint access is explicitly configured. The separation of concerns for different permission flags is well-executed. I have one suggestion to further improve maintainability.
| addEndpoints( | ||
| "/universerpc.Universe/Info", | ||
|
|
||
| "/universerpc.Universe/AssetRoots", | ||
| "/universerpc.Universe/QueryAssetRoots", | ||
| "/universerpc.Universe/AssetLeafKeys", | ||
| "/universerpc.Universe/AssetLeaves", | ||
| "/universerpc.Universe/QueryProof", | ||
|
|
||
| "/universerpc.Universe/FetchSupplyCommit", | ||
| "/universerpc.Universe/FetchSupplyLeaves", | ||
|
|
||
| "/authmailboxrpc.Mailbox/MailboxInfo", | ||
| "/authmailboxrpc.Mailbox/ReceiveMessages", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve maintainability and prevent typos, it's a good practice to define these RPC endpoint paths as constants instead of using string literals directly. This is especially useful as these paths are used in multiple places in this function and potentially elsewhere in the codebase.
You could define them in a const block at the package level, for example:
const (
// Universe RPC methods.
UniverseInfoPath = "/universerpc.Universe/Info"
UniverseAssetRootsPath = "/universerpc.Universe/AssetRoots"
UniverseQueryAssetRootsPath = "/universerpc.Universe/QueryAssetRoots"
UniverseAssetLeafKeysPath = "/universerpc.Universe/AssetLeafKeys"
UniverseAssetLeavesPath = "/universerpc.Universe/AssetLeaves"
UniverseQueryProofPath = "/universerpc.Universe/QueryProof"
UniverseFetchSupplyCommitPath = "/universerpc.Universe/FetchSupplyCommit"
UniverseFetchSupplyLeavesPath = "/universerpc.Universe/FetchSupplyLeaves"
// Mailbox RPC methods.
MailboxInfoPath = "/authmailboxrpc.Mailbox/MailboxInfo"
MailboxReceiveMessagesPath = "/authmailboxrpc.Mailbox/ReceiveMessages"
// ... and so on for other paths.
)Then you can use these constants throughout the function, which makes the code more robust and easier to read.
| addEndpoints( | |
| "/universerpc.Universe/Info", | |
| "/universerpc.Universe/AssetRoots", | |
| "/universerpc.Universe/QueryAssetRoots", | |
| "/universerpc.Universe/AssetLeafKeys", | |
| "/universerpc.Universe/AssetLeaves", | |
| "/universerpc.Universe/QueryProof", | |
| "/universerpc.Universe/FetchSupplyCommit", | |
| "/universerpc.Universe/FetchSupplyLeaves", | |
| "/authmailboxrpc.Mailbox/MailboxInfo", | |
| "/authmailboxrpc.Mailbox/ReceiveMessages", | |
| ) | |
| addEndpoints( | |
| UniverseInfoPath, | |
| UniverseAssetRootsPath, | |
| UniverseQueryAssetRootsPath, | |
| UniverseAssetLeafKeysPath, | |
| UniverseAssetLeavesPath, | |
| UniverseQueryProofPath, | |
| UniverseFetchSupplyCommitPath, | |
| UniverseFetchSupplyLeavesPath, | |
| MailboxInfoPath, | |
| MailboxReceiveMessagesPath, | |
| ) |
Pull Request Test Coverage Report for Build 18343162813Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, LGTM. 👍 👍
…whitelist Improve `MacaroonWhitelist` Structure and Permission Granularity
This PR refactors the
MacaroonWhitelistfunction to improve code clarity and enhance permission control:defaultMacaroonWhitelist, ensuring all endpoint access is explicitly controlled by the user's configuration (read,write,courier, etc.).These changes make the permissioning model more secure by preventing certain endpoints from being always accessible by default.