SM-866: Add state to the SDK & BWS CLI #388

coltonhurst · 2023-12-03T01:53:39Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This PR adds the ability to manage basic state via the SDK. It also updates the bws crate to use this SDK change, implementing basic state management for auth tokens. The core benefit is that reauthentication per command will no longer be needed, so that server auth requests will be mitigated.

Code changes

(to be added)

file.ext: Description of what was changed and why

Before you submit

Please add unit tests where it makes sense to do so (encouraged but not required)

…ditional changes

…tings

bitwarden-bot · 2023-12-03T05:39:59Z

Checkmarx One – Scan Summary & Details – 46489a45-7b43-4fcf-8bfc-dee0f6cc1927

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Unpinned Actions Full Length Commit SHA	/version-bump.yml: 56	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity	Issue	Source File / Package
	Unpinned Actions Full Length Commit SHA	/version-bump.yml: 59

dani-garcia

Some quick review comments

crates/bitwarden/src/client/client.rs

crates/bitwarden/src/error.rs

crates/bitwarden/src/lib.rs

crates/bitwarden/src/client/client.rs

Hinton

I had hoped we could leave the bitwarden crate mostly untouched and have the state mostly in the bws crate.

dani-garcia · 2023-12-04T16:54:26Z

We will have the same problem with the PHP bindings, so having a solution just for bws won't solve all the issues at least.

coltonhurst · 2023-12-04T17:11:13Z

I had hoped we could leave the bitwarden crate mostly untouched and have the state mostly in the bws crate.

Yeah, that's why I tried to leave "minimal" changes in the bitwarden crate, and a lot of the storage logic is in bws. However, as @dani-garcia mentioned, multiple integrations will need to be able to use state to avoid the reauthentication each time (PHP and Python specifically have been discussed).

If you're open to it we can leave the minimal "generalized" state changes in the bitwarden crate and keep the rest of the storage logic in bws. Then we can revisit a more robust approach to state as we discussed previously?

Update (Dec 5): PR has been updated, most of the logic is in the bitwarden crate so extensions can take advantage of state management, though it is in its own module. We can move this if needed, but it would be great to keep management in the bitwarden crate for integrations if possible.

…piry checks in Client

## Type of change  ``` - [ ] Bug fix - [ ] New feature development - [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc) - [ ] Build/deploy pipeline (DevOps) - [ ] Other ``` ## Objective  Refactor out the `Instant` type in Client and use unix timestamps. This is to support this PR: #388 where we need to serialize token expiry times. It also brings benefits to other areas like WASM, where `Instant` doesn't exist. ## Code changes   - **crates/bitwarden/src/auth/renew.rs:** Update the `renew_token` function to use timestamps instead of the `Instant` type - **crates/bitwarden/src/client/client.rs:** Update `Client` to store the unix timestamp for token expiration ## Before you submit - Please add **unit tests** where it makes sense to do so (encouraged but not required)

…commit to save work.

…en crate

dani-garcia

Some minor things to avoid some unneeded clones but otherwise LGTM

crates/bitwarden/src/auth/login/access_token.rs

crates/bitwarden/src/client/client_settings.rs

crates/bitwarden/src/auth/login/access_token.rs

crates/bitwarden/src/secrets_manager/state.rs

crates/bitwarden/src/auth/login/access_token.rs

crates/bitwarden/src/auth/renew.rs

crates/bitwarden/src/client/client.rs

crates/bws/src/config.rs

crates/bws/src/main.rs

codecov · 2023-12-17T21:11:19Z

Codecov Report

Attention: 108 lines in your changes are missing coverage. Please review.

Comparison is base (4c079e8) 45.45% compared to head (bd3c420) 44.81%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
crates/bitwarden/src/auth/login/access_token.rs	12.76%	41 Missing ⚠️
crates/bitwarden/src/secrets_manager/state.rs	0.00%	28 Missing ⚠️
crates/bitwarden/src/auth/renew.rs	5.88%	16 Missing ⚠️
crates/bws/src/main.rs	0.00%	11 Missing ⚠️
crates/bws/src/state.rs	0.00%	10 Missing ⚠️
crates/bws/src/config.rs	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #388      +/-   ##
==========================================
- Coverage   45.45%   44.81%   -0.64%     
==========================================
  Files         149      151       +2     
  Lines        6771     6881     +110     
==========================================
+ Hits         3078     3084       +6     
- Misses       3693     3797     +104

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…bws)

Hinton

Looks good, ideally we would have test cases for the new access token login logic, and ClientState logic.

## Type of change - [ ] Bug fix - [ ] New feature development - [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc) - [ ] Build/deploy pipeline (DevOps) - [x] Other ## Objective Enable authentication with the state file. Relates to #388. ## Code changes - **`languages/python/bitwarden_sdk/bitwarden_client.py`**: Add optional `state_file_path` parameter to `access_token_login` ## Before you submit - Please add **unit tests** where it makes sense to do so --------- Co-authored-by: Daniel García <dani-garcia@users.noreply.github.com>

coltonhurst added 4 commits November 16, 2023 10:50

SM-866: Add initial state MVP (not building, save dump)

1ac4cad

SM-866: Change error handling if the state file is not found, plus ad…

60d6351

…ditional changes

SM-866: Save encryption_key in state to generate Client EncryptionSet…

a34ef5c

…tings

SM-866: Remove get_client_state fn comment

9ac9a27

coltonhurst added 2 commits December 3, 2023 18:34

Merge master branch into sm/sm-866

5b2df50

Merge branch 'master' into sm/sm-866

be1e21b

coltonhurst self-assigned this Dec 3, 2023

coltonhurst added 4 commits December 3, 2023 20:31

SM-866: Cargo clippy changes

02da0b0

SM-866: Fix failing test_register_initialize_crypto test

ac6c714

SM-866: Fix failing OS builds

f5972f5

SM-866: Implement bws State

4721b75

coltonhurst marked this pull request as ready for review December 4, 2023 07:14

coltonhurst requested a review from dani-garcia December 4, 2023 07:15

SM-866: Variable naming refactor

5e9714e

dani-garcia reviewed Dec 4, 2023

View reviewed changes

Hinton reviewed Dec 4, 2023

View reviewed changes

coltonhurst added 3 commits December 4, 2023 12:53

SM-1023: Use unix timestamps instead of the Instant type for token ex…

a913501

…piry checks in Client

Merge branch 'master' into sm/sm-866

ee21fce

SM-866: Merge sm/sm-1032 into branch

051b471

coltonhurst mentioned this pull request Dec 4, 2023

SM-1023: Remove Instant from Client in SDK #397

Merged

coltonhurst added 5 commits December 5, 2023 11:23

SM-866: Add new access_token_login_from_state function. Intermediary …

3762e28

…commit to save work.

SM-866: Combine bws State and bitwarden StateManager into the bitward…

30bbf51

…en crate

SM-866: Refactor out encryption_key

6bf16c2

Merge master into branch

b9d29e2

SM-866: Fix failing tests

f23a0fb

coltonhurst requested a review from Hinton December 5, 2023 21:58

coltonhurst requested a review from dani-garcia December 12, 2023 19:34

coltonhurst added 2 commits December 12, 2023 14:39

SM-866: Minor updates

be0b374

SM-866: Delete file that remained from merge

751ff0d

dani-garcia previously approved these changes Dec 12, 2023

View reviewed changes

crates/bitwarden/src/auth/login/access_token.rs Outdated Show resolved Hide resolved

crates/bitwarden/src/auth/login/access_token.rs Outdated Show resolved Hide resolved

crates/bitwarden/src/auth/login/access_token.rs Outdated Show resolved Hide resolved

SM-866: Small refactorings

b1ba938

coltonhurst dismissed dani-garcia’s stale review via b1ba938 December 12, 2023 20:12

Merge branch 'main' into sm/sm-866

23421a7

coltonhurst requested a review from dani-garcia December 12, 2023 20:19

dani-garcia previously approved these changes Dec 13, 2023

View reviewed changes

Hinton reviewed Dec 14, 2023

View reviewed changes

dani-garcia mentioned this pull request Dec 14, 2023

Remove token from client #431

Closed

SM-866: Address PR comments

d0c0c10

coltonhurst dismissed dani-garcia’s stale review via d0c0c10 December 17, 2023 21:08

Merge branch 'main' into sm/sm-866

fad2bd3

coltonhurst added 3 commits December 17, 2023 16:21

SM-866: Update the changelog

0c8125f

SM-866: Run prettier

f45b5f5

SM-866: Only create a state file if the state_file_dir is specified (…

ce50cf1

…bws)

Hinton previously approved these changes Dec 18, 2023

View reviewed changes

coltonhurst added 2 commits December 18, 2023 12:37

Merge branch 'main' into sm/sm-866

43481d4

SM-866: Update usage of Error::Internal due to recent change

4443741

coltonhurst dismissed Hinton’s stale review via 4443741 December 18, 2023 17:55

SM-866: Cargo fmt

bd3c420

coltonhurst requested review from dani-garcia and Hinton December 18, 2023 17:59

dani-garcia approved these changes Dec 18, 2023

View reviewed changes

coltonhurst merged commit 9d4ec76 into main Dec 18, 2023
40 of 42 checks passed

coltonhurst deleted the sm/sm-866 branch December 18, 2023 18:27

tangowithfoxtrot mentioned this pull request Jan 11, 2024

[SM-1076] - add state_file_path param to access_token_login #496

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SM-866: Add state to the SDK & BWS CLI #388

SM-866: Add state to the SDK & BWS CLI #388

coltonhurst commented Dec 3, 2023

bitwarden-bot commented Dec 3, 2023 •

edited

Loading

dani-garcia left a comment

Hinton left a comment

dani-garcia commented Dec 4, 2023

coltonhurst commented Dec 4, 2023 •

edited

Loading

dani-garcia left a comment

codecov bot commented Dec 17, 2023 •

edited

Loading

Hinton left a comment

SM-866: Add state to the SDK & BWS CLI #388

SM-866: Add state to the SDK & BWS CLI #388

Conversation

coltonhurst commented Dec 3, 2023

Type of change

Objective

Code changes

Before you submit

bitwarden-bot commented Dec 3, 2023 • edited Loading

New Issues

Fixed Issues

dani-garcia left a comment

Choose a reason for hiding this comment

Hinton left a comment

Choose a reason for hiding this comment

dani-garcia commented Dec 4, 2023

coltonhurst commented Dec 4, 2023 • edited Loading

dani-garcia left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2023 • edited Loading

Codecov Report

Hinton left a comment

Choose a reason for hiding this comment

bitwarden-bot commented Dec 3, 2023 •

edited

Loading

coltonhurst commented Dec 4, 2023 •

edited

Loading

codecov bot commented Dec 17, 2023 •

edited

Loading