Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expression: MySQL compatible current_user function #7801

Merged
merged 8 commits into from
Oct 16, 2018
Merged

expression: MySQL compatible current_user function #7801

merged 8 commits into from
Oct 16, 2018

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Sep 27, 2018

What problem does this PR solve?

Currently current_user() returns the same as user(), which is incorrect.
Fixes #7613

What is changed and how it works?

Added support for a 'matcheduser' and 'matchedhost'. This will also be required for statements like SHOW GRANTS (without FOR USER option). See #7614

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change

Side effects

  • Increased code complexity

Related changes

  • Need to update the documentation

@morgo
Copy link
Contributor Author

morgo commented Sep 27, 2018

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Sep 27, 2018

I started looking at fixing SHOW GRANTS. It is relatively straightforward, but one question I have for reviewers is if SessionVars.User should contain the matched user instead, and have new attributes for the actual user+host.

This would make SHOW GRANTS work without modification. For example see how SHOW GRANTS needs a hack to convert the actual user to the matched user. I suspect there may be other commands that make this mistake:

morgo@ryzen:~/go/src/github.com/pingcap/tidb$ git diff
diff --git a/executor/builder.go b/executor/builder.go
index 1f9ef0e0c..51d9d4c44 100644
--- a/executor/builder.go
+++ b/executor/builder.go
@@ -42,6 +42,7 @@ import (
        "github.com/pingcap/tidb/table"
        "github.com/pingcap/tidb/types"
        "github.com/pingcap/tidb/util/admin"
+       "github.com/pingcap/tidb/util/auth"
        "github.com/pingcap/tidb/util/chunk"
        "github.com/pingcap/tidb/util/ranger"
        "github.com/pingcap/tidb/util/timeutil"
@@ -484,7 +485,10 @@ func (b *executorBuilder) buildShow(v *plannercore.Show) Executor {
                is:           b.is,
        }
        if e.Tp == ast.ShowGrants && e.User == nil {
-               e.User = e.ctx.GetSessionVars().User
+               e.User = &auth.UserIdentity{
+                       Username:        e.ctx.GetSessionVars().User.MatchedUsername,
+                       Hostname:        e.ctx.GetSessionVars().User.MatchedHostname,
+               }
        }
        if len(v.Conditions) == 0 {
                return e
diff --git a/planner/core/planbuilder.go b/planner/core/planbuilder.go
index 33c48c11a..f15890462 100644
--- a/planner/core/planbuilder.go
+++ b/planner/core/planbuilder.go
@@ -1576,7 +1576,15 @@ func buildShowSchema(s *ast.ShowStmt) (schema *expression.Schema) {
        case ast.ShowCreateDatabase:
                names = []string{"Database", "Create Database"}
        case ast.ShowGrants:
-               names = []string{fmt.Sprintf("Grants for %s", s.User)}
+
+               if s.User != nil {
+                       names = []string{fmt.Sprintf("Grants for %s", s.User)}
+               } else {
+                       // Default to the session user
+                       // Can't grab the name from here.
+                       names = []string{fmt.Sprintf("Grants for User")}
+               }
+
        case ast.ShowIndex:
                names = []string{"Table", "Non_unique", "Key_name", "Seq_in_index",
                        "Column_name", "Collation", "Cardinality", "Sub_part", "Packed",

I have not tested how much breaks yet by setting SessionVars.User to the matched user. Looking for reviewer feedback :-) Thx.

@winoros
Copy link
Member

winoros commented Sep 29, 2018

Package Privilege's maintainer is on vacation. There may not be quick response. :(

@morgo
Copy link
Contributor Author

morgo commented Oct 2, 2018

No problems :-)

I noticed one counterpoint today. In show processlist you want the actual identity rather than the matched identity, so this would create a bug if the meaning of SessionVars.User was changed.

https://github.com/pingcap/tidb/blob/master/session/session.go#L715-L718

Copy link
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think add the matched user & host fields is okay.
Well done! @morgo

@@ -40,6 +40,10 @@ type Manager interface {
// ConnectionVerification verifies user privilege for connection.
ConnectionVerification(user, host string, auth, salt []byte) bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about change this method signature to

// ConnectionVerification verifies user privilege for connection.
// Returns the actual user+host from the privileges table, including any wildcard characters.
ConnectionVerification(user, host string, auth, salt []byte) (string, string, bool)

Then we can remove the ConnectionMatchIdentity method ?

@@ -1000,16 +1000,20 @@ func (s *session) Auth(user *auth.UserIdentity, authentication []byte, salt []by

// Check IP.
if pm.ConnectionVerification(user.Username, user.Hostname, authentication, salt) {
user.MatchedUsername, user.MatchedHostname = pm.ConnectionMatchIdentity(user.Username, user.Hostname)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be done once if ConnectionVerification returns the matched user & host.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I thought about this some more, and I think we should call it AuthUsername + AuthHostname, since it describes which user/hostname matched for auth purposes. Would you agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree!

@morgo
Copy link
Contributor Author

morgo commented Oct 14, 2018

I've made the suggested changes, and renamed matched* to auth*. It looks cleaner - thank you for the feedback.

Should the authUser and AuthHost also be part of the UserPrivileges struct?

@tiancaiamao
Copy link
Contributor

LGTM
/run-all-tests

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 15, 2018
@tiancaiamao
Copy link
Contributor

/run-all-tests

@tiancaiamao
Copy link
Contributor

/run-unit-test

@tiancaiamao
Copy link
Contributor

PTAL @lysu

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lysu lysu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 16, 2018
@lysu lysu merged commit 19e4e2f into pingcap:master Oct 16, 2018
@morgo morgo deleted the current-user branch October 22, 2018 13:43
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Oct 25, 2018
bugfix fixed pingcap#7518

expression: MySQL compatible current_user function (pingcap#7801)

plan: propagate constant over outer join (pingcap#7794)

- extract `outerCol = const` from join conditions and filter conditions,
  substitute `outerCol` in join conditions with `const`;
- extract `outerCol = innerCol` from join conditions, derive new join
  conditions based on this column equal condition and `outerCol` related
  expressions in join conditions and filter conditions;

util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901)

stats: handle ddl event for partition table (pingcap#7903)

plan: implement Operand and Pattern of cascades planner. (pingcap#7910)

planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808)

plan: move projEliminate behind aggEliminate (pingcap#7909)

admin: fix admin check table bug of byte compare (pingcap#7887)

* admin: remove reflect deepEqual

stats: fix panic caused by empty histogram (pingcap#7912)

plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906)

* fix drop view if exist error (pingcap#7833)

executor: refine `explain analyze` (pingcap#7888)

executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863)

expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915)

stats: fix histogram boundaries overflow error (pingcap#7883)

ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771)

* ddl:support the definition of null change to not null using alter table

ddl: add check when create table with foreign key. (pingcap#7885)

* ddl: add check when create table with foreign key

planner: eliminate if null on non null column (pingcap#7924)

executor: fix a bug in point get (pingcap#7934)

planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930)

executor: fix panic when limit is too large (pingcap#7936)

ddl: add TiDB version to metrics (pingcap#7902)

stats: limit the length of sample values (pingcap#7931)

vendor: update tipb (pingcap#7893)

planner: support the Group and GroupExpr for the cascades planner (pingcap#7917)

store/tikv: log more information when other err occurs (pingcap#7948)

types: fix date time parse (pingcap#7933)

ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875)

stats: update delta info for partition table (pingcap#7947)

explaintest: add explain test for partition pruning (pingcap#7505)

util: move disjoint set to util package (pingcap#7950)

util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916)

executor: add the slow log for commit (pingcap#7951)

expression: add builtin json_keys (pingcap#7776)

privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955)

ddl: fix invailid ddl job panic (pingcap#7940)

*: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952)

Make the ast package get rid of the dependency of types.Datum

server: allow cors http request (pingcap#7939)

*: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970)

pr suggestion update

executor/aggfuncs: split unit tests to corresponding file (pingcap#7993)

store/tikv: fix typo (pingcap#7990)

executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999)

executor: let information_schema be the first database in ShowDatabases (pingcap#7938)

stats: use local feedback for partition table (pingcap#7963)

executor: add unit test for aggfuncs (pingcap#7966)

server: add log for binary execute statement (pingcap#7987)

admin: refine admin check decoder (pingcap#7862)

executor: improve wide table insert & update performance (pingcap#7935)

ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919)

fix reassigned partition id in truncate table does not take effect

add changelog for 2.1.0 rc4 (pingcap#8020)

*: make parser package dependency as small as possible (pingcap#7989)

parser: support `:=` in the `set` syntax (pingcap#8018)

According to MySQL document, `set` use the = assignment operator,
but the := assignment operator is also permitted

stats: garbage collect stats for partition table (pingcap#7962)

docs: add the proposal for the column pool (pingcap#7988)

expression: refine built-in func truncate to support uint arg (pingcap#8000)

stats: support show stats for partition table (pingcap#8023)

stats: update error rate for partition table (pingcap#8022)

stats: fix estimation for out of range point queries (pingcap#8015)

*: move parser to a separate repository (pingcap#8036)

executor: fix wrong result when index join on union scan. (pingcap#8031)

Do not modify Plan of dataReaderBuilder directly, because it would
impact next batch of outer rows, as well as other concurrent inner
workers. Instead, build a local child builder to store the child plan.

planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017)

*: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044)

* ddl, executor: fix the issue of executing DDL after executing SQL failure in txn

add unit test

remove debug info

add like evaluator case sensitive test

ddl, domain: make schema correct after canceling jobs (pingcap#7997)

unit test fix

code format

proposal: maintaining histograms in plan. (pingcap#7605)

support _tidb_rowid for table scan range (pingcap#8047)

var rename fix
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

current_user() function returns incorrect results
6 participants