carverauto/serviceradar

Fork 0

Updates/multi tenant cleanup #2678

Merged

mfreeman451 merged 6 commits from refs/pull/2678/head into staging

2026-01-15 18:20:02 +00:00

mfreeman451 commented

2026-01-15 07:35:43 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2317
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2317
Original created: 2026-01-15T07:35:43Z
Original updated: 2026-01-15T18:20:06Z
Original head: carverauto/serviceradar:updates/multi-tenant-cleanup
Original base: staging
Original merged: 2026-01-15T18:20:02Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Enhancement, Documentation

Description

Replace authorize?: false with explicit SystemActor usage across web-ng codebase
Consolidate hardcoded system_actor definitions to use ServiceRadar.Actors.SystemActor
Add comprehensive OpenSpec proposal for breaking out SaaS Control Plane from tenant runtime
Implement tenant-aware and platform-level actor patterns for authorization

Diagram Walkthrough

flowchart LR
  A["web-ng Code<br/>authorize?: false"] -->|Phase 1| B["Explicit SystemActor<br/>Usage"]
  B -->|Phase 2| C["Control Plane<br/>Separation"]
  C -->|Phase 3| D["JWT-Based<br/>Authorization"]
  D -->|Phase 4| E["Clean Single-Tenant<br/>OSS Deployment"]
  
  F["Hardcoded<br/>system_actor"] -->|Consolidate| B
  G["Cross-Tenant<br/>Queries"] -->|Move to CP| C
  H["TenantMembership<br/>Lookups"] -->|Replace with JWT| D

File Walkthrough

Relevant files

Enhancement

24 files

collector_controller.ex `Replace authorize?: false with SystemActor pattern`	+27/-10
onboarding_packages.ex `Convert to tenant-aware SystemActor usage`	+19/-20
index.ex `Add SystemActor for tenant and platform operations`	+21/-7
index.ex `Implement tenant and platform actor patterns`	+18/-5
show.ex `Replace authorize?: false with explicit actors`	+20/-6
nats_controller.ex `Add platform SystemActor for NATS operations`	+16/-5
index.ex `Use tenant-scoped SystemActor for integration queries`	+11/-5
onboarding_events.ex `Convert to tenant-aware SystemActor pattern`	+14/-10
api_auth.ex `Replace authorize?: false with platform SystemActor`	+11/-4
device_controller.ex `Add platform SystemActor for device operations`	+10/-3
index.ex `Implement platform and tenant actor patterns`	+10/-3
expire_packages_worker.ex `Use platform SystemActor for cross-tenant package expiry`	+9/-12
edge_controller.ex `Add platform SystemActor for edge operations`	+8/-4
enroll_controller.ex `Use platform SystemActor for enrollment operations`	+7/-2
scope.ex `Replace authorize?: false with platform SystemActor`	+6/-2
show.ex `Remove authorize?: false from NATS operations`	+3/-2
user_auth.ex `Use platform SystemActor for JWT validation`	+4/-1
show.ex `Add tenant-scoped SystemActor for agent queries`	+4/-1
tenant_controller.ex `Use platform SystemActor for tenant operations`	+3/-1
index.ex `Add platform SystemActor for tenant lookup`	+3/-1
register.ex `Use platform SystemActor for tenant creation`	+2/-1
tenant_context.ex `Replace authorize?: false with platform SystemActor`	+4/-1
tenant_resolver.ex `Replace hardcoded system_actor with SystemActor.platform`	+2/-7
index.ex `Remove authorize?: false from update operation`	+1/-1

Documentation

5 files

tasks.md `Comprehensive task breakdown for Control Plane separation`	+302/-0
design.md `Detailed design for tenant isolation and Control Plane architecture`	+238/-0
proposal.md `OpenSpec proposal for breaking out SaaS Control Plane`	+161/-0
spec.md `Specification requirements for tenant isolation`	+112/-0
spec.md `Specification for SystemActor authorization pattern`	+65/-0

Additional files

2 files

infrastructure.ex	+0/-146
inventory.ex	+0/-131

Imported from GitHub pull request. Original GitHub pull request: #2317 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2317 Original created: 2026-01-15T07:35:43Z Original updated: 2026-01-15T18:20:06Z Original head: carverauto/serviceradar:updates/multi-tenant-cleanup Original base: staging Original merged: 2026-01-15T18:20:02Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Enhancement, Documentation ___ ### **Description** - Replace `authorize?: false` with explicit `SystemActor` usage across web-ng codebase - Consolidate hardcoded system_actor definitions to use `ServiceRadar.Actors.SystemActor` - Add comprehensive OpenSpec proposal for breaking out SaaS Control Plane from tenant runtime - Implement tenant-aware and platform-level actor patterns for authorization ___ ### Diagram Walkthrough ```mermaid flowchart LR A["web-ng Code authorize?: false"] -->|Phase 1| B["Explicit SystemActor Usage"] B -->|Phase 2| C["Control Plane Separation"] C -->|Phase 3| D["JWT-Based Authorization"] D -->|Phase 4| E["Clean Single-Tenant OSS Deployment"] F["Hardcoded system_actor"] -->|Consolidate| B G["Cross-Tenant Queries"] -->|Move to CP| C H["TenantMembership Lookups"] -->|Replace with JWT| D ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><details><summary>24 files</summary><table> <tr> <td>collector_controller.ex<dd><code>Replace authorize?: false with SystemActor pattern</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-c3752db9bdbf3d914b0032e698ba69bdd077a86694275b43b629d9905c8e2156">+27/-10</a>  </td> </tr> <tr> <td>onboarding_packages.ex<dd><code>Convert to tenant-aware SystemActor usage</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-7e3946df4d1a246f68721125430e5256bb69b2711cd7ac2562b3d94a1084a8f2">+19/-20</a>  </td> </tr> <tr> <td>index.ex<dd><code>Add SystemActor for tenant and platform operations</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-a4bff6d93de18fa438c93bbc1dff8344d9886745d6922df3952139b37d01e495">+21/-7</a>    </td> </tr> <tr> <td>index.ex<dd><code>Implement tenant and platform actor patterns</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-8b6f7b4c958acfb5ba26524427695d750cf7cf441e45ae449413c03b4a166fb1">+18/-5</a>    </td> </tr> <tr> <td>show.ex<dd><code>Replace authorize?: false with explicit actors</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-2c15bfed2ddefa02ac6fda1c9ac16dde652f6945015a8d9a15df2b37faeb88cf">+20/-6</a>    </td> </tr> <tr> <td>nats_controller.ex<dd><code>Add platform SystemActor for NATS operations</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-d2a1739ac2b8778f5269ae65a891a8537a44721986bff8cc1824a2e122306a0a">+16/-5</a>    </td> </tr> <tr> <td>index.ex<dd><code>Use tenant-scoped SystemActor for integration queries</code>        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-61d0262af13a42905ebbd793e83537e61bfc09493df1664a82eb2536980ee1cd">+11/-5</a>    </td> </tr> <tr> <td>onboarding_events.ex<dd><code>Convert to tenant-aware SystemActor pattern</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-29467451a9eadcfa1c27005ce86b5472254b7e908cd5e0e8a66b3dbaafd81770">+14/-10</a>  </td> </tr> <tr> <td>api_auth.ex<dd><code>Replace authorize?: false with platform SystemActor</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-c47fca520429562a380c88ddaea56483dd328e09f17733251ce1afe520b7e976">+11/-4</a>    </td> </tr> <tr> <td>device_controller.ex<dd><code>Add platform SystemActor for device operations</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-ac55b43afa10dd331fa712def4e0af4608417e58727fbde08a6702ef180e46d4">+10/-3</a>    </td> </tr> <tr> <td>index.ex<dd><code>Implement platform and tenant actor patterns</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-ff336831554c0d0fe48b38caa1cc8d072531859f084c6f4d1638aa349c45adb8">+10/-3</a>    </td> </tr> <tr> <td>expire_packages_worker.ex<dd><code>Use platform SystemActor for cross-tenant package expiry</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-fc82319bea897abfec5e6ff582179ddf3d4e87d67d6b407dd7df98a1e3d6a521">+9/-12</a>    </td> </tr> <tr> <td>edge_controller.ex<dd><code>Add platform SystemActor for edge operations</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-05935a3b7cbb6dccb1dd5635980102662534d3cd929d751af33160f5c6714935">+8/-4</a>      </td> </tr> <tr> <td>enroll_controller.ex<dd><code>Use platform SystemActor for enrollment operations</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-d6a38ddd0262e27653bc95f472a2ab5b5189b69ca62d4c085aa63013b21b7573">+7/-2</a>      </td> </tr> <tr> <td>scope.ex<dd><code>Replace authorize?: false with platform SystemActor</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-8142f5d0719ccc41761eee96fa01036051e73b305766b3bb09e0c36945ef84dd">+6/-2</a>      </td> </tr> <tr> <td>show.ex<dd><code>Remove authorize?: false from NATS operations</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-80f438b5553fe21b91dcd1ccad6fcfae91b24d110e73cedf7a83e8325a786ec3">+3/-2</a>      </td> </tr> <tr> <td>user_auth.ex<dd><code>Use platform SystemActor for JWT validation</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-eed904963eb2089bfce4e634d5229889213b12d02489e07af53f1b1982a42d78">+4/-1</a>      </td> </tr> <tr> <td>show.ex<dd><code>Add tenant-scoped SystemActor for agent queries</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-5e622205d1abddd8ad7dcf7a8ca1be583804d622d3d38b75140e9b909cf0534a">+4/-1</a>      </td> </tr> <tr> <td>tenant_controller.ex<dd><code>Use platform SystemActor for tenant operations</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-57154a548449e2a4b1b8d04bf01810e189e96c82b5c5e4dfc38eb60b297c1b4a">+3/-1</a>      </td> </tr> <tr> <td>index.ex<dd><code>Add platform SystemActor for tenant lookup</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-d0d22a987aa942ef25088696daea862e36bd8ca083ce29e4670f63e62947c62f">+3/-1</a>      </td> </tr> <tr> <td>register.ex<dd><code>Use platform SystemActor for tenant creation</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-e1cb648c75f620fa25384feb7a7829a777cecebe7c31d37e0718ac3579e6fc58">+2/-1</a>      </td> </tr> <tr> <td>tenant_context.ex<dd><code>Replace authorize?: false with platform SystemActor</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-43b9067433609fbe694a7687be916c7d58b49008fbd5036329eb8c2fbbfd018d">+4/-1</a>      </td> </tr> <tr> <td>tenant_resolver.ex<dd><code>Replace hardcoded system_actor with SystemActor.platform</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-21cbb365b4e282a24cc832aea0ff8dfdaffe2b79f6a6acc704300e76ae359828">+2/-7</a>      </td> </tr> <tr> <td>index.ex<dd><code>Remove authorize?: false from update operation</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-ce489a06ca328b705897a7b71749c6519594920a192aa0d033944a046d743ef0">+1/-1</a>      </td> </tr> </table></details></td></tr><tr><td>Documentation</td><td><details><summary>5 files</summary><table> <tr> <td>tasks.md<dd><code>Comprehensive task breakdown for Control Plane separation</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-5b1d1e0ef4f2e033743bbab5ae7dc4a22fc09722980e7dd451699163e1ed2fbb">+302/-0</a>  </td> </tr> <tr> <td>design.md<dd><code>Detailed design for tenant isolation and Control Plane architecture</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-68f957d5c129996ba6d9511bb3553fae43815dcd7fdfaca1daeb076f0fc3b2da">+238/-0</a>  </td> </tr> <tr> <td>proposal.md<dd><code>OpenSpec proposal for breaking out SaaS Control Plane</code>        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-dfb7b8bed72ce4e3b0f45591b6cd19a396dc694601fa98453859d5a1954338ab">+161/-0</a>  </td> </tr> <tr> <td>spec.md<dd><code>Specification requirements for tenant isolation</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-86e199d1ed7854365473ff0d68413f23afcf79d9ee27c1f8a4a213b86d6f2730">+112/-0</a>  </td> </tr> <tr> <td>spec.md<dd><code>Specification for SystemActor authorization pattern</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-d243d447a51e5efb8101404254ee4dbe968641ec7a5e30945b60baac7a719501">+65/-0</a>    </td> </tr> </table></details></td></tr><tr><td>Additional files</td><td><details><summary>2 files</summary><table> <tr> <td>infrastructure.ex</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-eb9ad11c35a6b0afdb349dc9e4ad826dc4da2f355b0345d40e3ade6dfaa9b131">+0/-146</a>  </td> </tr> <tr> <td>inventory.ex</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2317/files#diff-a3a4fd8769b4d293500c8ae003aca2417bc7c239ff7b51f601214f589d43cace">+0/-131</a>  </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-01-15 07:36:30 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2317#issuecomment-3753259671
Original created: 2026-01-15T07:36:30Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Overprivileged system actor Description: Multiple actions now execute with `SystemActor.platform(:nats_controller)` (e.g., generating `NatsPlatformToken`, listing tenants, reading/creating `NatsOperator`), which is highly privileged and could enable platform-wide token/operator/tenant data access if these endpoints are reachable without strict platform-admin authentication/authorization enforcement. nats_controller.ex [34-257] Referred Code `actor = SystemActor.platform(:nats_controller) case NatsPlatformToken \|> Ash.Changeset.for_create(:generate, %{ purpose: :nats_bootstrap, expires_at: expires_at }) \|> Ash.create(actor: actor) do {:ok, token_record} -> conn \|> put_status(:created) \|> json(%{ token: token_record.token_secret, expires_at: DateTime.to_iso8601(token_record.expires_at) }) {:error, changeset} -> {:error, changeset} end end ... (clipped 203 lines)`
	Missing tenant context Description: `update_site/2` and `regenerate_config/1` now perform `Ash.update` using `SystemActor.platform(:edge_sites_live)` but without passing a tenant context, which can potentially cause cross-tenant or wrong-tenant updates for tenant-scoped resources depending on Ash multitenancy configuration defaults. show.ex [493-508] Referred Code `defp update_site(site, attrs) do # Note: actor should be passed from calling context for proper authorization actor = SystemActor.platform(:edge_sites_live) site \|> Ash.Changeset.for_update(:update, attrs) \|> Ash.update(actor: actor) end defp regenerate_config(leaf_server) do actor = SystemActor.platform(:edge_sites_live) leaf_server \|> Ash.Changeset.for_update(:reprovision, %{}) \|> Ash.update(actor: actor) end`
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Over-privileged actor: Tenant-scoped updates are executed using `SystemActor.platform/1`, which risks bypassing tenant isolation/authorization expectations for tenant resources. Referred Code `# Note: actor should be passed from calling context for proper authorization actor = SystemActor.platform(:edge_sites_live) site \|> Ash.Changeset.for_update(:update, attrs) \|> Ash.update(actor: actor) end defp regenerate_config(leaf_server) do actor = SystemActor.platform(:edge_sites_live) leaf_server \|> Ash.Changeset.for_update(:reprovision, %{}) \|> Ash.update(actor: actor)` Learn more about managing compliance generic rules or creating your own custom rules
⚪
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2317#issuecomment-3753259671 Original created: 2026-01-15T07:36:30Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary>Overprivileged system actor </summary> Description: Multiple actions now execute with <code>SystemActor.platform(:nats_controller)</code> (e.g., generating <code>NatsPlatformToken</code>, listing tenants, reading/creating <code>NatsOperator</code>), which is highly privileged and could enable platform-wide token/operator/tenant data access if these endpoints are reachable without strict platform-admin authentication/authorization enforcement. <a href='https://github.com/carverauto/serviceradar/pull/2317/files#diff-d2a1739ac2b8778f5269ae65a891a8537a44721986bff8cc1824a2e122306a0aR34-R257'>nats_controller.ex [34-257]</a> <details open><summary>Referred Code</summary> ```elixir actor = SystemActor.platform(:nats_controller) case NatsPlatformToken |> Ash.Changeset.for_create(:generate, %{ purpose: :nats_bootstrap, expires_at: expires_at }) |> Ash.create(actor: actor) do {:ok, token_record} -> conn |> put_status(:created) |> json(%{ token: token_record.token_secret, expires_at: DateTime.to_iso8601(token_record.expires_at) }) {:error, changeset} -> {:error, changeset} end end ... (clipped 203 lines) ``` </details></details></td></tr> <tr><td><details><summary>Missing tenant context </summary> Description: <code>update_site/2</code> and <code>regenerate_config/1</code> now perform <code>Ash.update</code> using <code>SystemActor.platform(:edge_sites_live)</code> but without passing a tenant context, which can potentially cause cross-tenant or wrong-tenant updates for tenant-scoped resources depending on Ash multitenancy configuration defaults. <a href='https://github.com/carverauto/serviceradar/pull/2317/files#diff-2c15bfed2ddefa02ac6fda1c9ac16dde652f6945015a8d9a15df2b37faeb88cfR493-R508'>show.ex [493-508]</a> <details open><summary>Referred Code</summary> ```elixir defp update_site(site, attrs) do # Note: actor should be passed from calling context for proper authorization actor = SystemActor.platform(:edge_sites_live) site |> Ash.Changeset.for_update(:update, attrs) |> Ash.update(actor: actor) end defp regenerate_config(leaf_server) do actor = SystemActor.platform(:edge_sites_live) leaf_server |> Ash.Changeset.for_update(:reprovision, %{}) |> Ash.update(actor: actor) end ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>🔴</td> <td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2317/files#diff-2c15bfed2ddefa02ac6fda1c9ac16dde652f6945015a8d9a15df2b37faeb88cfR494-R507'>Over-privileged actor</a>: Tenant-scoped updates are executed using <code>SystemActor.platform/1</code>, which risks bypassing tenant isolation/authorization expectations for tenant resources. <details open><summary>Referred Code</summary> ```elixir # Note: actor should be passed from calling context for proper authorization actor = SystemActor.platform(:edge_sites_live) site |> Ash.Changeset.for_update(:update, attrs) |> Ash.update(actor: actor) end defp regenerate_config(leaf_server) do actor = SystemActor.platform(:edge_sites_live) leaf_server |> Ash.Changeset.for_update(:reprovision, %{}) |> Ash.update(actor: actor) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>⚪</td> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2026-01-15 07:37:41 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2317#issuecomment-3753263567
Original created: 2026-01-15T07:37:41Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Use tenant-scoped actor for updates In `update_site/2`, replace the platform-level `SystemActor` with a tenant-scoped one. Pass the `socket` to the function to retrieve the tenant ID and create a more restrictive actor. web-ng/lib/serviceradar_web_ng_web/live/admin/edge_sites_live/show.ex [493-500] `-defp update_site(site, attrs) do - # Note: actor should be passed from calling context for proper authorization - actor = SystemActor.platform(:edge_sites_live) +defp update_site(site, attrs, socket) do + tenant_id = get_tenant_id(socket) + actor = SystemActor.for_tenant(tenant_id, :edge_sites_live) site \|> Ash.Changeset.for_update(:update, attrs) \|> Ash.update(actor: actor) end` Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that a platform-level actor is used for a tenant-specific operation, which violates the principle of least privilege and the PR's main goal. Using a tenant-scoped actor as proposed is a significant security improvement.	Medium
Possible issue	Remove authorization bypass in update Remove the `authorize?: false` option from the `Ash.update/2` call. The operation should rely on the `actor` already present in the changeset for authorization. web-ng/lib/serviceradar_web_ng_web/live/settings/rules_live/index.ex [75-81] `changeset = Ash.Changeset.for_update(rule, :update, %{enabled: !rule.enabled}, tenant: scope.tenant_schema, actor: %{tenant_id: scope.tenant_id, role: :admin} ) -case Ash.update(changeset, authorize?: false) do +case Ash.update(changeset) do` `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies and fixes a remaining `authorize?: false` call, which is a security risk and directly contradicts the main goal of the PR. This is a critical fix for security and consistency.	Medium
Possible issue	Use actual actor context In `expire_package/2`, pass the `actor` variable to `RecordEventWorker.enqueue/3` instead of the hardcoded string `"system"`. This ensures the authorization context is correctly passed to the background job. web-ng/lib/serviceradar_web_ng/edge/workers/expire_packages_worker.ex [69-72] `RecordEventWorker.enqueue(package.id, :expired, - actor: "system", + actor: actor, details: %{reason: "tokens_expired"} )` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that a hardcoded `"system"` string is used for an actor, which is inconsistent with the PR's pattern of passing explicit actor structs. Using the `actor` variable improves consistency and ensures proper authorization context is propagated to the background worker.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2317#issuecomment-3753263567 Original created: 2026-01-15T07:37:41Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>Security</td> <td> <details><summary>Use tenant-scoped actor for updates</summary> ___ **In <code>update_site/2</code>, replace the platform-level <code>SystemActor</code> with a tenant-scoped one. Pass the <code>socket</code> to the function to retrieve the tenant ID and create a more restrictive actor.** [web-ng/lib/serviceradar_web_ng_web/live/admin/edge_sites_live/show.ex [493-500]](https://github.com/carverauto/serviceradar/pull/2317/files#diff-2c15bfed2ddefa02ac6fda1c9ac16dde652f6945015a8d9a15df2b37faeb88cfR493-R500) ```diff -defp update_site(site, attrs) do - # Note: actor should be passed from calling context for proper authorization - actor = SystemActor.platform(:edge_sites_live) +defp update_site(site, attrs, socket) do + tenant_id = get_tenant_id(socket) + actor = SystemActor.for_tenant(tenant_id, :edge_sites_live) site |> Ash.Changeset.for_update(:update, attrs) |> Ash.update(actor: actor) end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that a platform-level actor is used for a tenant-specific operation, which violates the principle of least privilege and the PR's main goal. Using a tenant-scoped actor as proposed is a significant security improvement. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Remove authorization bypass in update</summary> ___ **Remove the <code>authorize?: false</code> option from the <code>Ash.update/2</code> call. The operation should rely on the <code>actor</code> already present in the changeset for authorization.** [web-ng/lib/serviceradar_web_ng_web/live/settings/rules_live/index.ex [75-81]](https://github.com/carverauto/serviceradar/pull/2317/files#diff-ce489a06ca328b705897a7b71749c6519594920a192aa0d033944a046d743ef0R75-R81) ```diff changeset = Ash.Changeset.for_update(rule, :update, %{enabled: !rule.enabled}, tenant: scope.tenant_schema, actor: %{tenant_id: scope.tenant_id, role: :admin} ) -case Ash.update(changeset, authorize?: false) do +case Ash.update(changeset) do ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies and fixes a remaining `authorize?: false` call, which is a security risk and directly contradicts the main goal of the PR. This is a critical fix for security and consistency. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Use actual actor context</summary> ___ **In <code>expire_package/2</code>, pass the <code>actor</code> variable to <code>RecordEventWorker.enqueue/3</code> instead of the hardcoded string <code>"system"</code>. This ensures the authorization context is correctly passed to the background job.** [web-ng/lib/serviceradar_web_ng/edge/workers/expire_packages_worker.ex [69-72]](https://github.com/carverauto/serviceradar/pull/2317/files#diff-fc82319bea897abfec5e6ff582179ddf3d4e87d67d6b407dd7df98a1e3d6a521R69-R72) ```diff RecordEventWorker.enqueue(package.id, :expired, - actor: "system", + actor: actor, details: %{reason: "tokens_expired"} ) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that a hardcoded `"system"` string is used for an actor, which is inconsistent with the PR's pattern of passing explicit actor structs. Using the `actor` variable improves consistency and ensures proper authorization context is propagated to the background worker. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>

mfreeman451 commented

2026-01-15 07:37:43 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2317#discussion_r2693291909
Original created: 2026-01-15T07:37:43Z
Original path: web-ng/lib/serviceradar_web_ng_web/controllers/api/collector_controller.ex
Original line: 13

I don't really understand why we have to do this at all anymore, we need to be using tenant specific CNPG credentials that only has access to their schema

Imported GitHub PR review comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2317#discussion_r2693291909 Original created: 2026-01-15T07:37:43Z Original path: web-ng/lib/serviceradar_web_ng_web/controllers/api/collector_controller.ex Original line: 13 --- I don't really understand why we have to do this at all anymore, we need to be using tenant specific CNPG credentials that only has access to their schema