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

[PM-16792] [PM-16822] Encapsulate encryptor and state provision within UserStateSubject #13195

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

audreyality
Copy link
Member

@audreyality audreyality commented Jan 31, 2025

๐ŸŽŸ๏ธ Tracking

https://bitwarden.atlassian.net/browse/PM-16792
https://bitwarden.atlassian.net/browse/PM-16822

๐Ÿ“” Objective

Encapsulate encryptor and state provision within UserStateSubject.

This PR also hoists the userId$ into an account$ dependency to ensure consistent user lifetime tracking from the host UI all the way through to the UserStateSubject. This helps avoid read/write sync bugs with CredentialGeneratorService apis, similar to those described in PM-11866.

๐Ÿฆ– Breaking changes

  • The following components and classes now require account$ or [account] bindings:
    • Settings components (generator components unaffected)
    • Password history components
    • CredentialGeneratorService
    • UserStateSubject
  • UserStateSubject requires a providers object containing service dependencies
  • Encryption frame size is now specified using ObjectKey; this was required

๐Ÿฆ• System evolution - Binding

My first pass through standardized observable definitions was tightly coupled to types and stream names. The behaviors, however, aren't typically type or name dependent. Bindings fix this by using generic mapped types that describe their expected behaviors while leaving the dependency name and type free-to-change.

The account$ binding is the first to be introduced. It represents data that is "pinned" to a specific state. That state emits its data once when subscribed and completes when the data should be discarded from memory.

At present the "Dependency" name is still being used. This is likely to change in a future PR, once more kinds of bindings exist.

๐Ÿฆ• System evolution - Semantic Loggers

I created a semantic logging feature by accident.

Debugging the user state key is a daunting task using a debugger. You have to know all the right places to put tracepoints to observe what's happening and breakpoints cause timing problems. Setting everything up is a chore, and in order to fix unit tests I found myself instrumenting the code with lots of repetitive console logs.

I decided to leave them in, but didn't want them active in production, so I extracted them into a logger type and stubbed it to disable logging. Then realized I'd made a semantic logger.

โฐ Reminders before review

There are a few places where I needed to look at the actual state while debugging. I marked those with an โŒ in the actual code. I think I got them all, but if you see that emoji it's code I need to clean up.

  • 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

@audreyality audreyality changed the title first pass on user state key refactor; breaks some tests Encapsulate encryptor and state provision within UserStateSubject Jan 31, 2025
@audreyality audreyality changed the title Encapsulate encryptor and state provision within UserStateSubject [PM-16792] [PM-16822] Encapsulate encryptor and state provision within UserStateSubject Jan 31, 2025

This comment was marked as resolved.

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.

1 participant