[merge ee first] fix: alining group name normalization between scim and igroup api

[alpetric]

No description provided.

[claude]

Claude finished @alpetric's task in 2m 17s —— View job

---

PR Review Complete ✅

Tasks

• Read changed files
• Analyze code changes
• Review for bugs and issues
• Provide comprehensive feedback
• Add reproduction instructions

---

Summary

This PR addresses group name normalization inconsistency between SCIM-managed groups and manually created instance groups (igroups). The changes ensure that instance groups created through the API use the same naming convention (lowercase with underscores instead of spaces) that SCIM groups use.

Code Quality Assessment

Strengths:

• ✅ Well-documented function with clear doc comment explaining the normalization rules
• ✅ Simple, straightforward implementation
• ✅ Consistent application of normalization in all relevant places (insert query, audit log, return message)
• ✅ The EE repo reference update indicates corresponding SCIM changes were made

Issues & Concerns:

🔴 Critical: Missing Normalization in Workspace Groups

The create_group function (line 230-288) does NOT apply name normalization, while create_igroup now does. This creates an inconsistency:

• Workspace groups (created via create_group): Accept names with spaces and mixed case as-is
• Instance groups (created via create_igroup): Normalize to lowercase with underscores

Impact: This asymmetry could cause confusion. If the goal is to align naming across all group types (as the PR title suggests), workspace groups should also use convert_name().

// In create_group (line 230), consider:
let normalized_name = convert_name(&ng.name);
check_name_conflict(&mut tx, &w_id, &normalized_name).await?;
// ... use normalized_name in the INSERT

However, this may be intentional if workspace groups and instance groups have different naming requirements. The PR description doesn't clarify this.

⚠️ Potential: Silent Conflict Handling

Line 304 uses ON CONFLICT DO NOTHING:

sqlx::query!(
    "INSERT INTO instance_group (name, summary, id) VALUES ($1, $2, $3) ON CONFLICT DO NOTHING",
    normalized_name,
    ...
)

Issue: If a normalized name already exists (e.g., user creates "My Group" when "my_group" exists), the operation silently succeeds without:

1. Informing the user of the conflict
2. Returning an error
3. Updating the existing group

Recommendation: Add explicit conflict checking similar to check_name_conflict used in create_group:

let exists = sqlx::query_scalar!(
    "SELECT EXISTS(SELECT 1 FROM instance_group WHERE name = $1)",
    normalized_name
)
.fetch_one(&mut *tx)
.await?
.unwrap_or(false);

if exists {
    return Err(Error::BadRequest(format!(
        "Instance group '{}' already exists (normalized from '{}')",
        normalized_name, ng.name
    )));
}

⚠️ Missing: Function Visibility

The convert_name function at line 60 is marked pub, which exports it from the module. Questions:

• Is this intentional for use by SCIM code in the EE repo?
• If not, should it be pub(crate) or private?

Given the PR title mentions SCIM alignment, this is likely intentional, but worth confirming.

📝 Minor: Typo in Commit Message

Commit message has a typo: "aling group name" should be "align group name"

Performance Considerations

✅ No concerns. The convert_name function is simple (string replacement + lowercase conversion) and only runs once per group creation. Performance impact is negligible.

Security Implications

✅ No new security issues. The normalization is a pure string transformation that:

• Doesn't introduce SQL injection risks (uses parameterized queries)
• Doesn't expose sensitive data
• Doesn't bypass authorization (still requires super admin via require_super_admin)

Note: The existing code already has proper authorization checks.

Best Practices Alignment

Follows Rust best practices:

• ✅ Uses str reference instead of String ownership in convert_name
• ✅ Returns owned String (appropriate for string transformation)
• ✅ Clear function naming

Potential improvement:
Consider making the function more explicit about what it does:

/// Normalize group names for database storage: converts to lowercase and replaces spaces with underscores.
/// This ensures SCIM-managed groups and manually-created instance groups use consistent naming.
///
/// # Examples
/// ```
/// assert_eq!(convert_name("My Group"), "my_group");
/// assert_eq!(convert_name("ADMIN_GROUP"), "admin_group");
/// ```
pub fn convert_name(name: &str) -> String {
    name.replace(" ", "_").to_lowercase()
}

Testing Recommendations

The PR doesn't include tests. Consider adding:

1. Unit tests for convert_name:

#[cfg(test)]
mod tests {
    use super::*;
    
    #[test]
    fn test_convert_name() {
        assert_eq!(convert_name("My Group"), "my_group");
        assert_eq!(convert_name("ADMIN"), "admin");
        assert_eq!(convert_name("Group With Spaces"), "group_with_spaces");
        assert_eq!(convert_name("already_normalized"), "already_normalized");
    }
}

2. Integration tests to verify:
   • Creating instance groups with spaces results in normalized names
   • Attempting to create groups that normalize to the same name is handled correctly
   • SCIM group creation aligns with manual group creation

Questions for Author

1. Should workspace groups (create_group) also use name normalization?
2. Is the silent ON CONFLICT DO NOTHING behavior intentional, or should it return an error?
3. Are there database migrations or existing groups that need name normalization applied retroactively?
4. What happens if a SCIM group "My Team" and manual group "my_team" both exist after this change?

---

Reproduction Instructions for Testing

To verify the group name normalization changes:

Navigate to the instance groups administration page (accessible only to super admins). Create a new instance group with a name containing spaces and uppercase letters, such as "Test Group Name". After creation, verify that the group appears in the instance groups list with the normalized name "test_group_name" (lowercase with underscores replacing spaces). Additionally, test creating a second group with a name that would normalize to the same value (e.g., "Test_Group_Name" or "TEST GROUP NAME") to confirm the conflict handling behavior matches expectations. The group should either be rejected with an error message or silently ignored depending on the intended behavior. Compare this with the SCIM-based group creation workflow to ensure both paths produce identical normalized names for groups that should be considered equivalent.

---

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	595949c
Status:	✅  Deploy successful!
Preview URL:	https://0dc50dfd.windmill.pages.dev
Branch Preview URL:	https://alp-scim-igroup-align.windmill.pages.dev

View logs