fix missing select for mw-server role

[tpurschke]

• The log-data replacement mutation deletes existing rows with owner_id before inserting the new batch.
• middleware-server had delete permission but no select permission on logging.log_entry; Hasura therefore omitted owner_id from its role-specific boolean-expression type.
• Granting the technical role select access to owner_id enables the scoped delete while retaining the atomic replace operation.

[tpurschke]

Review of PR #5202 — fix missing select for mw-server role

Scope: roles/api/files/replace_metadata.json (+9/-0), base develop. No prior reviews or review threads exist on this PR, so findings start at F1.

The root-cause analysis in the PR description is correct and matches the rest of the metadata: Hasura derives the columns of a role's <table>_bool_exp from that role's select permission, so a role holding only delete cannot filter on any column. The same pattern is visible elsewhere in this file — importer deletes import_control filtered on mgm_id/control_id and import_config filtered on import_id, and it holds select permission on exactly those columns. Granting middleware-server select on logging.log_entry is therefore the right fix, and it is not a privilege escalation: the role already has unrestricted insert/update/delete (filter: {}) on that table, and it is a technical role, not user-assignable.

The fix is, however, incomplete: the column list covers only one of the two where clauses the middleware issues against this table.

Findings

#	Criticality	Status	Subject
F1	high	new	deleteExpiredLogEntries still fails — log_time missing from the new select permission
F2	medium	new	Select column list is a hand-maintained subset of the filtered columns, with no guard against drift
F3	low	new	Malformed indentation }, { in replace_metadata.json

---

F1 — deleteExpiredLogEntries still fails: log_time is missing (high)

roles/api/files/replace_metadata.json, the new select_permissions entry for middleware-server on logging.log_entry, grants ["owner_id"] only. The middleware issues two distinct filtered deletes on that table:

• roles/common/files/fwo-api-calls/logging/replaceLogEntries.graphql:4 and deleteLogEntriesOfOwners.graphql:6 — where: {owner_id: {_in: $ownerIds}} → fixed by this PR.
• roles/common/files/fwo-api-calls/logging/deleteExpiredLogEntries.graphql:2 — where: {log_time: {_lt: $expiryTime}} → still broken, by exactly the same mechanism the PR describes: log_time is not in the role's select columns, so it is not a field of logging_log_entry_bool_exp for middleware-server and the mutation is rejected at validation time.

Impact: LogDataImport.DeleteExpiredEntries() (roles/middleware/files/FWO.Middleware.Server/LogDataImport.cs:400-405) is called unconditionally at the end of every Run() (LogDataImport.cs:38), after the sources have been imported. Its exception propagates out of Run() into ImportLogDataJob.Execute (roles/middleware/files/FWO.Middleware.Server/Jobs/ImportLogDataJob.cs:31-34), which converts it into an AlertCode.ImportLogData alert. So with this PR as it stands, the scheduled log data import raises an error alert on every run even when all sources imported cleanly, and the configured LogDataRetentionDays retention is never enforced — logging.log_entry grows without bound.

Suggested fix: add log_time to the granted columns. Given that the role already holds full write access to the table, aligning the select list with the insert list (log_count, source, destination, service_protocol, service_port, allowed, modelled, log_time, logging_rule_name, owner_id, plus id) is defensible and removes this whole class of breakage; the minimum correct fix is ["log_time", "owner_id"].

F2 — Select column list must be kept in sync with the query filters by hand (medium)

F1 is the second instance of one failure mode within one feature: a where/on_conflict clause in roles/common/files/fwo-api-calls/logging/*.graphql referencing a column the role's select permission does not list. Nothing detects the mismatch — it surfaces only as a runtime GraphQL validation error inside a Quartz job, reported as a generic import alert, which is how F1 escaped the first fix. roles/tests-integration/files/api/ contains only an ad-hoc curl script, and there is no unit test asserting the metadata/query alignment.

This is a maintainability trap rather than a defect in itself, but the next filtered query added against a technical role will hit it again. Nice to have, not a merge blocker: a check that parses the where column references out of the .graphql files under roles/common/files/fwo-api-calls/ and asserts each is covered by the calling role's select permission in replace_metadata.json would turn this class of bug into a build-time failure.

F3 — Malformed indentation in the JSON (low)

The inserted block ends with }, { on a single line, breaking the two-space, one-entry-per-line formatting used throughout the rest of the file. The JSON parses (verified), so this is cosmetic only, but it makes the next diff on this hunk noisy. Please reformat to match the surrounding entries.

Security pass

No issues found. Specifically checked, for code reachable from the diff:

• Privilege scope: the grant is select on a single column with filter: {} for middleware-server, a technical role that already holds unrestricted insert/update/delete on the same table. No escalation, and no new capability for any user-facing role.
• Tenant/owner isolation: unchanged. The owner-scoped filters for the user-facing roles (modeller → x-hasura-editable-owners, recertifier → x-hasura-recertifiable-owners) are untouched by this diff; the auditor role's unfiltered read of logging.log_entry is pre-existing and consistent with its global read-all semantics.
• Injection / input validation: no new query surface, no string-built filters; the affected mutations use typed GraphQL variables.
• Secrets / credentials / TLS / deserialization / SSRF: not touched by this diff.

FWO-specific checks

• Schema/upgrade safety: no SQL and no migration involved. replace_metadata.json is applied wholesale by roles/api/tasks/hasura-install.yml:627 on both fresh install and upgrade, so no separate upgrade step is needed and existing installations pick the change up automatically.
• Localization, UI help content, whats_new_facts: not applicable — this is a permission repair for an existing feature, no user-visible string or feature change.
• C#/Python coding guidelines, type annotations, unit tests: not applicable — no C# or Python code changed. See F2 for the coverage gap this leaves.

Summary and recommendation

The direction is right and the diagnosis is correct, but do not merge as is: F1 leaves half the original bug in place and will make the log data import job alert on every scheduled run while silently disabling retention.

• Must fix before merge: F1 — add log_time (and preferably the full column set) to the new middleware-server select permission.
• Should fix: F3 — reformat the inserted JSON block.
• Nice to have: F2 — an automated alignment check between the API call definitions and the role permissions.

Residual risk: there is no automated test covering Hasura role permissions in this repository, so F1's fix can only be confirmed by running the log data import against a real deployment. Please verify a full ImportLogDataJob run completes without an ImportLogData alert, with LogDataRetentionDays set so that expired rows actually exist.

---

Review depth: standard. The change touches authorization metadata, which normally argues for deep, but at 9 lines in one file the standard pass was already exhaustive over its surface: the complete permission set of logging.log_entry for all roles, every call site of the affected mutations, every filter column used against the table, and two corroborating tables (import_control, import_config) were all inspected. Performed directly against this skill's checklists on the primary model, not via a separate review capability. The security pass and every finding, rating and reconciliation decision were made on the primary model. No checks were delegated to a reduced model tier — the diff was small enough that delegation would have cost more than it saved; the mechanical checks (JSON validity, permission dump, filter-column extraction, prior-review history) were run inline as scripted commands. Usage budget could not be measured: this environment exposes no usage indicator to the agent, so the skill's proxy limits were enforced instead — 0 sub-agent dispatches, standard depth, reads confined to the changed file plus its direct callers and API call definitions; no pass was re-run.

[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