[fix](binlog) Require SELECT privilege for binlog TVF - #68088
Merged
morningman merged 3 commits intoSep 20, 2026
Merged
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: The binlog() table-valued function resolved and scanned its target table without checking the caller table privileges. A restricted user could therefore read row-binlog content from a table it could not query directly. Check SELECT privilege before loading table metadata so unauthorized calls return the standard table access-denied error.
### Release note
The binlog() table-valued function now requires SELECT privilege on its target table.
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.tablefunction.TableBinlogFunctionAuthTest
- Added a regression test and validated its Groovy syntax locally; cluster execution is left to CI
- Behavior changed: Yes. Unauthorized users can no longer query row-binlog content through binlog().
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: The binlog() TVF privilege check should participate in the standard TVF privilege-check phase. Performing authorization in the constructor couples access control to metadata initialization and bypasses the checkAuth contract. Override checkAuth(ConnectContext) to require SELECT privilege and keep construction authorization-free.
### Release note
The binlog() TVF now performs its SELECT privilege check through the standard TVF checkAuth hook.
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.tablefunction.TableBinlogFunctionAuthTest
- Behavior changed: No. This preserves the SELECT requirement while moving it to the standard authorization phase.
- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
morningman
previously approved these changes
Sep 17, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated static review completed. Requesting changes for two authorization gaps.
- The table-level
SELECThook does not apply row filters or data masks to raw binlog rows. - The hook runs only during rewrite, after target metadata binding, and is skipped by
DESCRIBE FUNCTION, analyzed-plan EXPLAIN, and binary prepare.
Checkpoint conclusions:
- Goal/scope: normal-query denial is wired and the diff is focused, but authorization is incomplete for the two paths above.
- Concurrency/lifecycle/configuration/persistence/transactions/protocol/rolling upgrade/observability: no new shared state, lock-order, lifecycle, configuration, persistence, transaction, FE-BE contract, compatibility, or observability issue was found. The name/object race was investigated and dismissed because it requires an independent privilege or policy mutation.
- Parallel/error paths: normal execution, normal EXPLAIN, CTEs, and prepared execution reach the hook; metadata-only paths do not, and target-specific errors precede denial.
- Tests: the direct unit test and normal deny/allow regression are structurally appropriate, but they do not cover policies or metadata-only paths. Per the review contract, I did not build or run tests; this conclusion is static-only.
- Performance: one privilege lookup is not material.
- Security: row/mask policy preservation remains the blocking issue described inline.
No additional user focus was supplied. Two fresh normal review passes and an adversarial pass converged with no further valuable findings.
Contributor
TPC-H: Total hot run time: 27878 ms |
Contributor
TPC-DS: Total hot run time: 153034 ms |
Contributor
ClickBench: Total hot run time: 24.03 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
morrySnow
dismissed
github-actions[bot]’s stale review
September 17, 2026 06:25
all should be dismiss
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static exact-head review of 5afc8150539ddaaf210a5f7a479775ddbfd2bfa4. Requesting changes for one distinct authorization-compatibility issue: the new table-only decision rejects supported column-scoped SELECT and plugin COLUMNS decisions. I did not repeat the live row-filter/data-mask or pre-rewrite metadata threads.
Checkpoint conclusions:
- Goal and correctness: the standard TVF hook closes the ordinary no-grant query path, but
MAIN-001shows that it does not preserve Doris's column authorization model. - Scope and control flow: all three changed files and the construction, analysis, rewrite, physical-TVF, authorization-source, and row-binlog schema paths were reviewed. Executable query, CTE/subquery, insert/CTAS, view/MV, and prepared-execution paths reach rewrite authorization; SQL result caching is disabled for TVFs.
- Concurrency, lifecycle, and identity: no new shared state or lock-order change exists. Rename/drop/recreate, case normalization, and temporary-table resolution did not yield a distinct authorization mismatch.
- Configuration, compatibility, and parallel paths: no configuration, FE-BE field, serialization, or rolling-upgrade surface changes. Cloud uses the same authorization route; the distinct compatibility defect is the
TABLEversusCOLUMNSresource decision described inline. - Persistence, transactions, and data writes: no new persistence, transaction, or write behavior.
- Tests and results: the unit test proves the local hook branches and constructor behavior; the regression proves the normal no-grant and whole-table-grant paths, but neither defines or covers column/plugin decisions. This review was static-only and ran no build or test. At submission time, GitHub reports compile/checkstyle passing while FE UT and regression jobs are pending; the PR body separately reports an author-run FE unit test and Groovy syntax validation.
- Observability and performance: denial uses the established table-access error and adds one planning-time authorization lookup, with no material performance or observability concern found.
No additional user focus was supplied. After one discovery round, a fresh full review and a separate risk-focused Round 2 both returned NO_NEW_VALUABLE_FINDINGS; the review is complete with the single distinct inline issue below.
Contributor
TPC-H: Total hot run time: 27554 ms |
Contributor
TPC-DS: Total hot run time: 153505 ms |
Contributor
ClickBench: Total hot run time: 23.98 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
morningman
approved these changes
Sep 20, 2026
morningman
added a commit
that referenced
this pull request
Sep 20, 2026
…on PRs from master in merge order (#67820 #68088) (#68236) Cherry-picked from #67820, #68088 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each ending with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017, #68073 and #68151. | # | Master commit | PR | Title | |---|---|---|---| | 1 | 44e3ae2 | #67820 | [fix](binlog) Track committed TSO and fence uncertain commits for bounded incremental reads | | 2 | 3de3a75 | #68088 | [fix](binlog) Require SELECT privilege for binlog TVF | Not included on purpose: - The 25 labelled PRs that already carry `incremental-computation-picked`. - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. ### How #67820 was picked This branch already carried #67820 through #67861, which backported the PR's first seven commits (up to `17272039558`) before the PR was merged. Between that cut and the merge the PR gained six more commits and five master merges, and the final design differs from what #67861 brought: the Meta Service recovery RPC (`get_tso_recovery_transactions`) and the FE startup/periodic recovery scan are gone, replaced by a durable per-instance commit-TSO fence (`txn_tso_fence_key`, `advance_tso_fence`, commit-time `TXN_COMMIT_TSO_EXPIRED` check, `enable_check_commit_tso_fence`), plus the review-feedback and "release maybe-committed TSO" fixes. A plain `cherry-pick -x 44e3ae2` conflicts in 23 files because the branch holds the intermediate design, so commit 1 was built by replaying, on top of the branch, exactly what the PR gained after the cut: - the PR's later commits `d16e67197b2`, `e53291d196b`, `864546d5531`, `8c55eb086e9`, `8d264ce294a`, `fc4f9969202`; - the content that landed inside its master merges, identified by diffing each merge against its `git merge-tree` automerge result: `c491a293426` (`setEnvTSOService` replaced by `Mockito.doReturn(tsoService).when(masterEnv).getTSOService()` — `masterEnv` is the same Mockito delegating mock here since #67813) and `29f133584b5` (the extra `TSOTransactionTrackerTest` coverage and two comment removals in `DorisFlightSqlProducer`). The other three merges only resolved import blocks against master-only code. The replay then got squashed into one commit with the master PR's message, the original author, and the `(cherry picked from commit 44e3ae2)` trailer. Adaptations, all recorded in the commit message: - `DorisFlightSqlProducer` / `DorisFlightSqlProducerTest` stay under `service/arrowflight` (the branch lacks the package move of #67866). - `8d264ce294a` is a no-op here: it removes a `catch (FlightRuntimeException)` block that #67883 added on master and this branch never had. - The branch-only `get_tso_recovery_transactions` RPC, its recovery scan, the five `TsoRecovery*` Meta Service tests and the NOLINT suppressions #67861 had added for them are removed, as on master. - The round-3 `mockVersionHelper()` adaptation in `CloudGlobalTransactionMgrTest` (no `VersionHelper.getVersionFromMeta(req, maxAttempts)` overload here, #66296) is kept. ### Prerequisite check - **#67820** declares #67181 and #67594 as related; #67181 (`e5a4e725fac`) is before the fork point and #67594 came with #67830. The rest of what the commit touches on master is import-block and neighbouring-code drift from unlabelled commits (#67866 / #67883 / #67966 Arrow Flight and session refactors, #67761 `get_prepare_txn_by_coordinator`, #66598 pre-rowset delete bitmaps, repair-tablet-index changes); none of it is used by the fence/committed-TSO logic. - **#68088** declares no related PR. The hook it implements (`TableValuedFunctionIf.checkAuth`) and the caller chain (`CheckPrivileges.visitLogicalTVFRelation` → `TableValuedFunction.checkAuth` → catalog function) are byte-identical between this branch and master, and the five-argument `AccessControllerManager.checkTblPriv` overload exists. Applied cleanly. ### Drift check against master - **#67820**: every one of the 54 files the master commit touches now contains the pick's content — the master commit reverse-applies cleanly per file onto this branch (50 files), and the four files where only the surrounding context differs (`config.h`, `meta_service_txn.cpp`, `StmtExecutor.java`, `StmtExecutorTest.java`) contain every added line and none of the removed ones. 33 of the 54 files are byte-identical to master at `44e3ae2b951`, including all of `fe/.../tso/`, `CloudGlobalTransactionMgr.java`, `MetaServiceProxy.java`, `keys.{h,cpp}`, `meta_service.h` and the regression suite/output. The remaining differences are unrelated master-only or branch-only code from unlabelled commits (#67761, #66598, #67866/#67883/#67966, repair-tablet-index, meta-cache columns, recycler configs) plus the documented `mockVersionHelper()` adaptation; no line in the committed-TSO/fence domain is left over from #67861. - **#68088**: all three files are byte-identical to master at `3de3a756f74`. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with every test class the picks touch: 12 classes, 175 tests, 0 failures, 0 errors, BUILD SUCCESS — `TSOServiceTest` 38, `CloudGlobalTransactionMgrTest` 37, `StmtExecutorTest` 27, `MetaServiceProxyTest` 22, `OlapScanNodeTest` 12, `TimeBasedChangeVisibleWaiterTest` 12, `TSOTransactionTrackerTest` 9 (the four tests added inside the PR's last master merge included), `DorisFlightSqlProducerTest` 7, `TsoStatusMetadataGeneratorTest` 5, `CloudCommittedTsoTest` 3, `SchemaTableTest` 2, `TableBinlogFunctionAuthTest` 1 (#68088). The `@Test` counts of the touched classes equal master's. - FE checkstyle on fe-core: 0 violations. - Meta Service: `-fsyntax-only` with the flags of `cloud/CMakeLists.txt` (`-Wall -Werror`, regenerated `gen_cpp/cloud.pb.h` with `AdvanceTsoFence*` / `TxnTsoFencePB` and without `GetTsoRecoveryTransactions*`) on `meta_service_txn.cpp`, `keys.cpp`, `http_encode_key.cpp`, `bvars.cpp` and, with `-DUNIT_TEST -DBE_TEST -fno-access-control`, on `meta_service_test.cpp`, `keys_test.cpp`, `http_encode_key_test.cpp`, `meta_service_helper_test.cpp`, `txn_lazy_commit_test.cpp`: no diagnostic in any line the pick touches (the only errors are the pre-existing macOS-only `pthread_setname_np` / `int64_t`-vs-`long` ones in untouched 2024/2025 code). clang-format 16 is clean on all 14 touched cloud files. - No BE file changes in this round (the BE side of #67820 was already byte-identical to master via #67861). - The new `auth_p0/test_binlog_tvf_auth.groovy` and the two #67820 suites parse cleanly (groovy parser check). --------- Co-authored-by: Luwei <814383175@qq.com> Co-authored-by: morrySnow <zhangwenxin@selectdb.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
The
binlog()table-valued function did not implement the standard TVF authorization hook. As a result, the Nereids privilege-check phase could not enforce table privileges for the target OLAP table, and a user denied directSELECTcould still read its row-binlog contents through the TVF.This change overrides
TableBinlogFunction.checkAuth(ConnectContext)and requiresSELECTon the target table. The TVF is therefore authorized through the standardCheckPrivilegesflow, independently of constructor and metadata initialization.Release note
The
binlog()table-valued function now requiresSELECTprivilege on its target table.Check List (For Author)
Validation performed:
./run-fe-ut.sh --run org.apache.doris.tablefunction.TableBinlogFunctionAuthTest(1 test, 0 failures; full 80-module reactor passed)mvn checkstyle:check -pl fe-core(0 violations)Added
test_binlog_tvf_auth; validated its Groovy syntax locally. Cluster execution is left to CI.Behavior changed:
binlog().Does this need documentation?
Check List (For Reviewer who merge this PR)