[CLOUDSTACK-5235] ask users current password when they are executing a password update by rafaelweingartner · Pull Request #2574 · apache/cloudstack

rafaelweingartner · 2018-04-16T13:43:17Z

Description

This is a long standing issue in Jira. It was asked for us to re-authenticate users with their old password when users execute a password update.

One thing to note though. Root/domain admins are able to update users password (users they have access to) without needing to provide the old password.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

Locally and via unit tests for the newly introduced code.

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

@blueorangutan package

blueorangutan · 2018-04-16T13:47:07Z

@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-04-16T14:03:58Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1933

rafaelweingartner · 2018-04-16T16:45:55Z

@blueorangutan package

blueorangutan · 2018-04-16T16:48:07Z

@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-04-16T17:16:09Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1934

borisstoyanov · 2018-04-17T07:42:59Z

@blueorangutan test

blueorangutan · 2018-04-17T07:44:07Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

borisstoyanov · 2018-04-17T08:19:00Z

@blueorangutan test

blueorangutan · 2018-04-17T08:20:28Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

nitin-maharana · 2018-04-17T10:43:08Z

api/src/main/java/com/cloud/user/AccountService.java


    /**
     * Creates a new user and account, stores the password as is so encrypted passwords are recommended.
-     *


@rafaelweingartner, Thanks for the fix. I think it's better to keep the comments, would be helpful in the documentation. Would be great if you describe a bit about each parameter.

Well, I do like Java documentation; however, in this case, the use of @param does not bring benefits. I mean, if each one of the parameters was fully described it would be ok, but that is not what happened here. That is why I removed them.

I am sorry Nitin, but I would rather not document those parameters. Instead, we should get rid of these methods with a lot of parameters. My changes affect only the updateUser method (I already refactored that whole method). Afterwards, I executed a cleanup in the classes that I had modified some code.

nitin-maharana · 2018-04-17T10:48:11Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

+     *
+     * If all checks pass, we encode the given password with the most preferable password mechanism given in {@link #_userPasswordEncoders}.
+     */
+    protected void validateUserPasswordAndUpdateIfNeeded(String password, UserVO user, String oldPassword) {


I think newPassword name would be more meaningful.

Do you mean validateUserNewPasswordAndUpdateIfNeeded?

I mean the first parameter (password -> newPassword)

nitin-maharana · 2018-04-17T11:32:11Z

ui/scripts/accounts.js

+                                                            label: 'label.old.password',
+                                                            isPassword: true,
+                                                            validation: {
+                                                                required: !(isAdmin() || isDomainAdmin())


As we are hiding this field for admin and domain admin. I think we can assign the value true for the required parameter.

I thought about that, but I was under the impression that the field would still be validated by the javascript framework we have. I could not find a hide parameter. Therefore, I am hiding it manually.

nitin-maharana · 2018-04-17T11:40:17Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

+            throw new InvalidParameterValueException("You must inform the old password when updating a user password.");
+        }
+        if (CollectionUtils.isEmpty(_userPasswordEncoders)) {
+            throw new CloudRuntimeException("No user authenticatiors configured!");


Typo. (authenticators)

Fixed, thanks!

nitin-maharana · 2018-04-17T16:40:52Z

Thanks, @rafaelweingartner for making the changes, I will review rest of the changes by tomorrow or day after.

rafaelweingartner · 2018-04-17T16:41:51Z

@nitin-maharana thank you for the review ;)
Thanks!

blueorangutan · 2018-04-18T09:59:48Z

Trillian test result (tid-2516)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 89580 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2574-t2516-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_restart_network_wo_cleanup	`Failure`	4.15	test_routers.py
test_01_redundant_vpc_site2site_vpn	`Failure`	325.35	test_vpc_vpn.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	0.99	test_hostha_kvm.py

…sword

rafaelweingartner · 2018-04-24T15:09:22Z

@borisstoyanov the errors here do not seem to be related to the changes introduced. Can I consider the test successful?

resmo · 2018-04-25T08:15:49Z

api/src/main/java/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java

            acceptedOnAdminPort = false)
    private String password;

+    @Parameter(name = ApiConstants.OLD_PASSWORD, type = CommandType.STRING, description = "Old password that was being used by the user. You must inform the old password when updating the password.", acceptedOnAdminPort = false)


I would prefer the word "current" instead of "old*

That is a good suggestion. Done.

resmo · 2018-04-25T12:34:24Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

-        if (!isAdmin && StringUtils.isBlank(oldPassword)) {
-            throw new InvalidParameterValueException("You must inform the old password when updating a user password.");
+        if (!isAdmin && StringUtils.isBlank(currentPassword)) {
+            throw new InvalidParameterValueException("You must inform the current password when updating a user password.");


"To set a new password the current password must be provided."

Done.
Thanks for the review!

resmo · 2018-04-25T12:34:34Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

-        if (!oldPasswordMatchesDataBasePassword) {
-            throw new InvalidParameterValueException("Old password does not match the database password.");
+        if (!currentPasswordMatchesDataBasePassword) {
+            throw new InvalidParameterValueException("Current password informed does not match the database password.");


I would prefer "Current password is incorrect."

Done.
Thanks for the second pair of eyes here.

GabrielBrascher

LGTM!

DaanHoogland · 2018-04-28T09:39:34Z

@nitin-maharana are you fine with these changes now?

nitin-maharana · 2018-05-01T07:29:17Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

+    }
+
+    /**
+     * Iterates over all configured user authenticators and tries to authenticated the user using the current password.


TYPO: authenticated -> authenticate.

Done.

Thanks for spotting this mistake.

nitin-maharana · 2018-05-01T07:56:02Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

-                                                                                        // to
-                                                                                        // update
-                                                                                        // itself
+            // same


Would be better if we combine all the words in a single line.

Well, these were already there. Probably the lines shifted because of my changes, but I can fix it without problems.

Side note: I do not appreciate code comments. They are a sign that the method is too big and complicated. This means that code should be extracted to smaller methods and then documented.

nitin-maharana

Code LGTM!

resmo

LGTM

blueorangutan · 2018-05-02T12:19:49Z

@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-05-02T12:49:38Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1995

Adição de informação do endereço IP da SSVM na rede de _storage_ Closes apache#2574 See merge request scclouds/scclouds!1182

rafaelweingartner changed the title ~~[CLOUDSTACK-5235] ask users olf password when they are executing a password update~~ [CLOUDSTACK-5235] ask users old password when they are executing a password update Apr 16, 2018

rafaelweingartner self-assigned this Apr 16, 2018

rafaelweingartner added this to the 4.12.0.0 milestone Apr 16, 2018

rafaelweingartner mentioned this pull request Apr 16, 2018

Fix Python code checkstyle execute by "systemvm\test\runtests.sh" #2576

Merged

12 tasks

nitin-maharana reviewed Apr 17, 2018

View reviewed changes

rafaelweingartner force-pushed the CLOUDSTACK-5235 branch from 46b4670 to 75ebd00 Compare April 17, 2018 12:25

rafaelweingartner added 4 commits April 18, 2018 13:15

[CLOUDSTACK-5235] Force users to enter old password when updating pas…

5989267

…sword

Formatting for checkstyle

686f886

Remove an unused import in AccountManagerImpl

f7f1079

Apply Nitin's suggestions

6f0dde6

rafaelweingartner force-pushed the CLOUDSTACK-5235 branch from 75ebd00 to 6f0dde6 Compare April 18, 2018 16:15

rafaelweingartner added type:enhancement component:management-server type:security labels Apr 20, 2018

resmo reviewed Apr 25, 2018

View reviewed changes

Change 'oldPassword' to 'currentPassword'

0b039bf

resmo reviewed Apr 25, 2018

View reviewed changes

Second review of Resmo

31c10c7

GabrielBrascher approved these changes Apr 25, 2018

View reviewed changes

rafaelweingartner changed the title ~~[CLOUDSTACK-5235] ask users old password when they are executing a password update~~ [CLOUDSTACK-5235] ask users current password when they are executing a password update Apr 26, 2018

nitin-maharana reviewed May 1, 2018

View reviewed changes

nitin-maharana approved these changes May 1, 2018

View reviewed changes

Fix typos found by Nitin

c86ccb2

resmo approved these changes May 1, 2018

View reviewed changes

rafaelweingartner merged commit 3adc2b8 into apache:master May 2, 2018

yadvr mentioned this pull request May 3, 2018

Backports for 4.11 branch #2621

Merged

12 tasks

Conversation

rafaelweingartner commented Apr 16, 2018

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

blueorangutan commented Apr 16, 2018

Uh oh!

blueorangutan commented Apr 16, 2018

Uh oh!

rafaelweingartner commented Apr 16, 2018

Uh oh!

blueorangutan commented Apr 16, 2018

Uh oh!

blueorangutan commented Apr 16, 2018

Uh oh!

borisstoyanov commented Apr 17, 2018

Uh oh!

blueorangutan commented Apr 17, 2018

Uh oh!

borisstoyanov commented Apr 17, 2018

Uh oh!

blueorangutan commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitin-maharana commented Apr 17, 2018

Uh oh!

rafaelweingartner commented Apr 17, 2018

Uh oh!

blueorangutan commented Apr 18, 2018

Uh oh!

rafaelweingartner commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Apr 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

rafaelweingartner May 1, 2018 •

edited

Loading