Bug 43384 - SAML login can't check why login failed anymore (password expired., etc)
SAML login can't check why login failed anymore (password expired., etc)
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: SAML
UCS 4.2
Other Linux
: P5 normal (vote)
: UCS 4.2
Assigned To: Jürn Brodersen
Florian Best
: interim-1
Depends on: 36215
Blocks: 51903 52278
  Show dependency treegraph
 
Reported: 2017-01-20 16:05 CET by Jürn Brodersen
Modified: 2020-11-02 13:35 CET (History)
2 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 4: Minor Usability: Impairs usability in secondary scenarios
Who will be affected by this bug?: 5: Will affect all installed domains
How will those affected feel about the bug?: 1: Nuisance – not a big deal but noticeable
User Pain: 0.114
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): Usability
Max CVSS v3 score:


Attachments
proposed patch (2.43 KB, patch)
2017-01-30 12:26 CET, Jürn Brodersen
Details | Diff
patch (1.76 KB, patch)
2017-02-03 13:17 CET, Florian Best
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jürn Brodersen univentionstaff 2017-01-20 16:05:31 CET
The saml login fails with Unknown User/Wrong Password if the password is expired.
This happens because a ldap bind with an expired password is not possible anymore. See Bug 36215.

The exception is thrown by:
univention-saml/simplesamlphp-modules/uldap/lib/Auth/Source/uLDAP.php
"$attributes = $this->ldapConfig->login($username, $password, $sasl_args);"
Comment 1 Florian Best univentionstaff 2017-01-23 15:17:05 CET
I disabled the according test cases:

ucs-test (7.0.10-4):
r76015 | Bug #43384: disable test case
Comment 2 Jürn Brodersen univentionstaff 2017-01-30 12:26:34 CET
Created attachment 8383 [details]
proposed patch

I put the account expiry checks in an extra function and check if a login might be possible before trying to login.
Comment 3 Florian Best univentionstaff 2017-01-30 12:44:51 CET
(In reply to Jürn Brodersen from comment #2)
> Created attachment 8383 [details]
> proposed patch
> 
> I put the account expiry checks in an extra function and check if a login
> might be possible before trying to login.

Nice! Can you please make the following adjustments and apply the patch?
1. The login method should be above of the new method is_login_possible().
2. Please add a try-except around ->login() and only execute is_login_possible() in the except block (and rename the method into something like get_user_attributes()).
Comment 4 Jürn Brodersen univentionstaff 2017-01-30 17:37:14 CET
r76174
r76182
Successful build
Package: univention-saml
Version: 4.0.2-4A~4.2.0.201701301505
Branch: ucs_4.2-0


r76187: enabled tests
Successful build
Package: ucs-test
Version: 7.0.10-12A~4.2.0.201701301550
Branch: ucs_4.2-0
Comment 5 Florian Best univentionstaff 2017-02-03 13:17:20 CET
Created attachment 8398 [details]
patch

In case LDAP bind succeeds with expired user account the check should also be performed. proposed patch attached.
Comment 6 Jürn Brodersen univentionstaff 2017-02-03 16:42:23 CET
r76408: Always check for expired password/user
Package: univention-saml
Version: 4.0.3-2A~4.2.0.201702031627
Branch: ucs_4.2-0

76410: Fixed timezone in saml test
Package: ucs-test
Version: 7.0.11-2A~4.2.0.201702031633
Branch: ucs_4.2-0
Comment 7 Florian Best univentionstaff 2017-02-07 15:43:57 CET
OK: SAML login with expired passwords show correct error message
OK: ucs-tests
Comment 8 Stefan Gohmann univentionstaff 2017-04-04 18:28:30 CEST
UCS 4.2 has been released:
 https://docs.software-univention.de/release-notes-4.2-0-en.html
 https://docs.software-univention.de/release-notes-4.2-0-de.html

If this error occurs again, please use "Clone This Bug".