NO-ISSUE: Tighten tenant creation restrictions

[jhernand]

Summary

• Remove the Create, Update and Delete operations from the public organizations API, returning
  Unimplemented for those RPCs, so that tenants can only be managed through the private API.
• Enforce in the private server that tenants use themselves as their own identifier and tenant
  (metadata.name is mandatory, id defaults to/must equal the name, metadata.tenant defaults
  to/must equal the name).
• Enforce immutability of the name and tenant columns at the database level using a PL/pgSQL
  trigger function check_immutable_columns. The trigger raises a custom Z0001 SQLSTATE with the
  changed column names as a JSON array in the detail field. The DAO translates this into an
  ErrImmutable error, and the generic server maps it to a gRPC InvalidArgument status.
• Update tests accordingly: migration tests verify the trigger exists, DAO tests exercise the
  immutability enforcement and error translation, and server tests cover the validation rules.

These constraints prepare the ground for introducing a foreign key on the tenant column in a later
patch, which will require every tenant row to reference itself consistently.

Test plan

• Unit tests pass (ginkgo run -r internal)
• Verify that the public API rejects create/update/delete with Unimplemented
• Verify that the private API rejects invalid tenant names and mismatched ids
• Verify that the database trigger rejects updates to immutable columns
• Verify that the DAO translates Z0001 errors into ErrImmutable
• Verify that the generic server returns InvalidArgument for immutable field updates

Summary by CodeRabbit

• Breaking Changes

  • Public organizations API no longer supports Create, Update, or Delete; only List and Get remain.

• Improvements

  • Private/administrative pathway now enforces tenant/name validation on create and prevents changing those fields.
  • Database enforces immutability for organization name and tenant; violating updates return clear validation errors.

• Tests

  • Expanded coverage for immutability, validation, and public vs. private behaviors, including negative cases.

[openshift-ci-robot]

@jhernand: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

• Remove the Create, Update and Delete operations from the public organizations API, returning
  Unimplemented for those RPCs, so that tenants can only be managed through the private API.
• Enforce in the private server that tenants use themselves as their own identifier and tenant
  (metadata.name is mandatory, id defaults to/must equal the name, metadata.tenant defaults
  to/must equal the name), and make both metadata.name and metadata.tenant immutable on update.
• Update tests accordingly and switch to auth.AllTenants/auth.SystemTenant constants.

These constraints prepare the ground for introducing a foreign key on the tenant column in a later
patch, which will require every tenant row to reference itself consistently.

Test plan

• Unit tests pass (ginkgo run -r internal)
• Verify that the public API rejects create/update/delete with Unimplemented
• Verify that the private API rejects invalid tenant names, mismatched ids, and immutable field updates

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 6f6521b6-d782-48d5-9504-8e6f558c4429

📥 Commits

Reviewing files that changed from the base of the PR and between 1096739 and 558a149.

📒 Files selected for processing (13)

• internal/database/dao/dao_errors.go
• internal/database/dao/dao_errors_test.go
• internal/database/dao/dao_immutability_test.go
• internal/database/dao/generic_dao_immutability_test.go
• internal/database/dao/generic_dao_update.go
• internal/database/migrations/46_add_immutable_column_trigger.up.sql
• internal/database/migrations/46_add_immutable_column_trigger_test.go
• internal/servers/generic_server.go
• internal/servers/organizations_server.go
• internal/servers/organizations_server_test.go
• internal/servers/private_organizations_server.go
• internal/servers/private_organizations_server_test.go
• internal/servers/servers_suite_test.go

💤 Files with no reviewable changes (1)

• internal/servers/organizations_server.go

---

Walkthrough

Public Organizations server write RPCs removed; PrivateOrganizationsServer builds a typed DAO, validates tenant metadata (mandatory name, id/tenant defaulting), and rejects immutable-field changes. A DB trigger enforces immutability; DAO maps trigger errors to ErrImmutable and generic server returns gRPC InvalidArgument. Tests and migration added.

Changes

Public/Private Organizations Server Separation with Tenancy Validation

Layer / File(s)	Summary
Public Organizations Server Reduction to Read-Only internal/servers/organizations_server.go, internal/servers/organizations_server_test.go	Public OrganizationsServer removes Create, Update, and Delete implementations; tests now provision data via PrivateOrganizationsServer and assert public write RPCs return Unimplemented.
Private Organizations Server DAO Wiring and Tenant Validation internal/servers/private_organizations_server.go, internal/servers/private_organizations_server_test.go	PrivateOrganizationsServer gains a typed *dao.GenericDAO[*privatev1.Organization] built in Build(). Create() enforces non-empty metadata.name, auto-defaults id/metadata.tenant to that name, and rejects mismatches with gRPC InvalidArgument. Tests cover tenant-focused CRUD and validation/defaulting cases.
DAO ErrImmutable and Database Trigger Migration internal/database/dao/dao_errors.go, internal/database/migrations/46_add_immutable_column_trigger.up.sql, internal/database/dao/dao_immutability_test.go, internal/database/migrations/46_add_immutable_column_trigger_test.go, internal/database/dao/dao_errors_test.go	Adds exported ErrImmutable with deterministic formatting; introduces a PL/pgSQL check_immutable_columns() function and BEFORE UPDATE trigger on organizations enforcing immutability for name and tenant; tests verify trigger behavior and migration presence.
GenericDAO Update Error Translation and Immutability Tests internal/database/dao/generic_dao_update.go, internal/database/dao/generic_dao_immutability_test.go	Add UpdateRequest.translateError to map PostgreSQL unique-violation to ErrAlreadyExists and trigger errors to ErrImmutable (parsing JSON detail), with warning logs for malformed detail. Add GenericDAO tests asserting Update() rejects immutable-field changes and allows permitted updates.
Generic Server Error Mapping & Test Suite Tenancy Alignment internal/servers/generic_server.go, internal/servers/servers_suite_test.go	Generic server maps dao.ErrImmutable to gRPC InvalidArgument. Test-suite tenancy gomock expectations updated to return auth.AllTenants and auth.SystemTenant.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PrivateOrgServer
  participant GenericDAO
  participant Postgres
  Client->>PrivateOrgServer: Create OrganizationsCreateRequest(metadata.name)
  PrivateOrgServer->>PrivateOrgServer: validate metadata.name, default id/tenant
  PrivateOrgServer->>GenericDAO: Create Organization
  GenericDAO->>Postgres: INSERT
  Postgres-->>GenericDAO: OK
  GenericDAO-->>PrivateOrgServer: CreateResponse
  PrivateOrgServer-->>Client: CreateResponse
  Client->>PrivateOrgServer: Update (change metadata.name)
  PrivateOrgServer->>GenericDAO: Update
  GenericDAO->>Postgres: UPDATE (trigger enforces immutable)
  Postgres-->>GenericDAO: ERROR (errImmutableCode, detail JSON)
  GenericDAO->>GenericDAO: translateError(pgErr) -> ErrImmutable
  GenericDAO-->>PrivateOrgServer: ErrImmutable
  PrivateOrgServer-->>Client: gRPC InvalidArgument (ErrImmutable message)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• osac-project/fulfillment-service#439: Related tenancy mock/test alignment changes.
• osac-project/fulfillment-service#480: Overlapping work on GenericServer update error mappings.

Suggested reviewers

• CrystalChun
• eranco74
• trewest
• adriengentil

Security Considerations

Severity: MEDIUM — Impact: Public write surface reduced; immutable-field enforcement moves to DB/DAO layer. Verify private-server authorization boundaries remain strict and that translated DB error messages do not leak unexpected internal details.

Poem

🛡️ Read-only doors now stand in line,
Private halls keep tenant name divine.
Triggers guard the unmovable key,
DAO and server speak consistently.
Tests confirm the rules align.

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly reflects the main change: tightening tenant creation restrictions by removing public API mutation operations and enforcing invariants on the private API.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets or credentials detected across 13 modified files. The $$ markers in SQL migration are standard PostgreSQL PL/pgSQL syntax.
No-Weak-Crypto	✅ Passed	No weak crypto algorithms, custom implementations, or insecure comparisons found. PR addresses tenant validation, immutability enforcement, and API restrictions—not cryptography.
No-Injection-Vectors	✅ Passed	No injection vectors detected. SQL uses parameterized queries, column names hardcoded, table names derived from protobuf descriptors, JSON unmarshaling safe, no eval/exec/pickle/yaml/shell patterns.
Container-Privileges	✅ Passed	No container or Kubernetes manifests were modified in this PR. Changes are exclusively Go code (servers, DAO, migrations) for tenant restrictions—no privileged settings introduced.
No-Sensitive-Data-In-Logs	✅ Passed	All logging is security-safe: includes only schema names, SQLSTATE codes, object IDs, and error messages. No passwords, tokens, API keys, PII, or customer data are exposed.
Ai-Attribution	✅ Passed	Commit includes proper AI attribution with 'Assisted-by: Cursor' trailer and no improper Co-Authored-By for AI tools; AI tool use is clearly documented.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/servers/private_organizations_server.go`:
- Around line 187-203: The code currently calls s.generic.Update(...) and
persists the change before checking immutable fields; move the immutability
checks so they run before invoking s.generic.Update: compare metadataBefore
(existing object's metadata) with the incoming request/object metadata to ensure
metadata.name and metadata.tenant are unchanged, and only call s.generic.Update
if both checks pass; reference the metadataBefore/metadataAfter comparison logic
and the s.generic.Update call (and the OrganizationUpdate request handling in
private_organizations_server.go) to locate and adjust the flow.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 0cf51149-28c9-4aae-91ca-a16d121242d3

📥 Commits

Reviewing files that changed from the base of the PR and between 3dd2c58 and 961109a.

📒 Files selected for processing (5)

• internal/servers/organizations_server.go
• internal/servers/organizations_server_test.go
• internal/servers/private_organizations_server.go
• internal/servers/private_organizations_server_test.go
• internal/servers/servers_suite_test.go

💤 Files with no reviewable changes (1)

• internal/servers/organizations_server.go

[CrystalChun]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CrystalChun, jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jhernand]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jhernand]

/override ci/prow/e2e-vmaas

[openshift-ci]

@jhernand: Overrode contexts on behalf of jhernand: ci/prow/e2e-vmaas

Details

In response to this:

/override ci/prow/e2e-vmaas

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.