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.

Sign up for GitHub

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

Jump to bottom

Fix parsing of search response #294

Merged
merged 8 commits into from
Nov 5, 2024
Merged

Fix parsing of search response #294

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2123f36
Fix parsing of search response
consideRatio Oct 31, 2024
148eeb8
Fix log level
consideRatio Nov 1, 2024
ccd244f
Ensure unique user in search result for get_user_attributes
consideRatio Nov 1, 2024
f45d87e
Coalesce similar log events to reduce code complexity
consideRatio Nov 1, 2024
00f8af1
refactor: a bit less code
consideRatio Nov 1, 2024
0732444
Tweak log messages
consideRatio Nov 1, 2024
70ea3cd
Tweak log messages further, make users actionable
consideRatio Nov 1, 2024
769f4fb
Update logging in resolve_username to only consider its purpose
consideRatio Nov 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 29 additions & 23 deletions ldapauthenticator/ldapauthenticator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,38 +455,41 @@ def resolve_username(self, username_supplied_by_user):
search_filter=search_filter,
attributes=[self.lookup_dn_user_dn_attribute],
)
response = conn.response
if len(response) == 0:

# identify unique search response entry
n_entries = len(conn.entries)
if n_entries == 0:
self.log.warning(
f"Failed to lookup a DN for username '{username_supplied_by_user}'"
)
return (None, None)
if len(response) > 1:
if n_entries > 1:
self.log.warning(
consideRatio marked this conversation as resolved.
Show resolved Hide resolved
f"Failed to lookup a unique DN for username '{username_supplied_by_user}'"
)
return (None, None)
if "attributes" not in response[0].keys():
entry = conn.entries[0]

# identify unique attribute value within the entry
attribute_values = entry.entry_attributes_as_dict.get(
self.lookup_dn_user_dn_attribute
)
if not attribute_values:
self.log.warning(
f"Failed to lookup attribute '{self.lookup_dn_user_dn_attribute}' for username '{username_supplied_by_user}'"
f"Failed to lookup attribute '{self.lookup_dn_user_dn_attribute}' "
f"for username '{username_supplied_by_user}'"
)
return (None, None)
if len(attribute_values) > 1:
self.log.error(
f"A lookup of the username '{username_supplied_by_user}' returned multiple "
f"values for the attribute '{self.lookup_dn_user_dn_attribute}': "
f"({', '.join(attribute_values)})"
)
return None, None

userdn = response[0]["dn"]
username = response[0]["attributes"][self.lookup_dn_user_dn_attribute]
if isinstance(username, list):
if len(username) == 0:
return (None, None)
elif len(username) == 1:
username = username[0]
else:
self.log.error(
f"A lookup of the username '{username_supplied_by_user}' returned a list "
f"of entries for the attribute '{self.lookup_dn_user_dn_attribute}': "
f"({', '.join(username)})"
)
return None, None

userdn = entry.entry_dn
username = attribute_values[0]
return (username, userdn)

def get_connection(self, userdn, password):
Expand Down Expand Up @@ -547,6 +550,9 @@ def get_user_attributes(self, conn, userdn):
search_filter="(objectClass=*)",
attributes=self.auth_state_attributes,
)
# FIXME: Handle situations with multiple entries below or comment
# why its not important to do.
#
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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}

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

#295

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

if found:
attrs = conn.entries[0].entry_attributes_as_dict
return attrs
Expand Down Expand Up @@ -632,14 +638,14 @@ async def authenticate(self, handler, data):
),
attributes=self.attributes,
)
n_users = len(conn.response)
if n_users == 0:
n_entries = len(conn.entries)
if n_entries == 0:
self.log.warning(
"Configured search_filter found no user associated with "
f"userattr='{self.user_attribute}' and username='{resolved_username}'"
)
return None
if n_users > 1:
if n_entries > 1:
self.log.warning(
"Configured search_filter found multiple users associated with "
f"userattr='{self.user_attribute}' and username='{resolved_username}', "
Expand Down