From e6363e15decdb156258eeb989ee58d980388b1ca Mon Sep 17 00:00:00 2001 From: Jesse Haber-Kucharsky Date: Wed, 3 Jan 2018 13:08:16 -0500 Subject: [PATCH] auth/resource: Construct from ctor The motivation behind this change is the idea that constructing a new instance of an object is the job of the constructor. One big benefit of this structure (with the addition of helpers for convenience) is that calls for emplacing instances (like `std::make_shared`, or `std::vector::emplace_back`) work without any difficulty. This would not be true for static construction functions. --- auth/default_authorizer.cc | 4 +- auth/password_authenticator.cc | 2 +- auth/resource.cc | 82 +++++++++---------- auth/resource.hh | 48 +++++++---- auth/service.cc | 4 +- cql3/Cql.g | 10 +-- cql3/statements/authorization_statement.cc | 2 +- cql3/statements/role-management-statements.cc | 14 ++-- service/client_state.cc | 12 +-- tests/auth_resource_test.cc | 67 +++++++-------- 10 files changed, 132 insertions(+), 113 deletions(-) diff --git a/auth/default_authorizer.cc b/auth/default_authorizer.cc index c8af2accc2..5580ae9177 100644 --- a/auth/default_authorizer.cc +++ b/auth/default_authorizer.cc @@ -260,7 +260,7 @@ future> auth::default_authorizer::list( for (auto& row : *res) { if (row.has(PERMISSIONS_NAME)) { auto username = row.get_as(ROLE_NAME); - auto resource = resource::from_name(row.get_as(RESOURCE_NAME)); + auto resource = parse_resource(row.get_as(RESOURCE_NAME)); auto ps = permissions::from_strings(row.get_set(PERMISSIONS_NAME)); ps = permission_set::from_mask(ps.mask() & set.mask()); @@ -337,6 +337,6 @@ future<> auth::default_authorizer::revoke_all(resource resource) { } const auth::resource_set& auth::default_authorizer::protected_resources() { - static const resource_set resources({ resource::data(meta::AUTH_KS, PERMISSIONS_CF) }); + static const resource_set resources({ make_data_resource(meta::AUTH_KS, PERMISSIONS_CF) }); return resources; } diff --git a/auth/password_authenticator.cc b/auth/password_authenticator.cc index 022058e583..6db91bf0b0 100644 --- a/auth/password_authenticator.cc +++ b/auth/password_authenticator.cc @@ -302,7 +302,7 @@ future<> auth::password_authenticator::drop(sstring username) { } const auth::resource_set& auth::password_authenticator::protected_resources() const { - static const resource_set resources({resource::data(meta::AUTH_KS, meta::roles_table::name)}); + static const resource_set resources({make_data_resource(meta::AUTH_KS, meta::roles_table::name)}); return resources; } diff --git a/auth/resource.cc b/auth/resource.cc index 82efdb228b..54afda2da3 100644 --- a/auth/resource.cc +++ b/auth/resource.cc @@ -109,53 +109,16 @@ resource::resource(resource_kind kind, std::vector parts) : resource(ki _parts.insert(_parts.end(), std::make_move_iterator(parts.begin()), std::make_move_iterator(parts.end())); } -resource resource::data(stdx::string_view keyspace) { - return resource(resource_kind::data, std::vector{sstring(keyspace)}); +resource::resource(data_resource_t, stdx::string_view keyspace) + : resource(resource_kind::data, std::vector{sstring(keyspace)}) { } -resource resource::data(stdx::string_view keyspace, stdx::string_view table) { - return resource(resource_kind::data, std::vector{sstring(keyspace), sstring(table)}); +resource::resource(data_resource_t, stdx::string_view keyspace, stdx::string_view table) + : resource(resource_kind::data, std::vector{sstring(keyspace), sstring(table)}) { } -resource resource::role(stdx::string_view role) { - return resource(resource_kind::role, std::vector{sstring(role)}); -} - -resource resource::from_name(stdx::string_view name) { - static const std::unordered_map reverse_roots = [] { - std::unordered_map result; - - for (const auto& pair : roots) { - result.emplace(pair.second, pair.first); - } - - return result; - }(); - - std::vector parts; - boost::split(parts, name, [](char ch) { return ch == '/'; }); - - if (parts.empty()) { - throw invalid_resource_name(name); - } - - const auto iter = reverse_roots.find(parts[0]); - if (iter == reverse_roots.end()) { - throw invalid_resource_name(name); - } - - const auto kind = iter->second; - parts.erase(parts.begin()); - - if (parts.size() > max_parts.at(kind)) { - throw invalid_resource_name(name); - } - - return resource(kind, std::move(parts)); -} - -resource resource::root_of(resource_kind kind) { - return resource(kind); +resource::resource(role_resource_t, stdx::string_view role) + : resource(resource_kind::role, std::vector{sstring(role)}) { } sstring resource::name() const { @@ -284,4 +247,37 @@ std::ostream& operator<<(std::ostream& os, const role_resource_view& v) { return os; } +resource parse_resource(stdx::string_view name) { + static const std::unordered_map reverse_roots = [] { + std::unordered_map result; + + for (const auto& pair : roots) { + result.emplace(pair.second, pair.first); + } + + return result; + }(); + + std::vector parts; + boost::split(parts, name, [](char ch) { return ch == '/'; }); + + if (parts.empty()) { + throw invalid_resource_name(name); + } + + const auto iter = reverse_roots.find(parts[0]); + if (iter == reverse_roots.end()) { + throw invalid_resource_name(name); + } + + const auto kind = iter->second; + parts.erase(parts.begin()); + + if (parts.size() > max_parts.at(kind)) { + throw invalid_resource_name(name); + } + + return resource(kind, std::move(parts)); +} + } diff --git a/auth/resource.hh b/auth/resource.hh index 00f0a90df7..fff20d1113 100644 --- a/auth/resource.hh +++ b/auth/resource.hh @@ -80,6 +80,16 @@ enum class resource_kind { std::ostream& operator<<(std::ostream&, resource_kind); +/// +/// Type tag for constructing data resources. +/// +struct data_resource_t final {}; + +/// +/// Type tag for constructing role resources. +/// +struct role_resource_t final {}; + /// /// Resources are entities that users can be granted permissions on. /// @@ -99,19 +109,13 @@ class resource final { std::vector _parts; public: - static resource root_of(resource_kind); - - static resource data(stdx::string_view keyspace); - static resource data(stdx::string_view keyspace, stdx::string_view table); - - static resource role(stdx::string_view role); - /// - /// Parse a resource name. + /// A root resource of a particular kind. /// - /// \throws \ref invalid_resource_name when the name is malformed. - /// - static resource from_name(stdx::string_view); + explicit resource(resource_kind); + resource(data_resource_t, stdx::string_view keyspace); + resource(data_resource_t, stdx::string_view keyspace, stdx::string_view table); + resource(role_resource_t, stdx::string_view role); resource_kind kind() const noexcept { return _kind; @@ -127,9 +131,6 @@ public: permission_set applicable_permissions() const; private: - // A root resource. - explicit resource(resource_kind kind); - resource(resource_kind, std::vector parts); friend class std::hash; @@ -138,6 +139,7 @@ private: friend bool operator<(const resource&, const resource&); friend bool operator==(const resource&, const resource&); + friend resource parse_resource(stdx::string_view); }; bool operator<(const resource&, const resource&); @@ -200,6 +202,24 @@ public: std::ostream& operator<<(std::ostream&, const role_resource_view&); +/// +/// Parse a resource from its name. +/// +/// \throws \ref invalid_resource_name when the name is malformed. +/// +resource parse_resource(stdx::string_view name); + +inline resource make_data_resource(stdx::string_view keyspace) { + return resource(data_resource_t{}, keyspace); +} +inline resource make_data_resource(stdx::string_view keyspace, stdx::string_view table) { + return resource(data_resource_t{}, keyspace, table); +} + +inline resource make_role_resource(stdx::string_view role) { + return resource(role_resource_t{}, role); +} + } namespace std { diff --git a/auth/service.cc b/auth/service.cc index 3533f170ac..7cda72f2d1 100644 --- a/auth/service.cc +++ b/auth/service.cc @@ -75,11 +75,11 @@ private: void on_update_view(const sstring& ks_name, const sstring& view_name, bool columns_changed) override {} void on_drop_keyspace(const sstring& ks_name) override { - _authorizer.revoke_all(auth::resource::data(ks_name)); + _authorizer.revoke_all(auth::make_data_resource(ks_name)); } void on_drop_column_family(const sstring& ks_name, const sstring& cf_name) override { - _authorizer.revoke_all(auth::resource::data(ks_name, cf_name)); + _authorizer.revoke_all(auth::make_data_resource(ks_name, cf_name)); } void on_drop_user_type(const sstring& ks_name, const sstring& type_name) override {} diff --git a/cql3/Cql.g b/cql3/Cql.g index cf562b66ee..f5c27571e4 100644 --- a/cql3/Cql.g +++ b/cql3/Cql.g @@ -1047,15 +1047,15 @@ resource returns [uninitialized res] ; dataResource returns [uninitialized res] - : K_ALL K_KEYSPACES { $res = auth::resource::root_of(auth::resource_kind::data); } - | K_KEYSPACE ks = keyspaceName { $res = auth::resource::data($ks.id); } + : K_ALL K_KEYSPACES { $res = auth::resource(auth::resource_kind::data); } + | K_KEYSPACE ks = keyspaceName { $res = auth::make_data_resource($ks.id); } | ( K_COLUMNFAMILY )? cf = columnFamilyName - { $res = auth::resource::data($cf.name->get_keyspace(), $cf.name->get_column_family()); } + { $res = auth::make_data_resource($cf.name->get_keyspace(), $cf.name->get_column_family()); } ; roleResource returns [uninitialized res] - : K_ALL K_ROLES { $res = auth::resource::root_of(auth::resource_kind::role); } - | K_ROLE role = userOrRoleName { $res = auth::resource::role(static_cast(role).to_string()); } + : K_ALL K_ROLES { $res = auth::resource(auth::resource_kind::role); } + | K_ROLE role = userOrRoleName { $res = auth::make_role_resource(static_cast(role).to_string()); } ; /** diff --git a/cql3/statements/authorization_statement.cc b/cql3/statements/authorization_statement.cc index c3115833e6..183e4acd1f 100644 --- a/cql3/statements/authorization_statement.cc +++ b/cql3/statements/authorization_statement.cc @@ -89,7 +89,7 @@ void cql3::statements::authorization_statement::maybe_correct_resource(auth::res const auto table = data_view.table(); if (table && keyspace->empty()) { - resource = auth::resource::data(state.get_keyspace(), *table); + resource = auth::make_data_resource(state.get_keyspace(), *table); } } } diff --git a/cql3/statements/role-management-statements.cc b/cql3/statements/role-management-statements.cc index 91f64068dd..2b7c7991db 100644 --- a/cql3/statements/role-management-statements.cc +++ b/cql3/statements/role-management-statements.cc @@ -87,7 +87,7 @@ future<> create_role_statement::check_access(const service::client_state& state) state.ensure_not_anonymous(); return async([this, &state] { - state.ensure_has_permission(auth::permission::CREATE, auth::resource::root_of(auth::resource_kind::role)).get0(); + state.ensure_has_permission(auth::permission::CREATE, auth::resource(auth::resource_kind::role)).get0(); if (*_options.is_superuser) { if (!auth::has_superuser(*state.get_auth_service(), *state.user()).get0()) { @@ -152,7 +152,7 @@ future<> alter_role_statement::check_access(const service::client_state& state) } if (*user.name != _role) { - state.ensure_has_permission(auth::permission::ALTER, auth::resource::role(_role)).get0(); + state.ensure_has_permission(auth::permission::ALTER, auth::make_role_resource(_role)).get0(); } else { const auto alterable_options = state.get_auth_service()->underlying_authenticator().alterable_options(); @@ -210,7 +210,7 @@ future<> drop_role_statement::check_access(const service::client_state& state) { state.ensure_not_anonymous(); return async([this, &state] { - state.ensure_has_permission(auth::permission::DROP, auth::resource::role(_role)).get0(); + state.ensure_has_permission(auth::permission::DROP, auth::make_role_resource(_role)).get0(); auto& as = *state.get_auth_service(); @@ -259,7 +259,7 @@ future<> list_roles_statement::check_access(const service::client_state& state) return async([this, &state] { if (state.check_has_permission( auth::permission::DESCRIBE, - auth::resource::root_of(auth::resource_kind::role)).get0()) { + auth::resource(auth::resource_kind::role)).get0()) { return; } @@ -342,7 +342,7 @@ list_roles_statement::execute(distributed&, service::que // only the roles granted to them. return cs.check_has_permission( auth::permission::DESCRIBE, - auth::resource::root_of(auth::resource_kind::role)).then([&cs, &rm, query_mode](bool has_describe) { + auth::resource(auth::resource_kind::role)).then([&cs, &rm, query_mode](bool has_describe) { if (has_describe) { return rm.query_all().then([&rm](auto&& roles) { return make_results(rm, std::move(roles)); }); } @@ -368,7 +368,7 @@ list_roles_statement::execute(distributed&, service::que future<> grant_role_statement::check_access(const service::client_state& state) { state.ensure_not_anonymous(); - return state.ensure_has_permission(auth::permission::AUTHORIZE, auth::resource::role(_role)); + return state.ensure_has_permission(auth::permission::AUTHORIZE, auth::make_role_resource(_role)); } future @@ -392,7 +392,7 @@ grant_role_statement::execute(distributed&, service::que future<> revoke_role_statement::check_access(const service::client_state& state) { state.ensure_not_anonymous(); - return state.ensure_has_permission(auth::permission::AUTHORIZE, auth::resource::role(_role)); + return state.ensure_has_permission(auth::permission::AUTHORIZE, auth::make_role_resource(_role)); } future<::shared_ptr> diff --git a/service/client_state.cc b/service/client_state.cc index 8bc6717012..e377f43fed 100644 --- a/service/client_state.cc +++ b/service/client_state.cc @@ -106,22 +106,22 @@ future<> service::client_state::has_all_keyspaces_access( return make_ready_future(); } validate_login(); - return ensure_has_permission(p, auth::resource::root_of(auth::resource_kind::data)); + return ensure_has_permission(p, auth::resource(auth::resource_kind::data)); } future<> service::client_state::has_keyspace_access(const sstring& ks, auth::permission p) const { - return has_access(ks, p, auth::resource::data(ks)); + return has_access(ks, p, auth::make_data_resource(ks)); } future<> service::client_state::has_column_family_access(const sstring& ks, const sstring& cf, auth::permission p) const { validation::validate_column_family(ks, cf); - return has_access(ks, p, auth::resource::data(ks, cf)); + return has_access(ks, p, auth::make_data_resource(ks, cf)); } future<> service::client_state::has_schema_access(const schema& s, auth::permission p) const { - return has_access(s.ks_name(), p, auth::resource::data(s.ks_name(), s.cf_name())); + return has_access(s.ks_name(), p, auth::make_data_resource(s.ks_name(), s.cf_name())); } future<> service::client_state::has_access(const sstring& ks, auth::permission p, auth::resource resource) const { @@ -157,10 +157,10 @@ future<> service::client_state::has_access(const sstring& ks, auth::permission p static thread_local std::set readable_system_resources = [] { std::set tmp; for (auto cf : { db::system_keyspace::LOCAL, db::system_keyspace::PEERS }) { - tmp.insert(auth::resource::data(db::system_keyspace::NAME, cf)); + tmp.insert(auth::make_data_resource(db::system_keyspace::NAME, cf)); } for (auto cf : db::schema_tables::ALL) { - tmp.insert(auth::resource::data(db::schema_tables::NAME, cf)); + tmp.insert(auth::make_data_resource(db::schema_tables::NAME, cf)); } return tmp; }(); diff --git a/tests/auth_resource_test.cc b/tests/auth_resource_test.cc index 464fdeea51..6aba18d385 100644 --- a/tests/auth_resource_test.cc +++ b/tests/auth_resource_test.cc @@ -32,7 +32,7 @@ BOOST_AUTO_TEST_CASE(root_of) { // data // - const auto dr = auth::resource::root_of(auth::resource_kind::data); + const auto dr = auth::resource(auth::resource_kind::data); BOOST_REQUIRE_EQUAL(dr.kind(), auth::resource_kind::data); const auto dv = auth::data_resource_view(dr); @@ -43,7 +43,7 @@ BOOST_AUTO_TEST_CASE(root_of) { // role // - const auto rr = auth::resource::root_of(auth::resource_kind::role); + const auto rr = auth::resource(auth::resource_kind::role); BOOST_REQUIRE_EQUAL(rr.kind(), auth::resource_kind::role); const auto rv = auth::role_resource_view(rr); @@ -51,14 +51,14 @@ BOOST_AUTO_TEST_CASE(root_of) { } BOOST_AUTO_TEST_CASE(data) { - const auto r1 = auth::resource::data("my_keyspace"); + const auto r1 = auth::make_data_resource("my_keyspace"); BOOST_REQUIRE_EQUAL(r1.kind(), auth::resource_kind::data); const auto v1 = auth::data_resource_view(r1); BOOST_REQUIRE_EQUAL(*v1.keyspace(), "my_keyspace"); BOOST_REQUIRE(!v1.table()); - const auto r2 = auth::resource::data("my_keyspace", "my_table"); + const auto r2 = auth::make_data_resource("my_keyspace", "my_table"); BOOST_REQUIRE_EQUAL(r2.kind(), auth::resource_kind::data); const auto v2 = auth::data_resource_view(r2); @@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(data) { } BOOST_AUTO_TEST_CASE(role) { - const auto r = auth::resource::role("joe"); + const auto r = auth::make_role_resource("joe"); BOOST_REQUIRE_EQUAL(r.kind(), auth::resource_kind::role); const auto v = auth::role_resource_view(r); @@ -79,35 +79,35 @@ BOOST_AUTO_TEST_CASE(from_name) { // data // - const auto dr1 = auth::resource::from_name("data"); - BOOST_REQUIRE_EQUAL(dr1, auth::resource::root_of(auth::resource_kind::data)); + const auto dr1 = auth::parse_resource("data"); + BOOST_REQUIRE_EQUAL(dr1, auth::resource(auth::resource_kind::data)); - const auto dr2 = auth::resource::from_name("data/my_keyspace"); - BOOST_REQUIRE_EQUAL(dr2, auth::resource::data("my_keyspace")); + const auto dr2 = auth::parse_resource("data/my_keyspace"); + BOOST_REQUIRE_EQUAL(dr2, auth::make_data_resource("my_keyspace")); - const auto dr3 = auth::resource::from_name("data/my_keyspace/my_table"); - BOOST_REQUIRE_EQUAL(dr3, auth::resource::data("my_keyspace", "my_table")); + const auto dr3 = auth::parse_resource("data/my_keyspace/my_table"); + BOOST_REQUIRE_EQUAL(dr3, auth::make_data_resource("my_keyspace", "my_table")); - BOOST_REQUIRE_THROW(auth::resource::from_name("data/foo/bar/baz"), auth::invalid_resource_name); + BOOST_REQUIRE_THROW(auth::parse_resource("data/foo/bar/baz"), auth::invalid_resource_name); // // role // - const auto rr1 = auth::resource::from_name("roles"); - BOOST_REQUIRE_EQUAL(rr1, auth::resource::root_of(auth::resource_kind::role)); + const auto rr1 = auth::parse_resource("roles"); + BOOST_REQUIRE_EQUAL(rr1, auth::resource(auth::resource_kind::role)); - const auto rr2 = auth::resource::from_name("roles/joe"); - BOOST_REQUIRE_EQUAL(rr2, auth::resource::role("joe")); + const auto rr2 = auth::parse_resource("roles/joe"); + BOOST_REQUIRE_EQUAL(rr2, auth::make_role_resource("joe")); - BOOST_REQUIRE_THROW(auth::resource::from_name("roles/joe/smith"), auth::invalid_resource_name); + BOOST_REQUIRE_THROW(auth::parse_resource("roles/joe/smith"), auth::invalid_resource_name); // // Generic errors. // - BOOST_REQUIRE_THROW(auth::resource::from_name("animal/horse"), auth::invalid_resource_name); - BOOST_REQUIRE_THROW(auth::resource::from_name(""), auth::invalid_resource_name); + BOOST_REQUIRE_THROW(auth::parse_resource("animal/horse"), auth::invalid_resource_name); + BOOST_REQUIRE_THROW(auth::parse_resource(""), auth::invalid_resource_name); } BOOST_AUTO_TEST_CASE(name) { @@ -115,28 +115,28 @@ BOOST_AUTO_TEST_CASE(name) { // data // - BOOST_REQUIRE_EQUAL(auth::resource::root_of(auth::resource_kind::data).name(), "data"); - BOOST_REQUIRE_EQUAL(auth::resource::data("my_keyspace").name(), "data/my_keyspace"); - BOOST_REQUIRE_EQUAL(auth::resource::data("my_keyspace", "my_table").name(), "data/my_keyspace/my_table"); + BOOST_REQUIRE_EQUAL(auth::resource(auth::resource_kind::data).name(), "data"); + BOOST_REQUIRE_EQUAL(auth::make_data_resource("my_keyspace").name(), "data/my_keyspace"); + BOOST_REQUIRE_EQUAL(auth::make_data_resource("my_keyspace", "my_table").name(), "data/my_keyspace/my_table"); // // role // - BOOST_REQUIRE_EQUAL(auth::resource::root_of(auth::resource_kind::role).name(), "roles"); - BOOST_REQUIRE_EQUAL(auth::resource::role("joe").name(), "roles/joe"); + BOOST_REQUIRE_EQUAL(auth::resource(auth::resource_kind::role).name(), "roles"); + BOOST_REQUIRE_EQUAL(auth::make_role_resource("joe").name(), "roles/joe"); } BOOST_AUTO_TEST_CASE(parent) { - const auto r1 = auth::resource::data("my_keyspace", "my_table"); + const auto r1 = auth::make_data_resource("my_keyspace", "my_table"); const auto r2 = r1.parent(); BOOST_REQUIRE(r2); - BOOST_REQUIRE_EQUAL(*r2, auth::resource::data("my_keyspace")); + BOOST_REQUIRE_EQUAL(*r2, auth::make_data_resource("my_keyspace")); const auto r3 = r2->parent(); BOOST_REQUIRE(r3); - BOOST_REQUIRE_EQUAL(*r3, auth::resource::root_of(auth::resource_kind::data)); + BOOST_REQUIRE_EQUAL(*r3, auth::resource(auth::resource_kind::data)); const auto r4 = r3->parent(); BOOST_REQUIRE(!r4); @@ -147,14 +147,17 @@ BOOST_AUTO_TEST_CASE(output) { // data // - BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource::root_of(auth::resource_kind::data)), ""); - BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource::data("my_keyspace")), ""); - BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource::data("my_keyspace", "my_table")), ""); + BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource(auth::resource_kind::data)), ""); + BOOST_REQUIRE_EQUAL(sprint("%s", auth::make_data_resource("my_keyspace")), ""); + + BOOST_REQUIRE_EQUAL( + sprint("%s", auth::make_data_resource("my_keyspace", "my_table")), + "
"); // // role // - BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource::root_of(auth::resource_kind::role)), ""); - BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource::role("joe")), ""); + BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource(auth::resource_kind::role)), ""); + BOOST_REQUIRE_EQUAL(sprint("%s", auth::make_role_resource("joe")), ""); }