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

Auth/PM-3275 - Changes to support TDE User without MP being able to Set a Password + misc refactoring #3242

Merged

Conversation

JaredSnider-Bitwarden
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden commented Aug 31, 2023

Type of change

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

Objective

Resolve PM-3275 where a TDE User without a MP who obtains the ability to reset other user passwords must set a password but they could not without these changes.

Associated clients PR here.

Notes for Reviewers

  • This is a large change set. Please reach out if any further clarification needed, and I'll be happy to jump on a call to clarify any of these.

Code changes

Originally, the scope of these this branch was to simply add a new endpoint for retrieving just the Master Password policy options for a user (which is done on the set-password.component client side), but it quickly grew as more changes and refactorings were required to support this new flow while meeting our latest code standards.

New MP Policy Endpoint + Existing Endpoint Deprecation

  • src/Api/Controllers/PoliciesController.cs
    • Add new, secured GetMasterPasswordPolicy(...) endpoint
    • Marked existing, anonymous GetByInvitedUser(...) endpoint as deprecated with associated tech debt ticket to clean it up later
      • We do not need an anonymous endpoint for this logic. In all known scenarios (user JIT provisioned via SSO and TDE user w/out MP), a user will be authenticated.
  • test/Api.Test/Controllers/PoliciesControllerTests.cs
    • There were no tests for the PoliciesController so I created a test file and tested the new GetMasterPasswordPolicy(...) method. I did not test all existing methods as that would be even more scope creep, but now we have a starting point for future tests.

Service to Command refactors per ADR-0008

  • Refactor UserService.SetPasswordAsync(...) into SetInitialMasterPasswordCommand

    • src/Core/Services/Implementations/UserService.cs
    • src/Core/Services/IUserService.cs
      • Remove SetPasswordAsync(...)
      • Convert UpdatePasswordHash(...) from private to public (+ add to interface) for consumption in new SetInitialMasterPasswordCommand
    • src/Core/Auth/UserFeatures/UserMasterPassword/Interfaces/ISetInitialMasterPasswordCommand.cs
      • Create interface for new command
    • src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs
      • Create new command
        • New Behavior: As TDE Users are generally already confirmed when obtaining elevated permissions, we had to add a check which only attempts to accept a user if the user is in the invited status.
    • test/Core.Test/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommandTests.cs
      • Add tests for new command
    • src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs
      • Add new UserServiceCollectionExtensions file which matching existing code patterns and bundles the addition of the existing UserService along with new commands like the SetInitialMasterPasswordCommand
    • src/SharedWeb/Utilities/ServiceCollectionExtensions.cs
      • Leverage new UserServiceCollectionExtensions to add UserService and any user commands
    • src/Api/Controllers/AccountsController.cs
      • Replace usages of _userService.SetPasswordAsync(...) with SetInitialMasterPasswordCommand
    • test/Api.Test/Controllers/AccountsControllerTests.cs
      • Mock SetInitialMasterPasswordCommand and pass to controller constructor
  • Refactor OrganizationService.AcceptUserAsync(...) into AcceptOrgUserCommand

    • As the new SetInitialMasterPasswordCommand leveraged the OrganizationService.AcceptUserAsync(...) method, I continued the command refactoring efforts at @justindbaur's request.
    • src/Core/Services/Implementations/OrganizationService.cs
    • src/Core/Services/IOrganizationService.cs
      • Remove all public overloads for AcceptUserAsync(...) and private helper AcceptUserAsync(...)
    • src/Core/OrganizationFeatures/OrganizationUsers/Interfaces/IAcceptOrgUserCommand.cs
      • Create interface for new command
      • Replace method overloads with deliberately named methods for improved readability / clarity.
    • src/Core/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs
    • test/Core.Test/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommandTests.cs
      • Test all AcceptOrgUser scenarios
    • test/Core.Test/Services/OrganizationServiceTests.cs
    • test/Core.Test/Services/UserServiceTests.cs
    • test/Api.Test/Controllers/OrganizationUsersControllerTests.cs
      • Replace IOrganizationService with IAcceptOrgUserCommand

Org Email Invite Token Creation Process Refactor

While building the AcceptOrgUserCommand.AcceptOrgUserByTokenAsync(...) method, due to the existing DataProtectionProvider implementation, I had to instantiate the command's data protector with the same purpose as the OrganizationService:

_dataProtector = dataProtectionProvider.CreateProtector("OrganizationServiceDataProtector").

In order to be able to decrypt the token included in an org user invite email generated in the OrganizationService, this wasn't ideal as the whole point of the command refactor was to move towards standalone commands. So, I decided to refactor the Org User Invite email token creation process to use our Tokenable framework instead.

This change affects the following flows:

  • Existing User clicks Org User invite email - logs in and is accepted into the org
    • Policies are retrieved on login by token
    • Accept user logic validates token before accepting user
  • New User clicks Org User invite email - registers and is accepted into the org
    • When a self hosted org has disabled open registration, we only allow account registration via email invite (we validate this using the token)
    • Accept user logic validates token before accepting user

Code changes:

  • src/Core/Auth/Models/Business/Tokenables/OrgUserInviteTokenable.cs
    • Created new tokenable for use in org user invite emails
    • Created multiple methods of validation + static methods to reduce code duplication in validating tokens and avoid hitting DB for retrieving an Org User
  • src/Core/Auth/Models/Business/Tokenables/OrgUserInviteTokenableFactory.cs
  • src/Core/Auth/Models/Business/Tokenables/IOrgUserInviteTokenableFactory.cs
    • The old, manually created org user invite email token honored the _globalSettings.OrganizationInviteExpirationHours as part of its validation process in CoreHelpers.UserInviteTokenIsValid(...).
    • In order to achieve feature parity in the new tokenable implementation, I had to create a factory for creating OrgUserInviteTokenables so that the expiration could be set accordingly.
  • test/Core.Test/Auth/Models/Business/Tokenables/OrgUserInviteTokenableTests.cs
    • Add tests for OrgUserInviteTokenable
  • src/Core/Services/Implementations/OrganizationService.cs
    • Refactor manual token creation process to use new OrgUserInviteTokenable
  • src/Core/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs
    • Update AcceptOrgUserByTokenAsync to be backwards compatible with old and new tokens (backwards compatibility is required as invites can already have been emailed out when this server change goes live).
    • Added PM-4142 for cleaning up backwards compatibility in future.
  • src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs
    • Register new command in services.
  • src/Api/Controllers/PoliciesController.cs
    • Update GetByToken(...) to have new token and old token validation
  • src/Core/Services/Implementations/UserService.cs
    • Update RegisterUserAsync(...) to have new token and old token validation
  • test/Common/Fakes/FakeDataProtectorTokenFactory.cs
    • Create fake implementation of the IDataProtectorTokenFactory which can be used to mock validation of any tokenable
  • test/Core.Test/Services/OrganizationServiceTests.cs
    • Update OrganizationServiceTests to reflect use of new OrgUserInviteTokenable

Org Invite working with old token:

PM-3275.-.Org.Invite.Refactor.Backwards.compatibility.proof.with.old.token.mov

Org Invite working with new token:

PM-3275.-.Org.Invite.working.with.new.token.mov

AccountsController PostSetPasswordAsync(...) Refactor to support TDE user setting a password

  • src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs
    • Refactor SetPasswordRequestModel to allow user asymmetric keys to be null; a TDE user will have already generated a user asymmetric key pair, so we should allow initial password setting without requiring new user keys.
  • src/Api/Controllers/AccountsController.cs
    • Add new lines
  • test/Api.Test/Controllers/AccountsControllerTests.cs
    • Add tests for PostSetPasswordAsync(...) method to guarantee that the change to allow null will not break anything

Misc Refactors

  • src/Core/Tokens/ExpiringTokenable.cs
    • Add summary documentation to clarify the intents behind the ExpiringTokenable class props / methods.
  • src/Core/Auth/Models/Business/Tokenables/SsoEmail2faSessionTokenable.cs
    • Removed unnecessary new lines

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • 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

…uthenticated clients to get an enabled MP org policy if it exists for the purposes of enforcing those policy requirements when setting a password.
…UserService.setPasswordAsync into new SetInitialMasterPasswordCommand (2) Refactor SetInitialMasterPasswordCommand to only accept post SSO users who are in the invited state

(3) Add TODOs for more cleanup work and more commands
@bitwarden-bot
Copy link

bitwarden-bot commented Aug 31, 2023

Logo
Checkmarx One – Scan Summary & Details709e9dcd-7ea7-4f5d-a3b2-b6e6c87d2f71

No New Or Fixed Issues Found

…izationService.cs AcceptUserAsync method so while I was in here I converted the AcceptUserAsync methods into a new AcceptOrgUserCommand.cs and turned the private method which accepted an existing org user public for use in the SetInitialMasterPasswordCommand
…d-get-mp-policy-endpoint

# Conflicts:
#	src/Api/Controllers/OrganizationUsersController.cs
#	src/Core/Services/IOrganizationService.cs
#	src/Core/Services/Implementations/OrganizationService.cs
…gId to retrieve orgUser with _organizationUserRepository.GetByOrganizationAsync which gets a single user instead of a collection
… to provide public static GetTokenLifeTime() method for testing (2) Add missed tests to SsoEmail2faSessionTokenable in preparation for building tests for new OrgUserInviteTokenable.cs
… tests as I've handled that separately in a new PR (#3270) for newly created task PM-3925
…cy-endpoint' into auth/pm-3275/tde-add-get-mp-policy-endpoint
…inguish between the Valid property and the TokenIsValid method.
…o use new OrgUserInviteTokenable instead of manual creation of a token
… to add new token validation while maintaining backwards compatibility for 1 release.
…d-get-mp-policy-endpoint

# Conflicts:
#	src/Core/Services/Implementations/UserService.cs
#	src/SharedWeb/Utilities/ServiceCollectionExtensions.cs
#	test/Core.Test/Services/UserServiceTests.cs
r-tome
r-tome previously approved these changes Oct 31, 2023
…d-get-mp-policy-endpoint

# Conflicts:
#	src/Core/Services/Implementations/OrganizationService.cs
@JaredSnider-Bitwarden
Copy link
Contributor Author

Passed QA. Awaiting final stamps for merging!

@JaredSnider-Bitwarden JaredSnider-Bitwarden merged commit ee61832 into master Nov 2, 2023
41 checks passed
@JaredSnider-Bitwarden JaredSnider-Bitwarden deleted the auth/pm-3275/tde-add-get-mp-policy-endpoint branch November 2, 2023 15:02
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