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

.NET wrapper for SDK #193

Merged
merged 114 commits into from
Sep 21, 2023
Merged

.NET wrapper for SDK #193

merged 114 commits into from
Sep 21, 2023

Conversation

bbujak
Copy link
Contributor

@bbujak bbujak commented Aug 26, 2023 •
edited
Loading

Type of change

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

Objective

Implemented .NET library that wraps native C library and exposed its commands through BitwardenClient class.

Code changes

  • Created a solution and 2 separated projects Bitwarden.Sdk (lib) and Bitwarden.Sdk.Samples (examples)
  • DeviceType and UserAgent are set in the constructor and can't be changed
  • added possibility to login using access token
  • added functionality for CRUD operations for ProjectsClient and SecretsClient
  • added GitHub action to build rust cross os/arch
  • added GitHub action to build .NET on pull request created
  • added GitHub action to deploy to NuGet on demand
  • added Bitwarden icon and readme file to be used in NuGet package
  • added formatting config file

justindbaur
justindbaur requested changes Aug 30, 2023
View reviewed changes
Copy link
Member

@justindbaur justindbaur left a comment

Choose a reason for hiding this comment

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

I would also like to see us add the C# styles from bitwarden/server and add them into the root editor config and run dotnet format. I would especially like a consistent bracing style and we use the C# (opening curly brace on new line) in server. https://github.com/bitwarden/server/blob/e679d3127abfd976e3af3d02783d869da77fb590/.editorconfig#L34-L125

languages/csharp/BitwardenClient.cs Outdated Show resolved Hide resolved
languages/csharp/BitwardenLibrary.cs Outdated Show resolved Hide resolved
languages/csharp/Projects.cs Outdated Show resolved Hide resolved
languages/csharp/Secrets.cs Outdated Show resolved Hide resolved
languages/csharp/Settings.cs Outdated Show resolved Hide resolved
languages/csharp/BitwardenClient.cs Outdated Show resolved Hide resolved
Boris Bujak added 2 commits August 30, 2023 16:34
- added editorconfig, code reformated
6d94e77 
- created SafeHandle to be used
- made BitwardenLibrary methods private and added wrapper
- rename Settings to BitwardenSettings
- rename Projects to ProjectsClient
- rename Secrets to SecretsClient
Merge remote-tracking branch 'origin/feature/csharp_-wrapper' into fe…
634cc18 
…ature/csharp_-wrapper
@bbujak bbujak requested a review from justindbaur August 30, 2023 19:32
@bbujak bbujak requested a review from vgrassia September 13, 2023 19:38
justindbaur
justindbaur reviewed Sep 15, 2023
View reviewed changes
Copy link
Member

@justindbaur justindbaur left a comment

Choose a reason for hiding this comment

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

Other than my one comment, this has my approval. Very good work.

languages/csharp/Bitwarden.Sdk/BitwardenClient.cs Outdated Show resolved Hide resolved
@justindbaur
Copy link
Member

@AHollowedHunter Thank you for the interest. I'm very open to setting up a multi-target for .NET Standard 2.0. As it stands now, our only dependency is System.Text.Json which targets net standard. I think we are going to go forward with just the .NET 6 for now and get it working and where we want then we can evaluate how easy it is to just drop in net standard support.

I don't think nullable should be a roadblock there. We just released Passwordless nuget package that targets .NET Standard 2.0 and has nullable enabled.

@AHollowedHunter
Copy link

@AHollowedHunter Thank you for the interest. I'm very open to setting up a multi-target for .NET Standard 2.0...

Good to know, thanks for the info :) I'm going to try out this package on some .NET 6 stuff first and I'll provide any more feedback as appropriate :)

@bbujak bbujak requested a review from justindbaur September 15, 2023 18:51
Hinton
Hinton reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@Hinton Hinton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is starting to wrap up. A few remaining things:

  • ClientSettings cleanup.
  • Methods should unwrap the result and handle errors appropriately. In .NET this means throwing.

languages/csharp/Bitwarden.Sdk/BitwardenClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk/BitwardenClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk/ProjectsClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk/SecretsClient.cs Outdated Show resolved Hide resolved
@bbujak bbujak requested a review from Hinton September 18, 2023 19:57
@lukpotSym lukpotSym mentioned this pull request Sep 19, 2023
Merged
Hinton
Hinton reviewed Sep 19, 2023
View reviewed changes
languages/csharp/Bitwarden.Sdk/BitwardenClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk/SecretsClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk/ProjectsClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk/ProjectsClient.cs Outdated Show resolved Hide resolved
@bbujak bbujak requested a review from Hinton September 19, 2023 14:20
Hinton
Hinton reviewed Sep 19, 2023
View reviewed changes
Copy link
Member

@Hinton Hinton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Last round of changes and we should be good.

Please also run dotnet format in the root of languages/csharp which will format the files per the editorconfig.

languages/csharp/Bitwarden.Sdk/ProjectsClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk/SecretsClient.cs Outdated Show resolved Hide resolved
languages/csharp/Bitwarden.Sdk.Samples/Program.cs Outdated Show resolved Hide resolved
languages/csharp/README.md Outdated Show resolved Hide resolved
languages/csharp/README.md Show resolved Hide resolved
@bbujak bbujak requested a review from Hinton September 19, 2023 20:10
@Hinton
Copy link
Member

Hinton commented Sep 21, 2023

Please run dotnet format in the root of languages/csharp which will format the files per the editorconfig.

@Hinton
Copy link
Member

Hinton commented Sep 21, 2023

@vgrassia can you review the workflows again and approve if they look good?

@Hinton Hinton enabled auto-merge (squash) September 21, 2023 18:31
@Hinton Hinton merged commit 662a92d into bitwarden:master Sep 21, 2023
72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justindbaur justindbaur justindbaur approved these changes

@vgrassia vgrassia vgrassia approved these changes

@Hinton Hinton Hinton approved these changes

@AHollowedHunter AHollowedHunter Awaiting requested review from AHollowedHunter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bbujak @AHollowedHunter @justindbaur @Hinton @vgrassia