fido: make PublicKeyCredentialRpEntity a struct

This aligns it with e.g. PublicKeyCredentialUserEntity.

Change-Id: I907d97d03e529eb44fcc14e9cf5ce0cd9ab77f27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1610656Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#661129}

content/browser/webauth/authenticator_type_converters.cc

@@ -168,10 +168,10 @@ TypeConverter<::device::PublicKeyCredentialRpEntity,
               PublicKeyCredentialRpEntityPtr>::
     Convert(const PublicKeyCredentialRpEntityPtr& input) {
   device::PublicKeyCredentialRpEntity rp_entity(input->id);
-  rp_entity.SetRpName(input->name);
-  if (input->icon)
-    rp_entity.SetRpIconUrl(*input->icon);
-
+  rp_entity.name = input->name;
+  if (input->icon) {
+    rp_entity.icon_url = input->icon;
+  }
   return rp_entity;
 }
 

device/fido/ctap_make_credential_request.cc

@@ -47,7 +47,8 @@ CtapMakeCredentialRequest::EncodeAsCBOR(
     const CtapMakeCredentialRequest& request) {
   cbor::Value::MapValue cbor_map;
   cbor_map[cbor::Value(1)] = cbor::Value(request.client_data_hash);
-  cbor_map[cbor::Value(2)] = request.rp.ConvertToCBOR();
+  cbor_map[cbor::Value(2)] =
+      PublicKeyCredentialRpEntity::ConvertToCBOR(request.rp);
   cbor_map[cbor::Value(3)] =
       PublicKeyCredentialUserEntity::ConvertToCBOR(request.user);
   cbor_map[cbor::Value(4)] =

device/fido/ctap_request_unittest.cc

@@ -20,7 +20,7 @@ namespace device {
 // https://fidoalliance.org/specs/fido-v2.0-rd-20170927/fido-client-to-authenticator-protocol-v2.0-rd-20170927.html
 TEST(CTAPRequestTest, TestConstructMakeCredentialRequestParam) {
   PublicKeyCredentialRpEntity rp("acme.com");
-  rp.SetRpName("Acme");
+  rp.name = "Acme";
 
   PublicKeyCredentialUserEntity user(
       fido_parsing_utils::Materialize(test_data::kUserId));
@@ -98,8 +98,8 @@ TEST(VirtualCtap2DeviceTest, ParseMakeCredentialRequestForVirtualCtapKey) {
   auto client_data_hash = std::get<1>(*request_and_hash);
   EXPECT_THAT(client_data_hash,
               ::testing::ElementsAreArray(test_data::kClientDataHash));
-  EXPECT_EQ(test_data::kRelyingPartyId, request.rp.rp_id());
-  EXPECT_EQ("Acme", request.rp.rp_name());
+  EXPECT_EQ(test_data::kRelyingPartyId, request.rp.id);
+  EXPECT_EQ("Acme", request.rp.name);
   EXPECT_THAT(request.user.id, ::testing::ElementsAreArray(test_data::kUserId));
   ASSERT_TRUE(request.user.name);
   EXPECT_EQ("johnpsmith@example.com", *request.user.name);

device/fido/mac/make_credential_operation.mm

@@ -45,7 +45,7 @@ MakeCredentialOperation::MakeCredentialOperation(
 MakeCredentialOperation::~MakeCredentialOperation() = default;
 
 const std::string& MakeCredentialOperation::RpId() const {
-  return request().rp.rp_id();
+  return request().rp.id;
 }
 
 void MakeCredentialOperation::Run() {

device/fido/make_credential_request_handler.cc

@@ -340,8 +340,7 @@ void MakeCredentialRequestHandler::HandleResponse(
     return;
   }
 
-  const auto rp_id_hash =
-      fido_parsing_utils::CreateSHA256Hash(request_.rp.rp_id());
+  const auto rp_id_hash = fido_parsing_utils::CreateSHA256Hash(request_.rp.id);
 
   if (!response || response->GetRpIdHash() != rp_id_hash) {
     FIDO_LOG(ERROR) << "Failing assertion request due to bad response from "

device/fido/make_credential_task.cc

@@ -125,7 +125,7 @@ void MakeCredentialTask::StartTask() {
 CtapGetAssertionRequest MakeCredentialTask::NextSilentSignRequest() {
   DCHECK(request_.exclude_list &&
          current_credential_ < request_.exclude_list->size());
-  CtapGetAssertionRequest request(request_.rp.rp_id(),
+  CtapGetAssertionRequest request(request_.rp.id,
                                   /*client_data_json=*/"");
   request.allow_list = {{request_.exclude_list->at(current_credential_)}};
   request.user_presence_required = false;

device/fido/public_key_credential_rp_entity.cc

@@ -14,41 +14,52 @@ namespace device {
 // static
 base::Optional<PublicKeyCredentialRpEntity>
 PublicKeyCredentialRpEntity::CreateFromCBORValue(const cbor::Value& cbor) {
-  if (!cbor.is_map() || cbor.GetMap().size() > 3)
+  if (!cbor.is_map() || cbor.GetMap().size() > 3) {
     return base::nullopt;
-
-  const auto& rp_map = cbor.GetMap();
-  bool is_rp_map_format_correct =
-      std::all_of(rp_map.begin(), rp_map.end(), [](const auto& element) {
-        if (!element.first.is_string() || !element.second.is_string())
-          return false;
-
-        const auto& key = element.first.GetString();
-        return (key == kEntityIdMapKey || key == kEntityNameMapKey ||
-                key == kIconUrlMapKey);
-      });
-
-  if (!is_rp_map_format_correct)
-    return base::nullopt;
-
-  const auto& id_it = rp_map.find(cbor::Value(kEntityIdMapKey));
-  const auto& name_it = rp_map.find(cbor::Value(kEntityNameMapKey));
-  const auto& icon_it = rp_map.find(cbor::Value(kIconUrlMapKey));
-  if (id_it == rp_map.end())
+  }
+  const cbor::Value::MapValue& rp_map = cbor.GetMap();
+  for (const auto& element : rp_map) {
+    if (!element.first.is_string() || !element.second.is_string()) {
+      return base::nullopt;
+    }
+    const std::string& key = element.first.GetString();
+    if (key != kEntityIdMapKey && key != kEntityNameMapKey &&
+        key != kIconUrlMapKey) {
+      return base::nullopt;
+    }
+  }
+  const auto id_it = rp_map.find(cbor::Value(kEntityIdMapKey));
+  if (id_it == rp_map.end()) {
     return base::nullopt;
+  }
   PublicKeyCredentialRpEntity rp(id_it->second.GetString());
-
-  if (name_it != rp_map.end())
-    rp.SetRpName(name_it->second.GetString());
-
-  if (icon_it != rp_map.end())
-    rp.SetRpIconUrl(GURL(icon_it->second.GetString()));
-
+  const auto name_it = rp_map.find(cbor::Value(kEntityNameMapKey));
+  if (name_it != rp_map.end()) {
+    rp.name = name_it->second.GetString();
+  }
+  const auto icon_it = rp_map.find(cbor::Value(kIconUrlMapKey));
+  if (icon_it != rp_map.end()) {
+    rp.icon_url = GURL(icon_it->second.GetString());
+  }
   return rp;
 }
 
+// static
+cbor::Value PublicKeyCredentialRpEntity::ConvertToCBOR(
+    const PublicKeyCredentialRpEntity& rp) {
+  cbor::Value::MapValue rp_map;
+  rp_map.emplace(kEntityIdMapKey, rp.id);
+  if (rp.name) {
+    rp_map.emplace(kEntityNameMapKey, *rp.name);
+  }
+  if (rp.icon_url) {
+    rp_map.emplace(kIconUrlMapKey, rp.icon_url->spec());
+  }
+  return cbor::Value(std::move(rp_map));
+}
+
 PublicKeyCredentialRpEntity::PublicKeyCredentialRpEntity(std::string rp_id)
-    : rp_id_(std::move(rp_id)) {}
+    : id(std::move(rp_id)) {}
 
 PublicKeyCredentialRpEntity::PublicKeyCredentialRpEntity(
     const PublicKeyCredentialRpEntity& other) = default;
@@ -64,28 +75,4 @@ PublicKeyCredentialRpEntity& PublicKeyCredentialRpEntity::operator=(
 
 PublicKeyCredentialRpEntity::~PublicKeyCredentialRpEntity() = default;
 
-PublicKeyCredentialRpEntity& PublicKeyCredentialRpEntity::SetRpName(
-    std::string rp_name) {
-  rp_name_ = std::move(rp_name);
-  return *this;
-}
-
-PublicKeyCredentialRpEntity& PublicKeyCredentialRpEntity::SetRpIconUrl(
-    GURL icon_url) {
-  rp_icon_url_ = std::move(icon_url);
-  return *this;
-}
-
-cbor::Value PublicKeyCredentialRpEntity::ConvertToCBOR() const {
-  cbor::Value::MapValue rp_map;
-  rp_map.emplace(kEntityIdMapKey, rp_id_);
-  if (rp_name_)
-    rp_map.emplace(kEntityNameMapKey, *rp_name_);
-
-  if (rp_icon_url_)
-    rp_map.emplace(kIconUrlMapKey, rp_icon_url_->spec());
-
-  return cbor::Value(std::move(rp_map));
-}
-
 }  // namespace device

device/fido/public_key_credential_rp_entity.h

@@ -16,13 +16,15 @@
 
 namespace device {
 
-// Data structure containing information about relying party that invoked
-// WebAuth API. Includes a relying party id, an optional relying party name,,
-// and optional relying party display image url.
-class COMPONENT_EXPORT(DEVICE_FIDO) PublicKeyCredentialRpEntity {
+// PublicKeyCredentialRpEntity identifies the web application creating or
+// challenging a WebAuthn credential.
+//
+// https://www.w3.org/TR/webauthn/#dictdef-publickeycredentialrpentity
+struct COMPONENT_EXPORT(DEVICE_FIDO) PublicKeyCredentialRpEntity {
  public:
   static base::Optional<PublicKeyCredentialRpEntity> CreateFromCBORValue(
       const cbor::Value& cbor);
+  static cbor::Value ConvertToCBOR(const PublicKeyCredentialRpEntity&);
 
   explicit PublicKeyCredentialRpEntity(std::string rp_id);
   PublicKeyCredentialRpEntity(const PublicKeyCredentialRpEntity& other);
@@ -32,19 +34,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) PublicKeyCredentialRpEntity {
   PublicKeyCredentialRpEntity& operator=(PublicKeyCredentialRpEntity&& other);
   ~PublicKeyCredentialRpEntity();
 
-  cbor::Value ConvertToCBOR() const;
-
-  PublicKeyCredentialRpEntity& SetRpName(std::string rp_name);
-  PublicKeyCredentialRpEntity& SetRpIconUrl(GURL icon_url);
-
-  const std::string& rp_id() const { return rp_id_; }
-  const base::Optional<std::string>& rp_name() const { return rp_name_; }
-  const base::Optional<GURL>& rp_icon_url() const { return rp_icon_url_; }
-
- private:
-  std::string rp_id_;
-  base::Optional<std::string> rp_name_;
-  base::Optional<GURL> rp_icon_url_;
+  std::string id;
+  base::Optional<std::string> name;
+  base::Optional<GURL> icon_url;
 };
 
 }  // namespace device

device/fido/u2f_command_constructor.cc

@@ -49,7 +49,7 @@ base::Optional<std::vector<uint8_t>> ConvertToU2fRegisterCommand(
       request.attestation_preference ==
       AttestationConveyancePreference::ENTERPRISE;
   return ConstructU2fRegisterCommand(
-      fido_parsing_utils::CreateSHA256Hash(request.rp.rp_id()),
+      fido_parsing_utils::CreateSHA256Hash(request.rp.id),
       request.client_data_hash, is_invidual_attestation);
 }
 
@@ -57,7 +57,7 @@ base::Optional<std::vector<uint8_t>> ConvertToU2fSignCommand(
     const CtapMakeCredentialRequest& request,
     base::span<const uint8_t> key_handle) {
   return ConstructU2fSignCommand(
-      fido_parsing_utils::CreateSHA256Hash(request.rp.rp_id()),
+      fido_parsing_utils::CreateSHA256Hash(request.rp.id),
       request.client_data_hash, key_handle);
 }
 

device/fido/u2f_command_constructor_unittest.cc

@@ -20,7 +20,7 @@ namespace {
 
 CtapMakeCredentialRequest ConstructMakeCredentialRequest() {
   PublicKeyCredentialRpEntity rp("acme.com");
-  rp.SetRpName("acme.com");
+  rp.name = "acme.com";
 
   PublicKeyCredentialUserEntity user(
       fido_parsing_utils::Materialize(test_data::kUserId));
@@ -73,7 +73,7 @@ TEST(U2fCommandConstructorTest, TestConvertCtapMakeCredentialToU2fRegister) {
 
 TEST(U2fCommandConstructorTest, TestU2fRegisterCredentialAlgorithmRequirement) {
   PublicKeyCredentialRpEntity rp("acme.com");
-  rp.SetRpName("acme.com");
+  rp.name = "acme.com";
 
   PublicKeyCredentialUserEntity user(
       fido_parsing_utils::Materialize(test_data::kUserId));

device/fido/u2f_register_operation.cc

@@ -147,7 +147,7 @@ void U2fRegisterOperation::OnRegisterResponseReceived(
       auto response =
           AuthenticatorMakeCredentialResponse::CreateFromU2fRegisterResponse(
               device()->DeviceTransport(),
-              fido_parsing_utils::CreateSHA256Hash(request().rp.rp_id()),
+              fido_parsing_utils::CreateSHA256Hash(request().rp.id),
               apdu_response->data());
       std::move(callback())
           .Run(CtapDeviceResponseCode::kSuccess, std::move(response));

device/fido/virtual_ctap2_device.cc

@@ -604,8 +604,7 @@ CtapDeviceResponseCode VirtualCtap2Device::OnMakeCredential(
   }
 
   // 6. Check for already registered credentials.
-  const auto rp_id_hash =
-      fido_parsing_utils::CreateSHA256Hash(request.rp.rp_id());
+  const auto rp_id_hash = fido_parsing_utils::CreateSHA256Hash(request.rp.id);
   if (request.exclude_list) {
     if (config_.reject_large_allow_and_exclude_lists &&
         request.exclude_list->size() > 1) {
@@ -1302,7 +1301,7 @@ void VirtualCtap2Device::InitPendingRPs() {
     DCHECK(!registration.second.is_u2f);
     DCHECK(registration.second.user);
     DCHECK(registration.second.rp);
-    if (!base::ContainsKey(rp_ids, registration.second.rp->rp_id())) {
+    if (!base::ContainsKey(rp_ids, registration.second.rp->id)) {
       mutable_state()->pending_rps.push_back(*registration.second.rp);
     }
   }
@@ -1345,11 +1344,12 @@ void VirtualCtap2Device::InitPendingRegistrations(
 void VirtualCtap2Device::GetNextRP(cbor::Value::MapValue* response_map) {
   DCHECK(!mutable_state()->pending_rps.empty());
   response_map->emplace(static_cast<int>(CredentialManagementResponseKey::kRP),
-                        mutable_state()->pending_rps.front().ConvertToCBOR());
+                        PublicKeyCredentialRpEntity::ConvertToCBOR(
+                            mutable_state()->pending_rps.front()));
   response_map->emplace(
       static_cast<int>(CredentialManagementResponseKey::kRPIDHash),
       fido_parsing_utils::CreateSHA256Hash(
-          mutable_state()->pending_rps.front().rp_id()));
+          mutable_state()->pending_rps.front().id));
   mutable_state()->pending_rps.pop_front();
 }
 

device/fido/win/webauthn_api.cc

@@ -196,9 +196,9 @@ AuthenticatorMakeCredentialBlocking(WinWebAuthnApi* webauthn_api,
                                     CtapMakeCredentialRequest request) {
   DCHECK(webauthn_api->IsAvailable());
 
-  base::string16 rp_id = base::UTF8ToUTF16(request.rp.rp_id());
-  base::string16 rp_name = base::UTF8ToUTF16(request.rp.rp_name().value_or(""));
-  base::string16 rp_icon_url = OptionalGURLToUTF16(request.rp.rp_icon_url());
+  base::string16 rp_id = base::UTF8ToUTF16(request.rp.id);
+  base::string16 rp_name = base::UTF8ToUTF16(request.rp.name.value_or(""));
+  base::string16 rp_icon_url = OptionalGURLToUTF16(request.rp.icon_url);
   WEBAUTHN_RP_ENTITY_INFORMATION rp_info{
       WEBAUTHN_RP_ENTITY_INFORMATION_CURRENT_VERSION, base::as_wcstr(rp_id),
       base::as_wcstr(rp_name), base::as_wcstr(rp_icon_url)};