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

[PM-3797 Part 2] Create Account Recovery Service #6667

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

jlf0dev
Copy link
Member

@jlf0dev jlf0dev commented Oct 23, 2023

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Consolidates account recovery logic into a service shared between components.

Part 2 of a PR stack intended in refactoring the key rotation logic.
Part 1 (Emergency Access): #6612

Note: After some feedback from AC, I've renamed AccountRecoveryService to OrganizationUserResetPasswordService in the next PR. Although we will eventually migrate the name to account recovery, doing it in these changes would leave it overall in a half migrated and confusing state.

Code changes

  • account-recovery.service.ts: New service to consolidate account recovery logic
  • people.component.ts: Pass ViewContainerRef to the ResetPasswordComponent so it can resolve the AccountRecoveryService as a dependency.

Screenshots

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@jlf0dev jlf0dev force-pushed the auth/pm-3797/emergency-access-service branch from 7bab555 to cde3ce3 Compare October 25, 2023 12:49
@jlf0dev jlf0dev force-pushed the auth/pm-3797/admin-recovery-service branch from e42a67e to b5d6e61 Compare October 25, 2023 12:55
@jlf0dev jlf0dev force-pushed the auth/pm-3797/emergency-access-service branch from cde3ce3 to 48ab69b Compare October 25, 2023 18:32
@jlf0dev jlf0dev force-pushed the auth/pm-3797/admin-recovery-service branch from b5d6e61 to 45345cb Compare October 25, 2023 18:33
@jlf0dev jlf0dev marked this pull request as ready for review October 31, 2023 16:21
@jlf0dev jlf0dev requested review from a team as code owners October 31, 2023 16:21
@jlf0dev jlf0dev requested review from vincentsalucci and ike-kottlowski and removed request for a team October 31, 2023 16:21
ike-kottlowski
ike-kottlowski previously approved these changes Oct 31, 2023
vincentsalucci
vincentsalucci previously approved these changes Nov 1, 2023
@jlf0dev jlf0dev force-pushed the auth/pm-3797/emergency-access-service branch from 7f77d3b to bd3d171 Compare November 2, 2023 12:44
@jlf0dev jlf0dev requested review from a team as code owners November 2, 2023 12:44
@jlf0dev jlf0dev force-pushed the auth/pm-3797/admin-recovery-service branch from 7a25bfe to c179925 Compare November 2, 2023 14:47
Base automatically changed from auth/pm-3797/emergency-access-service to master November 8, 2023 21:03
@jlf0dev jlf0dev dismissed stale reviews from vincentsalucci and ike-kottlowski November 8, 2023 21:03

The base branch was changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a module? Can't we directly provide AccountRecoveryService in a consuming component's providers? Even better, can AccountRecoveryService just be providedIn: "root"?

@jlf0dev jlf0dev force-pushed the auth/pm-3797/admin-recovery-service branch from 96c44a0 to 82a5755 Compare November 13, 2023 21:34
@bitwarden-bot
Copy link

bitwarden-bot commented Nov 13, 2023

Logo
Checkmarx One – Scan Summary & Detailsfd42375c-b851-41e8-a20c-024faca63aa8

New Issues

Severity Issue Source File / Package Checkmarx Insight
LOW Use_Of_Hardcoded_Password /apps/web/src/app/admin-console/organizations/members/services/account-recovery/account-recovery.service.spec.ts: 112 Attack Vector
LOW Use_Of_Hardcoded_Password /apps/web/src/app/admin-console/organizations/members/services/account-recovery/account-recovery.service.spec.ts: 112 Attack Vector

@willmartian willmartian dismissed their stale review November 20, 2023 00:54

My previous review was on code that was removed by the force push

@jlf0dev jlf0dev force-pushed the auth/pm-3797/admin-recovery-service branch from 82a5755 to 4417214 Compare November 28, 2023 17:40
Copy link
Member Author

jlf0dev commented Nov 28, 2023

eliykat
eliykat previously approved these changes Nov 28, 2023
ike-kottlowski
ike-kottlowski previously approved these changes Nov 29, 2023
@jlf0dev jlf0dev dismissed stale reviews from ike-kottlowski and eliykat via 9620161 November 30, 2023 22:24
@jlf0dev jlf0dev force-pushed the auth/pm-3797/admin-recovery-service branch from 953e7d4 to 9620161 Compare November 30, 2023 22:24
@jlf0dev jlf0dev force-pushed the auth/pm-3797/admin-recovery-service branch from 9620161 to 2c4a5d6 Compare November 30, 2023 22:32
@jlf0dev jlf0dev merged commit 641ae84 into master Dec 1, 2023
23 checks passed
@jlf0dev jlf0dev deleted the auth/pm-3797/admin-recovery-service branch December 1, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants