-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Handle password expiry in user_ldap #1023
Handle password expiry in user_ldap #1023
Conversation
use OCP\AppFramework\IAppContainer; | ||
|
||
class Application extends App { | ||
public function __construct (array $urlParams = array()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the argument, it's never needed these days
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint, done.
@blizzz: An additional thing to consider is whether and how to make the password policy dn configurable. Currently I'm just using |
7080212
to
81af16b
Compare
As discussed with @blizzz , the password policy DN will be configurable in the advanced directory settings. If the password policy DN is not configured, then password expiry handling is disabled completely. This way, we can safely erase the impact on users who don't require this feature. Moreover, the logic to handle password expiry will be moved from the |
f86f6ae
to
de52fcc
Compare
59ee4eb
to
97a63ad
Compare
Is it possible to implement it for AD by counting it?
https://msdn.microsoft.com/en-us/library/ms677943(v=vs.85).aspx It will be very helpfull for AD users to have it :) End what about notify at desktop client? |
@KB7777 The sentence about grace logins actually means that with AD, it is not possible to prompt users to renew their password in case it has already expired. However regarding notifications about passwords due to expire soon, AD has yet another obstacle which is that there is no Notifications are issued using the Nextcloud's Notification API, so I would assume that if the desktop client checks for new notifications, the password expiry alerts should show up there, too? |
Sounds like a heavy-weight workaround, and likely unreliable. Then, I would leave it out for now, as it should be reported by the server.
Correct |
For the record, the code has been further enhanced by handling the pwdReset Making use of the |
#3832 is in. @GitHubUser4234 you can continue (finally) 😄 |
Signed-off-by: Roger Szabo <roger.szabo@web.de>
Signed-off-by: Roger Szabo <roger.szabo@web.de>
Signed-off-by: Roger Szabo <roger.szabo@web.de>
Of course! I see there are commits incoming, just ping me when you're done :) |
:))) Oh yeah, the commits were adjustments due to the utterly outdated code base, but now continuous-integration/drone/pr and Scrutinizer look somewhat happy again, that should be all for now ✌️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good, I'd only like to talk about handlePasswordExpiry() (see comment). Other comments are just formal and no blockers.
apps/user_ldap/appinfo/routes.php
Outdated
@@ -21,7 +21,6 @@ | |||
* | |||
*/ | |||
|
|||
/** @var $this \OCP\Route\IRouter */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it removed?
$c->query('UserManager'), | ||
$server->getConfig(), | ||
$c->query('OCP\IL10N'), | ||
//$c->query('Session'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this line can go
* @param IURLGenerator $urlGenerator | ||
*/ | ||
function __construct($appName, | ||
IRequest $request, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation seems a bit off
if($this->config->getUserValue($user, 'user_ldap', 'needsPasswordReset') !== 'true') { | ||
return new RedirectResponse($this->urlGenerator->linkToRouteAbsolute('core.login.showLoginForm')); | ||
} | ||
$parameters = array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the short notation here: $parameters = [];
as you did on other places.
apps/user_ldap/lib/User/User.php
Outdated
&& $pwdExpireWarning && (count($pwdExpireWarning) > 0)) { | ||
$pwdMaxAgeInt = intval($pwdMaxAge[0]); | ||
$pwdExpireWarningInt = intval($pwdExpireWarning[0]); | ||
//pwdMaxAge=0 -> password never expires |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this and the following comment go?
* | ||
* @param array $params | ||
*/ | ||
public function handlePasswordExpiry($params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have several readAttributes in this method, this all costs. A web login does not happen so often, but what about logins via Clients (btw, did you test what happens when the password expired while you connected to Nextcloud with a client?). Perhaps some values can be loaded during login (see Manager::getAttributes()
and User::processAttributes
) and/or cached, if it makes sense.
Thanks @blizzz for the comments :) I'll solve all the formal issues in one go, once we have pinpointed a solution for the handlePasswordExpiry() comment.
Yes, e.g. the Nextcloud desktop client continues to work normally when the password expires (cookies?), however when a logout and login is performed after expiry, the sync indicator doesn't become green again which is somewhat expected.
Well, I guess logins for a client only happen often when it doesn't use cookies and therefore triggers a login for each request? Alright, here is a list of all the 7 attributes in question: Global attributes:
General user attribute: Transient user attributes:
While transient user attributes and the general user attribute could be loaded during login, the global attributes could be cached. What would be the appropriate method that could be used for caching the global attributes? But then there's another thing to consider: The whole "handle password expiry" logic currently resides in a code block which is surrounded by a check whether this feature is actually enabled. When we put the attributes used by this feature into several other locations, it would at least not be as clear cut as it is now and code is called even with the feature not supported/enabled. Do performance benefits outweigh this? Thanks a lot 🐱 |
Yes, session cookies. I assume the client periodically tries to re-login in the latter case. Is this a problem?
Exactly.
The connection instance has a cache object, cf. https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L315 and hhttps://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L320
That's a valid point. On the other hand there are not only performance benefits for the user, but also frequent request to the LDAP server which would could get more significant with big active user bases. A compromise could be to use Access::searchUsers with the user DN as base instead. It can read and return any number of attributes. (Or more work can be put in to either make readAttribute multi-attribute ready or to touch less other crud to implement an new readAttributes() method and deprecate the former one. The benefit is the search scope, but probably this would not differ too much from the searchUsers approach). |
Signed-off-by: Roger Szabo <roger.szabo@web.de>
Signed-off-by: Roger Szabo <roger.szabo@web.de>
Thanks @blizzz
One has to keep in mind that this scenario eats up grace logins and if the
As discussed, we replace the 7 All issues including the formal ones should be addressed now 🐱 |
Signed-off-by: Roger Szabo <roger.szabo@web.de>
Signed-off-by: Roger Szabo <roger.szabo@web.de>
@blizzz Could you have another look? cc @nextcloud/ldap |
Why dont you implement a(nother) grace period where the user is only able to login and change there password:
Of course this is only another workaround and it will not address problems discussed above (eg no login in grace period). A general word on expired passwords in AD: for me this is not a relevant problem as my users have the possibilty to login via a windows client. And thats nothibg else as the grace period above. |
@johnripper1 Not sure whether I have understood correctly, but as grace periods in terms of actual time periods are mentioned (which isn't supported by OpenLDAP), it seems the suggestion is to implement such time period based password policies in Nextcloud? Well, it sounds like out of scope, the goal of this PR is to actually handle password expiry enforced by the LDAP server (see discussions above), not by the client (Nextcloud). But apart from that, grace periods as described in the example have the problem that users who login seldomly, already passed their grace period (e.g. 10 days) when they login. With the grace login count approach supported by OpenLDAP however, these users can still renew their password, independently from when their last login was. And with this PR, they will be redirected to a "Renew password" page during login. |
@GitHubUser4234 @blizzz can you write down some steps to test this? |
@rullzer Besides the phpunit test cases, the following steps could be used to perform some manual tests:
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok did some messing around and it seems to do the job! Lets get this in.
Yippie, thanks a lot for testing! 😺 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me! :) 🚢
@blizzz Awesome, and thanks a lot for the great and friendly help consistently provided to make this PR possible :)) |
But I really say sorry for this. This simply took too long. Sorry that you waited that long to get it in. Thanks for your patience. |
Congrats to @GitHubUser4234 for this, I realize we didn't talk about this in the release announcement but it is a great improvement! I will think of a way to talk about this still. Edit: I just added that we support password policies on nextcloud.com/secure ;-) |
This is based on #600, it has been discussed at #25114 and covers the "Password Policies" part that is described there.
With the fact in mind that PHP doesn't support ldappasswd, users with expired passwords wouldn't be able to reset their passwords by themselves through ldapmodify. In general, Super Admins and Group Admins will have to reset the password of their users which can be done once #600 is merged. It would sure be great to still have an additional option for users to renew their expired passwords without bothering the Admins. There is a mechanism available, namely grace authentication, which determines whether and how often a user is allowed to log in after a password has expired.
An implementation in user_ldap would look like this:
Add a logic in the
checkPassword()
method to check after a successful bind whether thepwdGraceUseTime
attribute is holding values (indicating the grace logins after password expiry).If
pwdGraceUseTime
doesn't exist or doesn't contain a value, proceed as usual. And if (pwdChangedTime
+pwdMaxAge
- current time) <=pwdExpireWarning
, show a password expiry warning on the page to which the LoginController is going to redirect (using NotificationManager, right?).If
pwdGraceUseTime
contains a number of values lower than the value indicated by thepwdGraceAuthNLimit
attribute then redirect to a password renewal page where the user can renew his password. After successful renewal, the user is redirected to the default login page where he is prompted to login with his new password.Otherwise (meaning
pwdGraceUseTime
contains a number of values >= than the value indicated by thepwdGraceAuthNLimit
attribute) there is no more grace login available and the method returnsfalse
.Grace logins are supported by OpenLDAP, but not by Active Directory. This won't have an impact on Active Directory users as the
pwdGraceUseTime
attribute doesn't exist there.@blizzz: As usual, your opinion on the above is highly recommended 😸
Code "should" be ready by Tuesday (30.08.).