Le mardi 4 juillet 2017, 07:03:20 Andreas Heigl a écrit :
> > In my opinion the code is really ease to read with exceptions.
> >
> > try {
> > $user = ldap_exop_whoami($link);
> > }
> > catch(Throwable $e) {
> > }
>
> It definitely is easier to read. But let's not try too much in one go.
> As all of the current extension uses errors I'd currently stick to the
> errors and leave moving to extensions for a later change. Partly to keep
> at least some kind of consistency and partly to not come into the trap
> of moving to extensions completely and therefore breaking BC…For me moving to exception is an other subject that would need to be treated separately. And I’m not even sure I would vote for that. > > How is the behavior of the following? > > > > Change $oldPassword of current user to $newPassword? > > ldap_exop_passwd($link, '', $oldPassword, $newPassword) > ldap_exop_passwd($link, null, $oldPassword, $newPassword); > > Though passing an empty string should work also with the current code. > But I'd prefer to pass NULL > > > > Change $oldPassword of $user to empty string? Or random? Or is this an > > error? > > ldap_exop_passwd($link, $user, $oldPassword, ''); > > IMHO you can't change to an empty string. Because that would be like not > setting a password at all. I'd restrict that so far that providing an > empty password will cause the server to generate a random password that > is then returned. This change to a random password. > > My previous suggestion was to split the function into two versions to > > reduce the amount of arguments. > > > > string|FALSE ldap_exop_passwd(resource $link, string $user, string > > $newPassword [, string $oldPassword]) > > > > string|FALSE ldap_exop_random_passwd(resource $link, string $user [, > > string $oldPassword]) > > I would not do that as it bloats the API in an - in my eyes - > unnecessary way. Let's stick to one function for changing password… Yeah same here, one function for each EXOP makes sense and I don’t see the point of splitting into two functions. I don’t like the idea of switching oldpw and newpw order either as it may confuse people. > One thing though that I thought about: Chapter 4 of RFC 3062 explicitly > states that this function should only be available with confidentially > support like TLS. So perhaps we should check whether the data will be > transfered via a secure connection and - if not - raise an error? Hum I get the idea but is that really our place? I mean the API won’t prevent you from storing password without hashing for instance. And people can use ldap_modify to change the password without TLS, which is equally dangerous IMO. For me it should be possible, and useful at least for tests. So for the API I will move to string|FALSE ldap_exop_whoami(resource $link) - The returned string is the DN of the currently bound user. For ldap_exop_passwd, you proposed: string|FALSE ldap_exop_passwd(resource $link [, string $user [, string $oldPassword [, string $newPassword]]] - The returned string is the new password of the user. Either the given newPassword or a newly generated one. Does it makes sense to return the newPassword on success when it’s not generated? I think I would prefer if returning a string explicitely means a password was generated, otherwise it gets too confused (3 possible meaning for the returned value!). Côme
signature.asc
Description: This is a digitally signed message part.
