github-actions[bot] commented on code in PR #69623: URL: https://github.com/apache/doris/pull/69623#discussion_r4375463438
########## fe/fe-core/src/test/java/org/apache/doris/service/FrontendServiceImplStreamLoad2PCTest.java: ########## @@ -0,0 +1,245 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.service; + +import org.apache.doris.analysis.UserIdentity; +import org.apache.doris.auth.certificate.CertificateAuthDecision; +import org.apache.doris.auth.certificate.StreamLoadCertificateAuthHelper; +import org.apache.doris.catalog.Database; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.OlapTable; +import org.apache.doris.catalog.Table; +import org.apache.doris.common.AuthenticationException; +import org.apache.doris.common.DuplicatedRequestException; +import org.apache.doris.datasource.InternalCatalog; +import org.apache.doris.mysql.privilege.AccessControllerManager; +import org.apache.doris.mysql.privilege.Auth; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.qe.ExecuteEnv; Review Comment: [P1] Remove the nonexistent qe.ExecuteEnv import `ExecuteEnv` is declared in `org.apache.doris.service`, and `FrontendServiceImpl` accepts that type in its constructor. There is no `org.apache.doris.qe.ExecuteEnv` in this checkout, so this new import prevents FE test compilation before any of the added authorization tests can run. Remove the import: the test already belongs to `org.apache.doris.service`, so its `ExecuteEnv.class` reference resolves to the correct class. ########## fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java: ########## @@ -1850,6 +1866,7 @@ private void loadTxn2PCImpl(TLoadTxn2PCRequest request) throws UserException { if (transactionState == null) { throw new UserException("transaction [" + request.getTxnId() + "] not found"); } + checkTransactionUser(currentUser, transactionState); Review Comment: [P1] Authorize cloud abort-by-ID before the direct Meta Service call This check is bypassed by the normal cloud HTTP 2PC abort route. `LoadAction.executeStreamLoad2PC` redirects the authenticated caller to BE, whose `CloudStreamLoadExecutor::operate_txn_2pc` sends every abort with a positive transaction ID directly to `CloudMetaMgr::abort_txn`. That request contains the cluster ID and transaction ID, with no caller identity; Meta Service resolves the database from the transaction index and sets the transaction to ABORTED without checking `txn_user`. Thus Bob can submit `txn_operation: abort` and Alice's transaction ID through the usual FE HTTP endpoint, follow its redirect, and abort Alice's newly owned Stream Load transaction. This happens with the default configuration, independently of the commit-on-BE option. Route this external operation through the FE owner/LOAD checks, or enforce equivalent authorization before the direct mutation, and cover the real cloud HTTP dispatch in a two-user test. ########## fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java: ########## @@ -1880,6 +1897,16 @@ private void loadTxn2PCImpl(TLoadTxn2PCRequest request) throws UserException { } } + private void checkTransactionUser(UserIdentity currentUser, TransactionState transactionState) + throws AuthenticationException { + // Transactions without a txnUser retain their previous authorization during rolling upgrades. + if (transactionState.getTxnUser() != null && !transactionState.getTxnUser().equals(currentUser) Review Comment: [P1] Record an owner for SQL HTTP Stream 2PC transactions too The null-owner exemption also covers newly created user transactions from `/api/_http_stream`. With `two_phase_commit: true` and `INSERT ... SELECT ... FROM http_stream(...)`, that supported producer starts its transaction through `OlapInsertExecutor.beginTransaction`, which still calls the old short overload and ultimately supplies a null owner. `HttpStreamAction` precommits it, and the existing `test_http_stream_2pc.groovy` completes it through this same 2PC endpoint. Consequently, if Alice and Bob both have LOAD on the target table, Bob can commit or abort Alice's new SQL HTTP transaction by ID or label: this guard skips ownership and Bob passes the remaining LOAD check. Propagate the authenticated owner when that producer begins its transaction as well, and add two-user SQL HTTP 2PC coverage; these transactions should not be treated as legacy/internal records. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
