fix: list only permitted for enrichment tables list #4480
Conversation
WalkthroughThe pull request introduces modifications to the handling of stream types within the Changes
Suggested labels
Tip New featuresWalkthrough comment now includes:
Notes:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Subhra264
force-pushed
the
enrich_list_rbac
branch
from
df92401 to
bceeb3b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
src/service/stream.rs (1)
Line range hint
1-300: Improve error handling by avoiding unwraps.The use of
unwrapin several places (e.g., database schema retrievals) could lead to panics in production. Consider using more robust error handling to ensure the application remains stable and provides meaningful error messages to the users.Replace
unwrapwith error handling, for example:let schema = infra::schema::get(org_id, stream_name, stream_type) .await .map_err(|e| Error::new(ErrorKind::Other, e))?;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/handler/http/request/stream/mod.rs (1 hunks)
- src/service/metadata/distinct_values.rs (1 hunks)
- src/service/metadata/trace_list_index.rs (1 hunks)
- src/service/stream.rs (1 hunks)
Additional context used
Path-based instructions (4)
src/service/metadata/trace_list_index.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/metadata/distinct_values.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/stream/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/stream.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (3)
src/service/metadata/trace_list_index.rs (1)
129-143: Conditional feature flag usage is correct.The use of the
enterprisefeature flag to conditionally compile the new ownership management functionality is correctly implemented. The async call toset_ownership_if_not_existsis properly awaited, ensuring that the operation completes before proceeding.Ensure that the
enterpriseandopenfgafeatures are correctly configured in your environment to test this functionality effectively.src/service/metadata/distinct_values.rs (1)
183-197: Conditional feature flag usage is correct.The use of the
enterprisefeature flag to conditionally compile the new ownership management functionality is correctly implemented. The async call toset_ownership_if_not_existsis properly awaited, ensuring that the operation completes before proceeding.Ensure that the
enterpriseandopenfgafeatures are correctly configured in your environment to test this functionality effectively.src/handler/http/request/stream/mod.rs (1)
342-363: Stream type handling is improved.The modifications to the
listfunction to useOFGA_MODELSfor dynamically fetching model keys based onstream_typeenhance flexibility and maintainability. The use ofmap_orto provide a default value if the model key is not found is correctly implemented.Consider adding unit tests to verify that the
listfunction correctly handles variousstream_typevalues and correctly interacts withOFGA_MODELS.
| let s_type = match s_type { | ||
| StreamType::EnrichmentTables => "enrichment_table".to_string(), | ||
| _ => s_type.to_string(), | ||
| }; |
There was a problem hiding this comment.
Refactor suggestion for stream type handling.
The handling of stream_type using a match statement is clear and functional. However, consider using a method on the StreamType enum to encapsulate this logic, which would improve maintainability and reduce the complexity within the get_streams function.
Here's a proposed refactor:
impl StreamType {
fn as_permitted_string(&self) -> String {
match self {
StreamType::EnrichmentTables => "enrichment_table".to_string(),
_ => self.to_string(),
}
}
}Then, replace the match statement in get_streams with:
- let s_type = match s_type {
- StreamType::EnrichmentTables => "enrichment_table".to_string(),
- _ => s_type.to_string(),
- };
+ let s_type = s_type.as_permitted_string();
Subhra264
force-pushed
the
enrich_list_rbac
branch
from
509034f to
ff0f61e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/handler/http/request/stream/mod.rs (1 hunks)
- src/service/metadata/distinct_values.rs (1 hunks)
- src/service/metadata/trace_list_index.rs (1 hunks)
- src/service/stream.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/service/metadata/distinct_values.rs
- src/service/metadata/trace_list_index.rs
- src/service/stream.rs
Additional context used
Path-based instructions (1)
src/handler/http/request/stream/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (1)
src/handler/http/request/stream/mod.rs (1)
342-342: Approved import statement for OFGA_MODELS.The addition of
OFGA_MODELSis essential for the new functionality to dynamically fetch model keys based on stream types, aligning with the PR's objectives to enhance RBAC handling.
| use o2_enterprise::enterprise::openfga::meta::mapping::OFGA_MODELS; | ||
|
|
||
| let user_id = req.headers().get("user_id").unwrap(); | ||
| if let Some(s_type) = &stream_type { | ||
| if !s_type.eq(&StreamType::EnrichmentTables) && !s_type.eq(&StreamType::Metadata) { | ||
| match crate::handler::http::auth::validator::list_objects_for_user( | ||
| &org_id, | ||
| user_id.to_str().unwrap(), | ||
| "GET", | ||
| &s_type.to_string(), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(stream_list) => { | ||
| _stream_list_from_rbac = stream_list; | ||
| } | ||
| Err(e) => { | ||
| return Ok(crate::common::meta::http::HttpResponse::forbidden( | ||
| e.to_string(), | ||
| )); | ||
| } | ||
| let stream_type_str = s_type.to_string(); | ||
| match crate::handler::http::auth::validator::list_objects_for_user( | ||
| &org_id, | ||
| user_id.to_str().unwrap(), | ||
| "GET", | ||
| OFGA_MODELS | ||
| .get(stream_type_str.as_str()) | ||
| .map_or(stream_type_str.as_str(), |model| model.key), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(stream_list) => { | ||
| _stream_list_from_rbac = stream_list; | ||
| } | ||
| Err(e) => { | ||
| return Ok(crate::common::meta::http::HttpResponse::forbidden( | ||
| e.to_string(), | ||
| )); |
There was a problem hiding this comment.
Approved changes with a suggestion for error handling.
The modifications in the list function to use OFGA_MODELS for fetching model keys based on stream types are well-aligned with the PR's objectives. This approach enhances flexibility and maintainability.
However, consider adding more robust error handling for cases where user_id might not be present in the headers, as currently, the code unwraps it directly which could lead to a panic if it's absent.
Consider adding a check before unwrapping user_id:
let user_id = match req.headers().get("user_id") {
Some(uid) => uid,
None => return Ok(crate::common::meta::http::HttpResponse::forbidden("User ID is required".to_string())),
};
Subhra264
force-pushed
the
enrich_list_rbac
branch
from
ff0f61e to
2c5fcd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/handler/http/request/stream/mod.rs (1 hunks)
- src/service/metadata/distinct_values.rs (1 hunks)
- src/service/metadata/trace_list_index.rs (1 hunks)
- src/service/stream.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/service/metadata/distinct_values.rs
- src/service/metadata/trace_list_index.rs
- src/service/stream.rs
Additional context used
Path-based instructions (1)
src/handler/http/request/stream/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (1)
src/handler/http/request/stream/mod.rs (1)
342-363: Approved changes with a suggestion for error handling.The modifications in the
listfunction to useOFGA_MODELSfor fetching model keys based on stream types are well-aligned with the PR's objectives. This approach enhances flexibility and maintainability.However, consider adding more robust error handling for cases where
user_idmight not be present in the headers, as currently, the code unwraps it directly which could lead to a panic if it's absent.Consider adding a check before unwrapping
user_id:let user_id = match req.headers().get("user_id") { Some(uid) => uid, None => return Ok(crate::common::meta::http::HttpResponse::forbidden("User ID is required".to_string())), };
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes