fix: include org_id in list_objects for rbac #3841
Conversation
WalkthroughThe refactor involves enhancing token validation by restructuring authentication and authorization logic for efficiency, particularly when handling HTTP POST requests and setting headers. Key updates include a conditional authorization block based on user permissions and improved error handling. Additionally, passing the Changes
Sequence DiagramsToken Validation and Authorization FlowsequenceDiagram
participant Client
participant Server
participant Authenticator
participant Authorizer
Client->>Server: POST /validate-token
Server->>Authenticator: Validate Token
Authenticator-->>Server: Token Valid
Server->>Authorizer: Check User Permissions
Authorizer-->>Server: Permissions Granted
Server->>Client: 200 OK (Token Valid)
Note over Server: if token invalid or permissions denied:
Server->>Client: 401 Unauthorized
Object Listing with Organization ContextsequenceDiagram
participant Client
participant Server
participant DataStore
Client->>Server: GET /list-objects
Server->>DataStore: Fetch Objects(org_id)
DataStore-->>Server: Object List
Server->>Client: 200 OK (Object List)
Note over Server: includes org_id in DataStore request
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 (
|
![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 UI
Review profile: CHILL
Files selected for processing (1)
- src/handler/http/auth/validator.rs (2 hunks)
Additional context used
Path-based instructions (1)
src/handler/http/auth/validator.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 (2)
src/handler/http/auth/validator.rs (2)
730-736: Approved: Incorporation oforg_idinlist_objects.The addition of
org_idto the function signature correctly extends the RBAC capabilities as intended by the PR. Ensure that all calls to this function have been updated to pass the new parameter.Verification successful
Verified: All calls to
list_objectsandlist_objects_for_userinclude theorg_idparameter.The search results confirm that all instances of
list_objectsandlist_objects_for_useracross the codebase have been updated to include the neworg_idparameter, ensuring consistency and alignment with the updated function signature.
src/handler/http/auth/validator.rssrc/handler/http/request/dashboards/folders.rssrc/handler/http/request/stream/mod.rssrc/handler/http/request/dashboards/reports.rssrc/handler/http/request/functions/mod.rssrc/handler/http/request/pipelines.rssrc/handler/http/request/authz/fga.rssrc/handler/http/request/alerts/mod.rssrc/handler/http/request/alerts/templates.rssrc/handler/http/request/alerts/destinations.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `list_objects` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'list_objects'Length of output: 1556
Script:
#!/bin/bash # Description: Verify that all calls to `list_objects` match the new signature. # Extract lines around the matches to check the function call signature. rg --type rust -A 3 -B 3 'list_objects' src/handler/http/auth/validator.rsLength of output: 772
Script:
#!/bin/bash # Description: Verify that all calls to `list_objects_for_user` match the updated signature. # Extract lines around the matches to check the function call signature. rg --type rust -A 3 -B 3 'list_objects_for_user'Length of output: 6017
Line range hint
743-758: Approved: Updatedlist_objects_for_userto includeorg_id.The function now correctly includes
org_idto enhance role-based access control. This should help in fine-graining the access permissions based on organizational context.
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
cdae9a7 to
88af905
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 (1)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/auth/validator.rs
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
88af905 to
f327e03
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 (1)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/auth/validator.rs
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
f327e03 to
d80c816
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 (1)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/auth/validator.rs
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
d80c816 to
d519b39
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 (1)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/auth/validator.rs
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
d519b39 to
debb44b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/handler/http/auth/token.rs (1 hunks)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/auth/validator.rs
Additional context used
Path-based instructions (1)
src/handler/http/auth/token.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/handler/http/auth/token.rs (3)
86-87: Improve code readability by renaming the variable.Consider renaming the variable
usertoauthenticated_userto improve code readability and make it clear that this variable represents an authenticated user.- let user; + let authenticated_user;
88-88: Remove unnecessary comment.The comment
// / Hack for prometheus, need support POST and check the headeris unclear and seems unnecessary. If it's important, consider rephrasing it for clarity.- // / Hack for prometheus, need support POST and check the header + // Support for Prometheus: Ensure POST requests have the correct content type header
105-111: Improve authorization error handling.The error message "Unauthorized Access" can be more descriptive to help with debugging and logging.
- Err((ErrorForbidden("Unauthorized Access"), req)) + Err((ErrorForbidden("User does not have the required permissions"), req))
| header::HeaderName::from_static("user_id"), | ||
| header::HeaderValue::from_str(&res.0.user_email).unwrap(), |
There was a problem hiding this comment.
Handle potential errors when parsing the email.
The call to unwrap() can cause a panic if parsing the email fails. Consider using map_err to handle potential errors gracefully.
- header::HeaderValue::from_str(&res.0.user_email).unwrap(),
+ header::HeaderValue::from_str(&res.0.user_email).map_err(|e| {
+ log::error!("Failed to parse user email: {}", e);
+ ErrorUnauthorized("Invalid user email")
+ })?,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.
| header::HeaderName::from_static("user_id"), | |
| header::HeaderValue::from_str(&res.0.user_email).unwrap(), | |
| header::HeaderName::from_static("user_id"), | |
| header::HeaderValue::from_str(&res.0.user_email).map_err(|e| { | |
| log::error!("Failed to parse user email: {}", e); | |
| ErrorUnauthorized("Invalid user email") | |
| })?, |
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
debb44b to
f497d51
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/handler/http/auth/token.rs (1 hunks)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/auth/validator.rs
Additional context used
Path-based instructions (1)
src/handler/http/auth/token.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 (2)
src/handler/http/auth/token.rs (2)
Line range hint
115-127: LGTM! Ensure proper error handling and security checks.The function correctly retrieves the user's email from the token. Ensure that error handling and security checks are in place.
Line range hint
129-135: LGTM!The function correctly handles the non-enterprise case by returning a "Not Supported" error.
| if req.method().eq(&Method::POST) | ||
| && !req.headers().contains_key("content-type") | ||
| { | ||
| req.headers_mut().insert( | ||
| header::CONTENT_TYPE, | ||
| header::HeaderValue::from_static( | ||
| "application/x-www-form-urlencoded", | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Simplify header insertion logic.
The logic for inserting the content-type header can be simplified to improve readability.
- if req.method().eq(&Method::POST)
- && !req.headers().contains_key("content-type")
- {
- req.headers_mut().insert(
- header::CONTENT_TYPE,
- header::HeaderValue::from_static(
- "application/x-www-form-urlencoded",
- ),
- );
- }
+ if req.method() == Method::POST && !req.headers().contains_key("content-type") {
+ req.headers_mut().insert(
+ header::CONTENT_TYPE,
+ header::HeaderValue::from_static("application/x-www-form-urlencoded"),
+ );
+ }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.
| if req.method().eq(&Method::POST) | |
| && !req.headers().contains_key("content-type") | |
| { | |
| req.headers_mut().insert( | |
| header::CONTENT_TYPE, | |
| header::HeaderValue::from_static( | |
| "application/x-www-form-urlencoded", | |
| ), | |
| ); | |
| } | |
| if req.method() == Method::POST && !req.headers().contains_key("content-type") { | |
| req.headers_mut().insert( | |
| header::CONTENT_TYPE, | |
| header::HeaderValue::from_static("application/x-www-form-urlencoded"), | |
| ); | |
| } |
| match user { | ||
| Some(user) => { | ||
| // / Hack for prometheus, need support POST and check the header | ||
| let mut req = req; | ||
|
|
||
| if req.method().eq(&Method::POST) && !req.headers().contains_key("content-type") | ||
| { | ||
| if req.method().eq(&Method::POST) | ||
| && !req.headers().contains_key("content-type") | ||
| { | ||
| req.headers_mut().insert( | ||
| header::CONTENT_TYPE, | ||
| header::HeaderValue::from_static( | ||
| "application/x-www-form-urlencoded", | ||
| ), | ||
| ); | ||
| } | ||
| req.headers_mut().insert( | ||
| header::CONTENT_TYPE, | ||
| header::HeaderValue::from_static("application/x-www-form-urlencoded"), | ||
| header::HeaderName::from_static("user_id"), | ||
| header::HeaderValue::from_str(&res.0.user_email).unwrap(), | ||
| ); | ||
| if auth_info.bypass_check | ||
| || check_permissions(user_id, auth_info, Some(user.role)).await | ||
| { | ||
| Ok(req) | ||
| } else { | ||
| Err((ErrorForbidden("Unauthorized Access"), req)) | ||
| } | ||
| } | ||
| req.headers_mut().insert( | ||
| header::HeaderName::from_static("user_id"), | ||
| header::HeaderValue::from_str(&res.0.user_email).unwrap(), | ||
| ); | ||
| // send user role as None as it applies only to internal users | ||
| if auth_info.bypass_check || check_permissions(user_id, auth_info, None).await { | ||
| Ok(req) | ||
| } else { | ||
| Err((ErrorForbidden("Unauthorized Access"), req)) | ||
| } | ||
| } else { | ||
| Err((ErrorForbidden("Unauthorized Access"), req)) | ||
| _ => Err((ErrorForbidden("Unauthorized Access"), req)), |
There was a problem hiding this comment.
Ensure proper error handling for user retrieval.
The unwrap() call on line 103 can cause a panic if parsing the email fails. Consider using map_err to handle potential errors gracefully.
- header::HeaderValue::from_str(&res.0.user_email).unwrap(),
+ header::HeaderValue::from_str(&res.0.user_email).map_err(|e| {
+ log::error!("Failed to parse user email: {}", e);
+ ErrorUnauthorized("Invalid user email")
+ })?,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.
| match user { | |
| Some(user) => { | |
| // / Hack for prometheus, need support POST and check the header | |
| let mut req = req; | |
| if req.method().eq(&Method::POST) && !req.headers().contains_key("content-type") | |
| { | |
| if req.method().eq(&Method::POST) | |
| && !req.headers().contains_key("content-type") | |
| { | |
| req.headers_mut().insert( | |
| header::CONTENT_TYPE, | |
| header::HeaderValue::from_static( | |
| "application/x-www-form-urlencoded", | |
| ), | |
| ); | |
| } | |
| req.headers_mut().insert( | |
| header::CONTENT_TYPE, | |
| header::HeaderValue::from_static("application/x-www-form-urlencoded"), | |
| header::HeaderName::from_static("user_id"), | |
| header::HeaderValue::from_str(&res.0.user_email).unwrap(), | |
| ); | |
| if auth_info.bypass_check | |
| || check_permissions(user_id, auth_info, Some(user.role)).await | |
| { | |
| Ok(req) | |
| } else { | |
| Err((ErrorForbidden("Unauthorized Access"), req)) | |
| } | |
| } | |
| req.headers_mut().insert( | |
| header::HeaderName::from_static("user_id"), | |
| header::HeaderValue::from_str(&res.0.user_email).unwrap(), | |
| ); | |
| // send user role as None as it applies only to internal users | |
| if auth_info.bypass_check || check_permissions(user_id, auth_info, None).await { | |
| Ok(req) | |
| } else { | |
| Err((ErrorForbidden("Unauthorized Access"), req)) | |
| } | |
| } else { | |
| Err((ErrorForbidden("Unauthorized Access"), req)) | |
| _ => Err((ErrorForbidden("Unauthorized Access"), req)), | |
| match user { | |
| Some(user) => { | |
| // / Hack for prometheus, need support POST and check the header | |
| let mut req = req; | |
| if req.method().eq(&Method::POST) | |
| && !req.headers().contains_key("content-type") | |
| { | |
| req.headers_mut().insert( | |
| header::CONTENT_TYPE, | |
| header::HeaderValue::from_static( | |
| "application/x-www-form-urlencoded", | |
| ), | |
| ); | |
| } | |
| req.headers_mut().insert( | |
| header::HeaderName::from_static("user_id"), | |
| header::HeaderValue::from_str(&res.0.user_email).map_err(|e| { | |
| log::error!("Failed to parse user email: {}", e); | |
| ErrorUnauthorized("Invalid user email") | |
| })?, | |
| ); | |
| if auth_info.bypass_check | |
| || check_permissions(user_id, auth_info, Some(user.role)).await | |
| { | |
| Ok(req) | |
| } else { | |
| Err((ErrorForbidden("Unauthorized Access"), req)) | |
| } | |
| } | |
| _ => Err((ErrorForbidden("Unauthorized Access"), req)), |
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
f497d51 to
c6f3b23
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 (2)
- src/handler/http/auth/token.rs (1 hunks)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/handler/http/auth/token.rs
- src/handler/http/auth/validator.rs
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
c6f3b23 to
097da7e
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 (2)
- src/handler/http/auth/token.rs (1 hunks)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/handler/http/auth/token.rs
- src/handler/http/auth/validator.rs
Subhra264
force-pushed
the
fix/resource_same_name_rbac
branch
from
097da7e to
0478f8b
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 (2)
- src/handler/http/auth/token.rs (1 hunks)
- src/handler/http/auth/validator.rs (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/handler/http/auth/token.rs
- src/handler/http/auth/validator.rs
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced authentication and authorization handling for user requests with more efficient token validation. - **Bug Fixes** - Improved error handling and response based on user permissions and authorization status. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary by CodeRabbit