diff --git a/cql3/statements/create_keyspace_statement.cc b/cql3/statements/create_keyspace_statement.cc index 4e73968813..8a46602016 100644 --- a/cql3/statements/create_keyspace_statement.cc +++ b/cql3/statements/create_keyspace_statement.cc @@ -120,20 +120,13 @@ cql3::statements::create_keyspace_statement::prepare(data_dictionary::database d return std::make_unique(make_shared(*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. - }); }); }); } diff --git a/cql3/statements/create_keyspace_statement.hh b/cql3/statements/create_keyspace_statement.hh index 118e947c22..891b6f2c62 100644 --- a/cql3/statements/create_keyspace_statement.hh +++ b/cql3/statements/create_keyspace_statement.hh @@ -68,7 +68,7 @@ public: virtual std::unique_ptr 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> execute(query_processor& qp, service::query_state& state, const query_options& options) const override; diff --git a/cql3/statements/create_role_statement.hh b/cql3/statements/create_role_statement.hh index 74c2a8262a..5df1f2a2b7 100644 --- a/cql3/statements/create_role_statement.hh +++ b/cql3/statements/create_role_statement.hh @@ -39,7 +39,7 @@ public: std::unique_ptr 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; diff --git a/cql3/statements/create_table_statement.cc b/cql3/statements/create_table_statement.cc index efbbfa8cdf..25dd412da4 100644 --- a/cql3/statements/create_table_statement.cc +++ b/cql3/statements/create_table_statement.cc @@ -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(), diff --git a/cql3/statements/create_table_statement.hh b/cql3/statements/create_table_statement.hh index 3ceabecfe9..5299d1653f 100644 --- a/cql3/statements/create_table_statement.hh +++ b/cql3/statements/create_table_statement.hh @@ -77,7 +77,7 @@ public: virtual std::unique_ptr 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> execute(query_processor& qp, service::query_state& state, const query_options& options) const override; diff --git a/cql3/statements/function_statement.cc b/cql3/statements/function_statement.cc index aad8dc0b92..cce045a76d 100644 --- a/cql3/statements/function_statement.cc +++ b/cql3/statements/function_statement.cc @@ -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 diff --git a/cql3/statements/function_statement.hh b/cql3/statements/function_statement.hh index 2142f062be..84372d7ac5 100644 --- a/cql3/statements/function_statement.hh +++ b/cql3/statements/function_statement.hh @@ -43,11 +43,9 @@ protected: virtual void validate(query_processor& qp, const service::client_state& state) const override; virtual seastar::future> create(query_processor& qp, db::functions::function* old) const = 0; virtual seastar::future> 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> raw_arg_types, bool or_replace, bool if_not_exists); diff --git a/cql3/statements/role-management-statements.cc b/cql3/statements/role-management-statements.cc index cee9fc373f..8f200bd256 100644 --- a/cql3/statements/role-management-statements.cc +++ b/cql3/statements/role-management-statements.cc @@ -64,7 +64,7 @@ std::unique_ptr create_role_statement::prepare( return std::make_unique(::make_shared(*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 -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) { diff --git a/cql3/statements/schema_altering_statement.cc b/cql3/statements/schema_altering_statement.cc index 507d073fa2..fd54e14f3e 100644 --- a/cql3/statements/schema_altering_statement.cc +++ b/cql3/statements/schema_altering_statement.cc @@ -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 result) { + return execute0(qp, state, options).then([this, &state, internal](::shared_ptr 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; }); diff --git a/cql3/statements/schema_altering_statement.hh b/cql3/statements/schema_altering_statement.hh index eab23bb557..327ed17a79 100644 --- a/cql3/statements/schema_altering_statement.hh +++ b/cql3/statements/schema_altering_statement.hh @@ -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 cf_name) const override; diff --git a/test/cql-pytest/test_permissions.py b/test/cql-pytest/test_permissions.py index bfdd552a00..9b9b3dd70e 100644 --- a/test/cql-pytest/test_permissions.py +++ b/test/cql-pytest/test_permissions.py @@ -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" user = "cassandra" @@ -269,19 +271,40 @@ def test_grant_revoke_udf_permissions(cql): for resource in [f'function {keyspace}.{fun}(int, list)', 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)', + 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, val bigint) CALLED ON NULL INPUT RETURNS tuple LANGUAGE lua AS 'return {state[1] + val, state[2] + 1}'" - avg_partial_body_java = "(state tuple, val bigint) CALLED ON NULL INPUT RETURNS tuple LANGUAGE java AS 'return state.setLong(0, state.getLong(0) + val).setLong(1, state.getLong(1) + Long.valueOf(1));'" - div_body_lua = "(state tuple) CALLED ON NULL INPUT RETURNS bigint LANGUAGE lua AS 'return state[1]//state[2]'" - div_body_java = "(state tuple) 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, val bigint) CALLED ON NULL INPUT RETURNS tuple LANGUAGE lua AS 'return {state[1] + val, state[2] + 1}'" + div_body = "(state tuple) 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 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',