Simplify auth.authenticate #2728
Conversation
hmpf
force-pushed
the
simplify-authenticate
branch
from
85ca4e9
to
c182d0d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2728 +/- ##
==========================================
+ Coverage 56.69% 56.72% +0.02%
==========================================
Files 602 602
Lines 43971 43988 +17
==========================================
+ Hits 24931 24950 +19
+ Misses 19040 19038 -2 ☔ View full report in Codecov by Sentry. |
hmpf
force-pushed
the
simplify-authenticate
branch
4 times, most recently
from
6acbca8
to
1b0e2f7
Compare
| if account.check_password(password): | ||
| account = update_ldap_user(ldap_user, account, password) | ||
| return account |
There was a problem hiding this comment.
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...
hmpf
force-pushed
the
simplify-authenticate
branch
2 times, most recently
from
59468b8
to
8ad1121
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So, is the change you want a flow-chart?
hmpf
force-pushed
the
simplify-authenticate
branch
from
8ad1121
to
b6fa32a
Compare
|
hmpf
force-pushed
the
simplify-authenticate
branch
2 times, most recently
from
1cdafc0
to
096ea0c
Compare
hmpf
force-pushed
the
simplify-authenticate
branch
6 times, most recently
from
9868366
to
99d3387
Compare
|
Greatly simplify the logic by using early returns.
hmpf
force-pushed
the
simplify-authenticate
branch
from
99d3387
to
6c0d22c
Compare
Greatly simplify the logic by using early returns.