Secure create-user command #127

simonschaufi · 2018-02-02T19:45:14Z

Ask the user for the password

Fixes #123

Ask the user for the password Fixes #123

simonschaufi · 2018-02-02T19:46:56Z

Do you want to have an option to pass the password as parameter? Imho this could only be useful for demo installation or so...

kevinpapst · 2018-02-03T17:25:34Z

Yes please, an additional option for automated installer would be great :-)

kevinpapst

Please add an option to pass it via command line (for script based installations)

kevinpapst · 2018-02-08T20:54:09Z

src/Command/CreateUserCommand.php

@@ -79,9 +79,24 @@ protected function execute(InputInterface $input, OutputInterface $output)
    {
        $io = new SymfonyStyle($input, $output);

+        /* @var \Symfony\Component\Console\Helper\QuestionHelper $helper */


Please extract this part as method - e.g protected function askForPassword()

simonschaufi · 2018-02-10T15:24:07Z

ready to merge

done

kevinpapst · 2018-02-10T15:59:58Z

src/Command/CreateUserCommand.php

+        $helper = $this->getHelper('question');
+
+        $passwordQuestion = new Question('Please enter the password');
+        $passwordQuestion->setHidden(true);


Either we add a repeat question or we display the password. Otherwise its to easy to accidentaly create a user with a wrong password.

using mysqldump works the same way. I'm against it. you can enter 3 times an empty password until it stops.

Then lets leave it for now, can still be improved if user complain

kevinpapst · 2018-02-10T16:02:04Z

README.md

@@ -118,7 +118,7 @@ bin/console cache:warmup --env=prod
 Create your first user:

 ```bash
-bin/console kimai:create-user username password [email protected] ROLE_SUPER_ADMIN
+bin/console kimai:create-user username [email protected] ROLE_SUPER_ADMIN


+bin/console kimai:create-user username [email protected] ROLE_SUPER_ADMIN password

Either you can enter the password as optional last argument or you can enter it interactively for protecting your bash history

I would prefer not to write that in the Readme but in the full documentation. We don't want to encourage people for the first install to append it. For most of the people its just for the installation. then they will continue adding new users via the admin panel anyway.

Fine for me, but currently there is nothing documented. Then don't add the last argument but add a sentence that the user is asked for the password.

kevinpapst · 2018-02-10T16:02:19Z

src/Command/CreateUserCommand.php

+            ->addArgument('username', InputArgument::REQUIRED, 'The username of the user to be created (must be unique)')
+            ->addArgument('email', InputArgument::REQUIRED, 'Email address of the user to be created (must be unique)')
+            ->addArgument('role', InputArgument::OPTIONAL, 'A comma separated list of roles to assign. Examples: "ROLE_USER,ROLE_SUPER_ADMIN"', User::DEFAULT_ROLE)
+            ->addArgument('password', InputArgument::OPTIONAL, 'Password of the user to be created')


Password for the new user

kevinpapst · 2018-02-10T16:03:24Z

composer.lock

@@ -115,16 +115,16 @@
        },
        {
            "name": "beberlei/DoctrineExtensions",
-            "version": "v1.0.20",
+            "version": "v1.0.21",


Why did you run composer update?

why not? if something breaks, I don't want to be responsible for that.

I am against composer update during some random PR. Even though most of the packages have trusted developers behind them and use proper semver versioning, there is still a risk in upgrading packages without retesting the complete app.

so when is the best time to do it? since we have unit tests, we should know when something breaks.

with the current state of the unit tests, there is not a lot of trust. too many tests are using - btw. i will add a test to this PR later

again, lets leave it for now, but we should be careful later on when the app is in use

kevinpapst · 2018-02-10T18:00:37Z

Wanna check my commit?

simonschaufi · 2018-02-10T18:04:27Z

tests/Command/CreateUserCommandTest.php

+
+    protected function createUser($username, $email, $role, $password)
+    {
+


remove empty line

simonschaufi · 2018-02-10T18:04:38Z

tests/Command/CreateUserCommandTest.php

+
+        $command = $this->application->find('kimai:create-user');
+        $commandTester = new CommandTester($command);
+        $commandTester->execute(array(


use short array syntax

simonschaufi · 2018-02-10T18:05:48Z

src/Command/CreateUserCommand.php

-            if (trim($value) == '') {
-                throw new \Exception('The password cannot be empty');
+            $password = trim($value);
+            if (empty($password) || strlen($password) < 6) {


shouldn't we require not at least 8 chars? Well, it has to be in sync as creating users via the admin interface.

Yeah, I don't know. Its the same as in the entity right now. I am not sure if we should force users to use a strong password, which they will then just stick to their monitor ;-)

kevinpapst

Approved, thanks!

lock · 2018-11-18T21:36:11Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Secure create-user command

d4acd2d

Ask the user for the password Fixes #123

simonschaufi assigned kevinpapst Feb 2, 2018

Merge branch 'master' into secure-user-command

b58f177

kevinpapst previously requested changes Feb 8, 2018

View reviewed changes

extract Method

cd554cf

simonschaufi force-pushed the secure-user-command branch from 9623446 to cd554cf Compare February 10, 2018 15:23

kimai deleted a comment from kevinpapst Feb 10, 2018

kevinpapst requested changes Feb 10, 2018

View reviewed changes

simonschaufi and others added 2 commits February 10, 2018 17:20

Add hint

adf8137

added integration test #123

ceffe44

kevinpapst previously approved these changes Feb 10, 2018

View reviewed changes

simonschaufi commented Feb 10, 2018

View reviewed changes

code review #123

0f22c3e

kevinpapst dismissed their stale review via 0f22c3e February 10, 2018 18:09

kevinpapst approved these changes Feb 10, 2018

View reviewed changes

kevinpapst merged commit 93e1cd5 into master Feb 10, 2018

kevinpapst deleted the secure-user-command branch February 10, 2018 18:11

kevinpapst added this to the 0.2 milestone Jun 23, 2018

lock bot locked and limited conversation to collaborators Nov 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure create-user command #127

Secure create-user command #127

simonschaufi commented Feb 2, 2018

simonschaufi commented Feb 2, 2018

kevinpapst commented Feb 3, 2018

kevinpapst left a comment

kevinpapst Feb 8, 2018

simonschaufi commented Feb 10, 2018

kevinpapst Feb 10, 2018

simonschaufi Feb 10, 2018 •

edited

Loading

kevinpapst Feb 10, 2018

kevinpapst Feb 10, 2018

simonschaufi Feb 10, 2018 •

edited

Loading

kevinpapst Feb 10, 2018

kevinpapst Feb 10, 2018

kevinpapst Feb 10, 2018

simonschaufi Feb 10, 2018

kevinpapst Feb 10, 2018

simonschaufi Feb 10, 2018

kevinpapst Feb 10, 2018

kevinpapst commented Feb 10, 2018

simonschaufi Feb 10, 2018

simonschaufi Feb 10, 2018

simonschaufi Feb 10, 2018

kevinpapst Feb 10, 2018

kevinpapst left a comment

lock bot commented Nov 18, 2018


		protected function createUser($username, $email, $role, $password)
		{

Secure create-user command #127

Secure create-user command #127

Conversation

simonschaufi commented Feb 2, 2018

simonschaufi commented Feb 2, 2018

kevinpapst commented Feb 3, 2018

kevinpapst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonschaufi commented Feb 10, 2018

Choose a reason for hiding this comment

simonschaufi Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonschaufi Feb 10, 2018 • edited Loading

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

kevinpapst commented Feb 10, 2018

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

kevinpapst left a comment

Choose a reason for hiding this comment

lock bot commented Nov 18, 2018

simonschaufi Feb 10, 2018 •

edited

Loading

simonschaufi Feb 10, 2018 •

edited

Loading