[shaohuzhang1]

fix: preserve omitted auth config fields on partial updates

[shaohuzhang1]

Here is an analysis of the provided code snippet with identified irregularities, potential issues, and optimization suggestions:

1. Security Risk: Insecure Direct Object Reference (IDOR) / Mass Assignment

• Issue: The code iterates over instance.items() and applies setattr if hasattr(portal, field) is true. Even though file_fields excludes some keys, this pattern is generally dangerous. It allows an attacker or malformed input to modify any attribute on the portal object that isn't explicitly blocklisted. If auth_config contains sensitive keys (e.g., is_admin, permissions), merging user-provided dicts without sanitization could lead to privilege escalation or configuration tampering.
• Suggestion: Maintain a whitelist of allowed fields instead of a blacklist. Only iterate over fields you explicitly intend to update.

  ALLOWED_FIELDS = {"name", "description", "theme", "auth_config"} # example
  
  for field, value in instance.items():
      if field not in ALLOWED_FIELDS:
          continue
      if hasattr(portal, field):
          # Your logic here
          setattr(portal, field, value)

2. Logic Error: Conditional Field Handling After General Loop

• Issue: The special handling for "logo" appears to be in a separate loop (for field_name in ["logo"]) after the main setattr loop. However, "logo" is excluded from the main loop via file_fields. This is okay, but if there are other fields needing special processing later, the structure is fragile. More importantly, auth_config is handled inline, while logo is handled separately. This inconsistency makes maintenance harder.
• Suggestion: Group all special-case handling together or handle all updates uniformly unless the field requires entirely different logic (e.g., file uploads vs. JSON config).

3. Deep Merge Issue: Shallow Dictionary Merge

• Issue: The line {**(portal.auth_config or {}), **value} performs a shallow merge. If auth_config is a nested dictionary (common for auth configurations, e.g., {"oidc": {"client_id": "...", "secret": "..."}}), updating {"oidc": {"enabled": True}} will wipe out existing keys inside oidc (like client_id or secret).
• Suggestion: Use a deep merge utility or deepmerge library if partial updates to nested configs are intended.

  import deepmerge
  ...
  if field == "auth_config" and isinstance(value, dict):
      current = portal.auth_config or {}
      portal.auth_config = deepmerge.always_merger.merge(current, value)

4. Type Safety & Validation

• Issue: There’s no validation that value is acceptable before setting it. Especially for auth_config, ensure the final merged dict is valid before persisting. An invalid merge could break authentication.
• Suggestion: Validate the resulting auth_config after merging.

5. Minor Code Style & Clarity

• Suggestion: Extract the special-case logic into a helper method or a more explicit conditional structure for readability.

  if field == "auth_config" and isinstance(value, dict):
      merged_config = {**(portal.auth_config or {}), **value}
      # Optional: validate merged_config here
      setattr(portal, field, merged_config)
  else:
      setattr(portal, field, value)

---

✅ Recommended Refactored Code

def edit(self, instance, with_valid=True):
    # Whitelist of fields that can be directly set
    ALLOWED_DIRECT_FIELDS = {"name", "description", "title"}  # Add your actual fields here
    FILE_FIELDS = {"logo", "id", "create_time", "update_time"}
    
    for field, value in instance.items():
        if field in FILE_FIELDS:
            continue
                
        if field == "auth_config" and isinstance(value, dict):
            # Note shallow merge caveat - use deep merge if needed
            current_config = portal.auth_config or {}
            new_config = {**current_config, **value}
            
            # TODO: Add validation step here
            
            portal.auth_config = new_config
            
        elif field in ALLOWED_DIRECT_FIELDS:
            setattr(portal, field, value)
        else:
            # Log warning or ignore unexpected fields
            logger.warning(f"Ignoring unknown field in edit payload: {field}")

    # Handle special case for logo separately if needed
    if "logo" in instance:
        # Existing logo handling logic
        pass

Key Takeaways:

1. Replace blacklisting with whitelisting.
2. Consider deep merging for nested dictionaries like auth_config.
3. Avoid using generic setattr loops without strict control over allowed fields.