[PM-6037] Fix process reload not triggering on inactive account lock/logout

quexten commented 5 days ago

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-6037

📔 Objective

When logging out inactive accounts, no process reload is triggered, keeping auth tokens in renderer process memory. This is because the authservice specifically filtered for the active userId. This PR removes that check, ensuring process reloads for inactive accounts.

It seems that the loggedOut message was only called for the active user Id because it also changes the notification service connection status. Because of this, we now pass the userId-to-be-logged-out in the message, and compare this against the active userId in the message handler.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

codecov[bot] commented 5 days ago

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 29.30%. Comparing base (591f444) to head (592bd35). Report is 1 commits behind head on main.

Files	Patch %	Lines
apps/desktop/src/app/app.component.ts	0.00%	3 Missing :warning:
apps/web/src/app/app.component.ts	0.00%	3 Missing :warning:
libs/common/src/auth/services/auth.service.ts	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #9805 +/- ## ========================================== + Coverage 29.29% 29.30% +0.01% ========================================== Files 2532 2532 Lines 73822 73826 +4 Branches 13781 13785 +4 ========================================== + Hits 21623 21636 +13 + Misses 50577 50570 -7 + Partials 1622 1620 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 5 days ago

Checkmarx One – Scan Summary & Details – 5acb029f-8943-4f7a-b480-dae5db06b8fe

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Angular_Improper_Type_Pipe_Usage	/bitwarden_license/bit-web/src/app/admin-console/providers/providers-layout.component.html: 50	Attack Vector

quexten commented 4 days ago

I'll note the appcomponent broadcaster logic seems to be deprecated, but refactoring the process reload / lock events seems like a different task, so I made the (limited) changes here. We should revisit this logic at some point though.

quexten commented 3 days ago

I think there is this ticket https://bitwarden.atlassian.net/browse/PM-8544 to investigate the browsers logic related to logout, and this comment:

https://github.com/bitwarden/clients/blob/93a57e6724abdf4d59d1663f8c5ad9659f2a910c/apps/browser/src/popup/app.component.ts#L92

In a brief test, a timeout setting on a non-active account (the only way on browser to get a lock/logout on an inactive account that I know of), did seem to trigger a process reload / the popup to get closed.

bitwarden / clients