server icon indicating copy to clipboard operation
server copied to clipboard

MDEV-30908 Make SESSION_USER() comparable with CURRENT_USER()

Open christian7007 opened this issue 1 year ago • 6 comments

  • [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.

christian7007 avatar Jan 08 '24 18:01 christian7007

CLA assistant check
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.

CLAassistant avatar Jan 08 '24 18:01 CLAassistant

The changes on this PR were previously discussed with @vuvova and @grooverdan at: https://github.com/MariaDB/server/pull/2928

christian7007 avatar Jan 08 '24 18:01 christian7007

@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.

christian7007 avatar Jan 10 '24 01:01 christian7007

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.

christian7007 avatar Jan 10 '24 01:01 christian7007

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).

vuvova avatar Jan 12 '24 21:01 vuvova

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?

ottok avatar Apr 07 '24 04:04 ottok

@christian7007 just a reminder. this is still waiting. 11.7 window is closing in less than three weeks.

vuvova avatar Aug 22 '24 21:08 vuvova

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.

christian7007 avatar Aug 23 '24 20:08 christian7007

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.

vuvova avatar Aug 23 '24 20:08 vuvova

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.

christian7007 avatar Aug 23 '24 21:08 christian7007