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

keep nlohmann::json private #371

Merged
merged 4 commits into from
Oct 25, 2023
Merged

keep nlohmann::json private #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c16323
changed userinfo_vc to not publicize nlohmann::json
Sep 13, 2023
cc9e878
moved nlohmann_json to an include dependency instead of a link depend…
Oct 23, 2023
9345027
construct string instead of json object
Oct 25, 2023
7e00079
Fixed partial updated test
Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/hpke/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,23 @@ find_package(OpenSSL 1.1 REQUIRED)
file(GLOB_RECURSE LIB_HEADERS CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/include/*.h")
file(GLOB_RECURSE LIB_SOURCES CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp")

# https://gitlab.kitware.com/cmake/cmake/-/issues/15415#note_334852
# Warning: this will fail once nlohman_json stops being header-only!
get_target_property(JSON_INCLUDE_INTERFACE nlohmann_json::nlohmann_json INTERFACE_INCLUDE_DIRECTORIES)

add_library(${CURRENT_LIB_NAME} ${LIB_HEADERS} ${LIB_SOURCES})
add_dependencies(${CURRENT_LIB_NAME} bytes tls_syntax)
target_link_libraries(${CURRENT_LIB_NAME}
target_include_directories(${CURRENT_LIB_NAME}
PRIVATE
nlohmann_json::nlohmann_json OpenSSL::Crypto
"${JSON_INCLUDE_INTERFACE}")

target_link_libraries(${CURRENT_LIB_NAME}
PUBLIC
bytes tls_syntax)
bytes tls_syntax
PRIVATE
OpenSSL::Crypto
)

target_include_directories(${CURRENT_LIB_NAME}
PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
Expand Down
5 changes: 2 additions & 3 deletions lib/hpke/include/hpke/userinfo_vc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include <chrono>
#include <hpke/signature.h>
#include <map>
#include <nlohmann/json.hpp>

using namespace MLS_NAMESPACE::bytes_ns;

Expand All @@ -21,7 +20,7 @@ struct UserInfoClaimsAddress
std::optional<std::string> postal_code;
std::optional<std::string> country;

static UserInfoClaimsAddress from_json(const nlohmann::json& address_json);
static UserInfoClaimsAddress from_json(const std::string& address);
};

struct UserInfoClaims
Expand All @@ -48,7 +47,7 @@ struct UserInfoClaims
std::optional<UserInfoClaimsAddress> address;
std::optional<uint64_t> updated_at;

static UserInfoClaims from_json(const nlohmann::json& cred_subject_json);
static UserInfoClaims from_json(const std::string& cred_subject);
};

struct UserInfoVC
Expand Down
14 changes: 9 additions & 5 deletions lib/hpke/src/userinfo_vc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ struct UserInfoVC::ParsedCredential
epoch_time(payload.at("nbf").get<int64_t>()),
epoch_time(payload.at("exp").get<int64_t>()),

UserInfoClaims::from_json(vc.at("credentialSubject")),
UserInfoClaims::from_json(vc.at("credentialSubject").dump()),
std::move(public_key),

to_be_signed,
Expand All @@ -282,8 +282,10 @@ struct UserInfoVC::ParsedCredential
/// UserInfoClaimsAddress
///
UserInfoClaimsAddress
UserInfoClaimsAddress::from_json(const nlohmann::json& address_json)
UserInfoClaimsAddress::from_json(const std::string& address)
{
const auto& address_json = nlohmann::json::parse(address);

return {
get_optional<std::string>(address_json, address_formatted_attr),
get_optional<std::string>(address_json, address_street_address_attr),
Expand All @@ -298,13 +300,15 @@ UserInfoClaimsAddress::from_json(const nlohmann::json& address_json)
/// UserInfoClaims
///
UserInfoClaims
UserInfoClaims::from_json(const nlohmann::json& cred_subject_json)
UserInfoClaims::from_json(const std::string& cred_subject)
{
const auto& cred_subject_json = nlohmann::json::parse(cred_subject);

std::optional<UserInfoClaimsAddress> address_opt = {};

if (cred_subject_json.contains(address_attr)) {
address_opt =
UserInfoClaimsAddress::from_json(cred_subject_json.at(address_attr));
address_opt = UserInfoClaimsAddress::from_json(
cred_subject_json.at(address_attr).dump());
Comment on lines +310 to +311
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this back and forth between strings and JSON. Maybe there's some way to have static functions that take json? So that they're hidden from callers but we can still use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the difficult thing about this library. If we have any use of our json library in the public api's then the consumer will have to build and link to our json library. If they use another library then they would have to convert. I think you understand that. I will think about the static function that might do the conversion. Out only options in the signature of a function is string or bytes. what do you have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I had in mind was something like:

// Standard from_json method so that `json::get` works
static void from_json(const json& j, UserInfoClaimsAddress& addr) {
  // current body of UserInfoClaimsAddress::from_json
}

static void from_json(const json& j, UserInfoClaims& addr) { 
  // current body of UserInfoClaims::from_json
  // including using get_optional<UserInfoClaimsAddress>(...)
}

// String methods just call through to JSON methods
UserInfoClaims
UserInfoClaims::from_json(const std::string& cred_subject) {
  return json::parse(cred_subject).get<UserInfoClaims>();
}

UserInfoClaimsAddress
UserInfoClaimsAddress::from_json(const std::string& address) {
  return json::parse(address).get<UserInfoClaimsAddress>();
}

}

return {
Expand Down
9 changes: 5 additions & 4 deletions lib/hpke/test/userinfo_vc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ TEST_CASE("UserInfoClaims Field Parsing")
{ "updated_at", 42 }
};

const auto userinfo_claims = UserInfoClaims::from_json(credentialSubject);
const auto userinfo_claims =
UserInfoClaims::from_json(credentialSubject.dump());
Comment on lines +178 to +179
Copy link
Contributor

Choose a reason for hiding this comment

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

Same raw string trick here. (Raw strings can span multiple lines.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using the original json as the known answer for the tests. The tests in that section seem to only test that the strings get shuffled around correctly.


CHECK(userinfo_claims.sub == credentialSubject.at("sub"));
CHECK(userinfo_claims.name == credentialSubject.at("name"));
Expand Down Expand Up @@ -214,14 +215,14 @@ TEST_CASE("UserInfoClaims Field Parsing")
TEST_CASE("UserInfoClaims Edge Cases")
{
CHECK_THROWS_WITH(
UserInfoClaims::from_json({ { "updated_at", "42" } }),
UserInfoClaims::from_json(R"({"updated_at": "42"})"),
"[json.exception.type_error.302] type must be number, but is string");

CHECK_THROWS_WITH(
UserInfoClaims::from_json({ { "name", true } }),
UserInfoClaims::from_json(R"({"name": true})"),
"[json.exception.type_error.302] type must be string, but is boolean");

CHECK_THROWS_WITH(
UserInfoClaims::from_json({ { "email_verified", "true" } }),
UserInfoClaims::from_json(R"({"email_verified": "true"})"),
"[json.exception.type_error.302] type must be boolean, but is string");
}