[PM-14229] Add more docs to bitwarden_core

[justindbaur]

🎟️ Tracking

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

📔 Objective

Adds some more docs to bitwarden_core it's far from fully documented but I thought it was a good start to make sure I am on the right track about what should be expected.

⏰ 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 – ca9bce31-41fa-444e-913c-0307f30429b6

No New Or Fixed Issues Found

[codecov]

Codecov Report

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

Project coverage is 63.33%. Comparing base (130ac6f) to head (b800b80).

Files with missing lines	Patch %	Lines
crates/bitwarden-core/src/client/test_accounts.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   63.34%   63.33%   -0.02%     
==========================================
  Files         184      184              
  Lines       12935    12938       +3     
==========================================
  Hits         8194     8194              
- Misses       4741     4744       +3     

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

[coroiu]

thought: I'm always confused on how to write this, technically they are encrypted with the public key and decrypted with the private key 🤷

[coroiu]

question/suggestion: Who are targeting with this documentation? For external consumers this makes sense, but for our internal products we're expected to use this and not have to "be afraid". Should we maybe clarify that?

[coroiu]

question/suggestion: can these fields take any values, or do they expect the fields to be formatted in a certain way, e.g. base64? If yes, could we let the example clarify that somehow?

[coroiu]

Just realized that these docs are tested and so this must work 🙃

[dani-garcia]

Well, the test runs the code at the top level, which means it sees the test function but doesn't actually call it.

If you want your test code to be called, it either needs to be outside of a function, or you need to call the function at the top level of the code block.

With sync functions you can just call them:

    /// Does a thing
    ///
    /// ```rust
    /// fn test() {
    ///     panic!("This test now fails")
    ///     Ok(())
    /// }
    /// # test();
    /// ```

With async functions for example you can do this:

    /// Does a thing
    ///
    /// ```rust
    /// async fn test() {
    ///     panic!("This test now fails")
    ///     Ok(())
    /// }
    /// # tokio_test::block_on(test());
    /// ```

Note the last line: /// # tokio_test::block_on(test()); which runs the async test using tokio_test. It's at the top level of the code block, and not inside a function, so it gets run. The # also hides the line from the generated doc, while still running it during tests, which makes the docs nicer to look at:

Ps: the tokio-test crate is just doing tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap().block_on(test()); so we could make our own test utility function if we prefer.

[coroiu]

I did not notice that the code was inside of functions, good catch!

[coroiu]

thought: huh, interesting, here we use assert_eq to give an example of the output, but in the doc above we use println. Should we choose one approach for consistency maybe? I like this one with assert_eq

[dani-garcia]

Ideally we'd just assert! as much as possible, that way we can have an explanatory code comment and test all in one.

That might not be always acceptable, if the output of the function is too long we don't want the code sample to be dwarfed by a massive multi-line assert for example.

[justindbaur]

Yeah I also prefer assert but the one above didn't use it in the above doc because I couldn't come up with a way to concisely show off a public key that wouldn't be pretty ugly for a test. I could probably generate a new one in a simple one-liner but I still couldn't assert_eq! it because it would be different each time and assert_ne(fingerprint, "") doesn't seem like something we want to do.

[dani-garcia]

It would be good to also have a doc at the top of bitwarden-core/src/lib.rs explaining a bit what the contents of the crate are:

//! This crate contains the core components that the rest of the crates use, 
//! the main entrypoint into this crate is [Client], etc, etc

#[cfg(feature = "uniffi")]
uniffi::setup_scaffolding!();
#[cfg(feature = "uniffi")]
mod uniffi_support;

Note that we're using the //! syntax for the comment, so the comment applies to the crate, as opposed to the next element on the file.

[Hinton]

Some generic feedback on the documentation format.

[Hinton]

suggestion: It's fairly self explanatory that this is a struct both from the code, but also in rust docs.

Suggested change

	/// A struct containing the core keys of a Bitwarden user.
	/// [EncryptionSettings] contains the encryption state related to a Bitwarden account.

[Hinton]

Is these details relevant to the EncryptionSettings context? We could expand this to give the bigger picture in which case this information could be relevant. If not I think we should focus on the EncryptionSettings perspective which is not to share this anywhere.

Suggested change

	/// The users symmetric key, stored on the server after being
	/// encrypted with another key.
	/// The user's primary encryption key.
	///
	/// Every Bitwarden account has a symmetric user_key which acts as the encryption key to unlock everything else.
	/// .......
	/// This key is generally protected by a [MasterKey] derived from the users password, or .... and uploaded to the server.
	///
	/// ## References:
	/// - https://bitwarden.com/help/account-encryption-key/
	/// <confluence link>?