feature: get all users #9
Conversation
| @@ -13,6 +13,7 @@ import ( | |||
| // Client interface performs ldap auth operation | |||
| type Client interface { | |||
| Auth(username, password string) error | |||
| Users() (error, []string) | |||
There was a problem hiding this comment.
in go, we want to always return errors as the last value in multiple return value, so this guy should return ([]string, error)
| return err, users | ||
| } | ||
| defer conn.Close() | ||
|
|
There was a problem hiding this comment.
before doing the search, you need to do an initial bind with the read-only user
// perform initial read-only bind
if err = conn.Bind(c.ROUser.Name, c.ROUser.Pass); err != nil {
return err
}
(see Auth function)
| searchRequest := ldap.NewSearchRequest( | ||
| c.BaseDN, // The base dn to search | ||
| ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, | ||
| "(&(objectClass=organizationalPerson))", // The filter to apply |
There was a problem hiding this comment.
one thing i worry about here: is it always the case (or close enough to always) for us to assume that this is going to get all the users? part of me thinks it would be best to use the config Filter value, but instead of matching it against username like we do in auth, we just use a wildcard * and get all the users that could auth with a username via our Auth function.
I think that would keep things consistent. so instead of objectClass=organizationalPerson, let's just do what we have in Auth except something like this: fmt.Sprintf("(%v=%v)", c.Filter, "*" )
| c.BaseDN, // The base dn to search | ||
| ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, | ||
| "(&(objectClass=organizationalPerson))", // The filter to apply | ||
| []string{"dn", "cn"}, // A list attributes to retrieve |
There was a problem hiding this comment.
if all we want is the user's cn, let's just get this and not dn.
i think this is definitely a good start, and i wanna merge it after a little touching up (see other comments), BUT, i think we might want to think about adding a configuration field that is a string[] of "UserFields" or some such.
Then, we could use that to populate a different kind of User struct or perhaps have a user be a map[string]string with the fields provided from config as keys and populate the map with the values we get back.
No need to do this now though, just returning cns works. 👍
| @@ -19,6 +19,15 @@ func TestDappyAuth_HappyPath(t *testing.T) { | |||
| "should authenticate successfully") | |||
| } | |||
|
|
|||
| func TestDappyUsers(t *testing.T) { | |||
There was a problem hiding this comment.
so, since we have testify going here, let's go ahead and make use of it.
instead of doing the standard library thing here, maybe something more like this:
users, err := client.Users() <<< because we are going to switch those return values
assert.Nil(t, err, "there should be no error when fetching users")
assert.GreaterOrEqual(t, len(users), 1, "we should get back at least 1 user") <<< maybe more?
might even say we go so far as to hard code a couple users that come back (haven't run it yet, so i don't know off the top of my head, but could do stuff like this too:
assert.Contains(t, users, "tesla")
assert.Contains(t, users, "whatever other scientists we get back or whatever")
i donno. i like the idea of the tests being fairly brittle, but since these are really integration tests against a pretend server (https://www.forumsys.com/tutorials/integration-how-to/ldap/online-ldap-test-server/) maybe we shouldn't rely too heavily on what they have (though it hasn't changed in years, as that's kind of its point). I donno... thoughts here are appreciated.
get all users