feat(x/authz): Disable authz grant message grant #20687
Conversation
This reverts commit 214605b.
WalkthroughThe recent changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Interface
participant AuthzKeeper
User->>Interface: Create MsgGrant
Interface->>AuthzKeeper: Validate MsgGrant
AuthzKeeper-->>Interface: "authz msgGrant is not allowed"
Interface-->>User: Error: authz msgGrant is not allowed
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This comment has been minimized.
This comment has been minimized.
sontrinh16
changed the title
![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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- x/authz/keeper/keeper_test.go (1 hunks)
- x/authz/keeper/msg_server.go (1 hunks)
- x/authz/keeper/msg_server_test.go (1 hunks)
Additional context used
Path-based instructions (3)
x/authz/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/keeper/msg_server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (3)
x/authz/keeper/msg_server.go (1)
46-47: The implementation correctly blocks the authorization ofauthz.MsgGrantmessages as intended by the PR. This change aligns with the PR's goal to prevent accidental full account access grants.x/authz/keeper/msg_server_test.go (1)
203-214: The added test case "invalid grant with msg grant" correctly validates the new behavior inmsg_server.goby expecting an error when trying to grant anauthz.MsgGrant. This ensures that the new logic is effectively preventing the specific unwanted authorization scenario.x/authz/keeper/keeper_test.go (1)
83-83: The registration of the message server withauthzKeeperin the test setup is crucial for integration tests to simulate the actual environment accurately. This setup is essential for testing the new changes thoroughly.
| @@ -43,6 +43,9 @@ func (k Keeper) Grant(ctx context.Context, msg *authz.MsgGrant) (*authz.MsgGrant | |||
| if err := k.MsgRouterService.CanInvoke(ctx, t); err != nil { | |||
| return nil, sdkerrors.ErrInvalidType.Wrapf("%s doesn't exist", t) | |||
| } | |||
| if t == sdk.MsgTypeURL(&authz.MsgGrant{}) { | |||
There was a problem hiding this comment.
Maybe let's add a comment on why it is done here (msg server) and not in the keeper?
My guess is you did it like this so that modules can still create a grant of grants if needed, and this only break the user flow and not module code. If so I would precise that so it doesn't get refactored away in the futurre.
…smos/cosmos-sdk into disable_grant_authz_grant_msg
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/authz/CHANGELOG.md (1 hunks)
- x/authz/keeper/msg_server.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/authz/keeper/msg_server.go
Additional context used
Path-based instructions (1)
x/authz/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/authz/CHANGELOG.md
[uncategorized] ~32-~32: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ... MsgGrant to other accounts. Preventing user from accidentally authorizing their ent...
Markdownlint
x/authz/CHANGELOG.md
38-38: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
39-39: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
40-40: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
| @@ -29,6 +29,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
|
|||
| * [#18737](https://github.com/cosmos/cosmos-sdk/pull/18737) Added a limit of 200 grants pruned per `BeginBlock` and the `PruneExpiredGrants` message that prunes 75 expired grants on every run. | |||
| * [#20161](https://github.com/cosmos/cosmos-sdk/pull/20161) Added `RevokeAll` method to revoke all grants at once. | |||
| * [#20687](https://github.com/cosmos/cosmos-sdk/pull/20687) Prevent user to grant authz MsgGrant to other accounts. Preventing user from accidentally authorizing their entire account to a different account. | |||
There was a problem hiding this comment.
Consider revising the entry for clarity and grammatical correctness.
- * [#20687](https://github.com/cosmos/cosmos-sdk/pull/20687) Prevent user to grant authz MsgGrant to other accounts. Preventing user from accidentally authorizing their entire account to a different account.
+ * [#20687](https://github.com/cosmos/cosmos-sdk/pull/20687) Prevent users from granting authz MsgGrant to other accounts to avoid accidentally authorizing their entire account to a different user.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [#20687](https://github.com/cosmos/cosmos-sdk/pull/20687) Prevent user to grant authz MsgGrant to other accounts. Preventing user from accidentally authorizing their entire account to a different account. | |
| * [#20687](https://github.com/cosmos/cosmos-sdk/pull/20687) Prevent users from granting authz MsgGrant to other accounts to avoid accidentally authorizing their entire account to a different user. |
Tools
LanguageTool
[uncategorized] ~32-~32: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ... MsgGrant to other accounts. Preventing user from accidentally authorizing their ent...
Fix indentation for list items to maintain consistency with the rest of the document.
- * `NewMsgExec`, `NewMsgGrant` and `NewMsgRevoke` now takes strings as arguments instead of `sdk.AccAddress`.
- * `ExportGenesis` also returns an error.
- * `IterateGrants` returns an error, its handler function also returns an error.
+ * `NewMsgExec`, `NewMsgGrant` and `NewMsgRevoke` now takes strings as arguments instead of `sdk.AccAddress`.
+ * `ExportGenesis` also returns an error.
+ * `IterateGrants` returns an error, its handler function also returns an error.Committable suggestion was skipped due to low confidence.
Tools
LanguageTool
[uncategorized] ~32-~32: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ... MsgGrant to other accounts. Preventing user from accidentally authorizing their ent...
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/authz/README.md (1 hunks)
Additional context used
Path-based instructions (1)
x/authz/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/authz/README.md
[style] ~41-~41: This phrase is redundant. Consider using “outside”. (OUTSIDE_OF)
Context: ...defined for any Msg service method even outside of the module where the Msg method is defi...
[uncategorized] ~51-~51: Possible missing article found. (AI_HYDRA_LEO_MISSING_THE)
Context: ... Cosmos SDKx/authzmodule comes with following authorization types: #### GenericAutho...
[uncategorized] ~71-~71: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case. (AMOUNTOF_TO_NUMBEROF)
Context: ...SpendLimitthat specifies the maximum amount of tokens the grantee can spend. The `S...
[uncategorized] ~87-~87: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case. (AMOUNTOF_TO_NUMBEROF)
Context: ...ens` that keeps track of a limit to the amount of tokens that can be delegated/undeleg...
[style] ~98-~98: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...ng/types/authz.go#L15-L35 ``` ### Gas In order to prevent DoS attacks, granting `StakeAut...
[uncategorized] ~99-~99: Do not mix variants of the same word (‘authorize’ and ‘authorise’) within a single text. (EN_WORD_COHERENCY)
Context: ...gas.StakeAuthorizationallows you to authorize another account to delegate, undelegate...
[grammar] ~99-~99: After the number ‘10’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...iterates over these lists and charge 10 gas for each validator in both of the lists...
[uncategorized] ~101-~101: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE)
Context: ...e of particularmsgType) from the list and we are charging 20 gas per iteration. ...
[grammar] ~101-~101: After the number ‘20’, use a plural noun. Did you mean “gases”, “gasses”? (CD_NNU)
Context: ...`) from the list and we are charging 20 gas per iteration. ## State ### Grant Gr...
[uncategorized] ~107-~107: A comma may be missing after the conjunctive/linking adverb ‘Hence’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ... and Authorization type (its type URL). Hence we only allow one grant for the (grante...
[typographical] ~139-~139: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...cated in the key. ## Messages In this section we describe the processing of messages ...
[uncategorized] ~146-~146: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ...return an error. This is for preventing user from accidentally authorizing their ent...
[uncategorized] ~155-~155: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...l be created if noexpirationtime is provided sinceexpirationis optional). * prov...
Markdownlint
x/authz/README.md
15-15: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
16-16: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
17-17: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
19-19: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
20-20: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
22-22: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
23-23: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
24-24: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
25-25: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
26-26: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
29-29: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
30-30: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
31-31: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
Additional comments not posted (1)
x/authz/README.md (1)
146-147: Clarify the new restriction on MsgGrant in the documentationThe documentation states that an authorization grant for
authz MsgGrantis not allowed, which is a critical security feature. It's crucial to ensure that this information is clear and unambiguous to prevent misuse.Tools
LanguageTool
[uncategorized] ~146-~146: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ...return an error. This is for preventing user from accidentally authorizing their ent...
Description
Closes: #19737
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
MsgGrantauthorization, ensuring users cannot accidentally authorize their entire account to another account.Documentation
README.mdto reflect the new restriction onMsgGrantauthorization.Tests
MsgGrantauthorization, ensuring it triggers the appropriate error.