fix(sync): three ways a UserGroup sync loses a permission silently

[cplieger]

A sync that creates a resource and a UserGroup permission on that same resource creates the group, adds its users, reports success, and applies no permission. Hit this on a live deployment: a service user was denied Execute on an Action that the TOML plainly grants it. Running the sync a second time applied it, which is what made it look like a fluke rather than a bug.

Three commits. Tracing the first one turned up two more ways the same code path loses a permission without saying so, and they are small enough that splitting them across PRs seemed worse than one PR you can read commit by commit. Commit 3 is independent of the other two: drop it and they still stand.

Commit 1: apply UserGroups after resources, not before

Cause

Ordering. Two things both ran before any resource existed:

• user_groups::run_updates sat in the apply block under // No deps, ahead of every resource phase.
• user_groups::get_updates_for_execution ran in the pre-computation block. That is where expand_user_group_permissions resolves each permission target through Matcher against all_resources_cache(), keeping only the targets it can match. A target naming a resource the same sync is about to create expands to nothing and is dropped, with no error.

The apply block already documents exactly this hazard:

The ordering these are executed does matter, since latter resources may depend on prior synced resources already being updated with the declared state.

Procedure is last with // Depends on everything. A UserGroup permission names a resource, so UserGroups belong there too.

The change

Move the phase after Procedure, and recompute its deltas there rather than reusing the pre-computation, since that one expanded against the pre-sync cache. resource::create calls refresh_all_resources_cache(), so by that point the targets resolve. The earlier computation is kept solely to feed the existing no-changes early exit, which is why the TOML is cloned.

The recompute deliberately does not propagate its error. Every resource phase has already been applied by then, so a ? there would introduce a new mid-sync abort that skips deploy_from_cache and the last_sync_ts bookkeeping. It logs into the update instead, matching how run_updates already reports problems.

Cost

One extra get_updates_for_execution per sync when include_user_groups is on: a find_collect over user_groups, one over users, and one ListUserTargetPermissions per group. Happy to restructure if you would rather avoid the second pass, for example by having the expansion happen inside run_updates so the deltas are computed once at apply time. That is a larger change and I did not want to guess at your preference.

Interactions I checked

• delete = true. When a resource is removed during the resource phases, delete_all_permissions_on_resource purges its permission rows in the same phase, so the recompute's original_permissions no longer contains it and the expansion drops the target. Net no-op, same end state the old order reached by granting then deleting. I did not find a case where the recompute revokes a permission the old order preserved.
• A failing resource phase. The UserGroup phase still runs afterwards, as it did before.
• Wildcard targets now expand over resources created by the same sync, which is the fix, and no longer emit doomed updates against ones deleted by it.

Commit 2: report permission targets that match nothing

The first commit fixes the case where the target was resolvable and the ordering was wrong. The remaining case is a target that is not resolvable at all: a typo, a renamed resource, a pattern that matches nothing. expand_user_group_permissions drops it, run_updates reports success, and no line of output mentions it. A permission the TOML declares is simply never granted, and the sync is green.

This is the diagnostic I mentioned as future work in the first draft of this PR. It is here now rather than deferred, and without the warn! that made the draft noisy.

The expansion now collects each target that expanded to nothing and run_updates prints one WARN line per unique target into the UserGroups log the user already reads, naming the group and the target. An empty target id is reported the same way, since an id is a name or a regex over names and an empty one cannot match either.

Reported rather than fatal, deliberately: a target can match nothing because it names a resource that does not exist, or because a pattern legitimately matches nothing right now, and the sync cannot tell those apart. Hard-failing would break the second case, which is supported configuration.

Four details worth your attention:

• Which pass reports. The list that reaches the log comes from the post-resource recompute pass added by commit 1, so a target naming a resource this sync just created is not reported as missing. The pre-resource pass's list feeds only the RunSync no-changes gate, so a sync whose only finding is a dropped target does not exit "nothing to do" before the report can be written. I stopped short of claiming that cache is authoritative: refresh_all_resources_cache logs and swallows a load failure, so a stale cache can in principle produce a false report.
• Consequence of that gate change. A run whose only finding is a dropped target now proceeds through the apply block, where every phase is a no-op, and updates last_sync_ts / last_sync_hash. Previously it returned early. I convinced myself this is the more accurate of the two, since everything applicable really has been applied, and I checked that displayed sync state and the pending-updates alert are both derived from view-path data and so are unaffected. Flagging it because it is the one behavior change beyond the log line.
• The UI will not offer an Execute for a warning-only sync. Execute-tab visibility is computed from the view path, which discards this list on purpose (it is recomputed continuously, so reporting there would be noise rather than news). So the warning surfaces on any run that has other work to do, and on API or webhook runs, but a sync whose sole finding is a bad target will not grow an Execute button to surface it. Fixing that properly means an informational field on ResourceSyncInfo that does not feed has_updates, Pending state, or alerts. That is a UI change I did not want to make uninvited, so say the word if you want it.
• System targets are excluded from the check. They are not name matched, so a no-match result says nothing about them.

Shape note

get_updates_for_execution now returns a 4-tuple. A UserGroupDeltas struct would read better and there is precedent in SyncDeltas. I kept the tuple to hold the diff to the bug; happy to convert it if you would rather that landed here.

Commit 3: stop the sync revoking System permissions it exported itself

Independent of the first two, and the only one that deletes data rather than hiding information.

The export and the import disagree about System targets, and the diff resolves the disagreement by deleting:

1. convert_user_groups writes a group's System permissions into the file untouched, so an export produces TOML that declares them.
2. expand_user_group_permissions drops every System target on the way back in, so the incoming list never contains one.
3. original_permissions is read from the database with System passed through, and to_remove sets every original permission missing from the incoming list to PermissionLevel::None.

So syncing back a file that Komodo itself exported revokes the group's System permissions. The same asymmetry is visible in the view: curr comes from convert_user_groups and holds the System permission, the incoming side does not, so the sync displays a permission removal nobody asked for, and it reappears on every refresh until it is applied.

The fix is pass-through. A System target names no resource, so there is nothing to match it against and nothing to expand.

On severity, so you can weigh it honestly: this restores round trip fidelity, not access. get_user_permission_on_target returns None for System unconditionally, so a System permission on a group grants nothing today either way. What changes is that the sync stops deleting a permission row a user deliberately created, and stops rendering a diff that cannot be settled.

If you would rather System permissions simply not be syncable, the symmetric alternative is to filter them out of convert_user_groups instead. That alone does not fix the revocation, since to_remove compares against the database rather than the exported file, so it would need to_remove to skip System as well. I picked pass-through because it is one branch and keeps export and import agreeing, but say the word and I will invert it.

Verification

cargo check -p komodo_core and cargo check -p komodo_core --tests clean with no new warnings, cargo fmt --check clean. Reproduced the original failure and the fix against a live Core (a sync creating an Action plus a group granting Execute on it: permission missing before, applied after). No test added: bin/core has no test module today, and these paths need a database and the resource cache.

WARN with Color::Red follows the existing precedent in sync/file.rs; formatting::Color has only Red, Green and Blue.