-
-
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
Enable changing passwords in user_ldap #1715
Conversation
@GitHubUser4234, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @blizzz and @butonic to be potential reviewers. |
@@ -194,6 +195,16 @@ public function search($link, $baseDN, $filter, $attr, $attrsOnly = 0, $limit = | |||
|
|||
/** | |||
* @param LDAP $link | |||
* @param string $userDN |
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.
Some whitespace is missing here
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.
Should be fine now, thanks ~
Ok, this has been discussed in IRC and @blizzz had this great idea to make the error message a combination of a generic translatable message plus a hint message containing the non-translatable message returned by the LDAP server. I will apply this tmw. |
Applied & pushed. |
* @param string $password | ||
* @return bool | ||
*/ | ||
public function modReplace($link, $userDN, $password) { |
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.
Could you also add this method to the ILDAPWrapper interface, please?
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~
* @return bool | ||
*/ | ||
public function modReplace($link, $userDN, $password) { | ||
try { |
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.
Could we move the try-and-catch logic to Access::setPassword() to avoid logic in here? Would also avoid "funny" messages if it was used for another reason in the future while the same base error is thrown.
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~
try { | ||
return $this->invokeLDAPMethod('mod_replace', $link, $userDN, array('userPassword' => $password)); | ||
} catch(ConstraintViolationException $e) { | ||
throw new HintException('Password change rejected. Hint: '.$e->getMessage(), '', $e->getCode()); |
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.
Probably I was not clear about the second parameter which is currently the emtpy string, sorry. That one is the "Hint" parameter which accepts also text. I would pass $e->getMessage()
to it. As first parameter 'Password change rejected.'
would be good. To tranlsate that we would need to get from \OC::$server->getL10N('user_ldap');
, as we do not inject it at this point (in neither Access not User_LDAP). Injecting it is cumbersome of course with the necessary changes, so for I10L it's probably survivable to not do it 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.
I tried, but the result was that the first parameter didn't get displayed in the UI. When looking at the HintException code it looks like it either has to be first or second parameter?
public function getHint() {
if (empty($this->hint)) {
return $this->message;
}
return $this->hint;
}
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.
As discussed in IRC, necessary changes in core will be conducted, so that the UI can show both parameters.
@@ -101,6 +101,8 @@ | |||
<p><label for="ldap_dynamic_group_member_url"><?php p($l->t('Dynamic Group Member URL'));?></label><input type="text" id="ldap_dynamic_group_member_url" name="ldap_dynamic_group_member_url" title="<?php p($l->t('The LDAP attribute that on group objects contains an LDAP search URL that determines what objects belong to the group. (An empty setting disables dynamic group membership functionality.)'));?>" data-default="<?php p($_['ldap_dynamic_group_member_url_default']); ?>" /></p> | |||
<p><label for="ldap_nested_groups"><?php p($l->t('Nested Groups'));?></label><input type="checkbox" id="ldap_nested_groups" name="ldap_nested_groups" value="1" data-default="<?php p($_['ldap_nested_groups_default']); ?>" title="<?php p($l->t('When switched on, groups that contain groups are supported. (Only works if the group member attribute contains DNs.)'));?>" /></p> | |||
<p><label for="ldap_paging_size"><?php p($l->t('Paging chunksize'));?></label><input type="number" id="ldap_paging_size" name="ldap_paging_size" title="<?php p($l->t('Chunksize used for paged LDAP searches that may return bulky results like user or group enumeration. (Setting it 0 disables paged LDAP searches in those situations.)'));?>" data-default="<?php p($_['ldap_paging_size_default']); ?>" /></p> | |||
<p><label for="ldap_turn_on_pwd_change"><?php p($l->t('Enable LDAP password changes per user'));?></label><span class="inlinetable"><span class="tablerow left"><input type="checkbox" id="ldap_turn_on_pwd_change" name="ldap_turn_on_pwd_change" value="1" data-default="<?php p($_['ldap_turn_on_pwd_change_default']); ?>" title="<?php p($l->t('Allow LDAP users to change their password and allow Super Administrators and Group Administrators to change the password of their LDAP users. Only works when access control policies are configured accordingly on the LDAP server. As passwords are sent in plaintext to the LDAP server, transport encryption must be used and password hashing should be configured on the LDAP server.'));?>" /><span class="tablecell">(New password is sent as plain text to LDAP)</span></span> |
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.
Wrap the new "(New password…" text into <?php p($l->t($text)); ?>
so it can get translated
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.
oops, fixed that one 😅
/** | ||
* Set password for an LDAP user identified by a DN | ||
* @param string $userDN the user in question | ||
* @param LDAP $password the new password |
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.
$password should be of type string
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.
yep~
* @return bool | ||
*/ | ||
public function setPassword($uid, $password) { | ||
$ldapRecord = $this->getLDAPUserByLoginName($uid); |
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.
$uid
is not the login name, but the internal username. If both differ, it won't work (seen here while testing). You can directly fetch the corresponding user by $user = $this->access->userManager->get($uid);
. Then it'll work :)
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~~ I saw something similar in thecheckPassword()
method, does this apply there, too?
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.
checkPassword()
is provided with the login name (it is used for login), different to this method where we get the username/uid.
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.
Oh i see, then the naming of that parameter doesn't seem to reflect it, at least not for an outsider like me 😆 If acceptable, it might be helpful in the future when we'd change it from $uid
to $loginName
? I saw that some classes like OC\User\Manager
are in fact doing that while most classes don't.
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.
Totally agree!
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.
As discussed in IRC, the name change is fine, but would be done in a separate PR for fixing variable names.
Btw, with the next commit please do a signoff like |
632493b
to
b1b593c
Compare
Current coverage is 57.25% (diff: 33.33%)@@ master #1715 diff @@
==========================================
Files 1075 1075
Lines 61314 61344 +30
Methods 6849 6853 +4
Messages 0 0
Branches 0 0
==========================================
+ Hits 35112 35125 +13
- Misses 26202 26219 +17
Partials 0 0
|
Signed-off-by: Roger Szabo <roger.szabo@web.de> remove notification part Signed-off-by: Roger Szabo <roger.szabo@web.de> blizzz comments Signed-off-by: Roger Szabo <roger.szabo@web.de> morris comment Signed-off-by: Roger Szabo <roger.szabo@web.de> improved error message for changing password Signed-off-by: Roger Szabo <roger.szabo@web.de> blizz comments 20161013 Signed-off-by: Roger Szabo <roger.szabo@web.de> Signed-off-by: Roger Szabo <roger.szabo@web.de> Adjust HintException usage Signed-off-by: Roger Szabo <roger.szabo@web.de> Signed-off-by: Roger Szabo <roger.szabo@web.de>
b1b593c
to
8ad776a
Compare
After discussion in #1787 this got updated, rebased and pushed. |
Tested and works 👍 On the Users page, when changing a password and confirming with pressing Enter, the response handling fails (no confirmation, input field stays with focus, despite successful change - but some in error case), but this issue is also present with master and thus out of scope. Password change on personal page works. Another reviewer please? @nextcloud/ldap @LukasReschke |
* @param string $password the new password | ||
* @return bool | ||
*/ | ||
public function setPassword($userDN, $password) { |
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.
Missing tests. Will add myself so we get this done.
$user = $this->access->userManager->get($uid); | ||
|
||
if(!$user instanceof User) { | ||
throw new \Exception('LDAP setPassword: Could not get user object for uid ' . $uid . |
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.
Not covered by tests. Will add.
return $this->access->setPassword($user->getDN(), $password); | ||
} | ||
|
||
return false; |
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.
Not covered by tests. Will add.
* | ||
*/ | ||
public function setPassword($uid, $password) { | ||
return $this->handleRequest($uid, 'setPassword', array($uid, $password)); |
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.
Not covered by tests. Will add.
Replaced by #2286 where I added some tests. |
And merged 🚀 |
Wow, finally the rocket took off and that at light speed! Thanks A LOT @blizzz, @LukasReschke, @MorrisJobke for the effort put into this ✨ |
Continuing PR #600 here due to some GitHub issues, sorry for inconvenience caused. 😅
@blizzz : All your comments have hopefully been taken care of :)