-
Notifications
You must be signed in to change notification settings - Fork 17
[PM-12612] SDK-Managed Repository support #301
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
Conversation
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
==========================================
- Coverage 77.03% 76.75% -0.28%
==========================================
Files 268 270 +2
Lines 25399 25664 +265
==========================================
+ Hits 19565 19698 +133
- Misses 5834 5966 +132 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a753607
to
5b9dd03
Compare
## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-19479 ## 📔 Objective Implement a generic trait for accessing the client application's data storage directly. Because we want the store access to be typed, but `bitwarden_core` isn't aware of the models, `bitwarden_core` only implements a generic way to set and retrieve generic `impl Repository<T>` instances, somewhat like dependency injection. Then it's up to each team/feature crates to define which models and which stores are available. This feature is created in a new `bitwarden-state`, which will be expanded by a separate PR with the addition of SDK-managed state (Sqlite+IndexedDB). At the moment this crate contains: - A `Repository` trait which will be implemented by the clients (and the SDK in the future), and a `RepositoryItem` marker trait which will be used to mark which models are meant to be used with the repositories. - A `StateRegistry` which stores all the client-managed Repositories, and in the future will also handle the SDK-managed repositories. - A new `StateClient` subclient under platform that will be used by the applications to register their Repositories. Both the WASM and UniFFI crates also need some conversion code to implement the `Repository` traits. I've tried to simplify it as much as possible, and hide it behind a macro when that wasn't possible. Some limitations on the current design: - The current integration with web clients requires the State Provider definition to be `UserKeyDefinition<Record<string, T>>` to match the key-value pattern in `Repository`. This usually matches with what is being used for vault (encrypted ciphers/folders, etc), but it might fall short on other domains, like profile data, or user keys. - There's no great way of ensuring that all the client-managed Repositories have been registered, other than keeping a list. I've tried to use the `inventory` crate to keep a global list of all the existing implementations and then validating that they've all been registered, but that doesn't work for WASM. We might be able to use the `inventory` crate and just run it in tests though. For some documentation on how to use it, you can check the README: https://github.com/bitwarden/sdk-internal/blob/ps/state-traits/crates/bitwarden-state/README.md The continuation of this is in #301, which contains SDK managed repository support. The client implementation of both these PRs is in bitwarden/clients#14839 ## ⏰ 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 <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+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
# Conflicts: # Cargo.lock # crates/bitwarden-state/Cargo.toml # crates/bitwarden-state/src/registry.rs # crates/bitwarden-state/src/repository.rs # crates/bitwarden-vault/src/cipher/cipher.rs # Conflicts: # crates/bitwarden-vault/src/vault_client.rs # Conflicts: # Cargo.lock # crates/bitwarden-state/Cargo.toml Use bundled sqlite to solve compile issues Update readme Don't stringify the data in indexeddb Remove the comment Update readme
8602563
to
5e9d57a
Compare
…es (#329) ## 🎟️ Tracking <!-- Paste the link to the Jira or GitHub issue or otherwise describe / point to where this change is coming from. --> ## 📔 Objective <!-- Describe what the purpose of this PR is, for example what bug you're fixing or new feature you're adding. --> ## ⏰ 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 <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+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
|
# Conflicts: # Cargo.lock # crates/bitwarden-core/src/platform/state_client.rs # crates/bitwarden-state/src/registry.rs # crates/bitwarden-state/src/repository.rs # crates/bitwarden-wasm-internal/src/platform/mod.rs
…ompile time, use indexeddb db name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements SDK-managed repository support with platform-specific database backends, allowing the SDK to handle data persistence internally rather than relying on client implementations.
Key changes:
- Adds database abstraction layer with SQLite support for native platforms and IndexedDB for WebAssembly
- Introduces repository registration system with compile-time name validation
- Updates platform-specific bindings (UniFFI/WASM) to support SDK-managed state initialization
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
crates/bitwarden-state/src/sdk_managed/sqlite.rs |
SQLite database implementation with repository operations |
crates/bitwarden-state/src/sdk_managed/indexed_db.rs |
IndexedDB implementation for WebAssembly platforms |
crates/bitwarden-state/src/sdk_managed/mod.rs |
Database trait definition and common error handling |
crates/bitwarden-state/src/repository.rs |
Repository item registration system with name validation |
crates/bitwarden-state/src/registry.rs |
State registry with database initialization support |
crates/bitwarden-uniffi/src/platform/mod.rs |
UniFFI bindings for SDK-managed state |
crates/bitwarden-wasm-internal/src/platform/mod.rs |
WASM bindings for SDK-managed state |
crates/bitwarden-threading/src/thread_bound_runner.rs |
Manual Clone implementation removing generic bounds |
mut db: rusqlite::Connection, | ||
registrations: &[RepositoryItemData], | ||
) -> Result<Self, DatabaseError> { | ||
// Set WAL mode for better concurrency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WAL mode setting should be documented with a comment explaining why this specific journal mode is chosen and its benefits for this use case.
Copilot uses AI. Check for mistakes.
sdk_managed::{Database, DatabaseConfiguration, DatabaseError}, | ||
}; | ||
|
||
// TODO: Use connection pooling with r2d2 and r2d2_sqlite? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO comment indicates a performance concern. Consider creating a tracking issue for connection pooling implementation to ensure this optimization is not forgotten.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12612
📔 Objective
A continuation of #213, this PR implements some simple SDK managed data store based on SQLite (on non-wasm) and IndexedDB (on wasm).
Both databases are wrapped in a
Database
trait and conditionally compiled based on platform. Then from each database we can get multipleRepository
implementations that can be used to read and write data persistently. Each repository is mapped to a separate table in SQLite and Object Store in IndexedDb.Some limitations of the current system:
register_repository_item
calls, just to get something working, but note that this is removed in [PM-22589] Implement basic db migrations for indexeddb #410 and replaced with a better migration system.⏰ Reminders before review
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 confirmedissue 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