Skip to content
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

Feature: Forgot password #9509

Merged
merged 9 commits into from
Sep 10, 2024
Merged

Feature: Forgot password #9509

merged 9 commits into from
Sep 10, 2024

Conversation

vishesh92
Copy link
Member

@vishesh92 vishesh92 commented Aug 9, 2024

Docs PR: apache/cloudstack-documentation#432

Description

This PR adds the functionality to reset password for a user by email.

8 Global settings:

Category Type Name Default value Description Dynamic
Advanced Long password.reset.ttl 30 Password reset ttl in minutes Yes
Advanced String password.reset.email.sender null Password reset email sender Yes
Advanced String password.reset.smtp.host null Password reset smtp host No
Advanced Integer password.reset.smtp.port 25 Password reset smtp port No
Advanced Boolean password.reset.smtp.useAuth False Use auth for smtp in Password reset No
Advanced String password.reset.smtp.username null Password reset smtp username No
Secure String password.reset.smtp.password null Password reset smtp password No
Advanced String password.reset.mail.template Hello {{username}}!\nYou have requested to reset your password. Please click the following link to reset your password:\n{{{reset_link}}}\nIf you did not request a password reset, please ignore this email.\n\nRegards,\nThe CloudStack Team Password reset mail template. This uses mustache template engine. Available variables are: username, firstName, lastName, resetLink, token Yes

2 new APIs:

  1. Command: forgotPassword
    Params: username, domain
    Details: Sends an email to the user with a token which can be used to reset the password using resetPassword command.
  2. Command: resetPassword
    Params: username, domain, token, password
    Details: Resets the password for the user using the token generated via forgotPassword command.

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)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Screenshots (if appropriate):

How Has This Been Tested?

  1. Setup a mailserver or for testing purposes use the below command to setup maildev using docker
docker run -d -p 1080:1080 -p 1025:1025 maildev/maildev
  1. Update the above global settings to point the smtp host to the above docker container and restart the management server.
  2. Ensure an email is set for the test user
  3. In UI, click on forgot password and enter details.
  4. maildev runs a web ui to view all emails being sent. Open maildev's UI which is running on port 1080 and see if there are any emails.
  5. There should be an email from cloudstack to reset the password for the user. Click on the link in the email to reset the password or copy the token.
  6. Open the link in the email or navigate to {ACS MS}/user/resetPassword on the UI.
  7. Enter the details along with token from the email. Set the new password.
  8. Try login with the newly set password.

How did you try to break this feature and the system with this change?

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 28.30769% with 233 lines in your changes missing coverage. Please review.

Project coverage is 15.76%. Comparing base (8c8d115) to head (b3ffbf3).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
.../cloudstack/user/UserPasswordResetManagerImpl.java 41.84% 79 Missing and 3 partials ⚠️
.../auth/DefaultResetPasswordAPIAuthenticatorCmd.java 0.00% 69 Missing ⚠️
...auth/DefaultForgotPasswordAPIAuthenticatorCmd.java 0.00% 63 Missing ⚠️
server/src/main/java/com/cloud/api/ApiServer.java 60.60% 12 Missing and 1 partial ⚠️
...m/cloud/api/auth/APIAuthenticationManagerImpl.java 0.00% 3 Missing ⚠️
...ma/src/main/java/com/cloud/user/UserAccountVO.java 0.00% 2 Missing ⚠️
...c/main/java/com/cloud/user/AccountManagerImpl.java 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##               main    #9509     +/-   ##
===========================================
  Coverage     15.76%   15.76%             
- Complexity    12510    12524     +14     
===========================================
  Files          5621     5627      +6     
  Lines        491469   491884    +415     
  Branches      62967    60293   -2674     
===========================================
+ Hits          77460    77544     +84     
- Misses       405553   405878    +325     
- Partials       8456     8462      +6     
Flag Coverage Δ
uitests 4.03% <ø> (-0.02%) ⬇️
unittests 16.58% <28.30%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vishesh92 vishesh92 force-pushed the forgot-password branch 2 times, most recently from f24c534 to 61f676c Compare August 9, 2024 10:03
@vishesh92 vishesh92 force-pushed the forgot-password branch 3 times, most recently from 092390b to 0fc1cea Compare August 11, 2024 15:51
@apache apache deleted a comment from blueorangutan Aug 11, 2024
@apache apache deleted a comment from blueorangutan Aug 11, 2024
@apache apache deleted a comment from blueorangutan Aug 11, 2024
@apache apache deleted a comment from blueorangutan Aug 12, 2024
@apache apache deleted a comment from blueorangutan Aug 12, 2024
@apache apache deleted a comment from blueorangutan Aug 13, 2024
@apache apache deleted a comment from blueorangutan Aug 13, 2024
@vishesh92
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10631

@vishesh92
Copy link
Member Author

@blueorangutan test

@blueorangutan
Copy link

@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-11068)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 53619 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9509-t11068-kvm-ol8.zip
Smoke tests completed. 138 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_redundant_vpc_site2site_vpn Failure 457.46 test_vpc_vpn.py
test_01_vpc_site2site_vpn Failure 320.12 test_vpc_vpn.py

@vishesh92
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11028

@vishesh92
Copy link
Member Author

@JoaoJandre This feature is behind a flag to enable or disable the password recovery workflow. By default, it's disabled. Can we include this PR in 4.20 release as well?

@vishesh92 vishesh92 marked this pull request as ready for review September 6, 2024 11:40
@vishesh92
Copy link
Member Author

@blueorangutan test

@blueorangutan
Copy link

@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@JoaoJandre
Copy link
Contributor

@JoaoJandre This feature is behind a flag to enable or disable the password recovery workflow. By default, it's disabled. Can we include this PR in 4.20 release as well?

@vishesh92 same as #9633 (comment) :)

@blueorangutan
Copy link

[SF] Trillian Build Failed (tid-11418)

@blueorangutan
Copy link

[SF] Trillian Build Failed (tid-11420)

@vishesh92
Copy link
Member Author

@blueorangutan test

@blueorangutan
Copy link

@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian Build Failed (tid-11421)

@vishesh92
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11050

@vishesh92
Copy link
Member Author

@blueorangutan test

@blueorangutan
Copy link

@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@borisstoyanov
Copy link
Contributor

@blueorangutan help

@blueorangutan
Copy link

@borisstoyanov [SL] I understand these words: "help", "hello", "thanks", "package", "test"
Test command usage: test [mgmt os] [hypervisor] [keepEnv] [qemuEv] [basicZone|securityGroups]
Mgmt OS options: ['ol8', 'ol9', 'alma8', 'alma9', 'suse15', 'centos7', 'centos6', 'rocky8', 'ubuntu18', 'ubuntu22', 'ubuntu20', 'ubuntu24']
Hypervisor options: ['kvm-centos6', 'kvm-centos7', 'kvm-rocky8', 'kvm-ol8', 'kvm-ol9', 'kvm-alma8', 'kvm-alma9', 'kvm-ubuntu18', 'kvm-ubuntu20', 'kvm-ubuntu22', 'kvm-ubuntu24', 'kvm-suse15', 'vmware-55u3', 'vmware-60u2', 'vmware-65u2', 'vmware-67u3', 'vmware-70u1', 'vmware-70u2', 'vmware-70u3', 'vmware-80', 'vmware-80u1', 'vmware-80u2', 'vmware-80u3', 'xenserver-65sp1', 'xenserver-71', 'xenserver-74', 'xcpng74', 'xcpng76', 'xcpng80', 'xcpng81', 'xcpng82']
Note: when keepEnv is passed, you need to specify mgmt server os and hypervisor or use the matrix command.
when qemuEv is passed, it will deploy KVM hyperviosr hosts with qemu-kvm-ev, else it will default to stock qemu.
When basicZone and/or securityGroups are passed it will create a zone of the last type specified (default is Advanced)
Package command usage: package [all(default value),kvm,xen,vmware,hyperv,ovm] - a comma separated list can be passed with package command to bundle the required hypervisor's systemVM templates. Not passing any argument will bundle all - kvm,xen and vmware templates.

Blessed contributors for kicking Trillian test jobs: ['rohityadavcloud', 'shwstppr', 'vishesh92', 'Pearl1594', 'harikrishna-patnala', 'nvazquez', 'DaanHoogland', 'weizhouapache', 'borisstoyanov', 'vladimirpetrov', 'kiranchavala', 'andrijapanicsb', 'NuxRo', 'rajujith', 'alexandremattioli', 'sureshanaparti', 'abh1sar']

@blueorangutan
Copy link

[SF] Trillian test result (tid-11422)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 52163 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9509-t11422-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_secure_vm_migration Error 134.18 test_vm_life_cycle.py
test_01_secure_vm_migration Error 134.18 test_vm_life_cycle.py

Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, manually tested it.

@rohityadavcloud
Copy link
Member

@JoaoJandre @DaanHoogland are we accepting feature PRs now? This is ready for merging and has been reviewed & manually tested.

@JoaoJandre
Copy link
Contributor

LGTM, manually tested it.

@borisstoyanov Could you share what tests were done?

@borisstoyanov
Copy link
Contributor

sure @JoaoJandre, both positive/negative scenarios, as well with different sub-domains accounts. Also verified the token cannot be used to change other users pass and basic user discovery prevention where prompts a generic response either having matching user or not. Also there were some polishing comments for the email, it now uses html format and there is a text with hyperlink rather than raw hyperlink as before. I didn't see any issues, have you experienced any?

@JoaoJandre
Copy link
Contributor

sure @JoaoJandre, both positive/negative scenarios, as well with different sub-domains accounts. Also verified the token cannot be used to change other users pass and basic user discovery prevention where prompts a generic response either having matching user or not. Also there were some polishing comments for the email, it now uses html format and there is a text with hyperlink rather than raw hyperlink as before. I didn't see any issues, have you experienced any?

@borisstoyanov I have not tested the PR. But it is generally good practice to list the tests done and their steps. Like the PR description does.

@JoaoJandre JoaoJandre merged commit 0655075 into apache:main Sep 10, 2024
26 checks passed
@DaanHoogland DaanHoogland deleted the forgot-password branch September 11, 2024 07:23
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Sep 23, 2024
* Feature: Forgot password

* Address comments

* fixups

* Make forgot password disabled by default

* Apply suggestions from code review

* Address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants