From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Serge Petrenko Subject: [PATCH 1/2] box: remove compatibility mode for privileges Date: Tue, 30 Oct 2018 16:32:00 +0300 Message-Id: <3e7d3070f5bcb8a5d790a849d3cabd20d91d111d.1540903773.git.sergepetrenko@tarantool.org> In-Reply-To: References: In-Reply-To: References: To: vdavydov.dev@gmail.com Cc: tarantool-patches@freelists.org, Serge Petrenko List-ID: Before version 1.7.7 there were no CREATE or ALTER privileges. READ+WRITE permitted object creation and altering. In 1.10 CREATE and ALTER privileges were introduced together with a compatibility mode in access_check_ddl() which assumed user had CREATE and ALTER if it had READ and WRITE on a respective object. Now its time for us to remove this compatibility mode. Part of #3539 --- src/box/alter.cc | 55 +++++++++++++++------------------------- test/box/access.result | 40 +++++++++++++++++++++++------ test/box/access.test.lua | 18 +++++++++---- test/sql/iproto.result | 6 +++++ test/sql/iproto.test.lua | 2 ++ 5 files changed, 74 insertions(+), 47 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index a6bb5a0f0..6d2c59bbc 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -65,23 +65,10 @@ static void access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid, - enum schema_object_type type, enum priv_type priv_type, - bool is_17_compat_mode) + enum schema_object_type type, enum priv_type priv_type) { struct credentials *cr = effective_user(); user_access_t has_access = cr->universal_access; - /* - * XXX: pre 1.7.7 there was no specific 'CREATE' or - * 'ALTER' ACL, instead, read and write access on universe - * was used to allow create/alter. - * For backward compatibility, if a user has read and write - * access on the universe, grant it CREATE access - * automatically. - * The legacy fix does not affect sequences since they - * were added in 1.7.7 only. - */ - if (is_17_compat_mode && has_access & PRIV_R && has_access & PRIV_W) - has_access |= PRIV_C | PRIV_A; user_access_t access = ((PRIV_U | (user_access_t) priv_type) & ~has_access); @@ -1678,7 +1665,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) auto def_guard = make_scoped_guard([=] { space_def_delete(def); }); access_check_ddl(def->name, def->id, def->uid, SC_SPACE, - PRIV_C, true); + PRIV_C); RLIST_HEAD(empty_list); struct space *space = space_new_xc(def, &empty_list); /** @@ -1740,7 +1727,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) } } else if (new_tuple == NULL) { /* DELETE */ access_check_ddl(old_space->def->name, old_space->def->id, - old_space->def->uid, SC_SPACE, PRIV_D, true); + old_space->def->uid, SC_SPACE, PRIV_D); /* Verify that the space is empty (has no indexes) */ if (old_space->index_count) { tnt_raise(ClientError, ER_DROP_SPACE, @@ -1818,7 +1805,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) auto def_guard = make_scoped_guard([=] { space_def_delete(def); }); access_check_ddl(def->name, def->id, def->uid, SC_SPACE, - PRIV_A, true); + PRIV_A); if (def->id != space_id(old_space)) tnt_raise(ClientError, ER_ALTER_SPACE, space_name(old_space), @@ -1953,7 +1940,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) if (old_tuple && new_tuple) priv_type = PRIV_A; access_check_ddl(old_space->def->name, old_space->def->id, - old_space->def->uid, SC_SPACE, priv_type, true); + old_space->def->uid, SC_SPACE, priv_type); struct index *old_index = space_index(old_space, iid); /* @@ -2361,7 +2348,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) if (new_tuple != NULL && old_user == NULL) { /* INSERT */ struct user_def *user = user_def_new_from_tuple(new_tuple); access_check_ddl(user->name, user->uid, user->owner, user->type, - PRIV_C, true); + PRIV_C); auto def_guard = make_scoped_guard([=] { free(user); }); (void) user_cache_replace(user); def_guard.is_active = false; @@ -2371,7 +2358,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) } else if (new_tuple == NULL) { /* DELETE */ access_check_ddl(old_user->def->name, old_user->def->uid, old_user->def->owner, old_user->def->type, - PRIV_D, true); + PRIV_D); /* Can't drop guest or super user */ if (uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid == SUPER) { tnt_raise(ClientError, ER_DROP_USER, @@ -2398,7 +2385,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) */ struct user_def *user = user_def_new_from_tuple(new_tuple); access_check_ddl(user->name, user->uid, user->uid, - old_user->def->type, PRIV_A, true); + old_user->def->type, PRIV_A); auto def_guard = make_scoped_guard([=] { free(user); }); struct trigger *on_commit = txn_alter_trigger_new(user_cache_alter_user, NULL); @@ -2501,7 +2488,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) if (new_tuple != NULL && old_func == NULL) { /* INSERT */ struct func_def *def = func_def_new_from_tuple(new_tuple); access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION, - PRIV_C, true); + PRIV_C); auto def_guard = make_scoped_guard([=] { free(def); }); func_cache_replace(def); def_guard.is_active = false; @@ -2516,7 +2503,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) * who created it or a superuser. */ access_check_ddl(old_func->def->name, fid, uid, SC_FUNCTION, - PRIV_D, true); + PRIV_D); /* Can only delete func if it has no grants. */ if (schema_find_grants("function", old_func->def->fid)) { tnt_raise(ClientError, ER_DROP_FUNCTION, @@ -2530,7 +2517,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) struct func_def *def = func_def_new_from_tuple(new_tuple); auto def_guard = make_scoped_guard([=] { free(def); }); access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION, - PRIV_A, true); + PRIV_A); struct trigger *on_commit = txn_alter_trigger_new(func_cache_replace_func, NULL); txn_on_commit(txn, on_commit); @@ -2688,7 +2675,7 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event) assert(old_coll_id != NULL); access_check_ddl(old_coll_id->name, old_coll_id->id, old_coll_id->owner_id, SC_COLLATION, - PRIV_D, false); + PRIV_D); /* * Set on_commit/on_rollback triggers after * deletion from the cache to make trigger logic @@ -2704,7 +2691,7 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event) struct coll_id_def new_def; coll_id_def_new_from_tuple(new_tuple, &new_def); access_check_ddl(new_def.name, new_def.id, new_def.owner_id, - SC_COLLATION, PRIV_C, false); + SC_COLLATION, PRIV_C); struct coll_id *new_coll_id = coll_id_new(&new_def); if (new_coll_id == NULL) diag_raise(); @@ -2789,7 +2776,7 @@ priv_def_check(struct priv_def *priv, enum priv_type priv_type) } const char *name = schema_find_name(priv->object_type, priv->object_id); access_check_ddl(name, priv->object_id, grantor->def->uid, - priv->object_type, priv_type, false); + priv->object_type, priv_type); switch (priv->object_type) { case SC_UNIVERSE: if (grantor->def->uid != ADMIN) { @@ -3250,7 +3237,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event) ER_CREATE_SEQUENCE); assert(sequence_by_id(new_def->id) == NULL); access_check_ddl(new_def->name, new_def->id, new_def->uid, - SC_SEQUENCE, PRIV_C, false); + SC_SEQUENCE, PRIV_C); sequence_cache_replace(new_def); alter->new_def = new_def; } else if (old_tuple != NULL && new_tuple == NULL) { /* DELETE */ @@ -3259,7 +3246,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event) struct sequence *seq = sequence_by_id(id); assert(seq != NULL); access_check_ddl(seq->def->name, seq->def->id, seq->def->uid, - SC_SEQUENCE, PRIV_D, false); + SC_SEQUENCE, PRIV_D); if (space_has_data(BOX_SEQUENCE_DATA_ID, 0, id)) tnt_raise(ClientError, ER_DROP_SEQUENCE, seq->def->name, "the sequence has data"); @@ -3276,7 +3263,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event) struct sequence *seq = sequence_by_id(new_def->id); assert(seq != NULL); access_check_ddl(seq->def->name, seq->def->id, seq->def->uid, - SC_SEQUENCE, PRIV_A, false); + SC_SEQUENCE, PRIV_A); alter->old_def = seq->def; alter->new_def = new_def; } @@ -3357,20 +3344,20 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event) /* Check we have the correct access type on the sequence. * */ if (is_generated || !stmt->new_tuple) { access_check_ddl(seq->def->name, seq->def->id, seq->def->uid, - SC_SEQUENCE, priv_type, false); + SC_SEQUENCE, priv_type); } else { /* * In case user wants to attach an existing sequence, * check that it has read and write access. */ access_check_ddl(seq->def->name, seq->def->id, seq->def->uid, - SC_SEQUENCE, PRIV_R, false); + SC_SEQUENCE, PRIV_R); access_check_ddl(seq->def->name, seq->def->id, seq->def->uid, - SC_SEQUENCE, PRIV_W, false); + SC_SEQUENCE, PRIV_W); } /** Check we have alter access on space. */ access_check_ddl(space->def->name, space->def->id, space->def->uid, - SC_SPACE, PRIV_A, false); + SC_SPACE, PRIV_A); struct trigger *on_commit = txn_alter_trigger_new(on_commit_dd_space_sequence, space); diff --git a/test/box/access.result b/test/box/access.result index 8a022d032..9c190240f 100644 --- a/test/box/access.result +++ b/test/box/access.result @@ -104,7 +104,10 @@ test_run:cmd("setopt delimiter ''"); box.schema.user.create('rich') --- ... -box.schema.user.grant('rich', 'read,write', 'universe') +box.schema.user.grant('rich', 'read,write', 'space', '_func') +--- +... +box.schema.user.grant('rich', 'create', 'function') --- ... session.su('rich') @@ -130,15 +133,18 @@ box.space['_user']:delete{uid} --- - error: 'Failed to drop user or role ''rich'': the user has objects' ... -box.schema.user.revoke('rich', 'read,write', 'universe') +box.schema.user.revoke('rich', 'public') --- ... -box.schema.user.revoke('rich', 'public') +box.schema.user.revoke('rich', 'read,write', 'space', '_func') --- ... box.schema.user.revoke('rich', 'alter', 'user', 'rich') --- ... +box.schema.user.revoke('rich', 'create', 'function') +--- +... box.schema.user.disable("rich") --- ... @@ -345,7 +351,13 @@ session = box.session box.schema.user.create('uniuser') --- ... -box.schema.user.grant('uniuser', 'read, write, execute', 'universe') +box.schema.user.grant('uniuser', 'create', 'space') +--- +... +box.schema.user.grant('uniuser', 'write', 'space', '_schema') +--- +... +box.schema.user.grant('uniuser', 'write', 'space', '_space') --- ... session.su('uniuser') @@ -583,7 +595,22 @@ session = nil box.schema.user.create('twostep') --- ... -box.schema.user.grant('twostep', 'read,write,execute', 'universe') +box.schema.user.grant('twostep', 'create', 'space') +--- +... +box.schema.user.grant('twostep', 'create', 'function') +--- +... +box.schema.user.grant('twostep', 'write', 'space', '_schema') +--- +... +box.schema.user.grant('twostep', 'write', 'space', '_space') +--- +... +box.schema.user.grant('twostep', 'write', 'space', '_index') +--- +... +box.schema.user.grant('twostep', 'read,write', 'space', '_func') --- ... box.session.su('twostep') @@ -601,9 +628,6 @@ box.schema.func.create('test') box.session.su('admin') --- ... -box.schema.user.revoke('twostep', 'execute,read,write', 'universe') ---- -... box.schema.user.create('twostep_client') --- ... diff --git a/test/box/access.test.lua b/test/box/access.test.lua index f7c18b106..4baeb2ef6 100644 --- a/test/box/access.test.lua +++ b/test/box/access.test.lua @@ -50,7 +50,8 @@ end; usermax(); test_run:cmd("setopt delimiter ''"); box.schema.user.create('rich') -box.schema.user.grant('rich', 'read,write', 'universe') +box.schema.user.grant('rich', 'read,write', 'space', '_func') +box.schema.user.grant('rich', 'create', 'function') session.su('rich') uid = session.uid() box.schema.func.create('dummy') @@ -58,9 +59,10 @@ session.su('admin') box.space['_user']:delete{uid} box.schema.func.drop('dummy') box.space['_user']:delete{uid} -box.schema.user.revoke('rich', 'read,write', 'universe') box.schema.user.revoke('rich', 'public') +box.schema.user.revoke('rich', 'read,write', 'space', '_func') box.schema.user.revoke('rich', 'alter', 'user', 'rich') +box.schema.user.revoke('rich', 'create', 'function') box.schema.user.disable("rich") -- test double disable is a no op box.schema.user.disable("rich") @@ -154,7 +156,9 @@ box.schema.user.drop('testus') -- ------------------------------------------------------------ session = box.session box.schema.user.create('uniuser') -box.schema.user.grant('uniuser', 'read, write, execute', 'universe') +box.schema.user.grant('uniuser', 'create', 'space') +box.schema.user.grant('uniuser', 'write', 'space', '_schema') +box.schema.user.grant('uniuser', 'write', 'space', '_space') session.su('uniuser') us = box.schema.space.create('uniuser_space') session.su('admin') @@ -241,13 +245,17 @@ session = nil -- admin can't manage grants on not owned objects -- ----------------------------------------------------------- box.schema.user.create('twostep') -box.schema.user.grant('twostep', 'read,write,execute', 'universe') +box.schema.user.grant('twostep', 'create', 'space') +box.schema.user.grant('twostep', 'create', 'function') +box.schema.user.grant('twostep', 'write', 'space', '_schema') +box.schema.user.grant('twostep', 'write', 'space', '_space') +box.schema.user.grant('twostep', 'write', 'space', '_index') +box.schema.user.grant('twostep', 'read,write', 'space', '_func') box.session.su('twostep') twostep = box.schema.space.create('twostep') index2 = twostep:create_index('primary') box.schema.func.create('test') box.session.su('admin') -box.schema.user.revoke('twostep', 'execute,read,write', 'universe') box.schema.user.create('twostep_client') box.schema.user.grant('twostep_client', 'execute', 'function', 'test') box.schema.user.drop('twostep') diff --git a/test/sql/iproto.result b/test/sql/iproto.result index af474bcf5..ee34f6fc1 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -37,6 +37,9 @@ box.sql.execute('select * from test') box.schema.user.grant('guest','read,write,execute', 'universe') --- ... +box.schema.user.grant('guest', 'create', 'space') +--- +... cn = remote.connect(box.cfg.listen) --- ... @@ -583,6 +586,9 @@ box.sql.execute('drop table test') box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- ... +box.schema.user.revoke('guest', 'create', 'space') +--- +... space = nil --- ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index 220331b40..b06559075 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -10,6 +10,7 @@ space:replace{4, 5, '6'} space:replace{7, 8.5, '9'} box.sql.execute('select * from test') box.schema.user.grant('guest','read,write,execute', 'universe') +box.schema.user.grant('guest', 'create', 'space') cn = remote.connect(box.cfg.listen) cn:ping() @@ -202,6 +203,7 @@ cn:close() box.sql.execute('drop table test') box.schema.user.revoke('guest', 'read,write,execute', 'universe') +box.schema.user.revoke('guest', 'create', 'space') space = nil -- Cleanup xlog -- 2.17.1 (Apple Git-112)