[tarantool-patches] Re: [PATCH v2 1/2] access: rework struct credentials API

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 05/10/2019 13:42, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [19/10/05 13:47]:
>> Struct credentials is a cache of user's universal privileges. It
>> is static and is never changed after creation. That is a problem.
>> If a user privileges are updated, it is not reflected in his
>> existing credentials caches.
>>
>> This patch reworks credentials API so as now this struct is not
>> just a container for several numbers. It is an object with
>> standard methods like create(), destroy(). A credentials object
>> still is not updated together with its source user, but now at
>> least the API allows to fix that.
>>
>> Next patch will add a trigger to struct credentials to catch user
>> privilege changes and update the cache.
> 
> I see nothing wrong with this approach.
> 
> Re alternatives:
> 
> Re v1, there are good reasons to link all users session into a
> linked list - e.g. for instrumentation, to collect per-user
> statistics, to disconnect all sessions of a user, to set per-user
> quotas, etc, etc.

Yes, but all the same can be done with triggers.

> 
> You're using triggers, not linked lists - which gives some
> encapsulation indeed, but this will not allow to reuse this linked
> list for other purposes.

Why? Via a trigger you can spread any information. Void *event can
be anything. In my V1 it is struct user, but it can be easily changed.
For disconnect I was planning to pass NULL user.

> 
> Your patch also works with setuid credentials, of course, which
> is a benefit. But setuid credentials could be fixed differently,
> by linking all setuid functions of a user into a list  and
> updating the list when user access changes.
> 

My V2 patch works not only for sessions and functions. But for box.session.su
as well. This is an exceptional case. There is no a session nor a function.
Instead, struct credentials is stored right on the stack. With your approach
about lists we would need to add a new list each time we have a new object
having struct credentials. Now it is going to be 3 lists already: for
sessions, for setuid functions, for currently running box.session.su's.

I don't think setuid and session.su will ever need anything from user except
privs update, so it makes no sense to keep their lists. What is worse, it is
not just a break of encapsulation - it is cyclic dependency. With my patch
I managed to make user independent from session. With your approach user will
depend on sessions, functions, and vice versa. It is especially strange to see
such a proposal in scope of what you are writing in JSON updates review.

But I agree, that session list could be useful. Trigger-way would look to
complex when we will start fixing 4535, 4536 (about disconnects, creds
invalidation, etc). I propose this:

- Keep a list of all struct credentials in struct user. For privs update only.
  Here we register all credentials from sessions, functions, box.session.su's.

- In scope of 4535 and 4536 I will add a list of sessions. That list will
  receive complex updates such as user drop, manual connections close, statistics,
  etc.

For 2763 I propose to keep the solution simple. I tried to add session list
here, but it appeared to be unused. I've refactored my patchset and replaced
triggers with credentials list.

New commit message for the first commit (code is the same):

    access: rework struct credentials API
    
    Struct credentials is a cache of user's universal privileges. It
    is static and is never changed after creation. That is a problem.
    If a user privileges are updated, it is not reflected in his
    existing credentials caches.
    
    This patch reworks credentials API so as now this struct is not
    just a container for several numbers. It is an object with
    standard methods like create(), destroy(). A credentials object
    still is not updated together with its source user, but now at
    least the API allows to fix that.
    
    Next patch will link all struct credentials of a user into a list
    via which the user will be able to keep the credentials up to
    date.
    
    Part of #2763