github-actions[bot] commented on code in PR #65042: URL: https://github.com/apache/doris/pull/65042#discussion_r3503127092
########## regression-test/suites/auth_p0/test_http_node_action_auth.groovy: ########## @@ -0,0 +1,113 @@ +// 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. + +import org.junit.Assert; + +// Verify the node management endpoints (add/drop fe/be/broker) require +// authentication and ADMIN privilege. Without the check, any caller could +// add or drop cluster nodes via these REST APIs. +// +// NOTE on cluster safety: the bogus node addresses below use the RFC 5737 +// TEST-NET-1 range (192.0.2.0/24), which can never match a real FE/BE in any +// cluster. The negative (non-admin) cases use ADD, but the ADMIN check runs +// before the node operation, so the add is never executed. The positive +// (admin) cases use DROP, which on a non-existent node returns a harmless +// "does not exist" error -- it never mutates real cluster state. +suite("test_http_node_action_auth", "p0,auth,nonConcurrent") { + String suiteName = "test_http_node_action_auth" + String user = "${suiteName}_user" + String pwd = 'C123_567p' + String bogusFe = "192.0.2.111:12345" + String bogusBe = "192.0.2.112:12345" + try_sql("DROP USER ${user}") + sql """CREATE USER '${user}' IDENTIFIED BY '${pwd}'""" + + try { + sql """ ADMIN SET ALL FRONTENDS CONFIG ("enable_all_http_auth" = "true"); """ + + def operateFe = { user_name, password, action, check_func -> + httpTest { + basicAuthorization "${user_name}", "${password}" + endpoint "${context.config.feHttpAddress}" + uri "/rest/v2/manager/node/${action}/fe" + op "post" + body """{"role": "OBSERVER", "hostPort": "${bogusFe}"}""" + check check_func + } + } + + def operateBe = { user_name, password, action, check_func -> + httpTest { + basicAuthorization "${user_name}", "${password}" + endpoint "${context.config.feHttpAddress}" + uri "/rest/v2/manager/node/${action}/be" + op "post" + body """{"hostPorts": ["${bogusBe}"]}""" + check check_func + } + } + + // A non-admin user must be rejected by the ADMIN privilege check. + // The node operation is never reached, so nothing is mutated. + operateFe.call(user, pwd, "ADD") { + respCode, body -> + log.info("add fe (non-admin) body:${body}") + assertTrue("${body}".contains("Unauthorized")) + } + + operateBe.call(user, pwd, "ADD") { + respCode, body -> + log.info("add be (non-admin) body:${body}") + assertTrue("${body}".contains("Unauthorized")) + } + + sql """grant 'admin' to ${user}""" + + // After granting ADMIN, the request passes the auth check. We use DROP + // on a bogus (TEST-NET) node so the call reaches the operation but only + // gets a "does not exist" error -- it must no longer be rejected with an + // authorization error, and must not touch any real node. + operateFe.call(user, pwd, "DROP") { + respCode, body -> + log.info("drop fe (admin) body:${body}") + assertFalse("${body}".contains("Unauthorized")) + } + + operateBe.call(user, pwd, "DROP") { + respCode, body -> + log.info("drop be (admin) body:${body}") + assertFalse("${body}".contains("Unauthorized")) + } + + // The query qerror endpoint must require authentication. Without Review Comment: This only exercises the unauthenticated path with `no_such_query_id`, so it would still pass if `getStats` kept `executeCheckPassword` but lost the new `checkAuthByUserAndQueryId` call. Please add an authenticated ownership case for qerror as well: create a query/profile under one user or root, request that query id as a different non-admin and assert the ownership check rejects it, then verify the owner or an admin is not rejected as `Unauthorized` even if the qerror payload itself is absent. ########## regression-test/suites/auth_p0/test_http_node_action_auth.groovy: ########## @@ -0,0 +1,113 @@ +// 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. + +import org.junit.Assert; + +// Verify the node management endpoints (add/drop fe/be/broker) require Review Comment: Since this PR also added the auth gate to `operateBroker`, this suite should exercise `/rest/v2/manager/node/{action}/broker` too. Right now only the FE and BE helpers are called, so a regression that drops the broker check would still pass. You can keep it cluster-safe the same way as the FE/BE positive path: use a suite-local bogus broker name plus a TEST-NET host:port, assert a non-admin request is rejected before mutation, then assert an admin `DROP` reaches the broker operation and is not rejected as `Unauthorized`. -- 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]
