github-actions[bot] commented on code in PR #17753: URL: https://github.com/apache/doris/pull/17753#discussion_r1172182782
########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); Review Comment: warning: member access into incomplete type 'doris::TCheckAuthRequest' [clang-diagnostic-error] ```cpp auth_request.__set_priv_ctrl(priv_ctrl); ^ ``` **be/src/http/http_handler_with_auth.h:27:** forward declaration of 'doris::TCheckAuthRequest' ```cpp class TCheckAuthRequest; ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); + auth_request.__set_priv_type(_type); + return true; + } + + void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { Review Comment: warning: incomplete type 'doris::TPrivilegeHier' named in nested name specifier [clang-diagnostic-error] ```cpp void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { ^ ``` **be/src/http/http_handler_with_auth.h:28:** forward declaration of 'doris::TPrivilegeHier' ```cpp class TPrivilegeHier; ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); + auth_request.__set_priv_type(_type); + return true; + } + + void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { Review Comment: warning: incomplete type 'doris::TPrivilegeType' named in nested name specifier [clang-diagnostic-error] ```cpp void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { ^ ``` **be/src/http/http_handler_with_auth.h:29:** forward declaration of 'doris::TPrivilegeType' ```cpp class TPrivilegeType; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); // Register Tablets Distribution action TabletsDistributionAction* tablets_distribution_action = - _pool.add(new TabletsDistributionAction()); + _pool.add(new TabletsDistributionAction(_env)); + tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/api/tablets_distribution", tablets_distribution_action); Review Comment: warning: cannot initialize a parameter of type 'doris::HttpHandler *' with an lvalue of type 'doris::TabletsDistributionAction *' [clang-diagnostic-error] ```cpp tablets_distribution_action); ^ ``` **be/src/http/ev_http_server.h:43:** passing argument to parameter 'handler' here ```cpp bool register_handler(const HttpMethod& method, const std::string& path, HttpHandler* handler); ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); // Register Tablets Distribution action TabletsDistributionAction* tablets_distribution_action = - _pool.add(new TabletsDistributionAction()); + _pool.add(new TabletsDistributionAction(_env)); + tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/api/tablets_distribution", tablets_distribution_action); // Register tablet migration action - TabletMigrationAction* tablet_migration_action = _pool.add(new TabletMigrationAction()); + TabletMigrationAction* tablet_migration_action = _pool.add(new TabletMigrationAction(_env)); + tablet_migration_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/api/tablet_migration", tablet_migration_action); Review Comment: warning: cannot initialize a parameter of type 'doris::HttpHandler *' with an lvalue of type 'doris::TabletMigrationAction *' [clang-diagnostic-error] ```cpp tablet_migration_action); ^ ``` **be/src/http/ev_http_server.h:43:** passing argument to parameter 'handler' here ```cpp bool register_handler(const HttpMethod& method, const std::string& path, HttpHandler* handler); ^ ``` ########## be/src/http/http_handler_with_auth.cpp: ########## @@ -0,0 +1,85 @@ +// 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. + +#include "http_handler_with_auth.h" + +#include "gen_cpp/HeartbeatService_types.h" +#include "http/http_channel.h" +#include "runtime/client_cache.h" +#include "util/thrift_rpc_helper.h" +#include "utils.h" + +namespace doris { + +class TPrivilegeType; +class TPrivilegeHier; +class ThriftRpcHelper; + +HttpHandlerWithAuth::HttpHandlerWithAuth(ExecEnv* exec_env) { + _exec_env = exec_env; + _hier = TPrivilegeHier::GLOBAL; + _type = TPrivilegeType::NONE; +} + +int HttpHandlerWithAuth::on_header(HttpRequest* req) { + TCheckAuthRequest auth_request; + TCheckAuthResult auth_result; + AuthInfo auth_info; Review Comment: warning: variable has incomplete type 'doris::AuthInfo' [clang-diagnostic-error] ```cpp AuthInfo auth_info; ^ ``` **be/src/http/utils.h:25:** forward declaration of 'doris::AuthInfo' ```cpp struct AuthInfo; ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); + auth_request.__set_priv_type(_type); + return true; + } + + void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { + _hier = hier; + _type = type; + } + +private: + ExecEnv* _exec_env; + TPrivilegeHier::type _hier; Review Comment: warning: incomplete type 'doris::TPrivilegeHier' named in nested name specifier [clang-diagnostic-error] ```cpp TPrivilegeHier::type _hier; ^ ``` **be/src/http/http_handler_with_auth.h:28:** forward declaration of 'doris::TPrivilegeHier' ```cpp class TPrivilegeHier; ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); + auth_request.__set_priv_type(_type); Review Comment: warning: member access into incomplete type 'doris::TCheckAuthRequest' [clang-diagnostic-error] ```cpp auth_request.__set_priv_type(_type); ^ ``` **be/src/http/http_handler_with_auth.h:27:** forward declaration of 'doris::TCheckAuthRequest' ```cpp class TCheckAuthRequest; ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; Review Comment: warning: unknown type name 'TPrivilegeCtrl' [clang-diagnostic-error] ```cpp TPrivilegeCtrl priv_ctrl; ^ ``` ########## be/test/http/http_auth_test.cpp: ########## @@ -0,0 +1,88 @@ +// 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. + +#include <gtest/gtest.h> + +#include "common/config.h" +#include "http/ev_http_server.h" +#include "http/http_channel.h" +#include "http/http_handler.h" +#include "http/http_handler_with_auth.h" +#include "http/http_request.h" +#include "http/utils.h" + +namespace doris { + +class HttpAuthTestHandler : public HttpHandlerWithAuth { +public: + HttpAuthTestHandler(ExecEnv* exec_env) : HttpHandlerWithAuth(exec_env) {} + + ~HttpAuthTestHandler() override = default; + + void handle(HttpRequest* req) override {} + +private: + bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) override { + return !req.param("table").empty(); + }; +}; + +static HttpAuthTestHandler s_auth_handler = HttpAuthTestHandler(nullptr); + +class HttpAuthTest : public testing::Test {}; + +TEST_F(HttpAuthTest, disable_auth) { + EXPECT_FALSE(config::enable_auth); + + auto evhttp_req = evhttp_request_new(nullptr, nullptr); + HttpRequest req(evhttp_req); + EXPECT_EQ(s_auth_handler.on_header(&req), 0); + evhttp_request_free(evhttp_req); +} + +TEST_F(HttpAuthTest, enable_auth) { + config::enable_auth = true; + + // 1. empty auth info + { + auto evhttp_req = evhttp_request_new(nullptr, nullptr); + HttpRequest req1(evhttp_req); + EXPECT_EQ(s_auth_handler.on_header(&req1), -1); Review Comment: warning: cannot initialize object parameter of type 'doris::HttpHandlerWithAuth' with an expression of type 'doris::HttpAuthTestHandler' [clang-diagnostic-error] ```cpp EXPECT_EQ(s_auth_handler.on_header(&req1), -1); ^ ``` **thirdparty/installed/include/gtest/gtest.h:2043:** expanded from macro 'EXPECT_EQ' ```cpp EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^ ``` **thirdparty/installed/include/gtest/gtest_pred_impl.h:163:** expanded from macro 'EXPECT_PRED_FORMAT2' ```cpp GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^ ``` **thirdparty/installed/include/gtest/gtest_pred_impl.h:148:** expanded from macro 'GTEST_PRED_FORMAT2_' ```cpp GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \ ^ ``` **thirdparty/installed/include/gtest/gtest_pred_impl.h:76:** expanded from macro 'GTEST_ASSERT_' ```cpp if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^ ``` ########## be/test/http/http_auth_test.cpp: ########## @@ -0,0 +1,88 @@ +// 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. + +#include <gtest/gtest.h> + +#include "common/config.h" +#include "http/ev_http_server.h" +#include "http/http_channel.h" +#include "http/http_handler.h" +#include "http/http_handler_with_auth.h" +#include "http/http_request.h" +#include "http/utils.h" + +namespace doris { + +class HttpAuthTestHandler : public HttpHandlerWithAuth { +public: + HttpAuthTestHandler(ExecEnv* exec_env) : HttpHandlerWithAuth(exec_env) {} + + ~HttpAuthTestHandler() override = default; + + void handle(HttpRequest* req) override {} + +private: + bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) override { + return !req.param("table").empty(); + }; +}; + +static HttpAuthTestHandler s_auth_handler = HttpAuthTestHandler(nullptr); + +class HttpAuthTest : public testing::Test {}; + +TEST_F(HttpAuthTest, disable_auth) { + EXPECT_FALSE(config::enable_auth); + + auto evhttp_req = evhttp_request_new(nullptr, nullptr); + HttpRequest req(evhttp_req); + EXPECT_EQ(s_auth_handler.on_header(&req), 0); + evhttp_request_free(evhttp_req); +} + +TEST_F(HttpAuthTest, enable_auth) { + config::enable_auth = true; + + // 1. empty auth info + { + auto evhttp_req = evhttp_request_new(nullptr, nullptr); + HttpRequest req1(evhttp_req); + EXPECT_EQ(s_auth_handler.on_header(&req1), -1); + } + + // 2. empty param + { + auto evhttp_req = evhttp_request_new(nullptr, nullptr); + HttpRequest req2(evhttp_req); + auto auth = encode_basic_auth("doris", "passwd"); + req2._headers.emplace(HttpHeaders::AUTHORIZATION, auth); Review Comment: warning: '_headers' is a private member of 'doris::HttpRequest' [clang-diagnostic-error] ```cpp req2._headers.emplace(HttpHeaders::AUTHORIZATION, auth); ^ ``` **be/src/http/http_request.h:86:** declared private here ```cpp StringCaseUnorderedMap<std::string> _headers; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); Review Comment: warning: incomplete type 'doris::TPrivilegeHier' named in nested name specifier [clang-diagnostic-error] ```cpp tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); ^ ``` **be/src/http/http_handler_with_auth.h:28:** forward declaration of 'doris::TPrivilegeHier' ```cpp class TPrivilegeHier; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); // Register Tablets Distribution action TabletsDistributionAction* tablets_distribution_action = - _pool.add(new TabletsDistributionAction()); + _pool.add(new TabletsDistributionAction(_env)); + tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); Review Comment: warning: incomplete type 'doris::TPrivilegeType' named in nested name specifier [clang-diagnostic-error] ```cpp tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); ^ ``` **be/src/http/http_handler_with_auth.h:29:** forward declaration of 'doris::TPrivilegeType' ```cpp class TPrivilegeType; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); Review Comment: warning: incomplete type 'doris::TPrivilegeType' named in nested name specifier [clang-diagnostic-error] ```cpp tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); ^ ``` **be/src/http/http_handler_with_auth.h:29:** forward declaration of 'doris::TPrivilegeType' ```cpp class TPrivilegeType; ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); + auth_request.__set_priv_type(_type); + return true; + } + + void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { + _hier = hier; + _type = type; + } + +private: + ExecEnv* _exec_env; + TPrivilegeHier::type _hier; Review Comment: warning: private field '_hier' is not used [clang-diagnostic-unused-private-field] ```cpp TPrivilegeHier::type _hier; ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); + auth_request.__set_priv_type(_type); + return true; + } + + void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { + _hier = hier; + _type = type; + } + +private: + ExecEnv* _exec_env; + TPrivilegeHier::type _hier; + TPrivilegeType::type _type; Review Comment: warning: incomplete type 'doris::TPrivilegeType' named in nested name specifier [clang-diagnostic-error] ```cpp TPrivilegeType::type _type; ^ ``` **be/src/http/http_handler_with_auth.h:29:** forward declaration of 'doris::TPrivilegeType' ```cpp class TPrivilegeType; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ Status HttpService::start() { error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); Review Comment: warning: cannot initialize a parameter of type 'doris::HttpHandler *' with an lvalue of type 'doris::VersionAction *' [clang-diagnostic-error] ```cpp _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); ^ ``` **be/src/http/ev_http_server.h:43:** passing argument to parameter 'handler' here ```cpp bool register_handler(const HttpMethod& method, const std::string& path, HttpHandler* handler); ^ ``` ########## be/src/http/http_handler_with_auth.h: ########## @@ -0,0 +1,62 @@ +// 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. + +#pragma once + +#include "http_handler.h" +#include "runtime/exec_env.h" + +namespace doris { + +class ExecEnv; +class HttpRequest; +class RestMonitorIface; +class TCheckAuthRequest; +class TPrivilegeHier; +class TPrivilegeType; + +// Handler for on http request with auth +class HttpHandlerWithAuth : public HttpHandler { +public: + HttpHandlerWithAuth(ExecEnv* exec_env); + + ~HttpHandlerWithAuth() override = default; + + // return 0 if auth pass, otherwise -1. + int on_header(HttpRequest* req) override; + + // return true if fill privilege success, otherwise false. + virtual bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + TPrivilegeCtrl priv_ctrl; + priv_ctrl.priv_hier = _hier; + auth_request.__set_priv_ctrl(priv_ctrl); + auth_request.__set_priv_type(_type); + return true; + } + + void auth(TPrivilegeHier::type hier, TPrivilegeType::type type) { + _hier = hier; + _type = type; + } + +private: + ExecEnv* _exec_env; + TPrivilegeHier::type _hier; + TPrivilegeType::type _type; Review Comment: warning: private field '_type' is not used [clang-diagnostic-unused-private-field] ```cpp TPrivilegeType::type _type; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); // Register Tablets Distribution action TabletsDistributionAction* tablets_distribution_action = - _pool.add(new TabletsDistributionAction()); + _pool.add(new TabletsDistributionAction(_env)); + tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/api/tablets_distribution", tablets_distribution_action); // Register tablet migration action - TabletMigrationAction* tablet_migration_action = _pool.add(new TabletMigrationAction()); + TabletMigrationAction* tablet_migration_action = _pool.add(new TabletMigrationAction(_env)); + tablet_migration_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); Review Comment: warning: incomplete type 'doris::TPrivilegeType' named in nested name specifier [clang-diagnostic-error] ```cpp tablet_migration_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); ^ ``` **be/src/http/http_handler_with_auth.h:29:** forward declaration of 'doris::TPrivilegeType' ```cpp class TPrivilegeType; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -129,42 +132,51 @@ // register metrics { - auto action = _pool.add(new MetricsAction(DorisMetrics::instance()->metric_registry())); + auto action = + _pool.add(new MetricsAction(DorisMetrics::instance()->metric_registry(), _env)); _ev_http_server->register_handler(HttpMethod::GET, "/metrics", action); } - MetaAction* meta_action = _pool.add(new MetaAction()); + MetaAction* meta_action = _pool.add(new MetaAction(_env)); + meta_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); Review Comment: warning: incomplete type 'doris::TPrivilegeHier' named in nested name specifier [clang-diagnostic-error] ```cpp meta_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); ^ ``` **be/src/http/http_handler_with_auth.h:28:** forward declaration of 'doris::TPrivilegeHier' ```cpp class TPrivilegeHier; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); // Register Tablets Distribution action TabletsDistributionAction* tablets_distribution_action = - _pool.add(new TabletsDistributionAction()); + _pool.add(new TabletsDistributionAction(_env)); + tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/api/tablets_distribution", tablets_distribution_action); // Register tablet migration action - TabletMigrationAction* tablet_migration_action = _pool.add(new TabletMigrationAction()); + TabletMigrationAction* tablet_migration_action = _pool.add(new TabletMigrationAction(_env)); + tablet_migration_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); Review Comment: warning: incomplete type 'doris::TPrivilegeHier' named in nested name specifier [clang-diagnostic-error] ```cpp tablet_migration_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); ^ ``` **be/src/http/http_handler_with_auth.h:28:** forward declaration of 'doris::TPrivilegeHier' ```cpp class TPrivilegeHier; ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); Review Comment: warning: cannot initialize a parameter of type 'doris::HttpHandler *' with an lvalue of type 'doris::TabletsInfoAction *' [clang-diagnostic-error] ```cpp _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); ^ ``` **be/src/http/ev_http_server.h:43:** passing argument to parameter 'handler' here ```cpp bool register_handler(const HttpMethod& method, const std::string& path, HttpHandler* handler); ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -99,25 +99,28 @@ error_log_download_action); // Register BE version action - VersionAction* version_action = _pool.add(new VersionAction()); + VersionAction* version_action = _pool.add(new VersionAction(_env)); _ev_http_server->register_handler(HttpMethod::GET, "/api/be_version_info", version_action); // Register BE health action HealthAction* health_action = _pool.add(new HealthAction()); _ev_http_server->register_handler(HttpMethod::GET, "/api/health", health_action); // Register Tablets Info action - TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction()); + TabletsInfoAction* tablets_info_action = _pool.add(new TabletsInfoAction(_env)); + tablets_info_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); _ev_http_server->register_handler(HttpMethod::GET, "/tablets_json", tablets_info_action); // Register Tablets Distribution action TabletsDistributionAction* tablets_distribution_action = - _pool.add(new TabletsDistributionAction()); + _pool.add(new TabletsDistributionAction(_env)); + tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); Review Comment: warning: incomplete type 'doris::TPrivilegeHier' named in nested name specifier [clang-diagnostic-error] ```cpp tablets_distribution_action->auth(TPrivilegeHier::GLOBAL, TPrivilegeType::ADMIN); ^ ``` **be/src/http/http_handler_with_auth.h:28:** forward declaration of 'doris::TPrivilegeHier' ```cpp class TPrivilegeHier; ^ ``` ########## be/test/http/http_auth_test.cpp: ########## @@ -0,0 +1,88 @@ +// 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. + +#include <gtest/gtest.h> + +#include "common/config.h" +#include "http/ev_http_server.h" +#include "http/http_channel.h" +#include "http/http_handler.h" +#include "http/http_handler_with_auth.h" +#include "http/http_request.h" +#include "http/utils.h" + +namespace doris { + +class HttpAuthTestHandler : public HttpHandlerWithAuth { +public: + HttpAuthTestHandler(ExecEnv* exec_env) : HttpHandlerWithAuth(exec_env) {} + + ~HttpAuthTestHandler() override = default; + + void handle(HttpRequest* req) override {} + +private: + bool on_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) override { + return !req.param("table").empty(); + }; +}; + +static HttpAuthTestHandler s_auth_handler = HttpAuthTestHandler(nullptr); + +class HttpAuthTest : public testing::Test {}; + +TEST_F(HttpAuthTest, disable_auth) { + EXPECT_FALSE(config::enable_auth); + + auto evhttp_req = evhttp_request_new(nullptr, nullptr); + HttpRequest req(evhttp_req); + EXPECT_EQ(s_auth_handler.on_header(&req), 0); Review Comment: warning: cannot initialize object parameter of type 'doris::HttpHandlerWithAuth' with an expression of type 'doris::HttpAuthTestHandler' [clang-diagnostic-error] ```cpp EXPECT_EQ(s_auth_handler.on_header(&req), 0); ^ ``` **thirdparty/installed/include/gtest/gtest.h:2043:** expanded from macro 'EXPECT_EQ' ```cpp EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^ ``` **thirdparty/installed/include/gtest/gtest_pred_impl.h:163:** expanded from macro 'EXPECT_PRED_FORMAT2' ```cpp GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^ ``` **thirdparty/installed/include/gtest/gtest_pred_impl.h:148:** expanded from macro 'GTEST_PRED_FORMAT2_' ```cpp GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \ ^ ``` **thirdparty/installed/include/gtest/gtest_pred_impl.h:76:** expanded from macro 'GTEST_ASSERT_' ```cpp if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^ ``` ########## be/src/service/http_service.cpp: ########## @@ -129,42 +132,51 @@ // register metrics { - auto action = _pool.add(new MetricsAction(DorisMetrics::instance()->metric_registry())); + auto action = + _pool.add(new MetricsAction(DorisMetrics::instance()->metric_registry(), _env)); _ev_http_server->register_handler(HttpMethod::GET, "/metrics", action); Review Comment: warning: cannot initialize a parameter of type 'doris::HttpHandler *' with an lvalue of type 'doris::MetricsAction *' [clang-diagnostic-error] ```cpp _ev_http_server->register_handler(HttpMethod::GET, "/metrics", action); ^ ``` **be/src/http/ev_http_server.h:43:** passing argument to parameter 'handler' here ```cpp bool register_handler(const HttpMethod& method, const std::string& path, HttpHandler* handler); ^ ``` -- 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]
