auth: Pass resource by const ref.

This has the dual benefit of not enforcing copying on implementations of
the abstract interface and also limiting unnecessary copies.

As usual with Seastar, we follow the convention that a reference
parameter to a function is assumed valid for the duration of the
`future` that is returned. `do_with` helps here.

By adding some constants for root resources, we can avoid using
`seastar::do_with` at some call-sites involving `resource` instances.
This commit is contained in:
Jesse Haber-Kucharsky
2018-02-14 00:53:03 -05:00
parent 45631604b0
commit c1504cd4ff
16 changed files with 134 additions and 89 deletions

View File

@@ -56,23 +56,23 @@ public:
return allow_all_authorizer_name();
}
virtual future<permission_set> authorize(service&, stdx::string_view, resource) const override {
virtual future<permission_set> authorize(service&, stdx::string_view, const resource&) const override {
return make_ready_future<permission_set>(permissions::ALL);
}
virtual future<> grant(permission_set, resource, stdx::string_view) override {
virtual future<> grant(permission_set, const resource&, stdx::string_view) override {
throw exceptions::invalid_request_exception("GRANT operation is not supported by AllowAllAuthorizer");
}
virtual future<> revoke(permission_set, resource, stdx::string_view) override {
virtual future<> revoke(permission_set, const resource&, stdx::string_view) override {
throw exceptions::invalid_request_exception("REVOKE operation is not supported by AllowAllAuthorizer");
}
virtual future<std::vector<permission_details>> list(
service&,
permission_set,
std::optional<resource>,
std::optional<stdx::string_view>) const override {
const std::optional<resource>&,
const std::optional<stdx::string_view>&) const override {
throw exceptions::invalid_request_exception("LIST PERMISSIONS operation is not supported by AllowAllAuthorizer");
}
@@ -80,7 +80,7 @@ public:
return make_ready_future();
}
virtual future<> revoke_all(resource) override {
virtual future<> revoke_all(const resource&) override {
return make_ready_future();
}

View File

@@ -90,17 +90,17 @@ public:
///
/// The resulting permission set includes permissions obtained transitively through roles granted to this role.
///
virtual future<permission_set> authorize(service&, stdx::string_view role_name, resource) const = 0;
virtual future<permission_set> authorize(service&, stdx::string_view role_name, const resource&) const = 0;
///
/// Grant a set of permissions to a user for a particular \ref resource.
///
virtual future<> grant(permission_set, resource, stdx::string_view role_name) = 0;
virtual future<> grant(permission_set, const resource&, stdx::string_view role_name) = 0;
///
/// Revoke a set of permissions from a user for a particular \ref resource.
///
virtual future<> revoke(permission_set, resource, stdx::string_view role_name) = 0;
virtual future<> revoke(permission_set, const resource&, stdx::string_view role_name) = 0;
///
/// Query for granted permissions.
@@ -116,8 +116,8 @@ public:
list(
service&,
permission_set matching,
std::optional<resource> resource,
std::optional<stdx::string_view> role_name) const = 0;
const std::optional<resource>& resource,
const std::optional<stdx::string_view>& role_name) const = 0;
///
/// Revoke all permissions granted to a particular user.
@@ -127,7 +127,7 @@ public:
///
/// Revoke all permissions granted to any user for a particular resource.
///
virtual future<> revoke_all(resource) = 0;
virtual future<> revoke_all(const resource&) = 0;
///
/// System resources used internally as part of the implementation. These are made inaccessible to users.

View File

@@ -153,8 +153,8 @@ future<permission_set> default_authorizer::authorize_role_directly(
future<permission_set> default_authorizer::authorize(
service& ser,
stdx::string_view role_name,
resource resource) const {
return do_with(permission_set(), std::move(resource), [this, &ser, role_name](auto& ps, const auto& r) {
const resource& r) const {
return do_with(permission_set(), [this, &ser, role_name, &r](auto& ps) {
return ser.get_roles(role_name).then([this, &ser, &ps, &r](std::unordered_set<sstring> all_roles) {
return do_with(std::move(all_roles), [this, &ser, &ps, &r](const auto& all_roles) {
return parallel_for_each(all_roles, [this, &ser, &ps, &r](stdx::string_view role_name) {
@@ -170,7 +170,11 @@ future<permission_set> default_authorizer::authorize(
}
future<>
default_authorizer::modify(permission_set set, resource resource, stdx::string_view role_name, stdx::string_view op) {
default_authorizer::modify(
permission_set set,
const resource& resource,
stdx::string_view role_name,
stdx::string_view op) {
// TODO: why does this not check super user?
auto query = sprint(
"UPDATE %s.%s SET %s = %s %s ? WHERE %s = ? AND %s = ?",
@@ -189,19 +193,19 @@ default_authorizer::modify(permission_set set, resource resource, stdx::string_v
}
future<> default_authorizer::grant(permission_set set, resource resource, stdx::string_view role_name) {
return modify(std::move(set), std::move(resource), role_name, "+");
future<> default_authorizer::grant(permission_set set, const resource& resource, stdx::string_view role_name) {
return modify(std::move(set), resource, role_name, "+");
}
future<> default_authorizer::revoke(permission_set set, resource resource, stdx::string_view role_name) {
return modify(std::move(set), std::move(resource), role_name, "-");
future<> default_authorizer::revoke(permission_set set, const resource& resource, stdx::string_view role_name) {
return modify(std::move(set), resource, role_name, "-");
}
future<std::vector<permission_details>> default_authorizer::list(
service& ser,
permission_set set,
std::optional<resource> resource,
std::optional<stdx::string_view> role_name) const {
const std::optional<resource>& resource,
const std::optional<stdx::string_view>& role_name) const {
sstring query = sprint(
"SELECT %s, %s, %s FROM %s.%s",
ROLE_NAME,
@@ -215,7 +219,7 @@ future<std::vector<permission_details>> default_authorizer::list(
future<::shared_ptr<cql3::untyped_result_set>> f = make_ready_future<::shared_ptr<cql3::untyped_result_set>>();
if (role_name) {
f = ser.get_roles(*role_name).then([this, resource = std::move(resource), query, &f](
f = ser.get_roles(*role_name).then([this, &resource, query, &f](
std::unordered_set<sstring> all_roles) mutable {
if (resource) {
query += sprint(" WHERE %s IN ? AND %s = ?", ROLE_NAME, RESOURCE_NAME);
@@ -268,7 +272,7 @@ future<> default_authorizer::revoke_all(stdx::string_view role_name) {
});
}
future<> default_authorizer::revoke_all(resource resource) {
future<> default_authorizer::revoke_all(const resource& resource) {
auto query = sprint(
"SELECT %s FROM %s.%s WHERE %s = ? ALLOW FILTERING",
ROLE_NAME,

View File

@@ -69,23 +69,27 @@ public:
return default_authorizer_name();
}
virtual future<permission_set> authorize(service&, stdx::string_view, resource) const override;
virtual future<permission_set> authorize(service&, stdx::string_view, const resource&) const override;
virtual future<> grant(permission_set, resource, stdx::string_view) override;
virtual future<> grant(permission_set, const resource&, stdx::string_view) override;
virtual future<> revoke(permission_set, resource, stdx::string_view) override;
virtual future<> revoke(permission_set, const resource&, stdx::string_view) override;
virtual future<std::vector<permission_details>>
list(service&, permission_set, std::optional<resource>, std::optional<stdx::string_view>) const override;
list(
service&,
permission_set,
const std::optional<resource>&,
const std::optional<stdx::string_view>&) const override;
virtual future<> revoke_all(stdx::string_view) override;
virtual future<> revoke_all(resource) override;
virtual future<> revoke_all(const resource&) override;
virtual const resource_set& protected_resources() const override;
private:
future<> modify(permission_set, resource, stdx::string_view, stdx::string_view);
future<> modify(permission_set, const resource&, stdx::string_view, stdx::string_view);
///
/// Permissions granted directly to a role, rather than those inherited.

View File

@@ -44,8 +44,8 @@ permissions_cache::permissions_cache(const permissions_cache_config& c, service&
}) {
}
future<permission_set> permissions_cache::get(stdx::string_view role_name, resource r) {
return do_with(key_type(sstring(role_name), std::move(r)), [this](const auto& k) {
future<permission_set> permissions_cache::get(stdx::string_view role_name, const resource& r) {
return do_with(key_type(sstring(role_name), r), [this](const auto& k) {
return _cache.get(k);
});
}

View File

@@ -83,7 +83,7 @@ public:
return _cache.stop();
}
future<permission_set> get(stdx::string_view role_name, resource);
future<permission_set> get(stdx::string_view role_name, const resource&);
};
}

View File

@@ -280,4 +280,16 @@ resource parse_resource(stdx::string_view name) {
return resource(kind, std::move(parts));
}
static const resource the_root_data_resource{resource_kind::data};
const resource& root_data_resource() {
return the_root_data_resource;
}
static const resource the_root_role_resource{resource_kind::role};
const resource& root_role_resource() {
return the_root_role_resource;
}
}

View File

@@ -201,6 +201,8 @@ std::ostream& operator<<(std::ostream&, const role_resource_view&);
///
resource parse_resource(stdx::string_view name);
const resource& root_data_resource();
inline resource make_data_resource(stdx::string_view keyspace) {
return resource(data_resource_t{}, keyspace);
}
@@ -208,6 +210,8 @@ inline resource make_data_resource(stdx::string_view keyspace, stdx::string_view
return resource(data_resource_t{}, keyspace, table);
}
const resource& root_role_resource();
inline resource make_role_resource(stdx::string_view role) {
return resource(role_resource_t{}, role);
}

View File

@@ -240,9 +240,9 @@ future<bool> service::has_existing_legacy_users() const {
});
}
future<permission_set> service::get_permissions(stdx::string_view role_name, resource r) const {
return validate_role_exists(*this, role_name).then([this, role_name, r = std::move(r)] {
return _permissions_cache->get(role_name, std::move(r));
future<permission_set> service::get_permissions(stdx::string_view role_name, const resource& r) const {
return validate_role_exists(*this, role_name).then([this, role_name, &r] {
return _permissions_cache->get(role_name, r);
});
}

View File

@@ -121,7 +121,7 @@ public:
///
/// \returns an exceptional future with \ref nonexistant_role if the named role does not exist.
///
future<permission_set> get_permissions(stdx::string_view role_name, resource) const;
future<permission_set> get_permissions(stdx::string_view role_name, const resource&) const;
///
/// Query whether the named role has been granted a role that is a superuser.

View File

@@ -205,8 +205,8 @@ public:
}
virtual future<permission_set>
authorize(service& ser, stdx::string_view role_name, resource resource) const override {
return ser.has_superuser(role_name).then([resource](bool s) {
authorize(service& ser, stdx::string_view role_name, const resource& resource) const override {
return ser.has_superuser(role_name).then([&resource](bool s) {
static const permission_set transitional_permissions =
permission_set::of<
permission::CREATE,
@@ -219,29 +219,29 @@ public:
});
}
virtual future<> grant(permission_set ps, resource r, stdx::string_view s) override {
return _authorizer->grant(std::move(ps), std::move(r), s);
virtual future<> grant(permission_set ps, const resource& r, stdx::string_view s) override {
return _authorizer->grant(std::move(ps), r, s);
}
virtual future<> revoke(permission_set ps, resource r, stdx::string_view s) override {
return _authorizer->revoke(std::move(ps), std::move(r), s);
virtual future<> revoke(permission_set ps, const resource& r, stdx::string_view s) override {
return _authorizer->revoke(std::move(ps), r, s);
}
virtual future<std::vector<permission_details>>
list(
service& ser,
permission_set ps,
std::optional<resource> r,
std::optional<stdx::string_view> s) const override {
return _authorizer->list(ser, std::move(ps), std::move(r), std::move(s));
const std::optional<resource>& r,
const std::optional<stdx::string_view>& s) const override {
return _authorizer->list(ser, std::move(ps), r, s);
}
virtual future<> revoke_all(stdx::string_view s) override {
return _authorizer->revoke_all(s);
}
virtual future<> revoke_all(resource r) override {
return _authorizer->revoke_all(std::move(r));
virtual future<> revoke_all(const resource& r) override {
return _authorizer->revoke_all(r);
}
virtual const resource_set& protected_resources() const override {

View File

@@ -154,8 +154,10 @@ cql3::statements::list_permissions_statement::execute(
return map_reduce(
resources,
[&state, this](opt_resource r) {
auto& auth_service = *state.get_client_state().get_auth_service();
return auth_service.underlying_authorizer().list(auth_service, _permissions, std::move(r), _username);
return do_with(std::move(r), [this, &state](const opt_resource& r) {
auto& auth_service = *state.get_client_state().get_auth_service();
return auth_service.underlying_authorizer().list(auth_service, _permissions, r, _username);
});
},
std::vector<auth::permission_details>(),
[](std::vector<auth::permission_details> details, std::vector<auth::permission_details> pd) {

View File

@@ -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(auth::resource_kind::role)).get0();
state.ensure_has_permission(auth::permission::CREATE, auth::root_role_resource()).get0();
if (*_options.is_superuser) {
if (!auth::has_superuser(*state.get_auth_service(), *state.user()).get0()) {
@@ -257,9 +257,7 @@ future<> list_roles_statement::check_access(const service::client_state& state)
state.ensure_not_anonymous();
return async([this, &state] {
if (state.check_has_permission(
auth::permission::DESCRIBE,
auth::resource(auth::resource_kind::role)).get0()) {
if (state.check_has_permission(auth::permission::DESCRIBE, auth::root_role_resource()).get0()) {
return;
}
@@ -342,7 +340,7 @@ list_roles_statement::execute(distributed<service::storage_proxy>&, service::que
// only the roles granted to them.
return cs.check_has_permission(
auth::permission::DESCRIBE,
auth::resource(auth::resource_kind::role)).then([&cs, &rm, query_mode](bool has_describe) {
auth::root_role_resource()).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)); });
}
@@ -367,7 +365,10 @@ list_roles_statement::execute(distributed<service::storage_proxy>&, 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::make_role_resource(_role));
return do_with(auth::make_role_resource(_role), [this, &state](const auto& r) {
return state.ensure_has_permission(auth::permission::AUTHORIZE, r);
});
}
future<result_message_ptr>
@@ -389,7 +390,10 @@ grant_role_statement::execute(distributed<service::storage_proxy>&, 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::make_role_resource(_role));
return do_with(auth::make_role_resource(_role), [this, &state](const auto& r) {
return state.ensure_has_permission(auth::permission::AUTHORIZE, r);
});
}
future<result_message_ptr> revoke_role_statement::execute(

View File

@@ -106,25 +106,38 @@ future<> service::client_state::has_all_keyspaces_access(
return make_ready_future();
}
validate_login();
return ensure_has_permission(p, auth::resource(auth::resource_kind::data));
return do_with(auth::resource(auth::resource_kind::data), [this, p](const auto& r) {
return ensure_has_permission(p, r);
});
}
future<> service::client_state::has_keyspace_access(const sstring& ks,
auth::permission p) const {
return has_access(ks, p, auth::make_data_resource(ks));
return do_with(ks, auth::make_data_resource(ks), [this, p](auto const& ks, auto const& r) {
return has_access(ks, p, r);
});
}
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::make_data_resource(ks, cf));
return do_with(ks, auth::make_data_resource(ks, cf), [this, p](const auto& ks, const auto& r) {
return has_access(ks, p, r);
});
}
future<> service::client_state::has_schema_access(const schema& s, auth::permission p) const {
return has_access(s.ks_name(), p, auth::make_data_resource(s.ks_name(), s.cf_name()));
return do_with(
s.ks_name(),
auth::make_data_resource(s.ks_name(),s.cf_name()),
[this, p](auto const& ks, auto const& r) {
return has_access(ks, p, r);
});
}
future<> service::client_state::has_access(const sstring& ks, auth::permission p, auth::resource resource) const {
future<> service::client_state::has_access(const sstring& ks, auth::permission p, const auth::resource& resource) const {
if (ks.empty()) {
throw exceptions::invalid_request_exception("You have not set a keyspace for this session");
}
@@ -179,34 +192,36 @@ future<> service::client_state::has_access(const sstring& ks, auth::permission p
}
}
return ensure_has_permission(p, std::move(resource));
return ensure_has_permission(p, resource);
}
future<bool> service::client_state::check_has_permission(auth::permission p, auth::resource resource) const {
future<bool> service::client_state::check_has_permission(auth::permission p, const auth::resource& r) const {
if (_is_internal) {
return make_ready_future<bool>(true);
}
std::optional<auth::resource> parent = resource.parent();
return _auth_service->get_permissions(*_user->name, resource).then([this, p, parent = std::move(parent)](auth::permission_set set) {
if (set.contains(p)) {
return make_ready_future<bool>(true);
}
if (parent) {
return check_has_permission(p, std::move(*parent));
}
return make_ready_future<bool>(false);
return do_with(r.parent(), [this, p, &r](const auto& parent_r) {
return _auth_service->get_permissions(*_user->name, r).then([this, p, &parent_r](auth::permission_set set) {
if (set.contains(p)) {
return make_ready_future<bool>(true);
}
if (parent_r) {
return check_has_permission(p, *parent_r);
}
return make_ready_future<bool>(false);
});
});
}
future<> service::client_state::ensure_has_permission(auth::permission p, auth::resource resource) const {
return check_has_permission(p, resource).then([this, p, resource](bool ok) {
future<> service::client_state::ensure_has_permission(auth::permission p, const auth::resource& r) const {
return check_has_permission(p, r).then([this, p, &r](bool ok) {
if (!ok) {
throw exceptions::unauthorized_exception(sprint("User %s has no %s permission on %s or any of its parents",
*_user,
auth::permissions::to_string(p),
resource));
throw exceptions::unauthorized_exception(
sprint(
"User %s has no %s permission on %s or any of its parents",
*_user,
auth::permissions::to_string(p),
r));
}
});
}

View File

@@ -315,11 +315,11 @@ public:
future<> has_schema_access(const schema& s, auth::permission p) const;
private:
future<> has_access(const sstring&, auth::permission, auth::resource) const;
future<> has_access(const sstring&, auth::permission, const auth::resource&) const;
public:
future<bool> check_has_permission(auth::permission, auth::resource) const;
future<> ensure_has_permission(auth::permission, auth::resource) const;
future<bool> check_has_permission(auth::permission, const auth::resource&) const;
future<> ensure_has_permission(auth::permission, const auth::resource&) const;
void validate_login() const;
void ensure_not_anonymous() const; // unauthorized_exception on error

View File

@@ -80,7 +80,7 @@ BOOST_AUTO_TEST_CASE(from_name) {
//
const auto dr1 = auth::parse_resource("data");
BOOST_REQUIRE_EQUAL(dr1, auth::resource(auth::resource_kind::data));
BOOST_REQUIRE_EQUAL(dr1, auth::root_data_resource());
const auto dr2 = auth::parse_resource("data/my_keyspace");
BOOST_REQUIRE_EQUAL(dr2, auth::make_data_resource("my_keyspace"));
@@ -95,7 +95,7 @@ BOOST_AUTO_TEST_CASE(from_name) {
//
const auto rr1 = auth::parse_resource("roles");
BOOST_REQUIRE_EQUAL(rr1, auth::resource(auth::resource_kind::role));
BOOST_REQUIRE_EQUAL(rr1, auth::root_role_resource());
const auto rr2 = auth::parse_resource("roles/joe");
BOOST_REQUIRE_EQUAL(rr2, auth::make_role_resource("joe"));
@@ -115,7 +115,7 @@ BOOST_AUTO_TEST_CASE(name) {
// data
//
BOOST_REQUIRE_EQUAL(auth::resource(auth::resource_kind::data).name(), "data");
BOOST_REQUIRE_EQUAL(auth::root_data_resource().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");
@@ -123,7 +123,7 @@ BOOST_AUTO_TEST_CASE(name) {
// role
//
BOOST_REQUIRE_EQUAL(auth::resource(auth::resource_kind::role).name(), "roles");
BOOST_REQUIRE_EQUAL(auth::root_role_resource().name(), "roles");
BOOST_REQUIRE_EQUAL(auth::make_role_resource("joe").name(), "roles/joe");
}
@@ -136,7 +136,7 @@ BOOST_AUTO_TEST_CASE(parent) {
const auto r3 = r2->parent();
BOOST_REQUIRE(r3);
BOOST_REQUIRE_EQUAL(*r3, auth::resource(auth::resource_kind::data));
BOOST_REQUIRE_EQUAL(*r3, auth::root_data_resource());
const auto r4 = r3->parent();
BOOST_REQUIRE(!r4);
@@ -147,7 +147,7 @@ BOOST_AUTO_TEST_CASE(output) {
// data
//
BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource(auth::resource_kind::data)), "<all keyspaces>");
BOOST_REQUIRE_EQUAL(sprint("%s", auth::root_data_resource()), "<all keyspaces>");
BOOST_REQUIRE_EQUAL(sprint("%s", auth::make_data_resource("my_keyspace")), "<keyspace my_keyspace>");
BOOST_REQUIRE_EQUAL(
@@ -158,6 +158,6 @@ BOOST_AUTO_TEST_CASE(output) {
// role
//
BOOST_REQUIRE_EQUAL(sprint("%s", auth::resource(auth::resource_kind::role)), "<all roles>");
BOOST_REQUIRE_EQUAL(sprint("%s", auth::root_role_resource()), "<all roles>");
BOOST_REQUIRE_EQUAL(sprint("%s", auth::make_role_resource("joe")), "<role joe>");
}