Code style: please move const UUID & user_id to the next line.

yep, thanks

inconsistent indent

fixed

I'm not sure we need the column authenticated by because it's already included in the system.users table. Or it's better to name it auth_type, i.e. exactly how it's named in system.users.

done

profiles and roles can be changed by a logged user via commands SET PROFILE and SET ROLE. Maybe it's worth to log those changes too, what do you think?

well that could be a good extension to add after merging this PR

settings_profile seems to be not used

removed

Custom code in destructors rarely looks good. I think we can add a new member ext::scope_guard add_logout_event_to_session_log to the Context class, and set it in setUser() right after you add a login event to system.session_log. It allows to keep the destructor default, keep all session_log-related code in one place, and ensure that we add a logout event only if we add a login event.

unfortunately, ext::scope_guard would make Context uncopyable and I would have to craft a custom full-blown constructor.

I think it's better to move more code away from Context.cpp. We could add a function like

SessionLog::addLoginEvent(const Context & context)

which will collect all the required information from the session context, make a session log element and add it just in one function.

Done

We don't throw any exceptions not derived from std::exception, so this part seems to be unnecessary.

fixed

It's a bit more complicated. A parent profile can have its own parent profile, and so on. substituteProfiles() is a better place to collect all such parent profiles. And are you sure we need those parent profiles in system.session_log? Just a list of enabled roles isn't enough?

IIRC profiles can be assigned not only to roles, but to users too, and maybe to something else. So roles alone are definitely not enough.

There are two kinds of active roles. Current roles are the roles which are chosen to be used by user. Enabled roles includes

1. all the current roles, and
2. all the roles which are granted to the current roles, and
3. all the roles which are granted to 2, and
4. all the roles which are granted to 3 and so on.

I mean maybe we want to see the current roles in the session_log table, not the enabled ones, what do you think?

Ok, makes sense

It's maybe better to use Authentication::TypeInfo to fill those enum values. Just to repeat the names of these constants one less time.

Maybe it would be better to turn your test to an integration test. Just to affect other tests less. But I don't insist.

The function's name doesn't look good. The function doesn't terminate any session, because to terminate a session you kind of should destroy the corresponding TCPHandler. And because of you're definitely not going to do something like this here I think it's a bad name for the function. Let's rename the function to addLogOutToSessionLog() and move the condition where you check if it's a session context to the destructor.

It's more obvious to pass *this (i.e. the context) to addLogOut(), not access. And the same thing for addLogIn(). Also access (the private variable) can be null, please use the function getAccess() (which never returns null) instead.

And too many checks here, it seems the following would be enough:

auto session_log = getSessionLog();
if (session_log)
  session_log->addLogOut(*this);

Here no locks are required.

I've thought about this more. It seems after all the destructor is simply not the best place for this logic. I believe we should call this function from handlers, i.e. from TCPHandler, HTTPHandler and so on. The reasons:

1. Handlers provide more flexibility for the moment when we adds a logout event (for example we can do what we want with named sessions in HTTPHandler if we are not bound to the destructor);
2. Destructors look better without any logic except freeing the resources;

Moved out all session-related code to a separate handle-like class: Session. So the log-out event is now saved in Session's destructor.

I like your idea with the new Session class, it's nice to move some session-related logic outside of the quite overcomplicated Context class.

Let's name those profiles current, not active.
Likewise how roles are named: we have current roles and enabled roles. So we would have current profiles, and enabled profiles (enabled profiles would include current profiles, plus their parents, plus parents of those parents, and so on). Well you don't need enabled profiles for your PR but I think we still should name those profiles as current for consistency.

SettingsProfileCache keeps a map named all_profiles, which can be used to get all those names. Searching in the map is faster than the call manager->readName().

You can easily implement this by adding a new member variable of type const SettingsProfileCache & to EnabledSettings and also the function EnabledSettings::getCurrentProfileNames().

done, in a bit different way

Let's insert one more And to make the name like setSettingsAndConstraintsAndProfiles(). Because settings and settings constraints are different things. Or I think we can call it simply set() because there is no other set function in this class.

done

= default looks a bit better

Please don't forget to remove the dead code like this.

AppliedProfiles -> CurrentProfiles ?

Sure

applied_profiles -> current_profiles ?

yep

The function should not return const std::vector<UUID> &. It should return either std::shared_ptr<const std::vector<UUID>> or std::vector<UUID> instead. Because if it returns const std::vector<UUID> & it can be read outside the internal lock and be modified in setSettingsAndConstraintsAndProfiles() at the same time.

Done

It seems there is no point for this function to handle multiple profiles at once. This logic can be implemented in the caller and so it would allow make this function more simple. I mean this function can be declared instead as std::optional<String> getProfileName(const UUID & profile_id) const and the caller will iterate through IDs if it wants.

Ok, fixed

Who owns this session?

Somebody else, this is non-owning pointer. I had to poke this "hole" to remove MySQL-specific struct from context. So the MySQL-stream can access the sequence_id from MySQLSession (which was previously in Context::mysql)

Added a comment to clarify it

What is context_to_copy? Is this the global context?

That depends, usually it is a server context or something similar.

Why does the function getUserAuthentication() exist? I think we already have the function setUser() for checking password.

There are some complex authentication scenarios, e.g. in PostgreSql or MySQL handlers:

So now we have two separate classes: Session and NamedSession which look completely different. It doesn't look good to me. Can we somehow combine them? For example we could add all the fields from NamedSession into Session and make the class NamedSessions to keep Sessions instead of NamedSessions.

Removed NamedSession from Context.cpp, and now we have a NamedSessionData in the Session.cpp.