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-14229] Add more docs to bitwarden_core #39

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/bitwarden-core/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@ pub struct Client {
}

impl Client {
/// Constructs a new `Client` with the given `settings_input`.
///
/// # Examples
/// ```rust
/// use bitwarden_core::{Client, ClientSettings, DeviceType};
///
/// let client = Client::new(Some(ClientSettings {
/// identity_url: "https://identity.bitwarden.com".to_owned(),
/// api_url: "https://api.bitwarden.com".to_owned(),
/// user_agent: "Bitwarden Rust-SDK".to_owned(),
/// device_type: DeviceType::ChromeBrowser,
/// }));
/// ```
pub fn new(settings_input: Option<ClientSettings>) -> Self {
let settings = settings_input.unwrap_or_default();

Expand Down
10 changes: 10 additions & 0 deletions crates/bitwarden-core/src/client/encryption_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,20 @@ pub enum EncryptionSettingsError {
MissingPrivateKey,
}

/// A struct containing the core keys of a Bitwarden user.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[derive(Clone)]
pub struct EncryptionSettings {
/// The users symmetric key, stored on the server after being
/// encrypted with another key.
Comment on lines +36 to +37
Copy link
Member

@Hinton Hinton Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>?

user_key: SymmetricCryptoKey,
/// The users private key, stored on the server
/// encrypted with the users symmetric key.
pub(crate) private_key: Option<AsymmetricCryptoKey>,
/// A map of the users organization keys with the key being
/// the ID of the organization and the value being the symmetric
/// key. This map may be empty if the user is not a part of
/// any organizations. These keys are stored on the server
/// encrypted with the users private key.
Comment on lines +45 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

org_keys: HashMap<Uuid, SymmetricCryptoKey>,
}

Expand Down
2 changes: 2 additions & 0 deletions crates/bitwarden-core/src/client/flags.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// A struct containing fields of all known feature flags and their values.
#[derive(Debug, Default, Clone, serde::Deserialize)]
pub struct Flags {
/// A `bool` indicating whether or not cipher key encryption is enabled.
#[serde(default, rename = "enableCipherKeyEncryption")]
pub enable_cipher_key_encryption: bool,
}
Expand Down
3 changes: 3 additions & 0 deletions crates/bitwarden-core/src/client/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ pub(crate) struct Tokens {
pub(crate) refresh_token: Option<String>,
}

/// A struct contains various internal actions. Everything on this type
/// should be considered unstable and subject to change at any time. Use
/// with caution.
Comment on lines +44 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#[derive(Debug)]
pub struct InternalClient {
pub(crate) tokens: RwLock<Tokens>,
Expand Down
6 changes: 6 additions & 0 deletions crates/bitwarden-core/src/client/test_accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@
};

impl Client {
/// Creates a client initialized with a hard coded test account.
/// Useful for examples.
pub async fn test_account() -> Self {
Self::init_test_account(test_bitwarden_com_account()).await
}

Check warning on line 19 in crates/bitwarden-core/src/client/test_accounts.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/client/test_accounts.rs#L17-L19

Added lines #L17 - L19 were not covered by tests

pub async fn init_test_account(account: TestAccount) -> Self {
let client = Client::new(None);

Expand Down
40 changes: 40 additions & 0 deletions crates/bitwarden-core/src/platform/client_platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,54 @@ use super::{
};
use crate::{error::Result, Client};

/// A struct containing platform utilities.
pub struct ClientPlatform<'a> {
pub(crate) client: &'a Client,
}

impl<'a> ClientPlatform<'a> {
/// Will generate a fingerprint based on the `input`. Given the same `input` This
/// method will result in the exact same output.
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Examples
/// ```rust
/// use bitwarden_core::{Client, platform::FingerprintRequest};
///
/// async fn test() {
/// let client = Client::test_account().await;
/// let fingerprint_response = client.platform()
/// .fingerprint(&FingerprintRequest {
/// fingerprint_material: "my_material".to_owned(),
/// public_key: "...public key...".to_owned(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that these docs are tested and so this must work ๐Ÿ™ƒ

Copy link
Member

@dani-garcia dani-garcia Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// })
/// .unwrap();
///
/// println!("{}", fingerprint_response.fingerprint);
coroiu marked this conversation as resolved.
Show resolved Hide resolved
/// }
/// ```
pub fn fingerprint(&self, input: &FingerprintRequest) -> Result<FingerprintResponse> {
generate_fingerprint(input)
}

/// Will generate a fingerprint based on the given `fingerprint_material`
/// and the users public key. Given the same `fingerprint_material` and
/// the same user. This method will result in the exact same output.
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
///
/// The returned fingerprint is a string of 5 words seperated by hyphens.
///
/// # Examples
/// ```rust
/// use bitwarden_core::Client;
///
/// async fn test() {
/// let client = Client::test_account().await;
/// let fingerprint = client.platform()
/// .user_fingerprint("my_material".to_owned())
/// .unwrap();
///
/// assert_eq!(fingerprint, "dreamland-desecrate-corrosive-ecard-retry");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// }
/// ```
pub fn user_fingerprint(self, fingerprint_material: String) -> Result<String> {
generate_user_fingerprint(self.client, fingerprint_material)
}
Expand All @@ -27,6 +66,7 @@ impl<'a> ClientPlatform<'a> {
}

impl<'a> Client {
/// Retrieves a [`ClientPlatform`] for accessing Platform APIs.
pub fn platform(&'a self) -> ClientPlatform<'a> {
ClientPlatform { client: self }
}
Expand Down
3 changes: 3 additions & 0 deletions crates/bitwarden-core/src/platform/generate_fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use serde::{Deserialize, Serialize};

use crate::error::Result;

/// A struct containing the parts for making a fingerprint request.
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
Expand All @@ -16,9 +17,11 @@ pub struct FingerprintRequest {
pub public_key: String,
}

/// A struct containing the details of a successful request to generate a fingerprint.
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct FingerprintResponse {
/// A `String` containing 5 words seperated by hyphens.
pub fingerprint: String,
}

Expand Down