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.

Sign up for GitHub

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

Jump to bottom

[SM-1153] Part 1 Secrets sync | Generate API bindings #674

Merged
merged 6 commits into from May 15, 2024
Merged

[SM-1153] Part 1 Secrets sync | Generate API bindings #674

merged 6 commits into from May 15, 2024

Conversation

Thomas-Avery
Copy link
Contributor

Type of change

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

Objective

Expose the new secrets sync endpoint.

Server PR bitwarden/server#3906

Code changes

ran the command ./support/build-api.sh

Before you submit

  • Please add unit tests where it makes sense to do so

@Thomas-Avery Thomas-Avery self-assigned this Mar 21, 2024
@Thomas-Avery
Copy link
Contributor Author

The command ./support/build-api.sh replaces crates/bitwarden-api-api/Cargo.toml and crates/bitwarden-api-identity/Cargo.toml.

I reverted the changes for the Cargo.toml files.

I did notice the replacements had an extra dependency serde_with = "^2.0" do we need this included in the Cargo.toml files?

@codecov Codecov
Copy link

codecov bot commented Mar 21, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.77%. Comparing base (57bcbeb) to head (032363c).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #674   +/-   ##
=======================================
  Coverage   58.77%   58.77%           
=======================================
  Files         177      177           
  Lines       11501    11501           
=======================================
  Hits         6760     6760           
  Misses       4741     4741

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

@dani-garcia
Copy link
Member

Yeah for the moment the Cargo.toml changes need to be reverted manually. Thankfully those two files don't need to change very often outside of version bumps.

The generated bindings aren't using the serde_with crate so it doens't need to be included.

@Thomas-Avery Thomas-Avery mentioned this pull request Mar 22, 2024
Merged
coltonhurst
coltonhurst previously approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@coltonhurst coltonhurst left a comment •
edited

Choose a reason for hiding this comment

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

Just a thought... When this is ready we may want to re-run ./support/build-api.sh to verify these changes, and check if there are new changes we want to include or updates to non-SM items we need to include.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 26, 2024 •
edited

Logo
Checkmarx One – Scan Summary & Details – 1b25a306-25a2-4855-9fd1-22d2792d33fb

No New Or Fixed Issues Found

@Thomas-Avery
Copy link
Contributor Author

Went ahead and regenerated the API bindings off of main, since this should be ready to go now.

Most of the SM additional changes are coming from recent access policy endpoint updates. Since we don't use those endpoints in the SDK should be fine to bring in here.

@Thomas-Avery Thomas-Avery marked this pull request as ready for review May 15, 2024 14:49
dani-garcia
dani-garcia approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

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

LGTM! Bindings match with a local build

@Thomas-Avery Thomas-Avery merged commit f07d9a7 into main May 15, 2024
104 of 105 checks passed
@Thomas-Avery Thomas-Avery deleted the sm/sm-1153-update-bitwarden-api branch May 15, 2024 16:31
Thomas-Avery added a commit that referenced this pull request May 15, 2024
@Thomas-Avery
[SM-1153] Part 2 Secrets sync | Expose in SM SDK (#678)
295d3c5 
## Type of change

<!-- (mark with an `X`) -->

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

## Objective

<!--Describe what the purpose of this PR is. For example: what bug
you're fixing or what new feature you're adding-->

The purpose of this PR is to expose the ability to preform secrets syncs
via the Secrets Manager SDK.


Requires API bindings in #674

This should not be merged into main prior to:

- The server PR bitwarden/server#3906 being
merged and released
- The API bindings PR #674 being
merged into main


## Code changes

<!--Explain the changes you've made to each file or major component.
This should help the reviewer understand your changes-->
<!--Also refer to any related changes or PRs in other repositories-->


- **crates/bitwarden/src/secrets_manager/client_secrets.rs:** 
Add the `sync` method to the client secrets.

 - **crates/bitwarden/src/secrets_manager/secrets/mod.rs:** 
Expose `sync` `SecretsSyncRequest`, and `SecretsSyncResponse`

 - **crates/bitwarden/src/secrets_manager/secrets/sync.rs:** 
Implement `sync` `SecretsSyncRequest`, and `SecretsSyncResponse`

## Before you submit

- Please add **unit tests** where it makes sense to do so
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dani-garcia dani-garcia dani-garcia approved these changes

@coltonhurst coltonhurst coltonhurst approved these changes

@Hinton Hinton Awaiting requested review from Hinton

Assignees

@Thomas-Avery Thomas-Avery

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Thomas-Avery @dani-garcia @coltonhurst