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

fix mo_ctl #13926

Merged
merged 2 commits into from
Dec 31, 2023
Merged

fix mo_ctl #13926

merged 2 commits into from
Dec 31, 2023

Conversation

daviszhen
Copy link
Contributor

@daviszhen daviszhen commented Dec 29, 2023

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #https://github.com/matrixorigin/MO-Cloud/issues/2162

What this PR does / why we need it:

restrict mo_ctl privilege.
only can be executed by sys moadmin role.

@mergify mergify bot requested a review from sukki37 December 29, 2023 08:35
@mergify mergify bot added the kind/bug Something isn't working label Dec 29, 2023
@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Dec 29, 2023
@matrix-meow
Copy link
Contributor

@daviszhen Thanks for your contributions!

Review:

Title: fix mo_ctl

Body: The pull request fixes the mo_ctl privilege by restricting it to only be executed by the sys moadmin role.

Changes:

  1. In authenticate.go, a new function hasMoCtrl is added to check if the plan has mo_ctrl. If it does, it verifies if the account has the privilege to execute mo_ctrl. If not, it returns an error.
  2. In authenticate2.go, the hasMoCtrl function is implemented to check if the expression has mo_ctrl.
  3. In authenticate2_test.go, tests are added for the new functions verifyAccountCanExecMoCtrl and hasMoCtrl.
  4. In utils.go, a new function HasMoCtrl is added to check if the expression has mo_ctrl.

Overall, the changes made in this pull request address the issue of restricting the mo_ctl privilege to only be executed by the sys moadmin role. The new functions and tests ensure that the privilege is properly enforced.

However, there are a few suggestions for optimization and improvements:

  1. In authenticate.go, the hasMoCtrl function can be simplified by removing the unnecessary check for p != nil && p.GetQuery() != nil. Since p is already passed as a parameter, it is guaranteed to be non-nil.
  2. In authenticate2.go, the import statements can be organized to improve readability. The plan import can be removed as it is not used in this file.
  3. In authenticate2_test.go, the import statements can also be organized for better readability.
  4. In utils.go, the HasMoCtrl function can be simplified by removing the unnecessary default case in the switch statement. Since HasMoCtrl is only called with plan.Expr as the argument, there is no need to handle other types.

These optimizations and improvements will make the code cleaner and more efficient.

@mergify mergify bot merged commit 42254a7 into matrixorigin:1.1-dev Dec 31, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants