[tarantool-patches] Re[2]: [PATCH v4 05/20] refactoring: clear privilege managing triggers from exceptions
Ilya Kosarev
i.kosarev at tarantool.org
Mon Oct 21 14:35:36 MSK 2019
Hi!
Thanks for your review.
In v5 I will fix this.
Sincerely,
Ilya Kosarev
>Понедельник, 30 сентября 2019, 15:28 +03:00 от Georgy Kirichenko <georgy at tarantool.org>:
>
>Hi, the patch is mostly Ok, please see some comments below
>
>On Monday, September 23, 2019 6:56:56 PM MSK Ilya Kosarev wrote:
>> /**
>> @@ -708,8 +710,15 @@ public:
>> static struct trigger *
>> txn_alter_trigger_new(trigger_f run, void *data)
>> {
>> + size_t size = sizeof(struct trigger);
>> struct trigger *trigger = (struct trigger *)
>> - region_calloc_object_xc(&in_txn()->region, struct
>trigger);
>> + region_aligned_alloc(&in_txn()->region, size,
>> + alignof(struct trigger));
>> + if (trigger == NULL) {
>> + diag_set(OutOfMemory, size, "region", "new slab");
>Please use a structure name instead of a 'new slab' abbreviation (you can see
>an example after OutOfMemory search)
>> + return NULL;
>> + }
>> + trigger = (struct trigger *)memset(trigger, 0, size);
>> trigger->run = run;
>> trigger->data = data;
>> trigger->destroy = NULL;
>> @@ -979,6 +988,8 @@ alter_space_do(struct txn_stmt *stmt, struct alter_space
>> *alter) struct trigger *on_commit, *on_rollback;
>> on_commit = txn_alter_trigger_new(alter_space_commit, alter);
>> on_rollback = txn_alter_trigger_new(alter_space_rollback, alter);
>> + if (on_commit == NULL || on_rollback == NULL)
>> + diag_raise();
>>
>> /* Create a definition of the new space. */
>> space_dump_def(alter->old_space, &alter->key_list);
>> @@ -1938,8 +1949,9 @@ on_replace_dd_space(struct trigger * /* trigger */,
>> void *event) region);
>> auto def_guard =
>> make_scoped_guard([=] {
>space_def_delete(def); });
>> - access_check_ddl(def->name, def->id, def->uid,
>SC_SPACE,
>> - PRIV_C);
>> + if (access_check_ddl(def->name, def->id, def->uid,
>SC_SPACE,
>> + PRIV_C) != 0)
>> + return -1;
>> RLIST_HEAD(empty_list);
>> struct space *space = space_new_xc(def, &empty_list);
>> /**
>> @@ -1965,6 +1977,8 @@ on_replace_dd_space(struct trigger * /* trigger */,
>> void *event) */
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_create_space_rollback, space);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> if (def->opts.is_view) {
>> struct Select *select =
>sql_view_compile(sql_get(),
>> @@ -1990,16 +2004,21 @@ on_replace_dd_space(struct trigger * /* trigger */,
>> void *event) struct trigger *on_commit_view =
>>
>txn_alter_trigger_new(on_create_view_commit,
>>
>select);
>> + if (on_commit_view == NULL)
>> + return -1;
>> txn_stmt_on_commit(stmt, on_commit_view);
>> struct trigger *on_rollback_view =
>>
>txn_alter_trigger_new(on_create_view_rollback,
>>
>select);
>> + if (on_rollback_view == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt,
>on_rollback_view);
>> select_guard.is_active = false;
>> }
>> } 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);
>> + if (access_check_ddl(old_space->def->name, old_space-
>>def->id,
>> + old_space->def->uid, SC_SPACE,
>PRIV_D) != 0)
>> + return -1;
>> /* Verify that the space is empty (has no indexes) */
>> if (old_space->index_count) {
>> diag_set(ClientError, ER_DROP_SPACE,
>> @@ -2058,9 +2077,13 @@ on_replace_dd_space(struct trigger * /* trigger */,
>> void *event) ++schema_version;
>> struct trigger *on_commit =
>> txn_alter_trigger_new(on_drop_space_commit,
>old_space);
>> + if (on_commit == NULL)
>> + return -1;
>> txn_stmt_on_commit(stmt, on_commit);
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_drop_space_rollback, old_space);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> if (old_space->def->opts.is_view) {
>> struct Select *select =
>> @@ -2074,10 +2097,14 @@ on_replace_dd_space(struct trigger * /* trigger */,
>> void *event) struct trigger *on_commit_view =
>>
>txn_alter_trigger_new(on_drop_view_commit,
>>
>select);
>> + if (on_commit_view == NULL)
>> + return -1;
>> txn_stmt_on_commit(stmt, on_commit_view);
>> struct trigger *on_rollback_view =
>>
>txn_alter_trigger_new(on_drop_view_rollback,
>>
>select);
>> + if (on_rollback_view == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt,
>on_rollback_view);
>> update_view_references(select, -1, true,
>NULL);
>> select_guard.is_active = false;
>> @@ -2089,8 +2116,9 @@ on_replace_dd_space(struct trigger * /* trigger */,
>> void *event) region);
>> auto def_guard =
>> make_scoped_guard([=] {
>space_def_delete(def); });
>> - access_check_ddl(def->name, def->id, def->uid,
>SC_SPACE,
>> - PRIV_A);
>> + if (access_check_ddl(def->name, def->id, def->uid,
>SC_SPACE,
>> + PRIV_A) != 0)
>> + return -1;
>> if (def->id != space_id(old_space)) {
>> diag_set(ClientError, ER_ALTER_SPACE,
>> space_name(old_space),
>> @@ -2246,8 +2274,9 @@ on_replace_dd_index(struct trigger * /* trigger */,
>> void *event) enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D;
>> 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);
>> + if (access_check_ddl(old_space->def->name, old_space->def->id,
>> + old_space->def->uid, SC_SPACE, priv_type) !=
>0)
>> + return -1;
>> struct index *old_index = space_index(old_space, iid);
>>
>> /*
>> @@ -2684,8 +2713,9 @@ on_replace_dd_user(struct trigger * /* trigger */,
>> void *event) struct user *old_user = user_by_id(uid);
>> 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);
>> + if (access_check_ddl(user->name, user->uid, user->owner,
>user->type,
>> + PRIV_C) != 0)
>> + return -1;
>> auto def_guard = make_scoped_guard([=] { free(user);
>});
>> try {
>> (void) user_cache_replace(user);
>> @@ -2695,11 +2725,14 @@ on_replace_dd_user(struct trigger * /* trigger */,
>> void *event) def_guard.is_active = false;
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(user_cache_remove_user, new_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> } else if (new_tuple == NULL) { /* DELETE */
>> - access_check_ddl(old_user->def->name, old_user->def-
>>uid,
>> + if (access_check_ddl(old_user->def->name, old_user->def-
>>uid,
>> old_user->def->owner, old_user-
>>def->type,
>> - PRIV_D);
>> + PRIV_D) != 0)
>> + return -1;
>> /* Can't drop guest or super user */
>> if (uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid ==
>SUPER) {
>> diag_set(ClientError, ER_DROP_USER,
>> @@ -2719,6 +2752,8 @@ on_replace_dd_user(struct trigger * /* trigger */,
>> void *event) user_cache_delete(uid);
>> struct trigger *on_rollback =
>> txn_alter_trigger_new(user_cache_alter_user,
>old_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> } else { /* UPDATE, REPLACE */
>> assert(old_user != NULL && new_tuple != NULL);
>> @@ -2728,8 +2763,9 @@ on_replace_dd_user(struct trigger * /* trigger */,
>> void *event) * correct.
>> */
>> 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);
>> + if (access_check_ddl(user->name, user->uid, user->uid,
>> + old_user->def->type, PRIV_A) !=
>0)
>> + return -1;
>> auto def_guard = make_scoped_guard([=] { free(user);
>});
>> try {
>> user_cache_replace(user);
>> @@ -2739,6 +2775,8 @@ on_replace_dd_user(struct trigger * /* trigger */,
>> void *event) def_guard.is_active = false;
>> struct trigger *on_rollback =
>> txn_alter_trigger_new(user_cache_alter_user,
>old_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> }
>> return 0;
>> @@ -2984,10 +3022,13 @@ 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);
>> auto def_guard = make_scoped_guard([=] { free(def); });
>> - access_check_ddl(def->name, def->fid, def->uid,
>SC_FUNCTION,
>> - PRIV_C);
>> + if (access_check_ddl(def->name, def->fid, def->uid,
>SC_FUNCTION,
>> + PRIV_C) != 0)
>> + return -1;
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_create_func_rollback, NULL);
>> + if (on_rollback == NULL)
>> + return -1;
>> struct func *func = func_new(def);
>> if (func == NULL)
>> return -1;
>> @@ -3003,8 +3044,9 @@ on_replace_dd_func(struct trigger * /* trigger */,
>> void *event) * Can only delete func if you're the one
>> * who created it or a superuser.
>> */
>> - access_check_ddl(old_func->def->name, fid, uid,
>SC_FUNCTION,
>> - PRIV_D);
>> + if (access_check_ddl(old_func->def->name, fid, uid,
>SC_FUNCTION,
>> + PRIV_D) != 0)
>> + return -1;
>> /* Can only delete func if it has no grants. */
>> if (schema_find_grants("function", old_func->def->fid)) {
>> diag_set(ClientError, ER_DROP_FUNCTION,
>> @@ -3030,6 +3072,8 @@ on_replace_dd_func(struct trigger * /* trigger */,
>> void *event) txn_alter_trigger_new(on_drop_func_commit, old_func);
>> struct trigger *on_rollback =
>> txn_alter_trigger_new(on_drop_func_rollback,
>old_func);
>> + if (on_commit == NULL || on_rollback == NULL)
>> + return -1;
>> func_cache_delete(old_func->def->fid);
>> txn_stmt_on_commit(stmt, on_commit);
>> txn_stmt_on_rollback(stmt, on_rollback);
>> @@ -3193,6 +3237,8 @@ on_replace_dd_collation(struct trigger * /* trigger
>> */, void *event) txn_alter_trigger_new(on_drop_collation_commit, NULL);
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_drop_collation_rollback, NULL);
>> + if (on_commit == NULL || on_rollback == NULL)
>> + return -1;
>> /*
>> * TODO: Check that no index uses the collation
>> * identifier.
>> @@ -3212,9 +3258,10 @@ on_replace_dd_collation(struct trigger * /* trigger
>> */, void *event) }
>> struct coll_id *old_coll_id = coll_by_id(old_id);
>> assert(old_coll_id != NULL);
>> - access_check_ddl(old_coll_id->name, old_coll_id->id,
>> + if (access_check_ddl(old_coll_id->name, old_coll_id->id,
>> old_coll_id->owner_id,
>SC_COLLATION,
>> - PRIV_D);
>> + PRIV_D) != 0)
>> + return -1;
>> /*
>> * Set on_commit/on_rollback triggers after
>> * deletion from the cache to make trigger logic
>> @@ -3229,10 +3276,13 @@ on_replace_dd_collation(struct trigger * /* trigger
>> */, void *event) /* INSERT */
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_create_collation_rollback, NULL);
>> + if (on_rollback == NULL)
>> + return -1;
>> 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);
>> + if (access_check_ddl(new_def.name, new_def.id,
>new_def.owner_id,
>> + SC_COLLATION, PRIV_C) != 0)
>> + return -1;
>> struct coll_id *new_coll_id = coll_id_new(&new_def);
>> if (new_coll_id == NULL)
>> return -1;
>> @@ -3256,20 +3306,25 @@ on_replace_dd_collation(struct trigger * /* trigger
>> */, void *event) /**
>> * Create a privilege definition from tuple.
>> */
>> -void
>> +int
>> priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>> {
>> - priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
>> - priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
>> + if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) !
>= 0)
>> + return -1;
>> + if (tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id))
>!= 0)
>> + return -1;
>>
>> const char *object_type =
>> - tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
>> + tuple_field_cstr(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
>> + if (object_type == NULL)
>> + return -1;
>> priv->object_type = schema_object_type(object_type);
>>
>> const char *data = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>> if (data == NULL) {
>> - tnt_raise(ClientError, ER_NO_SUCH_FIELD_NO,
>> + diag_set(ClientError, ER_NO_SUCH_FIELD_NO,
>> BOX_PRIV_FIELD_OBJECT_ID +
>TUPLE_INDEX_BASE);
>> + return -1;
>> }
>> /*
>> * When granting or revoking privileges on a whole entity
>> @@ -3287,14 +3342,20 @@ priv_def_create_from_tuple(struct priv_def *priv,
>> struct tuple *tuple) }
>> FALLTHROUGH;
>> default:
>> - priv->object_id = tuple_field_u32_xc(tuple,
>> -
>BOX_PRIV_FIELD_OBJECT_ID);
>> + if (tuple_field_u32(tuple,
>> + BOX_PRIV_FIELD_OBJECT_ID, &(priv->object_id)) != 0)
>> + return -1;
>> }
>> if (priv->object_type == SC_UNKNOWN) {
>> - tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>> + diag_set(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>> object_type);
>> + return -1;
>> }
>> - priv->access = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ACCESS);
>> + uint32_t out;
>> + if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ACCESS, &out) != 0)
>> + return -1;
>> + priv->access = out;
>> + return 0;
>> }
>>
>> /*
>> @@ -3307,62 +3368,80 @@ priv_def_create_from_tuple(struct priv_def *priv,
>> struct tuple *tuple) * object can be changed during WAL write.
>> * In the future we must protect grant/revoke with a logical lock.
>> */
>> -static void
>> +static int
>> priv_def_check(struct priv_def *priv, enum priv_type priv_type)
>> {
>> - struct user *grantor = user_find_xc(priv->grantor_id);
>> + struct user *grantor = user_find(priv->grantor_id);
>> + if (grantor == NULL)
>> + return -1;
>> /* May be a role */
>> struct user *grantee = user_by_id(priv->grantee_id);
>> if (grantee == NULL) {
>> - tnt_raise(ClientError, ER_NO_SUCH_USER,
>> + diag_set(ClientError, ER_NO_SUCH_USER,
>> int2str(priv->grantee_id));
>> + return -1;
>> }
>> 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);
>> + if (access_check_ddl(name, priv->object_id, grantor->def->uid,
>> + priv->object_type, priv_type) != 0)
>> + return -1;
>> switch (priv->object_type) {
>> case SC_UNIVERSE:
>> if (grantor->def->uid != ADMIN) {
>> - tnt_raise(AccessDeniedError,
>> + diag_set(AccessDeniedError,
>> priv_name(priv_type),
>>
>schema_object_name(SC_UNIVERSE),
>> name,
>> grantor->def->name);
>> + return -1;
>> }
>> break;
>> case SC_SPACE:
>> {
>> - struct space *space = space_cache_find_xc(priv-
>>object_id);
>> + struct space *space = space_cache_find(priv->object_id);
>> + if (space == NULL)
>> + return -1;
>> if (space->def->uid != grantor->def->uid &&
>> grantor->def->uid != ADMIN) {
>> - tnt_raise(AccessDeniedError,
>> + diag_set(AccessDeniedError,
>> priv_name(priv_type),
>> schema_object_name(SC_SPACE),
>name,
>> grantor->def->name);
>> + return -1;
>> }
>> break;
>> }
>> case SC_FUNCTION:
>> {
>> - struct func *func = func_cache_find(priv->object_id);
>> + struct func *func = func_by_id(priv->object_id);
>> + if (func == NULL) {
>> + diag_set(ClientError, ER_NO_SUCH_FUNCTION,
>int2str(priv->object_id));
>> + return -1;
>> + }
>> if (func->def->uid != grantor->def->uid &&
>> grantor->def->uid != ADMIN) {
>> - tnt_raise(AccessDeniedError,
>> + diag_set(AccessDeniedError,
>> priv_name(priv_type),
>>
>schema_object_name(SC_FUNCTION), name,
>> grantor->def->name);
>> + return -1;
>> }
>> break;
>> }
>> case SC_SEQUENCE:
>> {
>> - struct sequence *seq = sequence_cache_find(priv-
>>object_id);
>> + struct sequence *seq = sequence_by_id(priv->object_id);
>> + if (seq == NULL) {
>> + diag_set(ClientError, ER_NO_SUCH_SEQUENCE,
>int2str(priv->object_id));
>> + return -1;
>> + }
>> if (seq->def->uid != grantor->def->uid &&
>> grantor->def->uid != ADMIN) {
>> - tnt_raise(AccessDeniedError,
>> + diag_set(AccessDeniedError,
>> priv_name(priv_type),
>>
>schema_object_name(SC_SEQUENCE), name,
>> grantor->def->name);
>> + return -1;
>> }
>> break;
>> }
>> @@ -3370,9 +3449,10 @@ priv_def_check(struct priv_def *priv, enum priv_type
>> priv_type) {
>> struct user *role = user_by_id(priv->object_id);
>> if (role == NULL || role->def->type != SC_ROLE) {
>> - tnt_raise(ClientError, ER_NO_SUCH_ROLE,
>> + diag_set(ClientError, ER_NO_SUCH_ROLE,
>> role ? role->def->name :
>> int2str(priv->object_id));
>> + return -1;
>> }
>> /*
>> * Only the creator of the role can grant or revoke it.
>> @@ -3381,29 +3461,33 @@ priv_def_check(struct priv_def *priv, enum priv_type
>> priv_type) if (role->def->owner != grantor->def->uid &&
>> grantor->def->uid != ADMIN &&
>> (role->def->uid != PUBLIC || priv->access !=
>PRIV_X)) {
>> - tnt_raise(AccessDeniedError,
>> + diag_set(AccessDeniedError,
>> priv_name(priv_type),
>> schema_object_name(SC_ROLE),
>name,
>> grantor->def->name);
>> + return -1;
>> }
>> /* Not necessary to do during revoke, but who cares. */
>> - role_check(grantee, role);
>> + if (role_check(grantee, role) != 0)
>> + return -1;
>> break;
>> }
>> case SC_USER:
>> {
>> struct user *user = user_by_id(priv->object_id);
>> if (user == NULL || user->def->type != SC_USER) {
>> - tnt_raise(ClientError, ER_NO_SUCH_USER,
>> + diag_set(ClientError, ER_NO_SUCH_USER,
>> user ? user->def->name :
>> int2str(priv->object_id));
>> + return -1;
>> }
>> if (user->def->owner != grantor->def->uid &&
>> grantor->def->uid != ADMIN) {
>> - tnt_raise(AccessDeniedError,
>> + diag_set(AccessDeniedError,
>> priv_name(priv_type),
>> schema_object_name(SC_USER),
>name,
>> grantor->def->name);
>> + return -1;
>> }
>> break;
>> }
>> @@ -3415,30 +3499,33 @@ priv_def_check(struct priv_def *priv, enum priv_type
>> priv_type) {
>> /* Only admin may grant privileges on an entire entity.
>*/
>> if (grantor->def->uid != ADMIN) {
>> - tnt_raise(AccessDeniedError,
>priv_name(priv_type),
>> + diag_set(AccessDeniedError,
>priv_name(priv_type),
>> schema_object_name(priv-
>>object_type), name,
>> grantor->def->name);
>> + return -1;
>> }
>> }
>> default:
>> break;
>> }
>> if (priv->access == 0) {
>> - tnt_raise(ClientError, ER_GRANT,
>> + diag_set(ClientError, ER_GRANT,
>> "the grant tuple has no privileges");
>> + return -1;
>> }
>> + return 0;
>> }
>>
>> /**
>> * Update a metadata cache object with the new access
>> * data.
>> */
>> -static void
>> +static int
>> grant_or_revoke(struct priv_def *priv)
>> {
>> struct user *grantee = user_by_id(priv->grantee_id);
>> if (grantee == NULL)
>> - return;
>> + return 0;
>> /*
>> * Grant a role to a user only when privilege type is 'execute'
>> * and the role is specified.
>> @@ -3446,14 +3533,19 @@ grant_or_revoke(struct priv_def *priv)
>> if (priv->object_type == SC_ROLE && !(priv->access & ~PRIV_X)) {
>> struct user *role = user_by_id(priv->object_id);
>> if (role == NULL || role->def->type != SC_ROLE)
>> - return;
>> - if (priv->access)
>> - role_grant(grantee, role);
>> - else
>> - role_revoke(grantee, role);
>> + return 0;
>> + if (priv->access) {
>> + if (role_grant(grantee, role) != 0)
>> + return -1;
>> + } else {
>> + if (role_revoke(grantee, role) != 0)
>> + return -1;
>> + }
>> } else {
>> - priv_grant(grantee, priv);
>> + if (priv_grant(grantee, priv) != 0)
>> + return -1;
>> }
>> + return 0;
>> }
>>
>> /** A trigger called on rollback of grant. */
>> @@ -3463,9 +3555,11 @@ revoke_priv(struct trigger *trigger, void *event)
>> (void) event;
>> struct tuple *tuple = (struct tuple *)trigger->data;
>> struct priv_def priv;
>> - priv_def_create_from_tuple(&priv, tuple);
>> + if (priv_def_create_from_tuple(&priv, tuple) != 0)
>> + return -1;
>> priv.access = 0;
>> - grant_or_revoke(&priv);
>> + if (grant_or_revoke(&priv) != 0)
>> + return -1;
>> return 0;
>> }
>>
>> @@ -3476,8 +3570,10 @@ modify_priv(struct trigger *trigger, void *event)
>> (void) event;
>> struct tuple *tuple = (struct tuple *)trigger->data;
>> struct priv_def priv;
>> - priv_def_create_from_tuple(&priv, tuple);
>> - grant_or_revoke(&priv);
>> + if (priv_def_create_from_tuple(&priv, tuple) != 0)
>> + return -1;
>> + if (grant_or_revoke(&priv) != 0)
>> + return -1;
>> return 0;
>> }
>>
>> @@ -3495,27 +3591,42 @@ on_replace_dd_priv(struct trigger * /* trigger */,
>> void *event) struct priv_def priv;
>>
>> if (new_tuple != NULL && old_tuple == NULL) { /* grant */
>> - priv_def_create_from_tuple(&priv, new_tuple);
>> - priv_def_check(&priv, PRIV_GRANT);
>> - grant_or_revoke(&priv);
>> + if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
>> + return -1;
>> + if (priv_def_check(&priv, PRIV_GRANT) != 0)
>> + return -1;
>> + if (grant_or_revoke(&priv) != 0)
>> + return -1;
>> struct trigger *on_rollback =
>> txn_alter_trigger_new(revoke_priv,
>new_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> } else if (new_tuple == NULL) { /* revoke */
>> assert(old_tuple);
>> - priv_def_create_from_tuple(&priv, old_tuple);
>> - priv_def_check(&priv, PRIV_REVOKE);
>> + if (priv_def_create_from_tuple(&priv, old_tuple) != 0)
>> + return -1;
>> + if (priv_def_check(&priv, PRIV_REVOKE) != 0)
>> + return -1;
>It would be looking better if you combine such blocks into an or condition
>like:
>if (priv_def_create_from_tuple(&priv, old_tuple) != 0 ||
> priv_def_check(&priv, PRIV_REVOKE) != 0)
> return -1;
>
>> priv.access = 0;
>> - grant_or_revoke(&priv);
>> + if (grant_or_revoke(&priv) != 0)
>> + return -1;
>> struct trigger *on_rollback =
>> txn_alter_trigger_new(modify_priv,
>old_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> } else { /* modify */
>> - priv_def_create_from_tuple(&priv, new_tuple);
>> - priv_def_check(&priv, PRIV_GRANT);
>> - grant_or_revoke(&priv);
>> + if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
>> + return -1;
>> + if (priv_def_check(&priv, PRIV_GRANT) != 0)
>> + return -1;
>> + if (grant_or_revoke(&priv) != 0)
>> + return -1;
>Please combine it into one condition here and other places
>> struct trigger *on_rollback =
>> txn_alter_trigger_new(modify_priv,
>old_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> }
>> return 0;
>> @@ -3656,6 +3767,8 @@ on_replace_dd_cluster(struct trigger *trigger, void
>> *event) struct trigger *on_commit;
>> on_commit =
>txn_alter_trigger_new(register_replica,
>>
>new_tuple);
>> + if (on_commit == NULL)
>> + return -1;
>> txn_stmt_on_commit(stmt, on_commit);
>> }
>> } else {
>> @@ -3672,6 +3785,8 @@ on_replace_dd_cluster(struct trigger *trigger, void
>> *event) struct trigger *on_commit;
>> on_commit = txn_alter_trigger_new(unregister_replica,
>>
>old_tuple);
>> + if (on_commit == NULL)
>> + return -1;
>> txn_stmt_on_commit(stmt, on_commit);
>> }
>> return 0;
>> @@ -3794,10 +3909,13 @@ on_replace_dd_sequence(struct trigger * /* trigger
>> */, void *event) if (old_tuple == NULL && new_tuple != NULL) { /
>* INSERT
>> */
>> new_def = sequence_def_new_from_tuple(new_tuple,
>>
>ER_CREATE_SEQUENCE);
>> - access_check_ddl(new_def->name, new_def->id, new_def-
>>uid,
>> - SC_SEQUENCE, PRIV_C);
>> + if (access_check_ddl(new_def->name, new_def->id,
>new_def->uid,
>> + SC_SEQUENCE, PRIV_C) != 0)
>> + return -1;
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_create_sequence_rollback, NULL);
>> + if (on_rollback == NULL)
>> + return -1;
>> seq = sequence_new_xc(new_def);
>> sequence_cache_insert(seq);
>> on_rollback->data = seq;
>> @@ -3807,8 +3925,9 @@ on_replace_dd_sequence(struct trigger * /* trigger */,
>> void *event) BOX_SEQUENCE_DATA_FIELD_ID);
>> seq = sequence_by_id(id);
>> assert(seq != NULL);
>> - access_check_ddl(seq->def->name, seq->def->id, seq-
>>def->uid,
>> - SC_SEQUENCE, PRIV_D);
>> + if (access_check_ddl(seq->def->name, seq->def->id, seq-
>>def->uid,
>> + SC_SEQUENCE, PRIV_D) != 0)
>> + return -1;
>> if (space_has_data(BOX_SEQUENCE_DATA_ID, 0, id)) {
>> diag_set(ClientError, ER_DROP_SEQUENCE,
>> seq->def->name, "the sequence
>has data");
>> @@ -3828,6 +3947,8 @@ on_replace_dd_sequence(struct trigger * /* trigger */,
>> void *event) txn_alter_trigger_new(on_drop_sequence_commit, seq);
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_drop_sequence_rollback, seq);
>> + if (on_commit == NULL || on_rollback == NULL)
>> + return -1;
>> sequence_cache_delete(seq->def->id);
>> txn_stmt_on_commit(stmt, on_commit);
>> txn_stmt_on_rollback(stmt, on_rollback);
>> @@ -3836,12 +3957,15 @@ on_replace_dd_sequence(struct trigger * /* trigger
>> */, void *event) ER_ALTER_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);
>> + if (access_check_ddl(seq->def->name, seq->def->id, seq-
>>def->uid,
>> + SC_SEQUENCE, PRIV_A) != 0)
>> + return -1;
>> struct trigger *on_commit =
>>
>txn_alter_trigger_new(on_alter_sequence_commit, seq->def);
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_alter_sequence_rollback, seq->def);
>> + if (on_commit == NULL || on_rollback == NULL)
>> + return -1;
>> seq->def = new_def;
>> txn_stmt_on_commit(stmt, on_commit);
>> txn_stmt_on_rollback(stmt, on_rollback);
>> @@ -3899,6 +4023,8 @@ on_replace_dd_sequence_data(struct trigger * /*
>> trigger */, void *event) */
>> struct trigger *on_rollback = txn_alter_trigger_new(
>> on_drop_sequence_data_rollback,
>old_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> sequence_reset(seq);
>> }
>> @@ -4015,21 +4141,25 @@ 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);
>> + if (access_check_ddl(seq->def->name, seq->def->id, seq-
>>def->uid,
>> + SC_SEQUENCE, priv_type) != 0)
>> + return -1;
>> } 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);
>> - access_check_ddl(seq->def->name, seq->def->id, seq-
>>def->uid,
>> - SC_SEQUENCE, PRIV_W);
>> + if (access_check_ddl(seq->def->name, seq->def->id, seq-
>>def->uid,
>> + SC_SEQUENCE, PRIV_R) != 0)
>> + return -1;
>> + if (access_check_ddl(seq->def->name, seq->def->id, seq-
>>def->uid,
>> + SC_SEQUENCE, PRIV_W) != 0)
>> + return -1;
>> }
>> /** Check we have alter access on space. */
>> - access_check_ddl(space->def->name, space->def->id, space->def-
>>uid,
>> - SC_SPACE, PRIV_A);
>> + if (access_check_ddl(space->def->name, space->def->id, space->def-
>>uid,
>> + SC_SPACE, PRIV_A) != 0)
>> + return -1;
>>
>> if (stmt->new_tuple != NULL) { /*
>INSERT, UPDATE */
>> char *sequence_path;
>> @@ -4052,6 +4182,8 @@ on_replace_dd_space_sequence(struct trigger * /*
>> trigger */, void *event) else
>> on_rollback =
>txn_alter_trigger_new(clear_space_sequence,
>>
>stmt->new_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> seq->is_generated = is_generated;
>> space->sequence = seq;
>> space->sequence_fieldno = sequence_fieldno;
>> @@ -4063,6 +4195,8 @@ on_replace_dd_space_sequence(struct trigger * /*
>> trigger */, void *event) struct trigger *on_rollback;
>> on_rollback = txn_alter_trigger_new(set_space_sequence,
>> stmt-
>>old_tuple);
>> + if (on_rollback == NULL)
>> + return -1;
>> assert(space->sequence == seq);
>> seq->is_generated = false;
>> space->sequence = NULL;
>> @@ -4153,6 +4287,8 @@ on_replace_dd_trigger(struct trigger * /* trigger */,
>> void *event) struct trigger *on_rollback = txn_alter_trigger_new(NULL,
>> NULL); struct trigger *on_commit =
>> txn_alter_trigger_new(on_replace_trigger_commit, NULL);
>> + if (on_commit == NULL || on_rollback == NULL)
>> + return -1;
>>
>> if (old_tuple != NULL && new_tuple == NULL) {
>> /* DROP trigger. */
>> @@ -4655,6 +4791,8 @@ on_replace_dd_fk_constraint(struct trigger * /*
>> trigger*/, void *event) struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_create_fk_constraint_rollback,
>> fk);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> fk_constraint_set_mask(fk,
>> &parent_space-
>>fk_constraint_mask,
>> @@ -4673,10 +4811,14 @@ on_replace_dd_fk_constraint(struct trigger * /*
>> trigger*/, void *event) struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_replace_fk_constraint_rollback,
>>
>old_fk);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> struct trigger *on_commit =
>>
>txn_alter_trigger_new(on_drop_or_replace_fk_constraint_commit,
>>
>old_fk);
>> + if (on_commit == NULL)
>> + return -1;
>> txn_stmt_on_commit(stmt, on_commit);
>> space_reset_fk_constraint_mask(child_space);
>>
>space_reset_fk_constraint_mask(parent_space);
>> @@ -4699,10 +4841,14 @@ on_replace_dd_fk_constraint(struct trigger * /*
>> trigger*/, void *event) struct trigger *on_commit =
>>
>txn_alter_trigger_new(on_drop_or_replace_fk_constraint_commit,
>> old_fk);
>> + if (on_commit == NULL)
>> + return -1;
>> txn_stmt_on_commit(stmt, on_commit);
>> struct trigger *on_rollback =
>>
>txn_alter_trigger_new(on_drop_fk_constraint_rollback,
>> old_fk);
>> + if (on_rollback == NULL)
>> + return -1;
>> txn_stmt_on_rollback(stmt, on_rollback);
>> space_reset_fk_constraint_mask(child_space);
>> space_reset_fk_constraint_mask(parent_space);
>> @@ -4830,6 +4976,8 @@ on_replace_dd_ck_constraint(struct trigger * /*
>> trigger*/, void *event) struct space *space =
>> space_cache_find_xc(space_id);
>> struct trigger *on_rollback = txn_alter_trigger_new(NULL, NULL);
>> struct trigger *on_commit = txn_alter_trigger_new(NULL, NULL);
>> + if (on_commit == NULL || on_rollback == NULL)
>> + return -1;
>>
>> if (new_tuple != NULL) {
>> bool is_deferred =
>> diff --git a/src/box/user.cc b/src/box/user.cc
>> index c46ff67d1..92a183c2a 100644
>> --- a/src/box/user.cc
>> +++ b/src/box/user.cc
>> @@ -180,18 +180,24 @@ user_destroy(struct user *user)
>> * Add a privilege definition to the list
>> * of effective privileges of a user.
>> */
>> -void
>> +int
>> user_grant_priv(struct user *user, struct priv_def *def)
>> {
>> struct priv_def *old = privset_search(&user->privs, def);
>> if (old == NULL) {
>> + size_t size = sizeof(struct priv_def);
>> old = (struct priv_def *)
>> - region_alloc_xc(&user->pool, sizeof(struct
>priv_def));
>> + region_alloc(&user->pool, size);
>> + if (old == NULL) {
>> + diag_set(OutOfMemory, size, "region", "new
>slab");
>> + return -1;
>> + }
>> *old = *def;
>> privset_insert(&user->privs, old);
>> } else {
>> old->access |= def->access;
>> }
>> + return 0;
>> }
>>
>> /**
>> @@ -305,11 +311,11 @@ user_set_effective_access(struct user *user)
>> /**
>> * Reload user privileges and re-grant them.
>> */
>> -static void
>> +static int
>> user_reload_privs(struct user *user)
>> {
>> if (user->is_dirty == false)
>> - return;
>> + return 0;
>> struct priv_def *priv;
>> /**
>> * Reset effective access of the user in the
>> @@ -326,26 +332,43 @@ user_reload_privs(struct user *user)
>> privset_new(&user->privs);
>> /* Load granted privs from _priv space. */
>> {
>> - struct space *space = space_cache_find_xc(BOX_PRIV_ID);
>> + struct space *space = space_cache_find(BOX_PRIV_ID);
>> + if (space == NULL)
>> + return -1;
>> char key[6];
>> /** Primary key - by user id */
>> - struct index *index = index_find_system_xc(space, 0);
>> + if (!space_is_memtx(space)) {
>> + diag_set(ClientError, ER_UNSUPPORTED,
>> + space->engine->name, "system
>data");
>> + return -1;
>> + }
>> + struct index *index = index_find(space, 0);
>> + if (index == NULL)
>> + return -1;
>> mp_encode_uint(key, user->def->uid);
>>
>> - struct iterator *it = index_create_iterator_xc(index,
>ITER_EQ,
>> + struct iterator *it = index_create_iterator(index,
>ITER_EQ,
>>
>key, 1);
>> + if (it == NULL)
>> + return -1;
>> IteratorGuard iter_guard(it);
>>
>> struct tuple *tuple;
>> - while ((tuple = iterator_next_xc(it)) != NULL) {
>> + if (iterator_next(it, &tuple) != 0)
>> + return -1;
>> + while (tuple != NULL) {
>> struct priv_def priv;
>> - priv_def_create_from_tuple(&priv, tuple);
>> + if (priv_def_create_from_tuple(&priv, tuple)
>!= 0)
>> + return -1;
>> /**
>> * Skip role grants, we're only
>> * interested in real objects.
>> */
>> if (priv.object_type != SC_ROLE || !
>(priv.access & PRIV_X))
>> - user_grant_priv(user, &priv);
>> + if (user_grant_priv(user, &priv) !
>= 0)
>> + return -1;
>> + if (iterator_next(it, &tuple) != 0)
>> + return -1;
>> }
>> }
>> {
>> @@ -358,12 +381,14 @@ user_reload_privs(struct user *user)
>> privset_ifirst(&role->privs, &it);
>> struct priv_def *def;
>> while ((def = privset_inext(&it))) {
>> - user_grant_priv(user, def);
>> + if (user_grant_priv(user, def) !=
>0)
>> + return -1;
>> }
>> }
>> }
>> user_set_effective_access(user);
>> user->is_dirty = false;
>> + return 0;
>> }
>>
>> /** }}} */
>> @@ -559,7 +584,7 @@ user_cache_free()
>>
>> /** {{{ roles */
>>
>> -void
>> +int
>> role_check(struct user *grantee, struct user *role)
>> {
>> /*
>> @@ -592,16 +617,18 @@ role_check(struct user *grantee, struct user *role)
>> */
>> if (user_map_is_set(&transitive_closure,
>> role->auth_token)) {
>> - tnt_raise(ClientError, ER_ROLE_LOOP,
>> + diag_set(ClientError, ER_ROLE_LOOP,
>> role->def->name, grantee->def->name);
>> + return -1;
>> }
>> + return 0;
>> }
>>
>> /**
>> * Re-calculate effective grants of the linked subgraph
>> * this user/role is a part of.
>> */
>> -void
>> +int
>> rebuild_effective_grants(struct user *grantee)
>> {
>> /*
>> @@ -653,7 +680,8 @@ rebuild_effective_grants(struct user *grantee)
>> struct user_map indirect_edges = user-
>>roles;
>> user_map_minus(&indirect_edges,
>&transitive_closure);
>> if (user_map_is_empty(&indirect_edges)) {
>> - user_reload_privs(user);
>> + if (user_reload_privs(user) != 0)
>> + return -1;
>> user_map_union(&next_layer,
>&user->users);
>> } else {
>> /*
>> @@ -674,6 +702,7 @@ rebuild_effective_grants(struct user *grantee)
>> user_map_union(&transitive_closure, ¤t_layer);
>> current_layer = next_layer;
>> }
>> + return 0;
>> }
>>
>>
>> @@ -682,35 +711,41 @@ rebuild_effective_grants(struct user *grantee)
>> * Grant all effective privileges of the role to whoever
>> * this role was granted to.
>> */
>> -void
>> +int
>> role_grant(struct user *grantee, struct user *role)
>> {
>> user_map_set(&role->users, grantee->auth_token);
>> user_map_set(&grantee->roles, role->auth_token);
>> - rebuild_effective_grants(grantee);
>> + if (rebuild_effective_grants(grantee) != 0)
>> + return -1;
>> + return 0;
>> }
>>
>> /**
>> * Update the role dependencies graph.
>> * Rebuild effective privileges of the grantee.
>> */
>> -void
>> +int
>> role_revoke(struct user *grantee, struct user *role)
>> {
>> user_map_clear(&role->users, grantee->auth_token);
>> user_map_clear(&grantee->roles, role->auth_token);
>> - rebuild_effective_grants(grantee);
>> + if (rebuild_effective_grants(grantee) != 0)
>> + return -1;
>> + return 0;
>> }
>>
>> -void
>> +int
>> priv_grant(struct user *grantee, struct priv_def *priv)
>> {
>> struct access *object = access_find(priv->object_type, priv-
>>object_id);
>> if (object == NULL)
>> - return;
>> + return 0;
>> struct access *access = &object[grantee->auth_token];
>> access->granted = priv->access;
>> - rebuild_effective_grants(grantee);
>> + if (rebuild_effective_grants(grantee) != 0)
>> + return -1;
>> + return 0;
>> }
>>
>> /** }}} */
>> diff --git a/src/box/user.h b/src/box/user.h
>> index 527fb2e7c..ae2b73e55 100644
>> --- a/src/box/user.h
>> +++ b/src/box/user.h
>> @@ -144,16 +144,6 @@ user_cache_replace(struct user_def *user);
>> void
>> user_cache_delete(uint32_t uid);
>>
>> -/* Find a user by name. Used by authentication. */
>> -static inline struct user *
>> -user_find_xc(uint32_t uid)
>> -{
>> - struct user *user = user_find(uid);
>> - if (user == NULL)
>> - diag_raise();
>> - return user;
>> -}
>> -
>> static inline struct user *
>> user_find_by_name_xc(const char *name, uint32_t len)
>> {
>> @@ -178,19 +168,19 @@ user_cache_free();
>> * and no loop in the graph will occur when grantee gets
>> * a given role.
>> */
>> -void
>> +int
>> role_check(struct user *grantee, struct user *role);
>>
>> /**
>> * Grant a role to a user or another role.
>> */
>> -void
>> +int
>> role_grant(struct user *grantee, struct user *role);
>>
>> /**
>> * Revoke a role from a user or another role.
>> */
>> -void
>> +int
>> role_revoke(struct user *grantee, struct user *role);
>>
>> /**
>> @@ -198,10 +188,10 @@ role_revoke(struct user *grantee, struct user *role);
>> * and re-evaluate effective access of all users of this
>> * role if this role.
>> */
>> -void
>> +int
>> priv_grant(struct user *grantee, struct priv_def *priv);
>>
>> -void
>> +int
>> priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple);
>>
>> /* }}} */
>
>
--
Ilya Kosarev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20191021/87df830b/attachment.html>
More information about the Tarantool-patches
mailing list