IMPALA-7721: Fix broken /catalog_object web API when getting a privilege Before this patch, /catalog_object web API was broken when getting a privilege due to an incorrect way of getting a role ID. IMPALA-7616 broke this even more due to a lack of test coverage in /catalog_object when authorization is enabled. This patch fixes the issue and makes the /catalog_object web API usable again for getting a privilege.
Testing: - Added a new BE test - Added a new E2E test - Ran all E2E authorization tests Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7 Reviewed-on: http://gerrit.cloudera.org:8080/11721 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/bad49e73 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/bad49e73 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/bad49e73 Branch: refs/heads/master Commit: bad49e73632f64a386ad1154201f99137af864d8 Parents: 072f3ee Author: Fredy Wijaya <[email protected]> Authored: Wed Oct 17 16:02:49 2018 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Sat Oct 20 00:19:18 2018 +0000 ---------------------------------------------------------------------- be/src/catalog/catalog-util-test.cc | 90 ++++++++++++- be/src/catalog/catalog-util.cc | 125 ++++++++++++++++++- be/src/catalog/catalog-util.h | 4 +- .../java/org/apache/impala/catalog/Catalog.java | 12 +- .../impala/catalog/PrincipalPrivilege.java | 2 +- tests/authorization/test_authorization.py | 40 ++++++ 6 files changed, 260 insertions(+), 13 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/be/src/catalog/catalog-util-test.cc ---------------------------------------------------------------------- diff --git a/be/src/catalog/catalog-util-test.cc b/be/src/catalog/catalog-util-test.cc index d37fc5c..91466f5 100644 --- a/be/src/catalog/catalog-util-test.cc +++ b/be/src/catalog/catalog-util-test.cc @@ -15,11 +15,14 @@ // specific language governing permissions and limitations // under the License. +#include <gutil/strings/substitute.h> + #include "catalog/catalog-util.h" #include "testutil/gtest-util.h" using namespace impala; using namespace std; +using namespace strings; void CompressAndDecompress(const std::string& input) { string compressed; @@ -32,7 +35,6 @@ void CompressAndDecompress(const std::string& input) { ASSERT_EQ(input, decompressed); } - TEST(CatalogUtil, TestCatalogCompression) { CompressAndDecompress(""); CompressAndDecompress("deadbeef"); @@ -45,5 +47,91 @@ TEST(CatalogUtil, TestCatalogCompression) { CompressAndDecompress(large_string); } +TEST(CatalogUtil, TestTPrivilegeFromObjectName) { + vector<tuple<string, TPrivilegeLevel::type>> actions = { + make_tuple("all", TPrivilegeLevel::ALL), + make_tuple("insert", TPrivilegeLevel::INSERT), + make_tuple("select", TPrivilegeLevel::SELECT), + make_tuple("refresh", TPrivilegeLevel::REFRESH), + make_tuple("create", TPrivilegeLevel::CREATE), + make_tuple("alter", TPrivilegeLevel::ALTER), + make_tuple("drop", TPrivilegeLevel::DROP), + make_tuple("owner", TPrivilegeLevel::OWNER) + }; + vector<tuple<string, bool>> grant_options = { + make_tuple("true", true), + make_tuple("false", false) + }; + + for (const auto& action: actions) { + for (const auto& grant_option: grant_options) { + TPrivilege server_privilege; + ASSERT_OK(TPrivilegeFromObjectName(Substitute( + "server=server1->action=$0->grantoption=$1", + get<0>(action), get<0>(grant_option)), &server_privilege)); + ASSERT_EQ(TPrivilegeScope::SERVER, server_privilege.scope); + ASSERT_EQ(get<1>(action), server_privilege.privilege_level); + ASSERT_EQ(get<1>(grant_option), server_privilege.has_grant_opt); + ASSERT_EQ("server1", server_privilege.server_name); + + TPrivilege uri_privilege; + ASSERT_OK(TPrivilegeFromObjectName(Substitute( + "server=server1->uri=/test-warehouse->action=$0->grantoption=$1", + get<0>(action), get<0>(grant_option)), &uri_privilege)); + ASSERT_EQ(TPrivilegeScope::URI, uri_privilege.scope); + ASSERT_EQ(get<1>(action), uri_privilege.privilege_level); + ASSERT_EQ(get<1>(grant_option), uri_privilege.has_grant_opt); + ASSERT_EQ("server1", uri_privilege.server_name); + ASSERT_EQ("/test-warehouse", uri_privilege.uri); + + TPrivilege db_privilege; + ASSERT_OK(TPrivilegeFromObjectName(Substitute( + "server=server1->db=functional->action=$0->grantoption=$1", + get<0>(action), get<0>(grant_option)), &db_privilege)); + ASSERT_EQ(TPrivilegeScope::DATABASE, db_privilege.scope); + ASSERT_EQ(get<1>(action), db_privilege.privilege_level); + ASSERT_EQ(get<1>(grant_option), db_privilege.has_grant_opt); + ASSERT_EQ("server1", db_privilege.server_name); + ASSERT_EQ("functional", db_privilege.db_name); + + TPrivilege table_privilege; + ASSERT_OK(TPrivilegeFromObjectName(Substitute( + "server=server1->db=functional->table=alltypes->action=$0->grantoption=$1", + get<0>(action), get<0>(grant_option)), &table_privilege)); + ASSERT_EQ(TPrivilegeScope::TABLE, table_privilege.scope); + ASSERT_EQ(get<1>(action), table_privilege.privilege_level); + ASSERT_EQ(get<1>(grant_option), table_privilege.has_grant_opt); + ASSERT_EQ("server1", table_privilege.server_name); + ASSERT_EQ("functional", table_privilege.db_name); + ASSERT_EQ("alltypes", table_privilege.table_name); + + TPrivilege column_privilege; + ASSERT_OK(TPrivilegeFromObjectName(Substitute( + "server=server1->db=functional->table=alltypes->column=id->action=$0->" + "grantoption=$1", get<0>(action), get<0>(grant_option)), &column_privilege)); + ASSERT_EQ(TPrivilegeScope::COLUMN, column_privilege.scope); + ASSERT_EQ(get<1>(action), column_privilege.privilege_level); + ASSERT_EQ(get<1>(grant_option), column_privilege.has_grant_opt); + ASSERT_EQ("server1", column_privilege.server_name); + ASSERT_EQ("functional", column_privilege.db_name); + ASSERT_EQ("alltypes", column_privilege.table_name); + ASSERT_EQ("id", column_privilege.column_name); + } + } + + TPrivilege privilege; + EXPECT_ERROR(TPrivilegeFromObjectName("abc=server1->action=select->grantoption=true", + &privilege), TErrorCode::GENERAL); + EXPECT_ERROR(TPrivilegeFromObjectName("server=server1->action=foo->grantoption=true", + &privilege), TErrorCode::GENERAL); + EXPECT_ERROR(TPrivilegeFromObjectName("server=server1->action=select->grantoption=foo", + &privilege), TErrorCode::GENERAL); + EXPECT_ERROR(TPrivilegeFromObjectName("", &privilege), TErrorCode::GENERAL); + EXPECT_ERROR(TPrivilegeFromObjectName("SERVER=server1->action=select->grantoption=true", + &privilege), TErrorCode::GENERAL); + EXPECT_ERROR(TPrivilegeFromObjectName("server;server1->action=select->grantoption=true", + &privilege), TErrorCode::GENERAL); +} + IMPALA_TEST_MAIN(); http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/be/src/catalog/catalog-util.cc ---------------------------------------------------------------------- diff --git a/be/src/catalog/catalog-util.cc b/be/src/catalog/catalog-util.cc index cc7bf49..5084828 100644 --- a/be/src/catalog/catalog-util.cc +++ b/be/src/catalog/catalog-util.cc @@ -17,6 +17,7 @@ #include <boost/algorithm/string.hpp> +#include <boost/algorithm/string_regex.hpp> #include <sstream> #include "catalog/catalog-util.h" @@ -24,6 +25,7 @@ #include "util/compress.h" #include "util/jni-util.h" #include "util/debug-util.h" +#include "util/string-parser.h" #include "common/names.h" @@ -36,6 +38,10 @@ jmethodID JniCatalogCacheUpdateIterator::pair_ctor; jclass JniCatalogCacheUpdateIterator::boolean_cl; jmethodID JniCatalogCacheUpdateIterator::boolean_ctor; +/// Populates a TPrivilegeLevel::type based on the given object name string. +Status TPrivilegeLevelFromObjectName(const std::string& object_name, + TPrivilegeLevel::type* privilege_level); + Status JniCatalogCacheUpdateIterator::InitJNI() { JNIEnv* env = getJNIEnv(); if (env == nullptr) return Status("Failed to get/create JVM"); @@ -202,16 +208,40 @@ Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type, catalog_object->principal.__set_principal_name(object_name); break; case TCatalogObjectType::PRIVILEGE: { - int pos = object_name.find("."); - if (pos == string::npos || pos >= object_name.size() - 1) { + // The format is <privilege name>.<principal ID>.<principal type> + vector<string> split; + boost::split(split, object_name, [](char c){ return c == '.'; }); + if (split.size() != 3) { stringstream error_msg; error_msg << "Invalid privilege name: " << object_name; return Status(error_msg.str()); } + string privilege_name = split[0]; + string principal_id = split[1]; + string principal_type = split[2]; catalog_object->__set_type(object_type); - catalog_object->__set_privilege(TPrivilege()); - catalog_object->privilege.__set_principal_id( - atoi(object_name.substr(0, pos).c_str())); + TPrivilege privilege; + Status status = TPrivilegeFromObjectName(privilege_name, &privilege); + if (!status.ok()) return status; + catalog_object->__set_privilege(privilege); + StringParser::ParseResult result; + int32_t pid = StringParser::StringToInt<int32_t>(principal_id.c_str(), + principal_id.length(), &result); + if (result != StringParser::PARSE_SUCCESS) { + stringstream error_msg; + error_msg << "Invalid principal ID: " << principal_id; + return Status(error_msg.str()); + } + catalog_object->privilege.__set_principal_id(pid); + if (principal_type == "ROLE") { + catalog_object->privilege.__set_principal_type(TPrincipalType::ROLE); + } else if (principal_type == "USER") { + catalog_object->privilege.__set_principal_type(TPrincipalType::USER); + } else { + stringstream error_msg; + error_msg << "Invalid principal type: " << principal_type; + return Status(error_msg.str()); + } break; } case TCatalogObjectType::CATALOG: @@ -224,6 +254,64 @@ Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type, return Status::OK(); } +Status TPrivilegeFromObjectName(const string& object_name, TPrivilege* privilege) { + DCHECK(privilege != nullptr); + // Format: + // server=val->action=val->grantoption=[true|false] + // server=val->uri=val->action=val->grantoption=[true|false] + // server=val->db=val->action=val->grantoption=[true|false] + // server=val->db=val->table=val->action=val->grantoption=[true|false] + // server=val->db=val->table=val->column=val->action=val->grantoption=[true|false] + vector<string> split; + boost::algorithm::split_regex(split, object_name, boost::regex("->")); + for (const auto& s: split) { + vector<string> key_value; + boost::split(key_value, s, [](char c){ return c == '='; }); + if (key_value.size() != 2) { + stringstream error_msg; + error_msg << "Invalid field name/value format: " << s; + return Status(error_msg.str()); + } + + if (key_value[0] == "server") { + privilege->__set_server_name(key_value[1]); + privilege->__set_scope(TPrivilegeScope::SERVER); + } else if (key_value[0] == "uri") { + privilege->__set_uri(key_value[1]); + privilege->__set_scope(TPrivilegeScope::URI); + } else if (key_value[0] == "db") { + privilege->__set_db_name(key_value[1]); + privilege->__set_scope(TPrivilegeScope::DATABASE); + } else if (key_value[0] == "table") { + privilege->__set_table_name(key_value[1]); + privilege->__set_scope(TPrivilegeScope::TABLE); + } else if (key_value[0] == "column") { + privilege->__set_column_name(key_value[1]); + privilege->__set_scope(TPrivilegeScope::COLUMN); + } else if (key_value[0] == "action") { + TPrivilegeLevel::type privilege_level; + Status status = TPrivilegeLevelFromObjectName(key_value[1], &privilege_level); + if (!status.ok()) return status; + privilege->__set_privilege_level(privilege_level); + } else if (key_value[0] == "grantoption") { + if (key_value[1] == "true") { + privilege->__set_has_grant_opt(true); + } else if (key_value[1] == "false") { + privilege->__set_has_grant_opt(false); + } else { + stringstream error_msg; + error_msg << "Invalid grant option: " << key_value[1]; + return Status(error_msg.str()); + } + } else { + stringstream error_msg; + error_msg << "Invalid privilege field name: " << key_value[0]; + return Status(error_msg.str()); + } + } + return Status::OK(); +} + Status CompressCatalogObject(const uint8_t* src, uint32_t size, string* dst) { scoped_ptr<Codec> compressor; RETURN_IF_ERROR(Codec::CreateCompressor(nullptr, false, THdfsCompression::LZ4, @@ -252,4 +340,31 @@ Status DecompressCatalogObject(const uint8_t* src, uint32_t size, string* dst) { return Status::OK(); } +Status TPrivilegeLevelFromObjectName(const std::string& object_name, + TPrivilegeLevel::type* privilege_level) { + DCHECK(privilege_level != nullptr); + if (object_name == "all") { + *privilege_level = TPrivilegeLevel::ALL; + } else if (object_name == "insert") { + *privilege_level = TPrivilegeLevel::INSERT; + } else if (object_name == "select") { + *privilege_level = TPrivilegeLevel::SELECT; + } else if (object_name == "refresh") { + *privilege_level = TPrivilegeLevel::REFRESH; + } else if (object_name == "create") { + *privilege_level = TPrivilegeLevel::CREATE; + } else if (object_name == "alter") { + *privilege_level = TPrivilegeLevel::ALTER; + } else if (object_name == "drop") { + *privilege_level = TPrivilegeLevel::DROP; + } else if (object_name == "owner") { + *privilege_level = TPrivilegeLevel::OWNER; + } else { + stringstream error_msg; + error_msg << "Invalid privilege level: " << object_name; + return Status(error_msg.str()); + } + return Status::OK(); +} + } http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/be/src/catalog/catalog-util.h ---------------------------------------------------------------------- diff --git a/be/src/catalog/catalog-util.h b/be/src/catalog/catalog-util.h index a01e9bb..bc50408 100644 --- a/be/src/catalog/catalog-util.h +++ b/be/src/catalog/catalog-util.h @@ -98,6 +98,9 @@ TCatalogObjectType::type TCatalogObjectTypeFromName(const std::string& name); Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type, const std::string& object_name, TCatalogObject* catalog_object); +/// Populates a TPrivilege based on the given object name string. +Status TPrivilegeFromObjectName(const std::string& object_name, TPrivilege* privilege); + /// Compresses a serialized catalog object using LZ4 and stores it back in 'dst'. Stores /// the size of the uncompressed catalog object in the first sizeof(uint32_t) bytes of /// 'dst'. The compression fails if the uncompressed data size exceeds 0x7E000000 bytes. @@ -109,7 +112,6 @@ Status CompressCatalogObject(const uint8_t* src, uint32_t size, std::string* dst /// catalog object. Status DecompressCatalogObject(const uint8_t* src, uint32_t size, std::string* dst) WARN_UNUSED_RESULT; - } #endif http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/fe/src/main/java/org/apache/impala/catalog/Catalog.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Catalog.java b/fe/src/main/java/org/apache/impala/catalog/Catalog.java index 879195f..fe49d5b 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Catalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/Catalog.java @@ -512,11 +512,11 @@ public abstract class Catalog { break; case PRIVILEGE: Principal tmpPrincipal = authPolicy_.getPrincipal( - objectDesc.getPrincipal().getPrincipal_id(), - objectDesc.getPrincipal().getPrincipal_type()); + objectDesc.getPrivilege().getPrincipal_id(), + objectDesc.getPrivilege().getPrincipal_type()); if (tmpPrincipal == null) { throw new CatalogException(String.format("No %s associated with ID: %d", - Principal.toString(objectDesc.getPrincipal().getPrincipal_type()) + Principal.toString(objectDesc.getPrivilege().getPrincipal_type()) .toLowerCase(), objectDesc.getPrivilege().getPrincipal_id())); } String privilegeName = PrincipalPrivilege.buildPrivilegeName( @@ -561,11 +561,13 @@ public abstract class Catalog { return "PRINCIPAL:" + catalogObject.getPrincipal().getPrincipal_name() .toLowerCase(); case PRIVILEGE: - // The combination of privilege name + principal ID is guaranteed to be unique. + // The combination of privilege name + principal ID + principal type is + // guaranteed to be unique. return "PRIVILEGE:" + PrincipalPrivilege.buildPrivilegeName(catalogObject.getPrivilege()) .toLowerCase() + "." + - Integer.toString(catalogObject.getPrivilege().getPrincipal_id()); + Integer.toString(catalogObject.getPrivilege().getPrincipal_id()) + "." + + catalogObject.getPrivilege().getPrincipal_type(); case HDFS_CACHE_POOL: return "HDFS_CACHE_POOL:" + catalogObject.getCache_pool().getPool_name().toLowerCase(); http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java index 39046f0..c375cfc 100644 --- a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java +++ b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java @@ -146,7 +146,7 @@ public class PrincipalPrivilege extends CatalogObjectImpl { @Override public String getUniqueName() { return "PRIVILEGE:" + getName().toLowerCase() + "." + Integer.toString( - getPrincipalId()); + getPrincipalId()) + "." + getPrincipalType().toString(); } public TCatalogObject toTCatalogObject() { http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/tests/authorization/test_authorization.py ---------------------------------------------------------------------- diff --git a/tests/authorization/test_authorization.py b/tests/authorization/test_authorization.py index cf5b2e9..9da43d3 100644 --- a/tests/authorization/test_authorization.py +++ b/tests/authorization/test_authorization.py @@ -23,6 +23,9 @@ import shutil import tempfile import json import grp +import re +import urllib + from time import sleep, time from getpass import getuser from ImpalaService import ImpalaHiveServer2Service @@ -438,3 +441,40 @@ class TestAuthorization(CustomClusterTestSuite): cols = row.split("\t") return cols[0:len(cols) - 1] assert map(columns, result.data) == expected + + @pytest.mark.execute_serially + @CustomClusterTestSuite.with_args( + impalad_args="--server_name=server1 --sentry_config=%s" % SENTRY_CONFIG_FILE, + catalogd_args="--sentry_config=%s" % SENTRY_CONFIG_FILE, + impala_log_dir=tempfile.mkdtemp(prefix="test_catalog_restart_", + dir=os.getenv("LOG_DIR"))) + def test_catalog_object(self, unique_role): + """IMPALA-7721: Tests /catalog_object web API for principal and privilege""" + self.role_cleanup(unique_role) + try: + self.client.execute("create role %s" % unique_role) + self.client.execute("grant select on database functional to role %s" % unique_role) + for service in [self.cluster.catalogd.service, + self.cluster.get_first_impalad().service]: + obj_dump = service.get_catalog_object_dump("PRINCIPAL", unique_role) + assert "catalog_version" in obj_dump + + # Get the privilege associated with that principal ID. + principal_id = re.search(r"principal_id \(i32\) = (\d+)", obj_dump) + assert principal_id is not None + obj_dump = service.get_catalog_object_dump("PRIVILEGE", urllib.quote( + "server=server1->db=functional->action=select->grantoption=false.%s.ROLE" % + principal_id.group(1))) + assert "catalog_version" in obj_dump + + # Get the principal that does not exist. + obj_dump = service.get_catalog_object_dump("PRINCIPAL", "doesnotexist") + assert "CatalogException" in obj_dump + + # Get the privilege that does not exist. + obj_dump = service.get_catalog_object_dump("PRIVILEGE", urllib.quote( + "server=server1->db=doesntexist->action=select->grantoption=false.%s.ROLE" % + principal_id.group(1))) + assert "CatalogException" in obj_dump + finally: + self.role_cleanup(unique_role)
