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 all commits
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
16 changes: 16 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 Expand Up @@ -187,3 +193,13 @@
org: None,
}
}

pub const PUBLIC_KEY: &str = "-----BEGIN PUBLIC KEY-----
MIIBITANBgkqhkiG9w0BAQEFAAOCAQ4AMIIBCQKCAQB6x0WIywZ7ys/5lFBNOWqP
uwtnCuX58vPVOOUttuaK1AfgZnCIpTbkaJUPKqZbeEi2uWlrDBQ5K0gEX9ASTXCw
i3ij1mVifAJgjI698VTS2Xn9vitYhBT8V0EQstUW0jIlng7qRyrHQ9owBzGUsv4s
1pGHKhgJcbQkh+hagu0s8cpA0BQl6L2BFi/H4DjD94m3LcJVj+z6FepQYJlLte+W
T3DjuHhwXWWRXiYP0/d/QeCBMMP72N4Xw25LmXOxN035JlKHuRazg0lHDj5C1BDY
nH4lWuWbVrLUBNzETLUAGJX6JEuVbwLWEb4R1yJHXecUueyPSCSxksY4rUedSGoP
AgMBAAE=
-----END PUBLIC KEY-----";
44 changes: 44 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,58 @@ 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 always result in the exact same output.
///
/// # Examples
/// ```rust
/// # use bitwarden_core::client::test_accounts::PUBLIC_KEY;
/// use base64::{engine::general_purpose::STANDARD, Engine};
/// use bitwarden_core::{Client, platform::FingerprintRequest};
///
/// fn test(client: Client) {
/// let fingerprint_response = client.platform()
/// .fingerprint(&FingerprintRequest {
/// fingerprint_material: "my_material".to_owned(),
/// public_key: STANDARD.encode(PUBLIC_KEY),
/// })
/// .unwrap();
///
/// assert_eq!(fingerprint_response.fingerprint, "unsure-unethical-bruising-semester-subscript");
/// }
///
/// # tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap().block_on(
/// # async { test(Client::test_account().await) });
/// ```
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 always result in the exact same output.
///
/// 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 +70,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