Block stage-replace (RTAS) on locked tables

[dxichen]

Summary

TablesServiceImpl#putTable() has two branches for an existing table:

• The stage-replace branch (CREATE OR REPLACE TABLE ... AS SELECT, i.e. createUpdateTableRequestBody.isStageReplace()), which only calls authorizationUtils.checkReplaceTablePrivilege(...).
• The ordinary update branch, which calls authorizationUtils.checkTableWritePathPrivileges(...) and isTableLocked(tableDto.get()), throwing UnsupportedClientOperationException(LOCKED_TABLE_OPERATION) if the table is locked.

The stage-replace branch never checks isTableLocked. Locking a table is meant to prevent further mutation of its definition (used e.g. during migrations, freezes, or investigations), but a CREATE OR REPLACE TABLE AS SELECT from the table's own creator can silently replace the entire table definition (schema, partitioning, sort order, properties, location) while the table is locked — bypassing the lock entirely.

Fix

Adds the same isTableLocked() check already used elsewhere in this method to the stage-replace branch, before the existing privilege check, throwing the same UnsupportedClientOperationException(LOCKED_TABLE_OPERATION) used by the other lock-enforced code paths in this class.

Testing

Added testStageReplaceBlockedOnLockedTable to TablesServiceTest (e2e/h2 Spring-Boot-backed test), since there was no existing coverage of stage-replace at all in this test class:

• Creates a table with replace.enabled=true.
• Locks it, then asserts a stage-replace putTable(...) call throws UnsupportedClientOperationException.
• Unlocks it, then asserts the same stage-replace call now succeeds.

Ran locally:

• ./gradlew :services:tables:test --tests "com.linkedin.openhouse.tables.e2e.h2.TablesServiceTest" — all 36 tests pass (including the new one), no regressions.
• ./gradlew :services:tables:test (full module) — passes.
• ./gradlew :services:tables:spotlessJavaCheck — passes.

Notes

This is a draft PR — happy to iterate on this based on review.

[dxichen]

Self-review notes while this is in draft:

1. Placement of the lock check: I put isTableLocked before checkReplaceTablePrivilege (privilege check second). This matches the ordering precedent of deleteTable... actually checking, the adjacent update branch does checkIfLockPoliciesUpdated → isTableLocked → checkTableWritePathPrivileges, i.e. lock check also runs before privilege check there. So the ordering here is consistent. ✅.

2. Related gap I noticed but did NOT fix here (scoping decision): the update branch also calls checkIfLockPoliciesUpdated(tableDto.get(), createUpdateTableRequestBody) before the lock check, which rejects requests that try to sneak in a lock-state change disguised as a metadata update (see lines ~130, 202-213). The stage-replace branch calls neither this nor any equivalent. Since CreateUpdateTableRequestBody.getPolicies() is also settable on a stage-replace request, in principle a stage-replace request could carry a lockState change alongside the schema replacement. I think this is currently harmless because isTableLocked(tableDto.get()) (checking the existing persisted table's lock state, not the request's) will already reject the whole operation if the table is locked — so you can't lock-and-replace in one shot. But if the table is unlocked, a stage-replace request that also flips lockState.locked=true in the same call isn't validated by checkIfLockPoliciesUpdated the way a normal update is. Wanted to flag this explicitly rather than silently expand scope — happy to add the same checkIfLockPoliciesUpdated call here too if maintainers think it's warranted, but wanted to keep this PR minimal/focused on the concrete lock-bypass bug first.

3. Test coverage: current test (testStageReplaceBlockedOnLockedTable) covers: (a) locked table blocks stage-replace, (b) unlocked table allows it. I did not add a test for the policies-smuggling scenario in point 2 since I'm not fixing it in this PR — can add if we decide to expand scope.

4. Ran full :services:tables:test module (not just the one test class) plus spotlessJavaCheck locally — all green, no regressions.

[dxichen]

Update: addressed point 2 from my self-review above. Pushed a second commit that adds the missing checkIfLockPoliciesUpdated(...) call to the stage-replace branch (mirroring the ordinary update branch), plus a new regression test testStageReplaceCannotSmuggleLockStateChange confirming a stage-replace request can no longer smuggle a lock-state change past the dedicated createLock/deleteLock APIs.

Re-ran locally after this change:

• :services:tables:test --tests "...TablesServiceTest" — 37/37 pass (both new tests + no regressions in the other 35).
• Full :services:tables:test module — passes.
• :services:tables:spotlessJavaCheck — passes (had to run spotlessApply once to fix formatting on the new test, now clean).

PR is now 2 commits / 2 files changed / +69 lines. Still in draft, will keep iterating as CI and review come in.

[dxichen]

✅ CI passed (build-run-tests / Build and Run Tests, 23m19s) on the full repo build for both commits. PR remains open, draft, and mergeable. Ready for maintainer review whenever convenient — will keep iterating here based on feedback.