[PM-3000] Add Environment URLs to Account Switcher

[rr-bw]

Type of change

• New feature development

Objective

Add the environment URL to the account switcher active active tab and dropdown accounts

Code changes

• apps/desktop/src/app/layout/account-switcher.component.html
  • Add server URL display for active account and inactive accounts
• apps/desktop/src/app/layout/account-switcher.component.ts
  • Retrieve server URLs for active and inactive accounts
  • Refactor SwitcherAccount class to InactiveAccount type
• libs/common/src/platform/services/environment.service.ts
  • add getHost() method to retrieve host for US, EU, or self-hosted environments
• apps/desktop/src/scss/header.scss
  • Update header styles

Video and Screenshot

Screen.Recording.2023-10-12.at.11.18.03.AM.mov

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – 4a981a08-ec99-4091-a56f-2a9822d1c2bc

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Client_Password_In_Comment	/libs/angular/src/auth/components/sso.component.ts: 280	Attack Vector
	Use_Of_Hardcoded_Password	/apps/desktop/src/platform/preload.ts: 25	Attack Vector
	Use_Of_Hardcoded_Password	/apps/desktop/src/platform/preload.ts: 23	Attack Vector
	Use_Of_Hardcoded_Password	/apps/desktop/src/platform/preload.ts: 21	Attack Vector
	Use_Of_Hardcoded_Password	/apps/desktop/src/platform/preload.ts: 19	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	Client_Password_In_Comment	/libs/angular/src/auth/components/sso.component.ts: 213
	Use_Of_Hardcoded_Password	/libs/importer/spec/securesafe-csv-importer.spec.ts: 41
	Use_Of_Hardcoded_Password	/libs/importer/spec/securesafe-csv-importer.spec.ts: 20

[jlf0dev]

Looks good! Just had a small suggestion but its non-blocking

[Hinton]

We seem to be directly modifying accounts state in a component. This shouldn't happen, any such action must go through the state service, but it's confusing in the first place why showing accounts would require that they be modified.

[Hinton]

Apologies for my previous comments they were quite abrupt.

I believe the account switcher has gone through a series of unfortunate refactors coupling it tighter and tighter to various states. This is a significant code smell and I believe this PR worsens the situation by loading the server config from state.

The StateService should be viewed as an internal services that other DomainServices can rely on to store and fetch their domain. Inter-domain fetching should be strictly forbidden. This means DomainServiceA is not allowed to call StateService to fetch DomainB.

Concrete blockers as I view it:

• We're introducing getServerConfig, this should at a minimum go through the config service first.
• The Utils.removeVaultFromHostname seems potentially dangerous. You could have a self-hosted vault on vault.company.com which would now show up as company.com. Is that intentional?
• We have special handling for bitwarden environments burned into the code for lock.component. I believe this is better served in the EnvironmentService? (Ideally long term I would like to see an Environment class that we pass along that can have a getter for presentational text but that feels better left for the Environment service refactors)

The broader question is, why are we loading the config service in the first place? The user has EnvironmentUrls which should contain this information.

---

There are some quick wins that can be had in this component in my opinion.

• Remove SwitcherAccount, the component should define it's own model for the template. This decouples it from internal state models. (Check ActiveAccount)
• Remove authenticationStatus from AccountProfile it's weird that a state model contains a field only populated by a component. This can either be solved by introducing a new type using & { authenticationStatus: boolean }

Once we have a proper AccountService we should be able to remove direct calls to state service for these things.

[rr-bw]

@Hinton Thanks for the very helpful feedback.

I've refactored quite a bit and I think this addresses all of your concerns and ensures this PR is not introducing any new calls to stateService from the component. Here is a summary of recent changes:

• I removed dependency on getServerConfig(...). Instead I created two new methods in the EnvironmentService:

  • getRegion(...) - allows us to optionally pass in a userId to get the region via StateService
  • getHost(...) - allows us to optionally pass in a userId and then uses the getRegion(...) method above to extract the host name for US, EU, or Self-Hosted

• I refactored the SwitcherAccount class to an InactiveAccount intersection type (extends ActiveAccount). This way the component is only dealing with the data it actually needs (that is, we are no longer extending Account, and no longer adding authenticationStatus to AccountProfile via the component).

  • As a side note, I did some renaming here, from SwitcherAccount to InactiveAccount, because I believe this more clearly distinguishes the two types of accounts we are working with: ActiveAccount and InactiveAccount.

I've also updated the PR with a demo video. Let me know what you think. Thanks again!

Tagging @jlf0dev and @trmartin4 for visibility

[Hinton]

Nice, this looks significantly more maintainable. Thanks for iterating on this!

[Hinton]

Great improvement.