Skip to content
New issue

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

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

Already on GitHub? # to your account

[Feature] Support LDAP Group Provider #56670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HangyuanLiu
Copy link
Contributor

@HangyuanLiu HangyuanLiu commented Mar 6, 2025

Why I'm doing:

What I'm doing:

Fixes #56669

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.4
    • 3.3
    • 3.2
    • 3.1
    • 3.0

@HangyuanLiu HangyuanLiu requested review from a team as code owners March 6, 2025 09:23
}
}
}
}
Copy link

Choose a reason for hiding this comment

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

The most risky bug in this code is:
Improper handling of potential null values for critical methods, specifically in the getLdapGroupDn() method being able to return null, which can lead to a NullPointerException.

You can modify the code like this:

public List<String> getLdapGroupDn() {
    if (properties.get(LDAP_GROUP_DN) == null) {
        // Return an empty list instead of null to avoid NullPointerException
        return List.of();
    } else {
        return List.of(properties.get(LDAP_GROUP_DN).split(";\\s*"));
    }
}

Copy link

sonarqubecloud bot commented Mar 6, 2025

Copy link

github-actions bot commented Mar 6, 2025

[Java-Extensions Incremental Coverage Report]

pass : 0 / 0 (0%)

Copy link

github-actions bot commented Mar 6, 2025

[FE Incremental Coverage Report]

fail : 3 / 119 (02.52%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/authentication/LDAPGroupProvider.java 0 114 00.00% [44, 78, 85, 86, 90, 92, 93, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 111, 113, 115, 116, 118, 124, 125, 126, 127, 129, 138, 140, 141, 142, 143, 144, 145, 146, 149, 150, 151, 152, 153, 154, 155, 159, 162, 163, 164, 165, 167, 171, 172, 175, 180, 181, 182, 185, 186, 187, 188, 189, 190, 192, 194, 199, 200, 201, 203, 205, 207, 210, 211, 212, 214, 217, 218, 219, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 233, 237, 241, 245, 249, 253, 257, 261, 265, 266, 268, 273, 277, 281, 286, 287, 289, 290, 291, 295, 296, 297, 298, 300]
🔵 com/starrocks/authentication/GroupProviderFactory.java 1 3 33.33% [46, 47]
🔵 com/starrocks/persist/gson/GsonUtils.java 2 2 100.00% []

Copy link

github-actions bot commented Mar 6, 2025

[BE Incremental Coverage Report]

pass : 0 / 0 (0%)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Group Provider
1 participant