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

Simplify auth.authenticate #2728

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+383 −299
Open

Simplify auth.authenticate #2728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5138713
Simplify auth.authenticate
hmpf Nov 10, 2023
6f0aaa8
Move more ldapauth stuff to ldap.py
hmpf Nov 10, 2023
9e001c7
Collect auth.ldap tests in the same file
hmpf Nov 14, 2023
6c0d22c
Increase test coverage of auth.ldap
hmpf Nov 14, 2023
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
65 changes: 16 additions & 49 deletions python/nav/web/auth/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,68 +49,35 @@
Returns account object if user was authenticated, else None.
"""
# FIXME Log stuff?
auth = False
account = None

# Try to find the account in the database. If it's not found we can try
# LDAP.
try:
account = Account.objects.get(login__iexact=username)
except Account.DoesNotExist:
if ldap.available:
user = ldap.authenticate(username, password)
# If we authenticated, store the user in database.
if user:
account = Account(
login=user.username, name=user.get_real_name(), ext_sync='ldap'
)
account.set_password(password)
account.save()
_handle_ldap_admin_status(user, account)
# We're authenticated now
auth = True

if account and account.locked:
# account autocreated if username is authenticated
account = ldap.authenticate(username, password)
return account

Check warning on line 60 in python/nav/web/auth/__init__.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/__init__.py#L59-L60 

Added lines #L59 - L60 were not covered by tests

if account.locked:
_logger.info("Locked user %s tried to log in", account.login)
return None

Check warning on line 64 in python/nav/web/auth/__init__.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/__init__.py#L64 

Added line #L64 was not covered by tests

if (
account
and account.ext_sync == 'ldap'
and ldap.available
and not auth
and not account.locked
):
if account.ext_sync == 'ldap' and ldap.available:
try:
auth = ldap.authenticate(username, password)
ldap_user = ldap.get_ldap_user(username, password)
except ldap.NoAnswerError:
# Fallback to stored password if ldap is unavailable
auth = False
pass

Check warning on line 70 in python/nav/web/auth/__init__.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/__init__.py#L70 

Added line #L70 was not covered by tests
else:
if auth:
account.set_password(password)
account.save()
_handle_ldap_admin_status(auth, account)
else:
return
if ldap_user:
account = ldap.update_ldap_user(ldap_user, account, password)
return account
return None
# Fallback to stored password if ldap is unavailable

if account and not auth:
auth = account.check_password(password)

if auth and account:
if account.check_password(password):
return account
else:
return None


def _handle_ldap_admin_status(ldap_user, nav_account):
is_admin = ldap_user.is_admin()
# Only modify admin status if an entitlement is configured in webfront.conf
if is_admin is not None:
admin_group = AccountGroup.objects.get(id=AccountGroup.ADMIN_GROUP)
if is_admin:
nav_account.groups.add(admin_group)
else:
nav_account.groups.remove(admin_group)
return None


def get_login_url(request):
Expand Down
64 changes: 60 additions & 4 deletions python/nav/web/auth/ldap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import nav.errors
from nav.config import NAVConfigParser
from nav.models.profiles import Account, AccountGroup

_logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -121,11 +122,66 @@
return lconn


def authenticate(login, password):
def authenticate(username, password):
"""
Attempt to authenticate the login name with password against the
configured LDAP server. If the user is authenticated, required
group memberships are also verified.
Authenticate the username and password against the configured LDAP server.

Required group memberships are also verified.

Returns an authenticated Account with updated groups, or None.
"""
if not available:
return None
ldap_user = get_ldap_user(username, password)
try:
account = Account.objects.get(login__iexact=username, ext_sync='ldap')
except Account.DoesNotExist:
if ldap_user:
account = autocreate_ldap_user(ldap_user, password)
return account
if account.locked:
_logger.info("Locked user %s tried to log in", account.login)
return None
if account.check_password(password):
account = update_ldap_user(ldap_user, account, password)
return account
Comment on lines +145 to +147
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ldap_user can be False, and then update_ldap_user will raise an exception. It is unclear what the best solution is though:

Change the if to:

if ldap_user and account.check_password(password):

If ldap_user is False authenticate will return None.

Split the if into two:

if ldap_user:
    account = update_ldap_user(ldap_user, account, password)

if account.check_password(password):
    return account

Worse, the latter if might not be needed at all because if ldap_user is True, we have already been authenticated with ldap, so authenticating again is unneccessary.

A third way is:

if not ldap_user:
    return None

account = update_ldap_user(ldap_user, account, password)

and then again choose whether to check the password again...

Comment on lines +125 to +147
Copy link
Member

Choose a reason for hiding this comment

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

This got confusing, real fast.

I read this as: If a re-visiting user is authenticated via LDAP, this additionally checks their entered password against the password stored in the DB. If their LDAP password changed at some point, this local verification will fail, and the user can no longer log in to NAV.

Then I realized that this function is actually only ever called for first-time users. The function contains code to handle the difference between new users and re-visiting users. It won't work well for re-visiting users, if I'm reading it correctly, but it will also never be called for re-visiting users, so what's the plan here?

Copy link
Contributor Author

@hmpf hmpf Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is confusing: it's partially duplicating what nav.web.auth.authenticate is doing as a small step to make the latter independent of ldap.

It will be called for re-visiting users once we split up nav.web.auth.authenticate. Then it will also change.

  • If ldap is not on, return None and fallback to local user.
  • If ldap is on:
    • If ldap_user is False, return None and fallback to local user
    • If ldap_user is not False it is authenticated, get and/or update the account and return the account

See my comment above about the fact that what happens if ldap_user is False currently isn't covered and that there are multiple ways to do it.

The hope is that we will one day have a local.authenticate in addition to ldap.authenticate, and that in nav.web.auth we will have something (e.g. loop_through_available_authentication_methods) that runs through all methods that ask for username/password in order and stops on the first to find an account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, is the change you want a flow-chart?

return None


def autocreate_ldap_user(ldap_user, password):
account = Account(

Check warning on line 152 in python/nav/web/auth/ldap.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/ldap.py#L152 

Added line #L152 was not covered by tests
login=ldap_user.username,
name=ldap_user.get_real_name(),
ext_sync='ldap',
)
account = update_ldap_user(ldap_user, account, password)
return account

Check warning on line 158 in python/nav/web/auth/ldap.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/ldap.py#L157-L158 

Added lines #L157 - L158 were not covered by tests


def update_ldap_user(ldap_user, account, password):
account.set_password(password)
account.save()
_handle_ldap_admin_status(ldap_user, account)
return account


def _handle_ldap_admin_status(ldap_user, nav_account):
is_admin = ldap_user.is_admin()
# Only modify admin status if an entitlement is configured in webfront.conf
if is_admin is not None:
admin_group = AccountGroup.objects.get(id=AccountGroup.ADMIN_GROUP)
if is_admin:
nav_account.groups.add(admin_group)

Check warning on line 174 in python/nav/web/auth/ldap.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/ldap.py#L172-L174 

Added lines #L172 - L174 were not covered by tests
else:
nav_account.groups.remove(admin_group)

Check warning on line 176 in python/nav/web/auth/ldap.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/ldap.py#L176 

Added line #L176 was not covered by tests


def get_ldap_user(login, password):
"""
Fetch an LDAPUser from an ldap server if login and password matches.

Returns an autenticated LDAPUser of a specific group or with specific
entitlements, or False.
"""
lconn = open_ldap()
server = _config.get('ldap', 'server')
Expand Down
177 changes: 2 additions & 175 deletions tests/unittests/general/webfront_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
from mock import patch, MagicMock, Mock
from django.test import RequestFactory

import pytest

import nav.web.auth.ldap
from nav.web import auth
from nav.web.auth import remote_user
from nav.web.auth.utils import ACCOUNT_ID_VAR
Expand All @@ -23,12 +20,12 @@ def test_authenticate_should_return_account_when_ldap_says_yes(self):
ldap_user = Mock()
ldap_user.is_admin.return_value = None # mock to avoid database access
with patch("nav.web.auth.ldap.available", new=True):
with patch("nav.web.auth.ldap.authenticate", return_value=ldap_user):
with patch("nav.web.auth.ldap.get_ldap_user", return_value=ldap_user):
assert auth.authenticate('knight', 'shrubbery') == LDAP_ACCOUNT

def test_authenticate_should_return_false_when_ldap_says_no(self):
with patch("nav.web.auth.ldap.available", new=True):
with patch("nav.web.auth.ldap.authenticate", return_value=False):
with patch("nav.web.auth.ldap.get_ldap_user", return_value=False):
assert not auth.authenticate('knight', 'shrubbery')

def test_authenticate_should_fallback_when_ldap_is_disabled(self):
Expand Down Expand Up @@ -168,173 +165,3 @@ def test_remote_user_set(self, fake_session):
assert (
request.session.get(ACCOUNT_ID_VAR, None) == REMOTE_USER_ACCOUNT.id
)


class TestLdapUser(object):
@patch.dict(
"nav.web.auth.ldap._config._sections",
{
'ldap': {
'__name__': 'ldap',
'basedn': 'empty',
'manager': 'empty',
'manager_password': 'empty',
'uid_attr': 'sAMAccountName',
'encoding': 'utf-8',
},
},
)
def test_search_result_with_referrals_should_be_considered_empty(self):
"""LP#1207737"""
conn = Mock(
**{
'search_s.return_value': [
(None, "restaurant"),
(None, "at the end of the universe"),
]
}
)
u = nav.web.auth.ldap.LDAPUser("zaphod", conn)
with pytest.raises(nav.web.auth.ldap.UserNotFound):
u.search_dn()

@patch.dict(
"nav.web.auth.ldap._config._sections",
{
'ldap': {
'__name__': 'ldap',
'basedn': 'empty',
'lookupmethod': 'direct',
'uid_attr': 'uid',
'encoding': 'utf-8',
'suffix': '',
}
},
)
def test_non_ascii_password_should_work(self):
"""LP#1213818"""
conn = Mock(
**{
'simple_bind_s.side_effect': lambda x, y: (
str(x),
str(y),
),
}
)
u = nav.web.auth.ldap.LDAPUser(u"zaphod", conn)
u.bind(u"æøå")

@patch.dict(
"nav.web.auth.ldap._config._sections",
{
'ldap': {
'__name__': 'ldap',
'basedn': 'cn=users,dc=example,dc=org',
'lookupmethod': 'direct',
'uid_attr': 'uid',
'encoding': 'utf-8',
'group_search': '(member=%%s)',
},
},
)
def test_is_group_member_for_non_ascii_user_should_not_raise(self):
"""LP#1301794"""

def fake_search(base, scope, filtr):
str(base)
str(filtr)
return []

conn = Mock(
**{
'search_s.side_effect': fake_search,
}
)
u = nav.web.auth.ldap.LDAPUser(u"Ægir", conn)
u.is_group_member('cn=noc-operators,cn=groups,dc=example,dc=com')


@patch.dict(
"nav.web.auth.ldap._config._sections",
{
'ldap': {
'__name__': 'ldap',
'basedn': 'cn=users,dc=example,dc=org',
'lookupmethod': 'direct',
'uid_attr': 'uid',
'encoding': 'utf-8',
'require_entitlement': 'president',
'admin_entitlement': 'boss',
'entitlement_attribute': 'eduPersonEntitlement',
},
},
)
class TestLdapEntitlements(object):
def test_required_entitlement_should_be_verified(self, user_zaphod):
u = nav.web.auth.ldap.LDAPUser("zaphod", user_zaphod)
assert u.has_entitlement('president')

def test_missing_entitlement_should_not_be_verified(self, user_marvin):
u = nav.web.auth.ldap.LDAPUser("marvin", user_marvin)
assert not u.has_entitlement('president')

def test_admin_entitlement_should_be_verified(self, user_zaphod):
u = nav.web.auth.ldap.LDAPUser("zaphod", user_zaphod)
assert u.is_admin()

def test_missing_admin_entitlement_should_be_verified(self, user_marvin):
u = nav.web.auth.ldap.LDAPUser("marvin", user_marvin)
assert not u.is_admin()


@patch.dict(
"nav.web.auth.ldap._config._sections",
{
'ldap': {
'__name__': 'ldap',
'basedn': 'cn=users,dc=example,dc=org',
'lookupmethod': 'direct',
'uid_attr': 'uid',
'encoding': 'utf-8',
'require_entitlement': 'president',
'admin_entitlement': '',
'entitlement_attribute': 'eduPersonEntitlement',
},
},
)
def test_no_admin_entitlement_option_should_make_no_admin_decision(user_zaphod):
u = nav.web.auth.ldap.LDAPUser("zaphod", user_zaphod)
assert u.is_admin() is None


#
# Pytest fixtures
#


@pytest.fixture
def user_zaphod():
return Mock(
**{
'search_s.return_value': [
(
u'uid=zaphod,cn=users,dc=example,dc=org',
{u'eduPersonEntitlement': [b'president', b'boss']},
)
]
}
)


@pytest.fixture
def user_marvin():
return Mock(
**{
'search_s.return_value': [
(
u'uid=marvin,cn=users,dc=example,dc=org',
{u'eduPersonEntitlement': [b'paranoid']},
)
]
}
)
Empty file added tests/unittests/web/auth/__init__.py
View file
Open in desktop
Empty file.
Loading
Loading