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-14235] Add more docs to bitwarden_vault #102

Open
wants to merge 2 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
4 changes: 4 additions & 0 deletions crates/bitwarden-vault/src/collection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use uuid::Uuid;

use crate::VaultParseError;

/// Encrypted Collection state
Copy link
Member

Choose a reason for hiding this comment

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

What does state imply?

Suggested change
/// Encrypted Collection state
/// Encrypted Collection

#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
Expand All @@ -24,6 +25,7 @@ pub struct Collection {
pub manage: bool,
}

/// Decrypted Collection state
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Decrypted Collection state
/// Decrypted Collection

#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
Expand All @@ -40,6 +42,7 @@ pub struct CollectionView {
}

impl LocateKey for Collection {
/// Returns a [SymmetricCryptoKey] on success, [CryptoError] on failure
Copy link
Member

Choose a reason for hiding this comment

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

This comment conveys the exact same information as the type interface. We generally want to avoid these type of comments since they can easily get out of sync with the actual code. We're also actively refactoring these logic.

fn locate_key<'a>(
&self,
enc: &'a dyn KeyContainer,
Expand All @@ -48,6 +51,7 @@ impl LocateKey for Collection {
enc.get_key(&Some(self.organization_id))
}
}

impl KeyDecryptable<SymmetricCryptoKey, CollectionView> for Collection {
fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<CollectionView, CryptoError> {
Ok(CollectionView {
Expand Down
2 changes: 2 additions & 0 deletions crates/bitwarden-vault/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ pub(crate) async fn sync(client: &Client, input: &SyncRequest) -> Result<SyncRes

#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
/// Data returned from `/accounts/profile`.
Copy link
Member

Choose a reason for hiding this comment

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

This is false. ProfileResponseModel from bitwarden-api-api is the response from the endpoint. This is a part of the SyncResponse which is the internal SDK response from performing a sync.

pub struct ProfileResponse {
pub id: Uuid,
pub name: String,
Expand All @@ -79,6 +80,7 @@ pub struct DomainResponse {

#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
/// Data returned from `/sync`.
Copy link
Member

Choose a reason for hiding this comment

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

This is false. SyncResponseModel from bitwarden-api-api is the response from the endpoint. This is an internal model containing the data returned by calling sync().

pub struct SyncResponse {
/// Data about the user, including their encryption keys and the organizations they are a part
/// of
Expand Down
58 changes: 58 additions & 0 deletions crates/bitwarden-vault/src/vault_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,78 @@ use crate::{
SyncRequest, SyncResponse,
};

/// A vault client.
///
/// # Examples
///
/// ```rust
/// use bitwarden_core::{Client,ClientSettings,DeviceType};
/// use bitwarden_vault::VaultClient;
///
/// 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,
/// }));
///
/// let vault = VaultClient::new(&client);
/// ```
Comment on lines +8 to +24
Copy link
Member

Choose a reason for hiding this comment

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

We don't want people to manually. wire up VaultClient's and you should generally use the Extension i.e. client.vault() -> VaultClient.

Copy link
Member

Choose a reason for hiding this comment

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

@dani-garcia should we remove pub from the Clients?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable yeah. I looked into it and VaultClient is the only partial client constructor that is pub too, the rest are private.

pub struct VaultClient<'a> {
/// A vault client.
Copy link
Member

Choose a reason for hiding this comment

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

This is not a vault client though. This is the root SDK client.

pub(crate) client: &'a Client,
}

impl<'a> VaultClient<'a> {
/// Constructs a new [VaultClient] with the given [Client]
///
/// # Examples
///
/// ```rust
/// # use bitwarden_core::{Client,ClientSettings,DeviceType};
/// # use bitwarden_vault::VaultClient;
/// # 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,
/// # }));
/// let vault = VaultClient::new(&client);
/// let ciphers = vault.ciphers();
/// // ...
/// ```
pub fn new(client: &'a Client) -> Self {
Self { client }
}

/// Syncs the [VaultClient] with the server.
///
/// # Examples
///
/// ```rust
/// # use bitwarden_core::{Client,ClientSettings,DeviceType};
/// # use bitwarden_vault::{VaultClient,SyncRequest, SyncResponse};
/// async fn sync_vault(client: &Client) {
/// let vault = VaultClient::new(client);
/// let request = SyncRequest {
/// exclude_subdomains: Some(false),
/// };
///
/// let result = vault.sync(&request).await;
/// match result {
/// Ok(response) => println!("Response: {:?}", response),
/// Err(error) => {
/// eprintln!("Sync failed: {:?}", error);
/// },
/// }
/// }
/// ```
Comment on lines 51 to +73
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't actually want to expose this as public since it's to my knowledge unused.

pub async fn sync(&self, input: &SyncRequest) -> Result<SyncResponse, SyncError> {
sync(self.client, input).await
}
}

/// An extension trait for the [VaultClient] struct to provide vault functionality.
pub trait VaultClientExt<'a> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not exactly sure what these "extensions" are or will be used for.

fn vault(&'a self) -> VaultClient<'a>;
}
Expand Down
Loading