[ISSUE #7354]🚀add ACL configuration and validation modules, including file loading and permission handling#7355
Conversation
… file loading and permission handling
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
WalkthroughThe PR introduces ACL file loading, validation, and permission handling for RocketMQ. It adds a ChangesACL Configuration Loading and Authorization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rocketmq-auth/src/permission.rs`:
- Around line 38-46: The parse() function currently maps unknown permission
strings to DENY while migration_actions_and_decision() treats unknown strings as
(vec![Action::All], Decision::Allow); pick one behavior and make them
consistent: either (preferred) change migration_actions_and_decision() to return
the same restrictive default as parse() (Decision::Deny and no actions) for
unknown inputs, updating the method that references
migration_actions_and_decision() and the match arms that handle unknown cases,
or if the permissive migration behavior is intentional add clear doc comments to
both parse() and migration_actions_and_decision() explaining the deliberate
difference (e.g., backward-compatibility for migrations) and why it is safe;
reference the functions parse and migration_actions_and_decision when making the
change or adding docs.
- Line 30: The ANY constant (pub const ANY: Self = Self(1 << 1)) is unused
except in a negative assertion and should be removed or explicitly documented;
either delete the ANY constant from permission.rs if not needed, or add a clear
doc comment or #[deprecated] attribute above ANY explaining its reserved/future
purpose and update any tests referencing it (the negative assertion at line
referencing ANY) so they reflect the change; ensure parse() and
migration_actions_and_decision() remain unaffected and run tests after the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5c019e1-b13b-46a1-9045-aa8ba4499fd8
📒 Files selected for processing (7)
rocketmq-auth/src/acl.rsrocketmq-auth/src/acl/loader.rsrocketmq-auth/src/acl/validator.rsrocketmq-auth/src/authorization/model/resource.rsrocketmq-auth/src/lib.rsrocketmq-auth/src/permission.rsrocketmq-common/src/common/base/plain_access_config.rs
|
|
||
| impl Permission { | ||
| pub const DENY: Self = Self(1); | ||
| pub const ANY: Self = Self(1 << 1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of Permission::ANY across the codebase
rg -n --type=rust 'Permission::ANY' -C3Repository: mxsm/rocketmq-rust
Length of output: 495
🏁 Script executed:
#!/bin/bash
# Get context around line 98 to understand the test intent
cat -n rocketmq-auth/src/permission.rs | sed -n '85,110p'Repository: mxsm/rocketmq-rust
Length of output: 1305
🏁 Script executed:
#!/bin/bash
# Check the full Permission struct definition and all constants
cat -n rocketmq-auth/src/permission.rs | sed -n '20,40p'Repository: mxsm/rocketmq-rust
Length of output: 779
🏁 Script executed:
#!/bin/bash
# Search for any comments or doc strings about Permission constants
rg -n --type=rust -B5 'pub const ANY' rocketmq-auth/src/permission.rsRepository: mxsm/rocketmq-rust
Length of output: 279
Remove unused ANY constant or add documentation explaining its purpose.
The ANY constant is only referenced in a negative assertion at line 98 and is never used in parse() or migration_actions_and_decision(). If this is reserved for future use, add a #[deprecated] attribute or doc comment to clarify intent; otherwise, remove it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rocketmq-auth/src/permission.rs` at line 30, The ANY constant (pub const ANY:
Self = Self(1 << 1)) is unused except in a negative assertion and should be
removed or explicitly documented; either delete the ANY constant from
permission.rs if not needed, or add a clear doc comment or #[deprecated]
attribute above ANY explaining its reserved/future purpose and update any tests
referencing it (the negative assertion at line referencing ANY) so they reflect
the change; ensure parse() and migration_actions_and_decision() remain
unaffected and run tests after the change.
| pub fn parse(value: Option<&str>) -> Self { | ||
| match value.map(str::trim).unwrap_or_default() { | ||
| "PUB" => Self::PUB, | ||
| "SUB" => Self::SUB, | ||
| "PUB|SUB" | "SUB|PUB" => Self(Self::PUB.0 | Self::SUB.0), | ||
| "DENY" | "" => Self::DENY, | ||
| _ => Self::DENY, | ||
| } | ||
| } |
There was a problem hiding this comment.
Document or reconcile inconsistent handling of unknown permission strings.
parse() defaults unknown inputs to DENY (line 44), which is a security-safe fallback. However, migration_actions_and_decision() maps unknown inputs to (vec![Action::All], Decision::Allow) (lines 57, 64), which is permissive. This semantic inconsistency could lead to confusion or security gaps if different code paths interpret the same unknown permission string differently.
If this divergence is intentional (e.g., parse for strict validation vs. migration for backward compatibility), add doc comments explaining the rationale. Otherwise, consider aligning both methods to use the same default behavior for unknown inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rocketmq-auth/src/permission.rs` around lines 38 - 46, The parse() function
currently maps unknown permission strings to DENY while
migration_actions_and_decision() treats unknown strings as (vec![Action::All],
Decision::Allow); pick one behavior and make them consistent: either (preferred)
change migration_actions_and_decision() to return the same restrictive default
as parse() (Decision::Deny and no actions) for unknown inputs, updating the
method that references migration_actions_and_decision() and the match arms that
handle unknown cases, or if the permissive migration behavior is intentional add
clear doc comments to both parse() and migration_actions_and_decision()
explaining the deliberate difference (e.g., backward-compatibility for
migrations) and why it is safe; reference the functions parse and
migration_actions_and_decision when making the change or adding docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7355 +/- ##
==========================================
+ Coverage 63.06% 63.12% +0.05%
==========================================
Files 1121 1124 +3
Lines 212016 212346 +330
==========================================
+ Hits 133718 134045 +327
- Misses 78298 78301 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
3 participants
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements