MDEV-30908 Make SESSION_USER() comparable with CURRENT_USER()
- [x] The Jira issue number for this PR is: MDEV-30908
Description
Update SESSION_USER() behaviour to be comparable with CURRENT_USER(). SESSION_USER() will return the user and host columns from mysql.user used to authenticate the user when the session was created.
Historically SESSION_USER() was an alias of USER() function. The main difference with USER() behaviour after this change is that SESSION_USER() now returns the host column from mysql.user instead of the client host or ip.
This will allow to retrieve the user and host from the session used to call a stored procedure when SQL SECURITY DEFINER is set. With the previous behaviour the host used by the client was returned instead of the value of the host column from mysql.user table making it harder to know exactly which row from mysql.user was used to authenticate the user.
Below there is an example of the output of this function compared with the existing alternatives, if we create mysql.user_function_demo() like:
CREATE PROCEDURE mysql.user_function_demo() SQL SECURITY DEFINER BEGIN
SELECT CURRENT_USER(), USER(), SESSION_USER();
END
use the command below to connect to the server:
$ mariadb -u test_user -h 127.0.0.1 # force connection to be over TCP, not Unix socket
and then execute the procedure:
CURRENT_USER() USER() SESSION_USER()
root@localhost [email protected]:12345 test_user@%
NOTE: SESSION_USER_IS_USER old mode is added to make the change backward compatible.
How can this PR be tested?
This PR adds a new MTR test for testing this new function.
Basing the PR against the correct MariaDB version
- [X] This is a new feature and the PR is based against the latest MariaDB development branch.
PR quality check
- [X] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
- [X] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
The changes on this PR were previously discussed with @vuvova and @grooverdan at: https://github.com/MariaDB/server/pull/2928
@vuvova regarding your comment about optional parenthesis here: https://github.com/christian7007/server/commit/4c9ec8155eb9d72edc4a30560ff941d70e634eb5#r135239559.
The first version I pushed for this PR allowing option parenthesis broke main.parser test at:
https://github.com/MariaDB/server/blob/c0c1c80346b926ea1358aa512374d72d513299b0/mysql-test/main/parser.test#L116
Maybe I'm missing something, but after some research my understanding is that the failure came from supporting SESSION_USER with optional parenthesis (if we add SESSION_USER_SYM as keyword_sp_var_and_label using optional_braces it will cause ambiguity and will make the parser building to fail). This was not supported before, and as adding this will break backward compatibility I pushed a new version for the changes making parenthesis mandatory for SESSION_USER() again.
Please, let me know if you think I'm missing something or if you think there is a better way of addressing this.
I also updated some MTR tests, there are still some CI failures but after looking at them I don't think they are related with my changes.
I've marked MDEV-30908 as being IN-REVIEW, assigned to myself. But I'll only look at it next month. There's still enough time for it to get into the next release (deadline is mid-March).
We know you @vuvova are busy, but perhaps you could do a superficial review to let us know if you agree with this change in principle?
@christian7007 just a reminder. this is still waiting. 11.7 window is closing in less than three weeks.
Hey @vuvova, I just pushed a new version with the two minor changes, I'm not sure if it being a reserved word needs any other adjustment, I just updated the comment. Let me know if something else is needed.
thanks! no, it didn't need any adjustments in the code, we don't feel that we have to reserve in MariaDB every keyword that is reserved in the SQL standard. It's indeed just a comment, clarifying that the keyword is part of the SQL standard and that it's reserved there. Kind of a keyword provenance. Nothing more.
Thanks for the explanation! I can see some CI tests failing, but looking at the failures it doesn't look related with my changes and I can see the tests are also unstable in the target branch. I understand that this is ready to merge, but please let me know if you need anything else from my side.