[Bug]: user_ldap crashes during login, password policy needs missing null check(s)
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 36.9k
- Forks
- 5.2k
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 713
Description
⚠️ This issue respects the following points: ⚠️
- This is a bug, not a question or a configuration/webserver/proxy issue.
- This issue is not already reported on Github (I've searched it).
- Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
- Nextcloud Server is running on 64bit capable CPU, PHP and OS.
- I agree to follow Nextcloud's Code of Conduct.
Bug description
function⋅handlePasswordExpiry($params) in apps/user_ldap/lib/User/User.php fail to handle multiple corener cases:
- First is after this (line 651](https://github.com/nextcloud/server/blob/baf74f0aa1b8947272b10532a3c3a1cff16b6085/apps/user_ldap/lib/User/User.php#L651)
This returns nothing if user never changed password (and he isn't on a grace time/reset)! - I presume (although I did NOT try it) this is wrong too (although it is null checked
line 668
Existing password policy doesn't mean we must have these attributes - pwdPolicy MAY have these attributes
Although I - and most others - will have all MAY attributes set, I actually don't use these, as I think forcing users to change their passwords often leads to passwords more likely to be written down.
ButpwdCheckQualityandpwdMinLengthis not zero on my server, and on other setups, these could be the only attributes set!
So these could be neither in cache, nor in search result!
Steps to reproduce
- Setup LDAP server, with a password policy that only has
pwdCheckQualityandpwdMinLengthattributes set. - Connect that user backend to Nextcloud
- Create a new LDAP user. Set their password during creation, not in a separate operation
- Try to login with the new user
Expected behavior
Newly created user should be able to log in!
If no pwdChangedTime attribute does not exist for a given user, it is not necessarily a grace login.
It it doesn't exists for a user
- If no
pwdMaxAgeandpwdExpireWarningis set in policy, then nothing to check
LDAP User could be created with throwaway token based registration, where he gets to choose his password during user creation! Maybe they are just asked at a helpdesk PC to type in a password, I know, it's horror.... - If these policy attrs are set, and
pwdChangedTimeis empty, usecreateTimeStamp
Furthermore, don't blindly override ppolicy cache with a search result that doesn't return every attribute
Installation method
Community Manual installation with Archive
Operating system
Other
PHP engine version
PHP 8.0
Web server
Other
Database engine version
PostgreSQL
Is this bug present after an update or on a fresh install?
Fresh Nextcloud Server install
Are you using the Nextcloud Server Encryption module?
Encryption is Disabled
What user-backends are you using?
- Default user-backend (database)
- LDAP/ Active Directory
- SSO - SAML
- Other
Configuration report
Irrelevant
List of activated Apps
Irrelevant
Nextcloud Signing status
No response
Nextcloud Logs
No response
Additional info
No response
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in apps/user_ldap/lib/User/User.php at handlePasswordExpiry($params), especially the referenced lines around 651 and 668. Reproduce the LDAP setup with only pwdCheckQuality and pwdMinLength, then trace how policy and user attributes are obtained. Done means newly created users can log in and missing password-policy attributes do not cause a crash or incorrectly replace cached values.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- authentication
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100