Add-userid-to-encryption-methods

[MGibson1]

This is not the owning entity of the key, but the user that is unlocked that triggered the request

🎟️ Tracking

📔 Objective

⏰ 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

[github-actions]

Checkmarx One – Scan Summary & Details – 5d96e661-6cb3-4271-b41d-2dcc50d3f948

Great job, no security vulnerabilities found in this Pull Request

[codecov]

Codecov Report

Attention: Patch coverage is 59.37500% with 26 lines in your changes missing coverage. Please review.

Project coverage is 69.93%. Comparing base (af7ae90) to head (468bcda).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/bitwarden-fido/src/authenticator.rs	0.00%	20 Missing ⚠️
...ates/bitwarden-core/src/auth/login/auth_request.rs	0.00%	1 Missing ⚠️
crates/bitwarden-core/src/client/test_accounts.rs	50.00%	1 Missing ⚠️
crates/bitwarden-core/src/mobile/crypto.rs	87.50%	1 Missing ⚠️
crates/bitwarden-uniffi/src/platform/fido2.rs	0.00%	1 Missing ⚠️
crates/bitwarden-uniffi/src/vault/ciphers.rs	0.00%	1 Missing ⚠️
...rates/bitwarden-wasm-internal/src/vault/ciphers.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   69.83%   69.93%   +0.10%     
==========================================
  Files         213      214       +1     
  Lines       16809    16989     +180     
==========================================
+ Hits        11738    11882     +144     
- Misses       5071     5107      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coroiu]

issue?: it seems odd to have a public function that changes the user_id without any additional context required. I feel like the user_id should only ever change when the client is initialized and possibly never again after that. Maybe there's some edge case where we'd want a client without Crypto but tied to a user? If that is the case then I think this function should instead be called something like init_user_id() and only allow itself to be called once. However, it's probably more likely that we'll split up the client into user and non-user scoped clients and then just require the user_id on creation.

Either way, I think we should remove the ability to change the user_id. My opinion is that it should be bundled into the initialize_user_crypto functions below, but I'm not married to that solution

[dani-garcia]

Yeah I agree, separate user-bound and non-user bounds clients would allow us to require user id during construction and enforce no changes to it, until we have those we can use a OnceLock to ensure the ID is only initialized once:

    pub(crate) user_id: OnceLock<Uuid>,

    pub fn init_user_id(&self, user_id: Uuid) -> Result<(), UserIdAlreadySetError> {
        self.user_id
            .set(user_id)
            .map_err(|_| UserIdAlreadySetError)
    }

    pub fn get_user_id(&self) -> Option<Uuid> {
        self.user_id.get().map(|x| *x)
    }

[MGibson1]

There definitely are user-bound, but not crypto-inited clients -- that's a locked SDK.

The thought was, that I only need the UserId during encryption (for now) and I'm not 100% familiar with our current usages and I wanted to minimize the impact of the breaking change.

I can certainly revise. I agree it makes sense to init userId only once -- that's an easy change -- Actually making the pure, environment-bound, and user-bound client contexts feel out of scope here, though. Do you agree?

[coroiu]

Oh yes splitting them up is way out of scope, we're gonna be working on that with Auth if I remember correctly. I was just thinking ahead a little, I could've been a little clearer there :) I think OnceLock that Dani suggested is a perfectly sound solution until we eventually start splitting stuff

[MGibson1]

This is done (468bcda), but there is an unfortunate side effect that crypto initialization can now error with a userId already set.

This will have to be updated once the clients are split, but for now is in line with the expectation that UserId is sent into crypto initialization. Once I implemented it, I got to thinking that for the same reasons it makes sense to error should multiple init_user_crypto requests be sent into a single client, so hopefully this can just morph into a CryptoAlreadyInitialized error or some such.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud