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

Ldap unboundid #12

Merged
merged 6 commits into from Apr 24, 2012

Conversation

Projects
None yet
3 participants
@jcrygier
Copy link
Contributor

jcrygier commented Apr 24, 2012

Replacement for Pull Request #11. I created a new branch as to not interfere with work done on master, and this causes a new Pull Request.

This branch is a fresh implementation of LDAP using the UnboundID library. Also, there are far fewer options, as this implementation is limited to authentication / team membership information stored in LDAP.

@gitblit

This comment has been minimized.

Copy link

gitblit commented on src/com/gitblit/GitBlitServer.java in f3b625d Apr 10, 2012

We'll probably get into trouble when testing on linux/unix boxes as non-admins because 389 < 1024. We might consider making that a higher port or making it configurable too.

This comment has been minimized.

Copy link
Owner Author

jcrygier replied Apr 10, 2012

Shouldn't be a problem....I was going to do that originally, but didn't think this would be a widely used feature. I'll crank that out this afternoon.

@gitblit

This comment has been minimized.

Don't we want to rip out this whole condition? Isn't the end result here whatever is in model?

This comment has been minimized.

Copy link
Owner Author

jcrygier replied Apr 10, 2012

I thought the UserModel was coming from the UI. So this ensures that changing the teams isn't hackable, since it will read before it updates. In all reality, this is a case that should NEVER happen, so it is possible to rip it out. Whichever way you prefer is fine with me.

This comment has been minimized.

Copy link

gitblit replied Apr 10, 2012

Oops. Great point. It can also come from an RPC so we definitely have to preserve this... and same for teams. ;)

@gitblit

This comment has been minimized.

Same here?

@gitblit

This comment has been minimized.

Copy link

gitblit commented on src/com/gitblit/LdapUserService.java in f3b625d Apr 10, 2012

Not too far down the road I'll want to pull display name and e-mail address. We could do that for 1.0.0, not sure if I'll be ready to use them by then. But we'll have them.

This comment has been minimized.

Copy link

mohamedmansour replied Apr 17, 2012

I am not in favour of querying the "cn" here. The User Model requires a username, when querying a CN, you are getting the "Full Name"

[user "mohamed mansour"]
    password = StoredInLDAP

It would be best to make it so we could have unique names:

 UserModel answer = new UserModel(userEntry.getAttributeValue("sAMAccountName"));

Will result in:

[user "mmansour"]
    password = StoredInLDAP

We could add fullName etc too from the CN.

This comment has been minimized.

Copy link

gitblit replied Apr 18, 2012

Hmm. Agreed. We definitely want the account name and not the display name.

This comment has been minimized.

Copy link
Owner Author

jcrygier replied Apr 18, 2012

Yes I agree. With all the sample data I've seen from my corporate LDAP, the CN is the login ID (and i believe that's quite standard). But it should be simple enough to grab the Attribute Name from a NEW property (as we can't hardcode it to sAMAccountName - as that's an Active Directory thing, I believe).

As for getting the full name, that will again have to be another property. However, GitBlit doesn't support Full Names quite yet (but James has indicated that it's likely on it's way).

I'll code that up shortly (when I have some free time later in the week).

This comment has been minimized.

Copy link

mohamedmansour replied Apr 18, 2012

Hmm, the cn is usually the common name which from my last two companies it is always my Full Name.It is not guaranteed to be unique too for the entire company if they use multiple organizations. The sAMAccountName for any object must be unique in the domain. The cn must only be unique in the container or organization. So if the company has multiple organizations, then you will have collisions.

At RIM, our CN is always our Full Name, if there are people with the same name, the CN changes. Perhaps the Active Directory team is doing all the LDAP stuff wrong, but instead of requerying the username, why wont we just pass in the simpleUsername to this method as the first parameter.

if (user == null)   // create user object for new authenticated user
  user = createUserFromLdap(simpleUsername, loggingInUser);

private UserModel createUserFromLdap(String username, SearchResultEntry userEntry) {
    UserModel answer = new UserModel(username);
    //If attributes other than user name ever from from LDAP, this is where to get them

    return answer;
}

That would fix my issue in this.

This comment has been minimized.

Copy link
Owner Author

jcrygier replied Apr 18, 2012

Yep, I like using the simpleUserName, as that's what they typed in at login. Also, it gives the opportunity to use the realm.ldap.accountPattern as the setting to control what the userName is (since in my example above, it's setting it to sAMAccountName - but it could be whatever they use {userName} with.

Thanks for the feedback, I'll get this committed ASAP.

@mohamedmansour

This comment has been minimized.

Copy link

mohamedmansour commented on distrib/gitblit.properties in f3b625d Apr 17, 2012

Many LDAP groups (windows) have spaces in them. According to the grammar of the valid distinguished names, it is described here, http://www.ietf.org/rfc/rfc2253.txt

How do you think we should treat it? We could always treat quotes as single entries and parse the DN from them.

You could change your split string in LdapUserService setAdminAttribute to

String[] admins = adminString.split(" (?=([^\"]*\"[^\"]*\")*[^\"]*$)");

Then we could parse out the quotes. Something like this:

private void setAdminAttribute(UserModel user) {
    String adminString = settings.getString(Keys.realm.ldap_admins, "");
    String[] admins = adminString.split(" (?=([^\"]*\"[^\"]*\")*[^\"]*$)");
    user.canAdmin = false;
    for (String admin : admins) {
        if (admin.charAt(0) == '"' && admin.charAt(admin.length() - 1) == '"') {
            admin = admin.substring(1, admin.length() - 1);
        }
        if (admin.startsWith("@")) { // Team
            if (user.getTeam(admin.substring(1)) != null)
                user.canAdmin = true;
        } else
            if (user.getName().equalsIgnoreCase(admin))
                user.canAdmin = true;
    }
}

This comment has been minimized.

Copy link

gitblit replied Apr 23, 2012

This is a good point. I would implement your idea in the existing StringUtils functions like so:
com.gitblit.utils.StringUtils

public static List<String> getStringsFromValue(String value, String separator) {
        List<String> strings = new ArrayList<String>();
        try {
            String[] chunks = value.split(separator + "(?=([^\"]*\"[^\"]*\")*[^\"]*$)");            
            for (String chunk : chunks) {
                chunk = chunk.trim();
                if (chunk.length() > 0) {
                    if (chunk.charAt(0) == '"' && chunk.charAt(chunk.length() - 1) == '"') {
                        // strip double quotes
                        chunk = chunk.substring(1, chunk.length() - 1).trim();
                    }
                    strings.add(chunk);
                }
            }
        } catch (PatternSyntaxException e) {
            throw new RuntimeException(e);
        }
        return strings;
    }

then I would modify the unit test as follows:
com.gitblit.tests.StringUtilsTest

    @Test
    public void testStringsFromValue() throws Exception {
        List<String> strings = StringUtils.getStringsFromValue("\"A A \" B \"C C\" D \"\" \"E\"");
        assertEquals(6, strings.size());
        assertEquals("A A", strings.get(0));
        assertEquals("B", strings.get(1));
        assertEquals("C C", strings.get(2));
        assertEquals("D", strings.get(3));
        assertEquals("", strings.get(4));
        assertEquals("E", strings.get(5));

        strings = StringUtils.getStringsFromValue("\"A A \", B, \"C C\", D, \"\", \"E\"", ",");
        assertEquals(6, strings.size());
        assertEquals("A A", strings.get(0));
        assertEquals("B", strings.get(1));
        assertEquals("C C", strings.get(2));
        assertEquals("D", strings.get(3));
        assertEquals("", strings.get(4));
        assertEquals("E", strings.get(5));
    }

and finally LdapUserService:

private void setAdminAttribute(UserModel user) {
    user.canAdmin = false;
    List<String>  admins = settings.getStrings(Keys.realm.ldap_admins);
    for (String admin : admins) {
        if (admin.startsWith("@")) { // Team
            if (user.getTeam(admin.substring(1)) != null)
                user.canAdmin = true;
        } else
            if (user.getName().equalsIgnoreCase(admin))
                user.canAdmin = true;
    }
}

This comment has been minimized.

Copy link

mohamedmansour replied Apr 23, 2012

Reusability for the win :) I like it! lgtm

This comment has been minimized.

Copy link
Owner Author

jcrygier replied Apr 23, 2012

I like it. I'll work on getting this in ASAP.

gitblit added a commit that referenced this pull request Apr 24, 2012

@gitblit gitblit merged commit 0cb7a9c into gitblit:master Apr 24, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment