bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Revert "Merge 'cql: update permissions when creating/altering a function/keyspace' from Wojciech Mitros"

Browse Source 
This reverts commit 52e4edfd5e, reversing
changes made to d2d53fc1db. The associated test
fails with about 10% probablity, which blocks other work.

Fixes #13919
Reopens #13747
This commit is contained in:
Avi Kivity
2023-05-29 14:06:22 +03:00
parent a35758607a
commit 27f7cc4032
11 changed files with 60 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
cql3/statements/create_keyspace_statement.cc
View File

@@ -120,20 +120,13 @@ cql3::statements::create_keyspace_statement::prepare(data_dictionary::database d
return std::make_unique<prepared_statement>(make_shared<create_keyspace_statement>(*this));
}
future<> cql3::statements::create_keyspace_statement::grant_permissions_to_creator(query_processor& qp, const service::client_state& cs) const {
return do_with(auth::make_data_resource(keyspace()), auth::make_functions_resource(keyspace()), [&cs](const auth::resource& r, const auth::resource& fr) {
future<> cql3::statements::create_keyspace_statement::grant_permissions_to_creator(const service::client_state& cs) const {
return do_with(auth::make_data_resource(keyspace()), [&cs](const auth::resource& r) {
return auth::grant_applicable_permissions(
*cs.get_auth_service(),
*cs.user(),
r).handle_exception_type([](const auth::unsupported_authorization_operation&) {
// Nothing.
}).then([&cs, &fr] {
return auth::grant_applicable_permissions(
*cs.get_auth_service(),
*cs.user(),
fr).handle_exception_type([](const auth::unsupported_authorization_operation&) {
// Nothing.
});
});
});
}

2
cql3/statements/create_keyspace_statement.hh
View File

@@ -68,7 +68,7 @@ public:
virtual std::unique_ptr<prepared_statement> prepare(data_dictionary::database db, cql_stats& stats) override;
virtual future<> grant_permissions_to_creator(query_processor& qp, const service::client_state&) const override;
virtual future<> grant_permissions_to_creator(const service::client_state&) const override;
virtual future<::shared_ptr<messages::result_message>>
execute(query_processor& qp, service::query_state& state, const query_options& options) const override;

2
cql3/statements/create_role_statement.hh
View File

@@ -39,7 +39,7 @@ public:
std::unique_ptr<prepared_statement> prepare(data_dictionary::database db, cql_stats& stats) override;
future<> grant_permissions_to_creator(query_processor& qp, const service::client_state&) const;
future<> grant_permissions_to_creator(const service::client_state&) const;
void validate(query_processor&, const service::client_state&) const override;

2
cql3/statements/create_table_statement.cc
View File

@@ -146,7 +146,7 @@ create_table_statement::prepare(data_dictionary::database db, cql_stats& stats)
abort();
}
future<> create_table_statement::grant_permissions_to_creator(query_processor& qp, const service::client_state& cs) const {
future<> create_table_statement::grant_permissions_to_creator(const service::client_state& cs) const {
return do_with(auth::make_data_resource(keyspace(), column_family()), [&cs](const auth::resource& r) {
return auth::grant_applicable_permissions(
*cs.get_auth_service(),

2
cql3/statements/create_table_statement.hh
View File

@@ -77,7 +77,7 @@ public:
virtual std::unique_ptr<prepared_statement> prepare(data_dictionary::database db, cql_stats& stats) override;
virtual future<> grant_permissions_to_creator(query_processor& qp, const service::client_state&) const override;
virtual future<> grant_permissions_to_creator(const service::client_state&) const override;
virtual future<::shared_ptr<messages::result_message>>
execute(query_processor& qp, service::query_state& state, const query_options& options) const override;

26
cql3/statements/function_statement.cc
View File

@@ -22,29 +22,13 @@ namespace statements {
future<> function_statement::check_access(query_processor& qp, const service::client_state& state) const { return make_ready_future<>(); }
future<> create_function_statement_base::check_access(query_processor& qp, const service::client_state& state) const {
create_arg_types(qp);
if (!functions::functions::find(_name, _arg_types)) {
co_await state.has_functions_access(qp.db(), _name.keyspace, auth::permission::CREATE);
} else if (_or_replace) {
_altering = true;
co_await state.has_function_access(qp.db(), _name.keyspace, auth::encode_signature(_name.name, _arg_types), auth::permission::ALTER);
}
}
co_await state.has_functions_access(qp.db(), _name.keyspace, auth::permission::CREATE);
if (_or_replace) {
create_arg_types(qp);
sstring encoded_signature = auth::encode_signature(_name.name, _arg_types);
future<> create_function_statement_base::grant_permissions_to_creator(query_processor& qp, const service::client_state& cs) const {
if (_altering) {
return make_ready_future<>();
co_await state.has_function_access(qp.db(), _name.keyspace, encoded_signature, auth::permission::ALTER);
}
std::string_view keyspace = _name.has_keyspace() ? _name.keyspace : cs.get_keyspace();
return do_with(auth::make_functions_resource(keyspace, auth::encode_signature(_name.name, _arg_types)), [&cs](const auth::resource& r) {
return auth::grant_applicable_permissions(
*cs.get_auth_service(),
*cs.user(),
r).handle_exception_type([](const auth::unsupported_authorization_operation&) {
// Nothing.
});
});
}
future<> drop_function_statement_base::check_access(query_processor& qp, const service::client_state& state) const

2
cql3/statements/function_statement.hh
View File

@@ -43,11 +43,9 @@ protected:
virtual void validate(query_processor& qp, const service::client_state& state) const override;
virtual seastar::future<shared_ptr<db::functions::function>> create(query_processor& qp, db::functions::function* old) const = 0;
virtual seastar::future<shared_ptr<db::functions::function>> validate_while_executing(query_processor&) const override;
virtual seastar::future<> grant_permissions_to_creator(query_processor& qp, const service::client_state& cs) const override;
bool _or_replace;
bool _if_not_exists;
mutable bool _altering = false;
create_function_statement_base(functions::function_name name, std::vector<shared_ptr<cql3_type::raw>> raw_arg_types,
bool or_replace, bool if_not_exists);

10
cql3/statements/role-management-statements.cc
View File

@@ -64,7 +64,7 @@ std::unique_ptr<prepared_statement> create_role_statement::prepare(
return std::make_unique<prepared_statement>(::make_shared<create_role_statement>(*this));
}
future<> create_role_statement::grant_permissions_to_creator(query_processor& qp, const service::client_state& cs) const {
future<> create_role_statement::grant_permissions_to_creator(const service::client_state& cs) const {
return do_with(auth::make_role_resource(_role), [&cs](const auth::resource& r) {
return auth::grant_applicable_permissions(
*cs.get_auth_service(),
@@ -94,7 +94,7 @@ future<> create_role_statement::check_access(query_processor& qp, const service:
}
future<result_message_ptr>
create_role_statement::execute(query_processor& qp,
create_role_statement::execute(query_processor&,
service::query_state& state,
const query_options&) const {
auth::role_config config;
@@ -104,12 +104,12 @@ create_role_statement::execute(query_processor& qp,
return do_with(
std::move(config),
extract_authentication_options(_options),
[this, &qp, &state](const auth::role_config& config, const auth::authentication_options& authen_options) {
[this, &state](const auth::role_config& config, const auth::authentication_options& authen_options) {
const auto& cs = state.get_client_state();
auto& as = *cs.get_auth_service();
return auth::create_role(as, _role, config, authen_options).then([this, &qp, &cs] {
return grant_permissions_to_creator(qp, cs);
return auth::create_role(as, _role, config, authen_options).then([this, &cs] {
return grant_permissions_to_creator(cs);
}).then([] {
return void_result_message();
}).handle_exception_type([this](const auth::role_already_exists& e) {

6
cql3/statements/schema_altering_statement.cc
View File

@@ -38,7 +38,7 @@ schema_altering_statement::schema_altering_statement(cf_name name, timeout_confi
{
}
future<> schema_altering_statement::grant_permissions_to_creator(query_processor& qp, const service::client_state&) const {
future<> schema_altering_statement::grant_permissions_to_creator(const service::client_state&) const {
return make_ready_future<>();
}
@@ -119,10 +119,10 @@ schema_altering_statement::execute(query_processor& qp, service::query_state& st
}
}
return execute0(qp, state, options).then([this, &qp, &state, internal](::shared_ptr<messages::result_message> result) {
return execute0(qp, state, options).then([this, &state, internal](::shared_ptr<messages::result_message> result) {
auto permissions_granted_fut = internal
? make_ready_future<>()
: grant_permissions_to_creator(qp, state.get_client_state());
: grant_permissions_to_creator(state.get_client_state());
return permissions_granted_fut.then([result = std::move(result)] {
return result;
});

2
cql3/statements/schema_altering_statement.hh
View File

@@ -48,7 +48,7 @@ protected:
*
* By default, this function does nothing.
*/
virtual future<> grant_permissions_to_creator(query_processor& qp, const service::client_state&) const;
virtual future<> grant_permissions_to_creator(const service::client_state&) const;
virtual bool depends_on(std::string_view ks_name, std::optional<std::string_view> cf_name) const override;

66
test/cql-pytest/test_permissions.py
View File

@@ -8,7 +8,7 @@
import pytest
import time
from cassandra.protocol import SyntaxException, InvalidRequest, Unauthorized, ConfigurationException
from cassandra.protocol import SyntaxException, InvalidRequest, Unauthorized
from util import new_test_table, new_function, new_user, new_session, new_test_keyspace, unique_name, new_type
# Test that granting permissions to various resources works for the default user.
@@ -127,6 +127,7 @@ def test_grant_revoke_data_permissions(cql, test_keyspace):
# Test that permissions for user-defined functions are serialized in a Cassandra-compatible way
def test_udf_permissions_serialization(cql):
schema = "a int primary key"
user = "cassandra"
with new_test_keyspace(cql, "WITH REPLICATION = { 'class': 'NetworkTopologyStrategy', 'replication_factor': 1 }") as keyspace, new_user(cql) as user:
with new_test_table(cql, keyspace, schema) as table:
# Creating a bilingual function makes this test case work for both Scylla and Cassandra
@@ -149,7 +150,7 @@ def test_udf_permissions_serialization(cql):
for permission in permissions:
cql.execute(f"GRANT {permission} ON {resource} TO {user}")
permissions = {row.resource: row.permissions for row in cql.execute(f"SELECT * FROM system_auth.role_permissions WHERE role = '{user}'")}
permissions = {row.resource: row.permissions for row in cql.execute(f"SELECT * FROM system_auth.role_permissions")}
assert permissions['functions'] == set(['ALTER', 'AUTHORIZE', 'CREATE', 'DROP', 'EXECUTE'])
assert permissions[f'functions/{keyspace}'] == set(['ALTER', 'AUTHORIZE', 'CREATE', 'DROP', 'EXECUTE'])
assert permissions[f'functions/{keyspace}/{div_fun}[org.apache.cassandra.db.marshal.LongType^org.apache.cassandra.db.marshal.Int32Type]'] == set(['ALTER', 'AUTHORIZE', 'DROP', 'EXECUTE'])
@@ -199,9 +200,9 @@ def test_drop_udf_with_same_name(cql):
schema = "a int primary key"
with new_test_keyspace(cql, "WITH REPLICATION = { 'class': 'NetworkTopologyStrategy', 'replication_factor': 1 }") as keyspace:
body1_lua = "(i int) CALLED ON NULL INPUT RETURNS bigint LANGUAGE lua AS 'return 42;'"
body1_java = "(i int) CALLED ON NULL INPUT RETURNS bigint LANGUAGE java AS 'return Long.valueOf(42);'"
body1_java = "(i int) CALLED ON NULL INPUT RETURNS bigint LANGUAGE java AS 'return 42;'"
body2_lua = "(i int, j int) CALLED ON NULL INPUT RETURNS bigint LANGUAGE lua AS 'return 42;'"
body2_java = "(i int, j int) CALLED ON NULL INPUT RETURNS bigint LANGUAGE java AS 'return Long.valueOf(42);'"
body2_java = "(i int, j int) CALLED ON NULL INPUT RETURNS bigint LANGUAGE java AS 'return 42;'"
body1 = body1_lua
body2 = body2_lua
try:
@@ -216,16 +217,17 @@ def test_drop_udf_with_same_name(cql):
with new_user(cql) as username:
with new_session(cql, username) as user_session:
grant(cql, 'DROP', f'FUNCTION {keyspace}.{fun}(int)', username)
with pytest.raises((InvalidRequest, Unauthorized)):
with pytest.raises(InvalidRequest):
user_session.execute(f"DROP FUNCTION {keyspace}.{fun}")
eventually_unauthorized(lambda: user_session.execute(f"DROP FUNCTION {keyspace}.{fun}(int, int)"))
grant(cql, 'DROP', f'FUNCTION {keyspace}.{fun}(int, int)', username)
with pytest.raises((InvalidRequest, Unauthorized)):
with pytest.raises(InvalidRequest):
user_session.execute(f"DROP FUNCTION {keyspace}.{fun}")
eventually_authorized(lambda: user_session.execute(f"DROP FUNCTION {keyspace}.{fun}(int)"))
eventually_authorized(lambda: user_session.execute(f"DROP FUNCTION {keyspace}.{fun}"))
# Test that permissions set for user-defined functions are enforced
# Tests for ALTER are separate, because they are qualified as cassandra_bug
def test_grant_revoke_udf_permissions(cql):
schema = "a int primary key, b list<int>"
user = "cassandra"
@@ -269,19 +271,40 @@ def test_grant_revoke_udf_permissions(cql):
for resource in [f'function {keyspace}.{fun}(int, list<int>)', f'all functions in keyspace {keyspace}', 'all functions']:
check_enforced(cql, username, permission='AUTHORIZE', resource=resource, function=grant_idempotent)
# This test case is artificially extracted from the one above,
# because it's qualified as cassandra_bug - the documentation quotes that ALTER is needed on
# functions if the definition is replaced (CREATE OR REPLACE FUNCTION (...)),
# and yet it's not enforced
def test_grant_revoke_alter_udf_permissions(cassandra_bug, cql):
schema = "a int primary key"
user = "cassandra"
with new_test_keyspace(cql, "WITH REPLICATION = { 'class': 'SimpleStrategy', 'replication_factor': 1 }") as keyspace:
with new_test_table(cql, keyspace, schema) as table:
fun_body_lua = "(i int) CALLED ON NULL INPUT RETURNS int LANGUAGE lua AS 'return 42;'"
fun_body_java = "(i int) CALLED ON NULL INPUT RETURNS int LANGUAGE java AS 'return 42;'"
fun_body = fun_body_lua
try:
with new_function(cql, keyspace, fun_body) as fun:
pass
except:
fun_body = fun_body_java
with new_user(cql) as username:
with new_session(cql, username) as user_session:
fun = "fun42"
grant(cql, 'ALTER', 'ALL FUNCTIONS', username)
check_enforced(cql, username, permission='CREATE', resource=f'all functions in keyspace {keyspace}',
function=lambda: user_session.execute(f"CREATE OR REPLACE FUNCTION {keyspace}.{unique_name()} {fun_body}"))
function=lambda: user_session.execute(f"CREATE OR REPLACE FUNCTION {keyspace}.{fun} {fun_body}"))
check_enforced(cql, username, permission='CREATE', resource='all functions',
function=lambda: user_session.execute(f"CREATE OR REPLACE FUNCTION {keyspace}.{unique_name()} {fun_body}"))
function=lambda: user_session.execute(f"CREATE OR REPLACE FUNCTION {keyspace}.{fun} {fun_body}"))
revoke(cql, 'ALTER', 'ALL FUNCTIONS', username)
cql.execute(f"CREATE FUNCTION IF NOT EXISTS {keyspace}.{fun} {fun_body}")
grant(cql, 'CREATE', 'ALL FUNCTIONS', username)
check_enforced(cql, username, permission='ALTER', resource=f'all functions in keyspace {keyspace}',
function=lambda: user_session.execute(f"CREATE OR REPLACE FUNCTION {keyspace}.{fun} {fun_body}"))
check_enforced(cql, username, permission='ALTER', resource='all functions',
function=lambda: user_session.execute(f"CREATE OR REPLACE FUNCTION {keyspace}.{fun} {fun_body}"))
check_enforced(cql, username, permission='ALTER', resource=f'FUNCTION {keyspace}.{fun}(int, list<int>)',
check_enforced(cql, username, permission='ALTER', resource=f'FUNCTION {keyspace}.{fun}(int)',
function=lambda: user_session.execute(f"CREATE OR REPLACE FUNCTION {keyspace}.{fun} {fun_body}"))
# Test that granting permissions on non-existent UDFs fails
@@ -293,12 +316,12 @@ def test_grant_perms_on_nonexistent_udf(cql):
revoke(cql, 'EXECUTE', 'ALL FUNCTIONS', username)
with pytest.raises(InvalidRequest):
grant(cql, 'EXECUTE', f'ALL FUNCTIONS IN KEYSPACE {keyspace}', username)
with pytest.raises((InvalidRequest, ConfigurationException)):
with pytest.raises(InvalidRequest):
grant(cql, 'EXECUTE', f'FUNCTION {keyspace}.{fun_name}(int)', username)
cql.execute(f"CREATE KEYSPACE IF NOT EXISTS {keyspace} WITH REPLICATION = {{ 'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1 }}")
grant(cql, 'EXECUTE', f'ALL FUNCTIONS IN KEYSPACE {keyspace}', username)
revoke(cql, 'EXECUTE', f'ALL FUNCTIONS IN KEYSPACE {keyspace}', username)
with pytest.raises((InvalidRequest, SyntaxException)):
with pytest.raises(InvalidRequest):
grant(cql, 'EXECUTE', f'FUNCTION {keyspace}.{fun_name}(int)', username)
fun_body_lua = "(i int) CALLED ON NULL INPUT RETURNS int LANGUAGE lua AS 'return 42;'"
@@ -314,24 +337,15 @@ def test_grant_perms_on_nonexistent_udf(cql):
cql.execute(f"DROP KEYSPACE IF EXISTS {keyspace}")
# Test that permissions for user-defined aggregates are also enforced.
def test_grant_revoke_uda_permissions(cql):
# scylla_only, because Lua is used as the target language
def test_grant_revoke_uda_permissions(scylla_only, cql):
schema = 'id bigint primary key'
with new_test_keyspace(cql, "WITH REPLICATION = { 'class': 'NetworkTopologyStrategy', 'replication_factor': 1 }") as keyspace:
with new_test_table(cql, keyspace, schema) as table:
for i in range(8):
cql.execute(f"INSERT INTO {table} (id) VALUES ({10**i})")
avg_partial_body_lua = "(state tuple<bigint, bigint>, val bigint) CALLED ON NULL INPUT RETURNS tuple<bigint, bigint> LANGUAGE lua AS 'return {state[1] + val, state[2] + 1}'"
avg_partial_body_java = "(state tuple<bigint, bigint>, val bigint) CALLED ON NULL INPUT RETURNS tuple<bigint, bigint> LANGUAGE java AS 'return state.setLong(0, state.getLong(0) + val).setLong(1, state.getLong(1) + Long.valueOf(1));'"
div_body_lua = "(state tuple<bigint, bigint>) CALLED ON NULL INPUT RETURNS bigint LANGUAGE lua AS 'return state[1]//state[2]'"
div_body_java = "(state tuple<bigint, bigint>) CALLED ON NULL INPUT RETURNS bigint LANGUAGE java AS 'return state.getLong(0)/state.getLong(1);'"
avg_partial_body = avg_partial_body_lua
div_body = div_body_lua
try:
with new_function(cql, keyspace, avg_partial_body):
pass
except:
avg_partial_body = avg_partial_body_java
div_body = div_body_java
avg_partial_body = "(state tuple<bigint, bigint>, val bigint) CALLED ON NULL INPUT RETURNS tuple<bigint, bigint> LANGUAGE lua AS 'return {state[1] + val, state[2] + 1}'"
div_body = "(state tuple<bigint, bigint>) CALLED ON NULL INPUT RETURNS bigint LANGUAGE lua AS 'return state[1]//state[2]'"
with new_function(cql, keyspace, avg_partial_body) as avg_partial, new_function(cql, keyspace, div_body) as div_fun:
custom_avg_body = f"(bigint) SFUNC {avg_partial} STYPE tuple<bigint, bigint> FINALFUNC {div_fun} INITCOND (0,0)"
with new_user(cql) as username:
@@ -347,7 +361,7 @@ def test_grant_revoke_uda_permissions(cql):
check_enforced(cql, username, permission='CREATE', resource='all functions',
function=create_aggr_idempotent)
cql.execute(f"CREATE AGGREGATE IF NOT EXISTS {keyspace}.{custom_avg} {custom_avg_body}")
grant(cql, 'CREATE', 'ALL FUNCTIONS', username)
check_enforced(cql, username, permission='ALTER', resource=f'all functions in keyspace {keyspace}',
function=lambda: user_session.execute(f"CREATE OR REPLACE AGGREGATE {keyspace}.{custom_avg} {custom_avg_body}"))
check_enforced(cql, username, permission='ALTER', resource='all functions',