feature: ability to change custom password fields #751

markus-96 · 2023-02-07T20:06:32Z

I think I have written everything needed for this feature. Only the messages shown on the page have to be changed, and also the translations. I will try my best here. I will remove [in progress] when everything is finished and after some additional testing.

You will be able to:

setup as many app-passwords as you want,
set the name of the app and the corresponding attribute,
setup additional password policies,
setup who changes the password,
the hash algorithm

You can use it also for updating user information, but I do not think this would be very user friendly.

Also, take care about the discussion regarding this feature: #745

markus-96 · 2023-02-07T20:16:35Z

It would also be great to share what you think!

In config.inc.local.php, the configuration could look like this:

$change_apppwd = array(
    array(
        'attribute' => 'sambaNTpassword',
        'label' => 'Wifi Password',
        'who_change_password' => 'user',
        'hash' => 'NTLM',
        'msg_changehelpextramessage' => "you have to do this and that and this password is meant for wifi",
        'pwd_policy_config' => array(
            'pwd_show_policy' => true,
            'pwd_no_reuse' => true,
            'pwd_min_length' => 6
        )
    ),
    array(
        'attribute' => 'dishwasher',
        'label' => 'Dishwasher',
        'who_change_password' => 'manager',
        'hash' => 'MD5',
        'pwd_policy_config' => array(
            'pwd_no_reuse' => true
        )
    )
);

markus-96 · 2023-02-11T16:40:26Z

i think it is ready for testing, it would be great if someone could have a short look on my code and say if my code meets the expectations of this project. Thanks in advance!

Default configuration for apppwds is the one set for the main password.

markus-96 · 2023-02-12T12:06:10Z

question: I have set the default configuration with for-loops in config.inc.php. Or should I move it to changeapppwd.php?

Is there a reason why $pwd_forbidden_chars = ""; and $pwd_show_policy_pos = "above"; is not set in conf.inc.php?

coudot · 2023-02-12T12:25:28Z

Hello @markus-96,

I have set the default configuration with for-loops in config.inc.php. Or should I move it to changeapppwd.php?

you should not put too much logic in config.inc.php, only configuration parameters.

Is there a reason why $pwd_forbidden_chars = ""; and $pwd_show_policy_pos = "above"; is not set in conf.inc.php?

This attributes are present in config.inc.php, the first is commented, but not the second

markus-96 · 2023-02-12T13:21:30Z

Hello @markus-96,

I have set the default configuration with for-loops in config.inc.php. Or should I move it to changeapppwd.php?

you should not put too much logic in config.inc.php, only configuration parameters.

Is one for loop ok for setting the defaults for each app password?

Is there a reason why $pwd_forbidden_chars = ""; and $pwd_show_policy_pos = "above"; is not set in conf.inc.php?

This attributes are present in config.inc.php, the first is commented, but not the second

Oh, seams like I have overseen this.. Then I am curious why $pwd_forbidden_chars is set on line 127 in index.php and not in config.inc.php?

And what is it about the logic in line 229, where $pwd_show_policy_pos is always set due to line 153?

coudot · 2023-02-12T16:59:43Z

Is one for loop ok for setting the defaults for each app password?

I think so.

Oh, seams like I have overseen this.. Then I am curious why $pwd_forbidden_chars is set on line 127 in index.php and not in config.inc.php?

It was a new attribute, so to avoid warnings if the attribute was not defined in previous config version, we set a default value in index.php. Maybe we should indeed initialize all parameters in config.inc.php, but this is out of scope of the current PR.

And what is it about the logic in line 229, where $pwd_show_policy_pos is always set due to line 153?

There is indeed no need for the test line 229, the code can be improved.

markus-96 · 2023-02-25T14:29:52Z

Do I need to add translations to every single lang file? Or is it ok to add the english translation everywhere?

coudot · 2023-02-25T14:36:17Z

Do I need to add translations to every single lang file? Or is it ok to add the english translation everywhere?

Yes, just put english everywhere

markus-96 · 2023-03-13T08:10:16Z

htdocs/index.php

@@ -18,6 +18,12 @@
 require_once("../lib/functions.inc.php");
 if ($use_captcha) {
    require_once("../lib/captcha.inc.php");
+} else if ($change_apppwd != false) {
+    for ($i = 0; $i < count($change_apppwd); $i++) {
+        if (isset($change_apppwd['use_captcha']) && $change_apppwd['use_captcha']) {


of course it has to be
if (isset($change_apppwd[i]['use_captcha']) && $change_apppwd[i]['use_captcha']) {
will fix today...

markus-96 · 2023-04-05T13:01:26Z

Hi, I have marked this as ready for review more than one month ago. Will there be any update soon?

As a background: This was my first real contact with PHP and this is also my first PR to an open source project. I am ready for any criticism since I want to get better. I am excited about what the community thinks about this PR and may also be a bit impatient, sorry for that..

coudot · 2023-04-05T14:09:44Z

Hello @markus-96

sorry for the delay, I don't have too much time, but the PR is targeted for the next version (1.6), no release date for the moment...

make it clear that the requested application is not configured

…actorization

markus-96 · 2023-08-12T20:36:17Z

Are the tests in the tests folder actually used? I am asking because it seems to me like they are incompatible since about 2017 with newer versions of phpunit greater than 5. I cant run tests with phpunit, version greater than 5, without changing that the test-classes do extend \PHPUnit\Framework\TestCase and not \PHPUnit_Framework_TestCase.

With phpunit version 5.7.25, I get the following:

PHP Warning:  Private methods cannot be final as they are never overridden by other classes in phar://C:/Users/user/Downloads/phpunit-5.7.25.phar/phpunit/Util/Configuration.php on line 162

Warning: Private methods cannot be final as they are never overridden by other classes in phar://C:/Users/user/Downloads/phpunit-5.7.25.phar/phpunit/Util/Configuration.php on line 162
PHP Fatal error:  Cannot use 'Mixed' as class name as it is reserved in phar://C:/Users/user/Downloads/phpunit-5.7.25.phar/phpdocumentor-type-resolver/Types/Mixed.php on line 20

Fatal error: Cannot use 'Mixed' as class name as it is reserved in phar://C:/Users/user/Downloads/phpunit-5.7.25.phar/phpdocumentor-type-resolver/Types/Mixed.php on line 20

I am running php v8.0.25 right now..

coudot · 2023-08-16T17:47:06Z

Are the tests in the tests folder actually used?

They should but CI has been disabled since travis changed its community approach.

coudot · 2023-08-16T17:48:26Z

@markus-96 Is it ready for review or do you plan to commit other modifications?

markus-96 · 2023-08-16T20:30:17Z

@markus-96 Is it ready for review or do you plan to commit other modifications?

some functions are not compatible with php7 (type declaration) , will change that. But that is the only thing that comes to my mind right now, will wait for your review!

markus-96 · 2023-08-16T20:40:56Z

Are the tests in the tests folder actually used?

They should but CI has been disabled since travis changed its community approach.

I have tried a bit with github workflows in this branch: https://github.com/markus-96/self-service-password/tree/phpunit-workflow
maybe this comes in handy, could create a new pull request for this or leave it up to you. The branch is a bit messed up, sorry for that

coudot · 2023-08-17T09:21:03Z

will wait for your review!

Thanks, I'll try to work on it next month, the goal is to include your contribution in next release.

coudot · 2023-08-17T09:21:38Z

I have tried a bit with github workflows in this branch: https://github.com/markus-96/self-service-password/tree/phpunit-workflow maybe this comes in handy, could create a new pull request for this or leave it up to you. The branch is a bit messed up, sorry for that

Thanks, I'll have a look.

davidcoutadeur

First, thanks for your contribution.

In my opinion, I think this feature is really not common. Anyway, it can be interesting to add it if we make it more generic. (See my comments in the code)

There is still work to do as you should refactor your code on top of "master" branch and not 1.5, and adapt to the evolution that appeared in the meantime.

Thanks for your work.

davidcoutadeur · 2024-03-06T14:51:53Z

conf/config.inc.php

@@ -399,11 +399,56 @@
 # Smarty debug mode - will popup debug information on web interface
 $smarty_debug = false;

+## App Passwords
+# Change App Passwords
+$change_apppwd = false;


the false type here is not compatible with parsing the array.
You should set a default value equal to empty array

$change_apppwd = [];

is adopted, default is an empty array. The check if this feature is enabled is now if (!empty($change_apppwd))

davidcoutadeur · 2024-03-06T14:52:47Z

conf/config.inc.php

+        if (!isset($change_apppwd[$i]['shadow_options']['shadow_expire_days'])) { $change_apppwd[$i]['shadow_options']['shadow_expire_days'] = -1; }
+    }
+    unset($i);
+}


You should not set these configuration parameters in config.inc.php, but rather in the code.

I asked here: #751 (comment)
and got the answer one for-loop is ok, but of course, I can move it. But setting the label will have to be done on every request, so this has to be in index.php then.

My answer was indeed to remove code from config, so it should be in your apppwd page code

Is one for loop ok for setting the defaults for each app password?

I think so.

Misunderstood that, is moved now to apppwd.php, except for ['label'] and ['msg_changehelpextramessage'], this is moved to index.php.

davidcoutadeur · 2024-03-06T14:58:07Z

docs/config_apppwd.rst

+Background
+----------
+
+This Feature enables you to configure individiual Application Passwords that are 


Some semantics here: why is this feature tagged as "application password".

If I understand the feature correctly, the goal is just to fill a custom password field.

I think we should make this feature more generic.

For example:

$change_apppwd = array( array( 'attribute' => "customPasswordField", 'hash' => "MD5", 'label' => "Custom password" ) )

The feature should be this generic, as far as I can tell. If you are in need for a custom password field, the password is needed for some application, for example WLAN. You want to store the hash in the field "wlanPassword", with an NTLM hash. On the website, you want the menu to include a button "Change password for WLAN".
This results in:

$change_apppwd = array( array( 'attribute' => "wlanPassword", 'hash' => "NTLM", 'label' => "WLAN" ) )

My PR does not provide an application called password. It is meant for changing passwords of certain applications.

Actually, your PR allows to change a custom LDAP field of any object.

In your use case, you name the object an "application", but it could be any other object, including a user, an organization,...

Whether this field (which turns to be a password) is used by 0, 1, or more applications, SSP or LDAP should not be aware of. (moreover it can change with time)

The object is defined by the login name, so it is bound to a user object.

The field can be any attribute, as long as the user object may include the attribute.

Of course, you can use it for changing any other attribute as well, especially when using 'hash' => 'clear'. But this is out of scope for this PR, as I said:

You can use it also for updating user information, but I do not think this would be very user friendly.

The main focus was on the case of needing hashes that are known to be insecure. It is rather odd that you are in need for them for two different use cases, but of course this can happen. But this can be clarified to the user with the label key.

If it is in general that you do not like the term "application" used in the documentation and in the coding, please tell me, I do not know what else I should call it... Maybe Custom Password Field? But how should the web form clarify for what?

These are my thinkings right now, maybe I come to a better solution.

replaced apppwd with custompwdfield, in code and documentation.

These were some groundbreaking changes, please give me some time testing everything if everything is still working as expected.

davidcoutadeur · 2024-03-06T14:59:45Z

docs/config_apppwd.rst

+.. tip:: If you do not set an Arraykey in this configuration, the settings for the
+   main password will be applied!
+
+For more information, follow the steps mentioned below.


please fix the README with more generic terms. (see comment above)

davidcoutadeur · 2024-03-06T15:30:58Z

htdocs/changeapppwd.php

@@ -0,0 +1,225 @@
+<?php


Please refactor your PR on top of the master branch.

This requires some work.

Take care that there have been evolutions since then:

externalize ldap function to \Ltb\Ldap library. see changes in change.php file

adding translations to lang/ files

adding some features (ppolicy criteria)

Also, there are a lot of code duplication here. Maybe we could factorize more things, but currently I don't know how.

thanks, I know. My fork somehow has to get the code of master. It has been a long time since I was looking on this PR.

created a local branch from ltb-project:master and merged it locally to markus-96:1.5, then resolved the conflicts.

I have to delete my fork so that I can fork from master directly, hopefully this PR is closed as soon as possible.

* adding some features (ppolicy criteria)

For entropy checks:
This can not be done between "app"-passwords and the user-password, because on the app-password side you need to know the old password, which you do not. So: While changing the "app"-password, you can check the entropy between "app"-password and user-password, but if you change the user-password, you can not check the entropy to the already set "app"-password. So this check is useless in this aspect.

I will try to adopt the added .js-functionality

should be resolved now

Hello,

Thanks for the taking into consideration my remarks. Globally, it is much better.

Your branch 1.5 and origin/master are in conflict, and furthermore you rebase in the wrong direction. This makes the rebase very complicated, so I preferred to do a global patch from your branch, and apply it to the master branch.

See this issue for referencing the feature: #864
And this new MR for continuing the work: #865

Hello,

Thanks for creating the new branch. But I have some problem:

I can not push to change-custom-password-fields, how should I continue working on this? Should I create a new fork and create PRs for every commit to change-custom-password-fields?

Hello,

Thanks for creating the new branch. But I have some problem:

I can not push to change-custom-password-fields, how should I continue working on this? Should I create a new fork and create PRs for every commit to change-custom-password-fields?

You could clone again or synchronize your personal repository, work on your local change-custom-password-fields branch, and propose a PR to self-service-password change-custom-password-fields branch

davidcoutadeur · 2024-03-06T15:34:18Z

htdocs/index.php

+        $change_apppwd_labels[$i] = array();
+        $change_apppwd_labels[$i]['label'] = $change_apppwd[$i]['label'];
+        $change_apppwd_labels[$i]['msg_changehelpextramessage'] = $change_apppwd[$i]['msg_changehelpextramessage'];
+    }


The password-app button is not displayed like the others, and seems ugly. (see capture)

Is this with the new version of bootstrap? It is not included in my PR yet, it was working with bootstrap v3.4.1, this seams to be bootstrap 5.3.1. Am I right?
Here, it looks like the following:

should be resolved now

davidcoutadeur · 2024-03-06T16:00:06Z

lib/functions.inc.php

+ * @param array $hash_options additional options to be used by the hashing algorithm
+ * @return string
+ */
+function make_password($password, $hash, $hash_options): string {


Typically, I think that these functions should go into https://github.com/ltb-project/ltb-ldap

@coudot what is your opinion on this?

See ltb-project/ltb-common#7

davidcoutadeur · 2024-03-06T16:01:41Z

lib/functions.inc.php

-                }
-            }
-        }
+        $hash = get_hash_type($ldap, $dn, $pwd_attribute);


Same comment as above: I think these functions should go into https://github.com/ltb-project/ltb-ldap

as above: see ltb-project/ltb-common#7

davidcoutadeur · 2024-03-06T16:04:02Z

templates/menu.tpl

+                            ><i class="fa fa-fw fa-terminal"></i> {$msg_menuapppwd|cat:$app.label} </a>
+                        </li>
+                    {/foreach}
+                {/if}


Same comment as above: the password-app button is not displayed like the others, and seems ugly. (see capture)

Same as above, I did not yet adopt it to bootstrap 5.3.1

should be resolved now

still a lot of work to do

This reverts commit 109b926.

in the form, I had to use the same ids as in change.tpl for the ppolicy js to work. Also, I have to reuse the policy config variables.

davidcoutadeur · 2024-03-13T12:16:26Z

I have reworked this PR in #865

See this PR for next discussions and comments.

initial commit for app passwords

9d229f0

coudot added the enhancement label Feb 8, 2023

coudot added this to the 1.6.0 milestone Feb 8, 2023

apppwd policy and messages

e17aa26

markus-96 added 3 commits February 12, 2023 13:01

policy and captcha

cf8497a

default conf

4c13490

Default configuration for apppwds is the one set for the main password.

license, cleaning

5421575

markus-96 added 3 commits February 24, 2023 14:18

clean everything up, minor & major bug fixes

adc6386

initial documentation, untested

4c785ff

Update config_apppwd.rst

21eaec5

markus-96 changed the title ~~[in progress] feature: ability to change app passwords~~ feature: ability to change app passwords Feb 24, 2023

markus-96 marked this pull request as ready for review February 24, 2023 14:40

lang files updated

9fe8617

markus-96 commented Mar 13, 2023

View reviewed changes

Update index.php

683b910

markus-96 and others added 5 commits May 19, 2023 23:57

Merge branch 'ltb-project:1.5' into 1.5

e8e0f3c

get rid of exploding the get-request..

a15f3c0

unknownapp message

ad4c230

make it clear that the requested application is not configured

remove redundant captcha configuration

e115fcd

forgotten echo..

3daec54

check against apppwds, use function change_password, PHPUnit Tests, f…

ff1527c

…actorization

phpunit fix, lang ca reverse change

937dc8a

coudot self-requested a review August 16, 2023 17:47

remove type declaration for compatibility with php7

1050dad

markus-96 mentioned this pull request Feb 27, 2024

Hashing passwords with password reset ltb-project/service-desk#50

Closed

davidcoutadeur self-requested a review March 6, 2024 13:38

davidcoutadeur reviewed Mar 6, 2024

View reviewed changes

rebase

109b926

still a lot of work to do

markus-96 changed the base branch from 1.5 to master March 6, 2024 18:30

coudot assigned davidcoutadeur Mar 7, 2024

Markus Beckschulte added 8 commits March 7, 2024 17:12

Revert "rebase"

a826cf0

This reverts commit 109b926.

Merge branch 'newmaster' into 1.5

57a3a31

fix entropy tests

a90add6

change and move default config

6f2c494

adopt to new ppolicy checks and new bootstrap version

137c50c

in the form, I had to use the same ids as in change.tpl for the ppolicy js to work. Also, I have to reuse the policy config variables.

add translations

19448fd

rename feature

404a70f

some fixes

37b08e0

markus-96 changed the title ~~feature: ability to change app passwords~~ feature: ability to change custom password fields Mar 10, 2024

davidcoutadeur mentioned this pull request Mar 13, 2024

feature: ability to change custom password fields #864

Closed

davidcoutadeur closed this Mar 13, 2024

markus-96 mentioned this pull request Mar 18, 2024

ability to change custom password fields - working PR #871

Closed

feature: ability to change custom password fields #751

feature: ability to change custom password fields #751

Conversation

markus-96 commented Feb 7, 2023

markus-96 commented Feb 7, 2023 • edited Loading

markus-96 commented Feb 11, 2023

markus-96 commented Feb 12, 2023

coudot commented Feb 12, 2023

markus-96 commented Feb 12, 2023

coudot commented Feb 12, 2023

markus-96 commented Feb 25, 2023

coudot commented Feb 25, 2023

Choose a reason for hiding this comment

markus-96 commented Apr 5, 2023

coudot commented Apr 5, 2023

markus-96 commented Aug 12, 2023

coudot commented Aug 16, 2023

coudot commented Aug 16, 2023

markus-96 commented Aug 16, 2023

markus-96 commented Aug 16, 2023

coudot commented Aug 17, 2023

coudot commented Aug 17, 2023

davidcoutadeur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidcoutadeur commented Mar 13, 2024

markus-96 commented Feb 7, 2023 •

edited

Loading