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.

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

Cpp Wrapper for sdk #259

Merged
merged 37 commits into from
Nov 30, 2023
Merged

Conversation

slobokv83
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

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

Code changes

  • Implemented BitwardenLibrary for connection with bitwarden_c library and its three functions: init, run_command, and free_mem
  • Implemented CRUD operations for the Projects and Secrets for Bitwarden Secrets Manager using the API
  • Developed building mechanism that is building C++ dynamic library using Cmake
  • Added example in the examples directory
  • Added documentation in .md files for CRUD operations use, building the dynamic library, and for the client use

@slobokv83 slobokv83 requested a review from a team as a code owner September 29, 2023 14:30
@dani-garcia
Copy link
Member

Some comments about the current PR, plus solutions for some of the issues brought up on the last email:

We should update the schema generation, to something like this:

  const cpp = await quicktype({
    inputData,
    lang: "cpp",
    rendererOptions: {
      namespace: "Bitwarden::Sdk",
      "include-location": "global-include",
    },
  });

  cpp.lines.forEach((line, idx) => {
    // Replace DOMAIN for URI_DOMAIN, because DOMAIN is an already defined macro
    cpp.lines[idx] = line.replace(/DOMAIN/g, "URI_DOMAIN");
  });

  writeToFile("./languages/cpp/include/schemas.hpp", cpp.lines);

With this change we generate a hpp file instead of cpp, which should solve that warning when including a cpp file. At the same time we're doing the DOMAIN rename to avoid conflicts with the predefined macro. Ultimately we might decide to rename this from our side on all the clients, but for now this should work.

I've also added two parameters to the schema generation:

  • Changed the namespace from the default quicktype to Bitwarden::Sdk. This will require updating the code and changing a few using namespace quicktype; to using namespace Bitwarden::Sdk; and quicktype::to_json to Bitwarden::Sdk::to_json
  • Changed the include location of the JSON to be global (#include <nlohmann/json.hpp>) instead of local (#include "json.hpp"), which I think makes more sense.

About the errors when compiling in release mode vs debug mode, make sure that the schemas and the libraries are generated with the same version of the SDK. The object field was removed from the ProjectResponse in the SDK about a month ago: https://github.com/bitwarden/sdk/pull/242/files#diff-3d8ec8aaa44ddda5d8b6751bcd05fa6ed203dcea8a4ef67693ca99aeaf2d0634L37.
So if the schemas were generated before then it can cause issues.


On another topic, I had some compile errors when trying to test the example. Don't know if this is because whatever clang version I use, but I had to wrap the strings in std::string() in Wrapper.cpp:

    clientSettings.set_api_url(std::string("https://bitwarden.test:8080/api"));
    clientSettings.set_identity_url(std::string("https://bitwarden.test:8080/identity"));
    clientSettings.set_user_agent(std::string("Bitwarden CPP-SDK"));

The commands I've used to compile this on my Mac, for reference:

brew install cmake
brew install boost
brew install nlohmann-json

# Run this from the cpp directory

## Compile the library
mkdir -p build
cd build
cmake .. -DNLOHMANN=/opt/homebrew/include -DBOOST=/opt/homebrew/include -DTARGET=../../target/release/libbitwarden_c.dylib
cmake --build .

## Compile the example
cd ../examples
clang++ -std=c++20 -I../include -I/opt/homebrew/include -I/opt/homebrew/include -L../build/ -o MyBitwardenApp Wrapper.cpp -lBitwardenClient -ldl

# Using DYLD_LIBRARY_PATH to avoid having to copy the library
ACCESS_TOKEN=... ORGANIZATION_ID=... DYLD_LIBRARY_PATH=../build ./MyBitwardenApp

@slobokv83
Copy link
Contributor Author

slobokv83 commented Oct 16, 2023 via email

@slobokv83
Copy link
Contributor Author

slobokv83 commented Oct 23, 2023

Hi Daniel,

  • schema generation is updated as you suggested
  • schemas.hpp is now included instead of schemas.cpp
  • namespace Bitwarden::Sdk is used instead of quicktype
  • in the ExampleUse.md is described how to set the env variable for the path of the dynamic library (DYLD_LIBRARY_PATH for MacOS)
  • declarations are put for the clientSettings in the examples/Wrapper.cpp:,

boost::optional<std::string> apiUrl("https://api.bitwarden.com");
boost::optional<std::string> identityUrl("https://identity.bitwarden.com");
boost::optional<std::string> user_agent("Bitwarden CPP-SDK");

  • in the README.md, the description of the clientSettings is also adapted

Please let me know if anything else should be changed.

Kindly,
Slobodan

.vscode/c_cpp_properties.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
languages/cpp/ExampleUse.md Outdated Show resolved Hide resolved
languages/cpp/ExampleUse.md Outdated Show resolved Hide resolved
languages/cpp/include/CommandRunner.h Outdated Show resolved Hide resolved
.github/workflows/build-cli.yml Outdated Show resolved Hide resolved
dani-garcia
dani-garcia previously approved these changes Nov 20, 2023
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, please solve the merge conflict, otherwise this is good to go for me

@bitwarden-bot
Copy link

bitwarden-bot commented Nov 20, 2023

Logo
Checkmarx One – Scan Summary & Details62e7430f-8f41-41d0-bb14-d8ea750ab3c3

No New Or Fixed Issues Found

@slobokv83
Copy link
Contributor Author

slobokv83 commented Nov 20, 2023

@dani-garcia

LGTM, please solve the merge conflict, otherwise this is good to go for me

It is resolved, I retrieved the previous settings.json.

dani-garcia
dani-garcia previously approved these changes Nov 20, 2023
@vgrassia vgrassia changed the title Cpp wrapper for sdk Cpp Wrapper for sdk Nov 20, 2023
@vgrassia vgrassia removed the request for review from a team November 20, 2023 17:33
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.

Some small changes to match the rest of the implementations

languages/cpp/src/Secrets.cpp Outdated Show resolved Hide resolved
languages/cpp/src/BitwardenClient.cpp Outdated Show resolved Hide resolved
dani-garcia
dani-garcia previously approved these changes Nov 29, 2023
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@dani-garcia dani-garcia merged commit 0f5f11b into bitwarden:master Nov 30, 2023
49 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants