[Tech debt] Refactor authService and remove LogInHelper

[eliykat]

Type of change

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

Objective

AuthService is a bit of a mess because the private logInHelper method contains code used by every single type of authentication (e.g. MP, SSO, API, 2FA). This makes it extremely difficult to reason about what type of authentication calls what code, where new code should go, and when code can be removed.

This refactor aims to improve this by:

• separating out code that is used by all login methods and code that is only required by particular login methods
• simplifying public and private interfaces (in particular, reducing the number of nullable/optional parameters)
• moving code into other services where appropriate

Code changes

Peripheral changes

Here are the smaller/peripheral changes you should understand first.

Models:

• make tokenRequest an abstract base class
  • add subclasses for the different types of requests: passwordTokenRequest, apiTokenRequest, ssoTokenRequest. Extract specific code into the subclasses.
  • group two factor data into an object with a defined interface: TokenRequestTwoFactor (defined in the base tokenRequest file)
• authResult: add getters. Remove the twoFactor property because we can calculate this information rather than storing it.

Other services:

• KeyConnectorService: this now has the code that enrols a new SSO user in Key Connector, which used to be handled by authService. This could likely be refactored/combined with migrateUser (which handles converting existing users), but that's out of scope for now.
• TwoFactorService: a new service which handles the two factor-related operations that were previously in AuthService. This reduces the amount that AuthService is responsible for given that it's already quite complex.
• TokenService: this interface did not match the implementation. I assume this is an oversight from account switching. I've updated the interface accordingly.
• ApiService: update to use the new tokenService interface. Remove the null coalescing when creating the identity token string, this is handled by the subclasses.

CLI

• login.command.ts: this used to be a 6-pronged conditional, which called either the standard login methods (LogIn, LogInApi, LogInSso) if no 2FA was provided, or the *Complete version of those methods if 2FA was provided. This has been simplified to just call those 3 standard login methods, and they will handle whether or not 2FA is provided.
  • this is begging to be broken up into subclasses, but that's out of scope this time
• Note: CLI is broken at the moment due to account switching, so I still need to do some basic testing.

Angular components:

• update to use new interfaces and services

AuthService changes

• as mentioned above:
  • strip out Key Connector logic into that service
  • strip out 2FA methods into that new service
  • remove LogInComplete, LogInSsoComplete, and LogInApiComplete. These were called by the CLI client if the user provided 2FA info upfront. Now we use buildTwoFactor to handle those situations, and we don't need separate entry methods.
• extract the logic for performing a login into 3 new classes: apiLogInDelegate, passwordLogInDelegate and ssoLogInDelegate, plus their common base class. The base class has the common logic and the subclasses provide their own implementations as required, depending on the login method used. This is where the LogInHelper code has been moved to.
• authService is now only responsible for creating and initializing the correct delegate class and asking it to start the login process. It's essentially the factory for the different implementations.

I don't know if "delegate" is the correct verbiage here, but it's neither a service (which are long-lived and initialized during bootstrapping) nor a model (which only manipulate data and shouldn't have service dependencies). I've stuck them in the misc folder for this reason.

Tests

Add tests. My first tests were very broad and contained lots of overlapping assertions, just as guardrails while I was refactoring. Once I finished refactoring, I rewrote them to test the specific behaviour that is expected of each delegate.

Other clients

• cli: Update client for authService refactor cli#448
• desktop: Update client for authService refactor desktop#1239
• web: Update client for authService refactor web#1387
• browser: TODO once we can update jslib
• directory-connector: [Tech debt] Refactor authService directory-connector#213 (note this has some real changes, not just dependency updates)

Testing requirements

This will require really comprehensive regression testing of all login methods and combinations. Here are the code paths I can think of that I've touched:

• Logging in with a Master Password
• Logging in with an Api Key (CLI only)
• Logging in with an Organization Api Key (directory connector, gui and cli)
• Logging in with SSO:
  • existing user
  • new user provisioning flow (i.e. user does not have a Bitwarden account yet)
  • existing user with Key Connector
  • new user provisioning flow with Key Connector
• Different 2FA combinations:
  • no 2FA
  • Providing 2FA at the same time as main credentials (CLI only, when you pass it as an argument)
  • Providing 2FA as part of the normal two-step prompt
  • using a saved 2FA token ("remember me")
• CAPTCHA verification

Before you submit

• I have checked for linting errors (npm run lint) (required)
• I have added unit tests where it makes sense to do so (encouraged but not required)
• This change requires a documentation update (notify the documentation team)
• This change has particular deployment requirements (notify the DevOps team)

[eliykat]

@MGibson1 I think I've now refactored this to within an inch of its life. Summary of further changes:

1. rename delegate to strategy
2. remove init and static factory method in each Strategy. They now take the credentials directly into their logIn method
3. get rid of all the primitive arguments and pass the credentials as objects. This lets us have a single logIn method inauthService, which can switch between strategies depending on the type of credentials received.
4. resolved your comment above

I'm happier with how this is looking now.

[MGibson1]

I'm going to tag this with a hold until we can confirm with QA on merging

[eliykat]

rc has been cut so I'm merging this.