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

KEYCLOAK-19743 Fix null username in ldap #8717

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

mrk-andreev
Copy link
Contributor

@mrk-andreev mrk-andreev commented Nov 7, 2021

Fix MembershipType.getGroupMembers when username is null (invalid configuration).

Context: method MembershipType.getGroupMembers return list of usernames. Username can be fetched by:

  • userDn.getFirstRdn().getAttrValue(ldapConfig.getRdnLdapAttribute())
  • LDAPUtils.getUsername(ldapUser, ldapConfig)

Inside LDAPUtils.getUsername(ldapUser, ldapConfig) null check is look like:

public static String getUsername(LDAPObject ldapUser, LDAPConfig config) {
        String usernameAttr = config.getUsernameLdapAttribute();
        String ldapUsername = ldapUser.getAttributeAsString(usernameAttr);

        if (ldapUsername == null) {
            throw new ModelException("User returned from LDAP has null username! Check configuration of your LDAP mappings. Mapped username LDAP attribute: " +
                    config.getUsernameLdapAttribute() + ", user DN: " + ldapUser.getDn() + ", attributes from LDAP: " + ldapUser.getAttributes());
        }

        return ldapUsername;
    }

But in case userDn.getFirstRdn().getAttrValue(ldapConfig.getRdnLdapAttribute()) there are no null verification.

So MembershipType.getGroupMembers get return list with nulls. This behavior breaks UserCacheSession:getUserByUsername when we transform username to lowercase

@Override
public UserModel getUserByUsername(RealmModel realm, String username) {
        logger.tracev("getUserByUsername: {0}", username);
        username = username.toLowerCase();

I suggest to throw exception ModelException like LDAPUtils.getUsername do.

Issue: https://issues.redhat.com/browse/KEYCLOAK-19743

@mposolda
Copy link
Contributor

@mrk-andreev Sorry for late response. The PR looks good, but are you please able to rebase this PR on top of latest Keycloak main?

@mposolda
Copy link
Contributor

@mrk-andreev Sorry, I've merged the latest Keycloak main to your branch. So no rebase needed. Nothing needed for you now.

@mposolda mposolda merged commit 581def5 into keycloak:main Sep 30, 2022
@mposolda mposolda linked an issue Sep 30, 2022 that may be closed by this pull request
@mabartos mabartos mentioned this pull request Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ldap membership getGroupMembers when username not found
2 participants