clients icon indicating copy to clipboard operation
clients copied to clipboard

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

Open quexten opened this issue 1 year ago â€ĸ 4 comments

đŸŽŸī¸ 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

quexten avatar Jun 24 '24 14:06 quexten

Codecov Report

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

Project coverage is 35.11%. Comparing base (0080fcc) to head (bdf4ae2). Report is 1 commits behind head on main.

:white_check_mark: All tests successful. No failed tests found.

Files with missing lines 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
@@            Coverage Diff             @@
##             main    #9805      +/-   ##
==========================================
- Coverage   35.12%   35.11%   -0.01%     
==========================================
  Files        2691     2691              
  Lines       83765    83767       +2     
  Branches    15919    15921       +2     
==========================================
- Hits        29421    29415       -6     
- Misses      53376    53384       +8     
  Partials      968      968              

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

codecov[bot] avatar Jun 24 '24 14:06 codecov[bot]

Logo Checkmarx One – Scan Summary & Details – 98aa00ec-2ed3-405d-a5f7-c0ac9cd653c1

No New Or Fixed Issues Found

github-actions[bot] avatar Jun 24 '24 14:06 github-actions[bot]

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 avatar Jun 25 '24 08:06 quexten

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.

quexten avatar Jun 26 '24 10:06 quexten

Looks like @jlf0dev got removed from the auth team and no longer counts as a reviewer for them, re-requested auth-dev

MGibson1 avatar Sep 12 '24 18:09 MGibson1