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

planner, privilege: check user priv on SET GLOBAL #8837

Merged
merged 7 commits into from Jan 5, 2019

Conversation

Projects
None yet
6 participants
@morgo
Copy link
Member

morgo commented Dec 26, 2018

What problem does this PR solve?

Fixes #7403

What is changed and how it works?

Checks that SET GLOBAL requires the super privilege. Does not check any session level vars - I took a look, and I don't believe any in TiDB require privilege check.

(In MySQL commands like SET SESSION sql_log_bin=0 require super privs, but I believe this is not applicable to TiDB).

It is not 100% compatible with MySQL, since the error codes/messages differ, but this is not a new issue, and is inherint to all checking in visitInfo. I will create a separate issue to discuss how to enhance it, since I think it should have an argument to pass the error message on failure.

Check List

Tests

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

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity
  • Breaking backward compatibility (broken apps)

Related changes

  • Need to be included in the release note

This change is Reviewable

@morgo morgo requested a review from tiancaiamao Dec 26, 2018

@morgo

This comment has been minimized.

Copy link
Member

morgo commented Dec 26, 2018

PTAL @tiancaiamao , thx!

@morgo morgo added the type/bug-fix label Dec 26, 2018

@morgo

This comment has been minimized.

Copy link
Member

morgo commented Dec 26, 2018

/run-all-tests

morgo added some commits Dec 28, 2018

@tiancaiamao
Copy link
Contributor

tiancaiamao left a comment

LGTM

@lamxTyler
Copy link
Member

lamxTyler left a comment

LGTM

@lamxTyler lamxTyler added status/LGT2 and removed status/LGT1 labels Jan 4, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@cfff965). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8837   +/-   ##
=========================================
  Coverage          ?   67.54%           
=========================================
  Files             ?      363           
  Lines             ?    75101           
  Branches          ?        0           
=========================================
  Hits              ?    50725           
  Misses            ?    19903           
  Partials          ?     4473
Impacted Files Coverage Δ
planner/core/planbuilder.go 49.07% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfff965...ba2bada. Read the comment docs.

@ngaut

ngaut approved these changes Jan 5, 2019

@morgo morgo merged commit 081a2c5 into pingcap:master Jan 5, 2019

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@morgo morgo deleted the morgo:set-permission-check branch Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment