Skip to content

Conversation

ouyuanning
Copy link
Contributor

@ouyuanning ouyuanning commented Jun 5, 2025

User description

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #21920

What this PR does / why we need it:

fix bug: user can create user without permission


PR Type

Bug fix, Tests


Description

  • Fix privilege check to prevent unauthorized user creation

  • Update tests to verify user creation privilege enforcement

  • Add scenarios for account and role-based user creation


Changes walkthrough 📝

Relevant files
Bug fix
authenticate2.go
Enforce privilege check for user creation                               

pkg/frontend/authenticate2.go

  • Explicitly set yes = false for
    PrivilegeTypeCanGrantRoleToOthersInCreateUser
  • Ensures users without proper privilege cannot create users
  • Minor formatting/comment updates in privilege checking logic
  • +21/-20 
    Tests
    create_user_default_role.result
    Update test results for user creation privilege checks     

    test/distributed/cases/tenant/privilege/create_user_default_role.result

  • Add test results for new account and user creation scenarios
  • Show expected internal error when lacking privilege
  • Reflects stricter privilege enforcement in test outputs
  • +10/-0   
    create_user_default_role.sql
    Add and update SQL tests for user creation privilege         

    test/distributed/cases/tenant/privilege/create_user_default_role.sql

  • Add SQL tests for account, role, and user creation flows
  • Test user creation with and without proper privileges
  • Include session changes to simulate different user contexts
  • +13/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    qodo-merge-pro bot commented Jun 5, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Privilege Enforcement

    The PR sets yes = false for the PrivilegeTypeCanGrantRoleToOthersInCreateUser privilege check, but the commented-out code suggests there might have been a more nuanced implementation planned. Verify this is the intended behavior rather than a temporary fix.

    	yes = false
    } else {

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 5, 2025
    @mergify mergify bot added the kind/bug Something isn't working label Jun 5, 2025
    Copy link

    qodo-merge-pro bot commented Jun 5, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @mergify mergify bot merged commit 4867f5b into matrixorigin:main Jun 5, 2025
    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 Review effort 2/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants