Unclear behavior of test_create_space

The following discussion from !767 (merged) should be addressed:

• @d.rodionov started a discussion: (+1 comment)

  Hmm, I think, the whole test_create_space replaces access check that is invoked for create table op. It doesnt feel good or bad, this just quite obscure. Alternative can be to attempt creation from admin for validation and leave access control check for other machinery.

  It is weird to say, but now it feels like the whole acl thing could've been implemented in this try/rollback fashion

I tried to look at whats happening but didnt find the answer quickly. For some reason test create space succeeds even if user doesnt have privilege to create a space. This is unexpected but actually the behavior we'd likely want to keep. Maybe execution of triggers is delayed to commit point which never happens. Though I see that on_replace_dd_space is actually called (according to my ebpf script). Mystery

assigned to @d.rodionov

cc @gmoshkin maybe you have some ideas

Can you fill me in on the discussion? What seems to be the problem?

For some reason test create space succeeds even if user doesnt have privilege to create a space

We can't figure out why this happens. Though the end to end scenario works now as we check privileges later, and even if test creation succeeds we will return error in case of insufficient privileges

I'm not sure, but if this function is called with effective user id of admin, the operation may still succeed, because technically speaking "creating a space" is not the same as "owning a space". Did you try calling this function under the user in question?

Yep, you are correct we are indeed doing it under admin (at least in sql), though we are passing a different owner. I added an assert_eq!(self.owner, effective_user_id()); and it failed with 32 != 1. The only thing to find out is where we switch to admin

Oh, you're both right, I failed to see let su = session::su(ADMIN_USER_ID).expect("cant fail because session is available"); just above the match in reenterable_schema_change_request. We do that because user doesnt have access to raft spaces which are used during cas. In lua api this fails earlier with an error that user doesnt have permission to read from raft/property space so it just crashes in this case:

picodata> pico.create_table({
        >         name = 'friends_of_peppa',
        >         format = {
        >             {name = 'id', type = 'unsigned', is_nullable = false},
        >             {name = 'name', type = 'string', is_nullable = false},
        >         },
        >         primary_key = {'id'},
        >         distribution = 'global',
        >         timeout = 3,
        >     })
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Store(Other(Tarantool(TarantoolError { code: 42, message: "Read access to space '_raft_state' is denied for user 'U1'"

I think this is OK to close, not sure if we want to fix lua API though, lets discuss this on the meeting

mentioned in issue #428 (closed)

added 1 deleted label

Btw in the case of space creation failure we will call rollback two times in a row. I am also not sure if this is a valid behavior

https://git.picodata.io/picodata/tarantool/-/blob/2.11.0-picodata/src/box/txn.c#L1303-1307

rolling back a transaction when there is none is a no-op, it's safe

closed