-
Notifications
You must be signed in to change notification settings - Fork 20
[PM-26542] Add listing items functionality with session-based authentication with environment variable support for API key login #492
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
base: main
Are you sure you want to change the base?
Conversation
…turn session token to reuse
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #492 +/- ##
==========================================
- Coverage 78.10% 77.57% -0.54%
==========================================
Files 283 284 +1
Lines 27628 27818 +190
==========================================
Hits 21579 21579
- Misses 6049 6239 +190 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Apply cargo fmt to fix import ordering and line wrapping to comply with CI style checks.
Separate external crate imports from internal crate imports with blank lines to comply with nightly rustfmt group_imports feature used in CI.
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 looks good to me. Will re-approve if you decide to rename text_or_env_prompt
, but that's not blocking.
Great job! No new security vulnerabilities introduced in this pull request |
|
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.
Let me know if there is future work slated to flush out the implementation of the vault related items. If list
is just being used as an easy implementation to test against, the changes are good with me.
crates/bw/src/vault/list.rs
Outdated
|
||
#[derive(Debug)] | ||
pub struct ListOptions { | ||
pub object: String, |
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.
⛏️ With their only being a finite amount of options an enum can narrow the available options here.
pub enum ObjectType {
#[value(name = "items")]
Items,
#[value(name = "folders")]
Folders,
#[value(name = "collections")]
Collections,
#[value(name = "organizations")]
Organizations,
#[value(name = "org-collections")]
OrgCollections,
#[value(name = "org-members")]
OrgMembers,
}
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.
Changes:
- Added
ObjectType
enum withValueEnum
derive for clap integration - Updated
ListOptions.object
field fromString
toObjectType
- Replaced string matching with enum pattern matching in list()
- Removed manual error handling for invalid object types
The enum uses kebab-case naming to maintain CLI compatibility.
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 ListOptions
struct is still needed because it contains the filtering parameters (search
, folderid
, collectionid
, organizationid
, trash
) that apply to list operations.
The CLI now provides type-safe validation and shows available options in help output.
pub collectionid: Option<String>, | ||
pub organizationid: Option<String>, | ||
pub trash: bool, | ||
} |
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.
❓ Archive is near future feature that was recently implemented in the CLI, should that be considered here?
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.
I've added an unimplemented archive feature to match Node.js CLI:
- Add
archive
command with todo!() placeholder - Add
--archived
flag to list command - Update
ListOptions
struct with archived field
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.
Updated to match searchService.searchCiphersBasic
- now searches ID (8+ chars), name, subtitle, and URIs with case-insensitive matching.
Please let me know if tests are needed.
crates/bw/src/vault/list.rs
Outdated
// Search filter (case-insensitive search in name) | ||
if let Some(ref search_term) = options.search { | ||
let search_lower = search_term.to_lowercase(); | ||
if !item.name.to_lowercase().contains(&search_lower) { |
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 current implementation of search within the CLI checks the id
, name
, subtitle
and uris
. searchService.searchCiphersBasic, I would think we'd want parity to that here.
Add unimplemented archive feature to match Node.js CLI: - Add `archive` command with todo!() placeholder - Add `--archived` flag to list command - Update ListOptions struct with archived field Should align with bitwarden/clients#16502 for future implementation.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26542
📔 Objective
Implements session-based authentication for the Rust Bitwarden CLI, enabling stateless command execution similar to the Node.js CLI. Users can now authenticate once with
bw login api-key
and reuse the session for subsequent commands.Added environment variable support (BW_CLIENTID, BW_CLIENTSECRET, BW_PASSWORD) to the bw login api-key command and made it return a base64-encoded session key (like the Node.js CLI, although this one is reaally long) that can be potentially used with --session (or BW_SESSION) for subsequent vault operations.
Session Management
export_session()
to serialize full client stateimport_session()
to restore complete authenticated state from session stringEnvironment Variable Support
Added support for environment variables in
bw login api-key
:BW_CLIENTID
(orBW_CLIENT_ID
): API client IDBW_CLIENTSECRET
(orBW_CLIENT_SECRET
): API client secretBW_PASSWORD
: Master passwordList Command Implementation
bw list items
with filtering options:--search
: Text search across items--folderid
: Filter by folder--collectionid
: Filter by collection--organizationid
: Filter by organization--trash
: Show deleted itemsSession Usage
The global
--session
flag andBW_SESSION
environment variable now work across all commands, automatically restoring client state before execution.Usage Example
⏰ 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