Skip to content

*: fix permissions of brie RESTORE to be RESTORE_ADMIN#25039

Merged
ti-chi-bot merged 5 commits into
pingcap:masterfrom
morgo:fix-restore-permissions
Jun 8, 2021
Merged

*: fix permissions of brie RESTORE to be RESTORE_ADMIN#25039
ti-chi-bot merged 5 commits into
pingcap:masterfrom
morgo:fix-restore-permissions

Conversation

@morgo
Copy link
Copy Markdown
Contributor

@morgo morgo commented Jun 1, 2021

What problem does this PR solve?

Issue Number: close #24912

Problem Summary:

Previously the RESTORE command was intended to require BACKUP_ADMIN privilege, but this did not work correctly, as the internal execute commands required additional privileges (ALTER).

After discussion, RESTORE is a riskier operation than BACKUP and so it should require a distinct set of fine-grained permissions. The outcome of the discussion is that we would add the RESTORE_ADMIN dynamic privilege.

This is manually tested because it requires a tikv server to correctly reproduce the original issue.

What is changed and how it works?

What's Changed:

Executing the RESTORE or SHOW RESTORES statements now requires the RESTORE_ADMIN privilege. For backward compatibility, the SUPER privilege is also permitted, but not recommended.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn: will provide.

Check List

Tests

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

Side effects

  • None

Release note

  • Executing the RESTORE or SHOW RESTORES statements now requires the RESTORE_ADMIN privilege. For backward compatibility, the SUPER privilege is also permitted, but not recommended.

@morgo morgo requested a review from kennytm June 1, 2021 19:16
@morgo morgo requested review from a team as code owners June 1, 2021 19:16
@morgo morgo requested review from qw4990 and removed request for a team June 1, 2021 19:16
@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 1, 2021
@morgo morgo mentioned this pull request Jun 1, 2021
Merged
10 tasks
@morgo morgo force-pushed the fix-restore-permissions branch from 1855fab to 5f64c63 Compare June 1, 2021 19:25
@morgo morgo requested a review from a team June 2, 2021 14:42
kennytm
kennytm approved these changes Jun 3, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

/lgtm

@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 3, 2021

@kennytm: Please use GitHub review feature instead of /lgtm [cancel] when you want to submit review to the pull request.
For how to use GitHub review feature, see also this document provided by GitHub.

For the reason we drop support to the commands, see also this page.
This reply is being used as a temporary reply during the migration of review process and will be removed on July 1st.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 3, 2021
@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 3, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • AilinKid
  • kennytm

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 3, 2021
@AilinKid
Copy link
Copy Markdown
Contributor

AilinKid commented Jun 3, 2021

/lgtm

@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 3, 2021

@AilinKid: Please use GitHub review feature instead of /lgtm [cancel] when you want to submit review to the pull request.
For how to use GitHub review feature, see also this document provided by GitHub.

For the reason we drop support to the commands, see also this page.
This reply is being used as a temporary reply during the migration of review process and will be removed on July 1st.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 3, 2021

/merge

@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 3, 2021

@morgo: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

Details

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@morgo morgo added the sig/sql-infra SIG: SQL Infra label Jun 3, 2021
@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 3, 2021

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 3, 2021
@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 3, 2021

/merge cancel

@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jun 3, 2021
@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 3, 2021

The test failure is because SHOW PRIVILEGES now has different results. I'll fix it tomorrow.

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 3, 2021

/run-all-tests --tidb-test=pr/1205

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 3, 2021

/run-all-tests --tidb-test=pr/1205

@morgo morgo mentioned this pull request Jun 7, 2021
Open
@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 8, 2021

/merge

@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 8, 2021

This pull request has been accepted and is ready to merge.

DetailsCommit hash: e4dbf2b

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 8, 2021
@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 8, 2021

/run-common-test
/run-unit-test
/run-integration-common-test

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 8, 2021

/run-integration-br-test

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Jun 8, 2021

/run-common-test
/run-integration-br-test

@ti-chi-bot ti-chi-bot merged commit 9de1753 into pingcap:master Jun 8, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 8, 2021
@morgo @ti-srebot
cherry pick pingcap#25039 to release-5.1
381fe60 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jun 8, 2021
Merged
@ti-srebot
Copy link
Copy Markdown
Contributor

ti-srebot commented Jun 8, 2021

cherry pick to release-5.1 in PR #25227

@morgo morgo deleted the fix-restore-permissions branch June 8, 2021 02:45
ti-chi-bot pushed a commit that referenced this pull request Jun 15, 2021
@ti-srebot
*: fix permissions of brie RESTORE to be RESTORE_ADMIN (#25039) (#25227)
987ff3b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kennytm kennytm kennytm approved these changes

@AilinKid AilinKid AilinKid approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees

No one assigned

Labels

needs-cherry-pick-release-5.1 sig/execution SIG execution sig/migrate sig/sql-infra SIG: SQL Infra size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Restore database requires ALTER TABLE privilege

5 participants

@morgo @ti-chi-bot @AilinKid @ti-srebot @kennytm