fix: also auto add/del igroup members to workspaces where configured #6888
Conversation
|
/updatesqlx |
|
Claude finished @alpetric's task —— View job Code Review Complete ✅Todo List:
SummaryThis PR adds automatic workspace synchronization when users are added to or removed from instance groups. The implementation correctly mirrors the behavior in both directions (add/remove) and includes a bug fix for the return message in ✅ Positive Aspects
🔍 Issues & RecommendationsCritical Issues1. Potential Race Condition & Inconsistency (line 646)
Recommendation: Modify Performance Concerns2. N+1 Query Pattern (lines 641-647) Recommendation:
3. Missing Index Consideration (line 641) Recommendation: Verify a GIN index exists on Code Quality4. Complex Nested Option Chaining (line 643) ws.auto_add_instance_groups_roles.and_then(|r| r.get(&name).and_then(|v| v.as_str().map(String::from))).unwrap_or_else(|| "developer".to_string())Recommendation: Extract to a helper function for better testability and readability (see inline comment). 5. Duplicate Import Pattern Minor Issues6. Error Propagation 7. Missing Comment on Sync Behavior
🔒 Security Review✅ No Critical Security Issues
📋 Testing InstructionsTo test this functionality through the app interface: Navigate to the Instance Settings area as a super admin and access the Instance Groups management page. Create or select an existing instance group, then configure one or more workspaces to automatically add members of this instance group with specific roles (developer, operator, or admin). Add a user email to the instance group and verify that the user is automatically added to all configured workspaces with the correct permissions - for admin roles, confirm they have admin privileges in those workspaces. Then remove the user from the instance group and verify they are automatically removed from the workspaces where they were added via this instance group (users added through other means should remain). Check the audit logs to confirm all operations are properly recorded. Overall Assessment: This is a solid implementation that correctly adds the required functionality. The main concerns are around performance with many workspaces and the two-step admin privilege grant. Consider addressing the critical race condition issue before merging, and the performance concerns can be monitored post-deployment. |
|
Starting sqlx update... |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| #[cfg(feature = "private")] | ||
| { | ||
| use crate::workspaces_ee::auto_add_user; | ||
| let workspaces = sqlx::query!("SELECT workspace_id, auto_add_instance_groups_roles FROM workspace_settings WHERE $1 = ANY(COALESCE(auto_add_instance_groups, '{}'))", &name).fetch_all(&mut *tx).await?; |
There was a problem hiding this comment.
Performance Consideration: This query fetches all workspaces that have this instance group configured. Consider the following optimizations:
- Select only necessary columns: Instead of
SELECT workspace_id, auto_add_instance_groups_roles, explicitly list only the columns needed to avoid transferring unnecessary data. - Consider indexing: If
auto_add_instance_groupsis frequently queried withANY(), ensure there's an appropriate index (GIN index for array operations) onworkspace_settings.auto_add_instance_groups. - Batch operations: The current implementation executes
auto_add_userin a loop. Ifauto_add_userperforms individual INSERT statements, this could result in N queries. Consider if there's a way to batch these operations.
The query pattern with COALESCE(auto_add_instance_groups, '{}') is good defensive programming to handle NULL values.
| use crate::workspaces_ee::auto_add_user; | ||
| let workspaces = sqlx::query!("SELECT workspace_id, auto_add_instance_groups_roles FROM workspace_settings WHERE $1 = ANY(COALESCE(auto_add_instance_groups, '{}'))", &name).fetch_all(&mut *tx).await?; | ||
| for ws in workspaces { | ||
| let role = ws.auto_add_instance_groups_roles.and_then(|r| r.get(&name).and_then(|v| v.as_str().map(String::from))).unwrap_or_else(|| "developer".to_string()); |
There was a problem hiding this comment.
Complex Nested Logic: This line chains multiple Option operations which reduces readability. Consider extracting this to a helper function or adding intermediate variables:
let role = ws.auto_add_instance_groups_roles
.and_then(|r| r.get(&name))
.and_then(|v| v.as_str().map(String::from))
.unwrap_or_else(|| "developer".to_string());Could be refactored to:
fn get_role_for_group(
roles: Option<serde_json::Value>,
group_name: &str,
) -> String {
roles
.and_then(|r| r.get(group_name))
.and_then(|v| v.as_str().map(String::from))
.unwrap_or_else(|| "developer".to_string())
}
// Then use:
let role = get_role_for_group(ws.auto_add_instance_groups_roles, &name);This would improve maintainability and testability.
| let role = ws.auto_add_instance_groups_roles.and_then(|r| r.get(&name).and_then(|v| v.as_str().map(String::from))).unwrap_or_else(|| "developer".to_string()); | ||
| let (is_admin, is_operator) = match role.as_str() { "admin" => (true, false), "operator" => (false, true), _ => (false, false) }; | ||
| auto_add_user(&email, &ws.workspace_id, &is_operator, &mut tx, &authed, Some(serde_json::json!({"source": "instance_group", "group": &name}))).await?; | ||
| if is_admin { sqlx::query!("UPDATE usr SET is_admin = true WHERE workspace_id = $1 AND email = $2", &ws.workspace_id, &email).execute(&mut *tx).await?; } |
There was a problem hiding this comment.
Security & Correctness Concern: The is_admin update happens AFTER auto_add_user, as a separate query. This creates a potential race condition and inconsistency:
- Race condition: If another transaction reads the user's admin status between
auto_add_userand this UPDATE, they'll see incorrect permissions temporarily. - Error handling: If this UPDATE fails but
auto_add_usersucceeds, the user will be added but won't have admin permissions, which violates the intended state. - Separate query overhead: This adds an extra round trip to the database for each admin workspace.
Recommendation: Check if auto_add_user can accept an is_admin parameter to set the admin status atomically in a single operation. If not, consider:
- Adding a comment explaining why this two-step process is necessary
- Ensuring proper error handling to rollback the transaction if this fails
- Documenting this behavior for future maintainers
Deploying windmill with
|
| Latest commit: |
908ff1b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f6a14267.windmill.pages.dev |
| Branch Preview URL: | https://alp-igroup-workspace-group-a.windmill.pages.dev |
|
Successfully ran sqlx update |
| .await?; | ||
|
|
||
| // Remove user from workspaces where they were added via this instance group | ||
| #[cfg(feature = "private")] |
| .await?; | ||
|
|
||
| // Sync user to workspaces configured with this instance group | ||
| #[cfg(feature = "private")] |
2 participants
No description provided.