fix: Forti changes not correctly propagated

[Laennart]

closes #4446

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[tpurschke]

careful - if memory serves me right, we are dealing with multiple src/dst zones per rule now and are therefore using a cross-ref table.
imho all zone fields in rule should be removed instead.
@Imat00 - can you comment on this?

[Imat00]

Yes, the zone assignments are stored in the cross-reference tables (rule_from_zone / rule_to_zone). A rule can have multiple source and destination zones, and changes are tracked via created / removed.

I think the issue in #4446 relates to how these changes are propagated to the changelog / removal handling.

[Y4nnikH]

careful - if memory serves me right, we are dealing with multiple src/dst zones per rule now and are therefore using a cross-ref table. imho all zone fields in rule should be removed instead. @Imat00 - can you comment on this?

yes, a rule can have multiple src/dst zones, just as it can have multiple src/dst network objects. so we should use the same pattern: zone uids in the rule, joined by "|", as well as a crossref table which links the database ids. this is especially needed to fix the issue #4446, because if the uids are not in the rule, the rule object does not change for added/removed zones and we cannot record this arguably security-relevant change by creating a changelog rule entry

[tpurschke]

Key issue — zone-string ordering is inconsistent across the three producers

The same logical value is built three different ways:

Producer | Sort | Dedup -- | -- | -- Python importer (fos_rule.py:107, fmgr_rule.py:201) — LIST_DELIMITER.join(names) | ❌ interface order | ❌ Migration backfill (9.1.9.sql) — string_agg(DISTINCT z.zone_name ... ORDER BY z.zone_name) | ✅ alphabetical | ✅ C# NormalizedRule (NormalizedRule.cs:128) — `string.Join(" | ", ...Order())` | ✅ alphabetical

Because equality now includes these fields, the mismatch matters:

• Spurious changes on first import after upgrade. The backfill writes sorted/deduped strings; the next Forti import normalizes in interface order. For any rule with ≥2 zones where interface order ≠ alphabetical (or with duplicate zones), __eq__ returns false → the rule is flagged as changed and a changelog "C" entry is written, even though nothing actually changed. On large FortiManager imports this could be a one-time burst of false change entries — ironic given this PR is about change-detection accuracy.
• Single-zone rules (the common case) are unaffected, which is why it may pass casual testing.

Recommendation: make all three produce a canonical form — sort and dedup. The cheapest fix is in the Python normalization (e.g. LIST_DELIMITER.join(sorted(set(rule_src_zone_names)))), and add .Distinct() to the C# side, so all three match DISTINCT ... ORDER BY. Do confirm whether zone order is itself ever semantically meaningful on Forti — if it is, the backfill is the one that's wrong and should preserve order instead; but the link table likely can't reconstruct order anyway, so canonical-sorted everywhere is the pragmatic choice.

Smaller points

• Dead int columns / leftover TODO. Keeping rule_from_zone/rule_to_zone (int) on the rule table is reasonably justified in the SQL comment, but prepare_rule_for_import previously set them and the Rule model now drops them entirely — so those columns are now never written by the importer. Fine, but it leaves real tech debt; consider a tracked follow-up issue rather than the informal "for now" comment, since the link tables remain the source of truth for resolved zones.
• .agents submodule bump (c677fd6… → 74356aa…) appears unrelated to this fix. Unless intentional, drop it from the PR to keep the change focused.
• Migration safety is good: ADD COLUMN IF NOT EXISTS, to_regclass guards, and COALESCE(...) <> ... to avoid no-op updates. Note the guards check public.rule_from_zone/rule_to_zone as relations (to_regclass) — confirm those link tables are actually tables/views by those exact names in all deployments, otherwise the backfill silently RETURNs and leaves zones NULL (then the next import re-detects everything as changed — same symptom as above, larger blast radius).
• No tests. CLAUDE.md requires unit tests for new behavior, and roles/importer/files/importer/test already exists. Given the equality change is the crux of the fix, a test asserting that two RuleNormalized differing only in rule_src_zone/rule_dst_zone are now unequal (and flagged security-relevant) would lock in the fix and guard against regressions. Worth adding before merge.
• Hasura metadata: the column was added in 9 permission blocks — verify those cover the insert/update permission set for the importer role specifically (not just select), otherwise the new columns won't be writable on import.

Verdict

Correct, well-targeted fix for #4446, with clean and defensively-written migration. The one thing I'd block on is the zone-string ordering inconsistency between the importer, the backfill, and the C# normalizer — without canonicalization the next import after upgrade can emit false "changed" entries, undercutting the very feature this PR delivers. Add a regression test and drop the stray .agents bump.

Key issue — zone-string ordering is inconsistent across the three producers The same logical value is built three different ways:

Producer Sort Dedup
Python importer (fos_rule.py:107, fmgr_rule.py:201) — LIST_DELIMITER.join(names) ❌ interface order ❌
Migration backfill (9.1.9.sql) — string_agg(DISTINCT z.zone_name ... ORDER BY z.zone_name) ✅ alphabetical ✅
C# NormalizedRule (NormalizedRule.cs:128) — string.Join(" ", ...Order()) ✅ alphabetical
Because equality now includes these fields, the mismatch matters:

Spurious changes on first import after upgrade. The backfill writes sorted/deduped strings; the next Forti import normalizes in interface order. For any rule with ≥2 zones where interface order ≠ alphabetical (or with duplicate zones), eq returns false → the rule is flagged as changed and a changelog "C" entry is written, even though nothing actually changed. On large FortiManager imports this could be a one-time burst of false change entries — ironic given this PR is about change-detection accuracy.
Single-zone rules (the common case) are unaffected, which is why it may pass casual testing.
Recommendation: make all three produce a canonical form — sort and dedup. The cheapest fix is in the Python normalization (e.g. LIST_DELIMITER.join(sorted(set(rule_src_zone_names)))), and add .Distinct() to the C# side, so all three match DISTINCT ... ORDER BY. Do confirm whether zone order is itself ever semantically meaningful on Forti — if it is, the backfill is the one that's wrong and should preserve order instead; but the link table likely can't reconstruct order anyway, so canonical-sorted everywhere is the pragmatic choice.

Smaller points
Dead int columns / leftover TODO. Keeping rule_from_zone/rule_to_zone (int) on the rule table is reasonably justified in the SQL comment, but prepare_rule_for_import previously set them and the Rule model now drops them entirely — so those columns are now never written by the importer. Fine, but it leaves real tech debt; consider a tracked follow-up issue rather than the informal "for now" comment, since the link tables remain the source of truth for resolved zones.
.agents submodule bump (c677fd6… → 74356aa…) appears unrelated to this fix. Unless intentional, drop it from the PR to keep the change focused.
Migration safety is good: ADD COLUMN IF NOT EXISTS, to_regclass guards, and COALESCE(...) <> ... to avoid no-op updates. Note the guards check public.rule_from_zone/rule_to_zone as relations (to_regclass) — confirm those link tables are actually tables/views by those exact names in all deployments, otherwise the backfill silently RETURNs and leaves zones NULL (then the next import re-detects everything as changed — same symptom as above, larger blast radius).
No tests. CLAUDE.md requires unit tests for new behavior, and roles/importer/files/importer/test already exists. Given the equality change is the crux of the fix, a test asserting that two RuleNormalized differing only in rule_src_zone/rule_dst_zone are now unequal (and flagged security-relevant) would lock in the fix and guard against regressions. Worth adding before merge.
Hasura metadata: the column was added in 9 permission blocks — verify those cover the insert/update permission set for the importer role specifically (not just select), otherwise the new columns won't be writable on import.
Verdict
Correct, well-targeted fix for #4446, with clean and defensively-written migration. The one thing I'd block on is the zone-string ordering inconsistency between the importer, the backfill, and the C# normalizer — without canonicalization the next import after upgrade can emit false "changed" entries, undercutting the very feature this PR delivers. Add a regression test and drop the stray .agents bump.

[Laennart]

@tpurschke I added an issue to track the dead columns: #4789

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Y4nnikH]

reviewed and tested on customer dev machine