Fix parsing of search response

[consideRatio]

Fixes #292 and a bug that maybe never surfaced where things could work before 2.0.0 when the conn.response list had multiple elements thanks to being ordered in a way where the first element was the one of relevance, even though LDAP specification sais any ordering is allowed. Maybe the ldap3 client orders them, but who knows.

Also fixes #295

Investigation

• Should we use conn.entries instead of conn.response? How is conn.entries populated? Is it the type=searchResEntry responses?
  Answer: yes we should use conn.entries, it is only including type=searchResEntry responses, see the code here.

References

• discussion: Incorrect handling for LDAP responses with Search Reference Results. #292
• ldap3 docs: https://ldap3.readthedocs.io/en/latest/searches.html#entries
• LDAP docs: https://datatracker.ietf.org/doc/html/rfc4511#section-4.5.2

[manics]

Assuming this works can you add a unit test for _filter_search_response using the responses in #292 (comment) as test data?

[consideRatio]

@manics I'll make sure to include that if the function is retained, but I think maybe use of conn.entries can make it irrelevant. The documentation isn't so clear, so I look to investigate further.

[consideRatio]

@manics @franciscaestecker this is ready for review now. I think the bug resided in this projet's use of conn.response while we expected the data from conn.entries that doesn't include anything but the searchResEntry type of responses.

[manics]

Maybe we should throw an error, same as in resolve_username? If there's a possibility of the entries corresponding to different Identities this implies a change in the LDAP server could lead to a different ordering of responses, resulting in a user gaining access to another user's account.

If it's two entries for the same user we still need to understand what the difference is, in case some attributes are different which could lead to inconsistent configuration of the singleuser server.

[consideRatio]

I don't think users will get access to another JupyterHub account, as its just impacting the auth state (see code below from the end of af the authenticate function) - but they could get access to another users ldap data through their jupyterhub account.

I'll open an issue for this to be tracked separately from this PR - do you think its important to get fixed before release?

        user_attributes = self.get_user_attributes(conn, userdn)
        self.log.debug("username:%s attributes:%s", login_username, user_attributes)

        username = resolved_username if self.use_lookup_dn_username else login_username
        auth_state = {
            "ldap_groups": ldap_groups,
            "user_attributes": user_attributes,
        }
        return {"name": username, "auth_state": auth_state}

[consideRatio]

@manics btw off topic, but I just wanted to say I greatly appreciate your effort into the jupyterhub - you regularly make me very thankful!!

[manics]

My preference is to include this in the next release.

I think we should try and avoid any ambiguities in authenticator given their importance- I'd rather we were too strict and then relax things later. Ideally we'd get more input from people with expertise in LDAP, but it's clear we don't have that, and unfortunately the only way we can get real-world input is to release something and wait for bug reports.

[consideRatio]

Yeah I agree, I figure it makes sense to be in a separate PR though - I'll work it next!

[consideRatio]

#295

[consideRatio]

I've come to disagree with my former self, this is in scope of "Fix parsing of search response".

I pushed the commit to this PR!

[consideRatio]

I iterated a bit on the log messages as well to make errors have some pointer on what may be configured wrong or similar. I'm hands off now!

[manics]

The more detailed logging is good! I'm worried about including login ... denied in resolve_username though, since the decision is made in authenticate(). If there's a future refactor this could easily be missed, resulting in incorrect logs. Can we split the errors logs, so the resolve_username error contains the detail of why a username wasn't resolve, and put the definite denied error log in

ldapauthenticator/ldapauthenticator/ldapauthenticator.py

Lines 598 to 600 in 70ea3cd

	resolved_username, resolved_dn = self.resolve_username(login_username)
	if not resolved_dn:
	return None

instead?

The two log messages will still occur together so there's no loss of context.

[consideRatio]

Good point @manics, I've made the resolve_username function not draw conclusions on whats happening outside the function and instead focused on logging things in scope for the function to log based on its purpose only.

[manics]

Thanks!
@franciscaestecker please could you try this PR? Assuming it fixes your bug reported in #292 we can release this in the next few days as 2.0.2

[consideRatio]

@manics I'll proceed with a release