From: Miod Vallat Date: Wed, 27 Aug 2025 12:32:37 +0000 (+0200) Subject: Rework patchZone() signature and split it in multiple pieces. NFC X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=67161693f5896ff581df6684bda1e61758e78be1;p=thirdparty%2Fpdns.git Rework patchZone() signature and split it in multiple pieces. NFC Signed-off-by: Miod Vallat --- diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc index d9644fcfa..35e3df664 100644 --- a/pdns/ws-auth.cc +++ b/pdns/ws-auth.cc @@ -92,7 +92,7 @@ double Ewma::getMax() const return d_max; } -static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInfo& domainInfo, HttpRequest* req, HttpResponse* resp); +static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInfo& domainInfo, const vector& rrsets, HttpResponse* resp); // QTypes that MUST NOT have multiple records of the same type in a given RRset. static const std::set onlyOneEntryTypes = {QType::CNAME, QType::DNAME, QType::SOA}; @@ -2254,7 +2254,14 @@ static void apiServerZoneDetailDELETE(HttpRequest* req, HttpResponse* resp) static void apiServerZoneDetailPATCH(HttpRequest* req, HttpResponse* resp) { ZoneData zoneData{req}; - patchZone(zoneData.backend, zoneData.zoneName, zoneData.domainInfo, req, resp); + Json document = req->json(); + + auto rrsets = document["rrsets"]; + if (!rrsets.is_array()) { + throw ApiException("No rrsets given in update request"); + } + + patchZone(zoneData.backend, zoneData.zoneName, zoneData.domainInfo, rrsets.array_items(), resp); } static void apiServerZoneDetailGET(HttpRequest* req, HttpResponse* resp) @@ -2332,25 +2339,67 @@ static void apiServerZoneRectify(HttpRequest* req, HttpResponse* resp) resp->setSuccessResult("Rectified"); } -// NOLINTNEXTLINE(readability-function-cognitive-complexity): TODO Refactor this function. -static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInfo& domainInfo, HttpRequest* req, HttpResponse* resp) +// Validate the "changetype" field of a Json patch record. +// Returns true if this is a replace operation, false if this is a delete +// operation. +// Throws an exception if unrecognized. +static bool validateChangeType(const Json& rrset) { - bool zone_disabled = false; - SOAData soaData; - - vector new_records; - vector new_comments; - vector new_ptrs; + string changetype = toUpper(stringFromJson(rrset, "changetype")); + if (changetype == "DELETE") { + return false; + } + if (changetype == "REPLACE") { + return true; + } + throw ApiException("Changetype not understood"); +} - Json document = req->json(); +// Replace the rrset for `qname' in zone `zonename' with the contents of +// `new_records', making sure to remove no longer needed ENT entries, and +// also enforcing the exclusivity rules (at most one CNAME, DNAME and SOA, +// etc). +static void replaceZoneRecords(DomainInfo& domainInfo, const ZoneName& zonename, vector& new_records, const DNSName& qname, const QType qtype) +{ + bool ent_present = false; + bool dname_seen = qtype == QType::DNAME; + bool ns_seen = qtype == QType::NS; - auto rrsets = document["rrsets"]; - if (!rrsets.is_array()) { - throw ApiException("No rrsets given in update request"); + domainInfo.backend->APILookup(QType(QType::ANY), qname, static_cast(domainInfo.id), false); + DNSResourceRecord resourceRecord; + while (domainInfo.backend->get(resourceRecord)) { + if (resourceRecord.qtype.getCode() == QType::ENT) { + ent_present = true; + // that's fine, we will override it + continue; + } + dname_seen |= resourceRecord.qtype == QType::DNAME; + ns_seen |= resourceRecord.qtype == QType::NS; + if (qtype.getCode() != resourceRecord.qtype.getCode() + && (exclusiveEntryTypes.count(qtype.getCode()) != 0 + || exclusiveEntryTypes.count(resourceRecord.qtype.getCode()) != 0)) { + // leave database handle in a consistent state + domainInfo.backend->lookupEnd(); + throw ApiException("RRset " + qname.toString() + " IN " + qtype.toString() + ": Conflicts with pre-existing RRset"); + } + } + if (dname_seen && ns_seen && qname != zonename.operator const DNSName&()) { + throw ApiException("RRset " + qname.toString() + " IN " + qtype.toString() + ": Cannot have both NS and DNAME except in zone apex"); } + if (!new_records.empty() && ent_present) { + QType qt_ent{QType::ENT}; + if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qt_ent, new_records)) { + throw ApiException("Hosting backend does not support editing records."); + } + } + if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qtype, new_records)) { + throw ApiException("Hosting backend does not support editing records."); + } +} +static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInfo& domainInfo, const vector& rrsets, HttpResponse* resp) +{ domainInfo.backend->startTransaction(zonename); - try { string soa_edit_api_kind; string soa_edit_kind; @@ -2358,10 +2407,12 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf domainInfo.backend->getDomainMetadataOne(zonename, "SOA-EDIT", soa_edit_kind); bool soa_edit_done = false; - set> seen; + vector new_records; + vector new_comments; + set> seen; - for (const auto& rrset : rrsets.array_items()) { - string changetype = toUpper(stringFromJson(rrset, "changetype")); + for (const auto& rrset : rrsets) { + bool isReplaceOperation = validateChangeType(rrset); DNSName qname = apiNameToDNSName(stringFromJson(rrset, "name")); apiCheckQNameAllowedCharacters(qname.toString()); QType qtype; @@ -2370,18 +2421,22 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf throw ApiException("RRset " + qname.toString() + " IN " + stringFromJson(rrset, "type") + ": unknown type given"); } - if (seen.count({qname, qtype, changetype}) != 0) { - throw ApiException("Duplicate RRset " + qname.toString() + " IN " + qtype.toString() + " with changetype: " + changetype); + if (seen.count({qname, qtype, isReplaceOperation}) != 0) { + throw ApiException("Duplicate RRset " + qname.toString() + " IN " + qtype.toString() + " with changetype: " + (isReplaceOperation ? "replace" : "delete")); } - seen.insert({qname, qtype, changetype}); + seen.insert({qname, qtype, isReplaceOperation}); - if (changetype == "DELETE") { + if (!isReplaceOperation) { // delete all matching qname/qtype RRs (and, implicitly comments). if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qtype, vector())) { throw ApiException("Hosting backend does not support editing records."); } } - else if (changetype == "REPLACE") { + else { + if (domainInfo.kind == DomainInfo::Consumer) { + // Allow deleting all RRsets, just not modifying them. + throw ApiException("Modifying RRsets in Consumer zones is unsupported"); + } // we only validate for REPLACE, as DELETE can be used to "fix" out of zone records. if (!qname.isPartOf(zonename)) { throw ApiException("RRset " + qname.toString() + " IN " + qtype.toString() + ": Name is out of zone"); @@ -2399,7 +2454,7 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf try { if (replace_records) { - // ttl shouldn't be part of DELETE, and it shouldn't be required if we don't get new records. + // ttl shouldn't be required if we don't get new records. uint32_t ttl = uintFromJson(rrset, "ttl"); gatherRecords(rrset, qname, qtype, ttl, new_records); @@ -2425,51 +2480,7 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf } if (replace_records) { - bool ent_present = false; - bool dname_seen = false; - bool ns_seen = false; - - domainInfo.backend->APILookup(QType(QType::ANY), qname, static_cast(domainInfo.id), false); - DNSResourceRecord resourceRecord; - while (domainInfo.backend->get(resourceRecord)) { - if (resourceRecord.qtype.getCode() == QType::ENT) { - ent_present = true; - /* that's fine, we will override it */ - continue; - } - if (qtype == QType::DNAME || resourceRecord.qtype == QType::DNAME) { - dname_seen = true; - } - if (qtype == QType::NS || resourceRecord.qtype == QType::NS) { - ns_seen = true; - } - if (qtype.getCode() != resourceRecord.qtype.getCode() - && (exclusiveEntryTypes.count(qtype.getCode()) != 0 - || exclusiveEntryTypes.count(resourceRecord.qtype.getCode()) != 0)) { - - // leave database handle in a consistent state - domainInfo.backend->lookupEnd(); - - throw ApiException("RRset " + qname.toString() + " IN " + qtype.toString() + ": Conflicts with pre-existing RRset"); - } - } - - if (dname_seen && ns_seen && qname != zonename.operator const DNSName&()) { - throw ApiException("RRset " + qname.toString() + " IN " + qtype.toString() + ": Cannot have both NS and DNAME except in zone apex"); - } - if (!new_records.empty() && domainInfo.kind == DomainInfo::Consumer) { - // Allow deleting all RRsets, just not modifying them. - throw ApiException("Modifying RRsets in Consumer zones is unsupported"); - } - if (!new_records.empty() && ent_present) { - QType qt_ent{0}; - if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qt_ent, new_records)) { - throw ApiException("Hosting backend does not support editing records."); - } - } - if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qtype, new_records)) { - throw ApiException("Hosting backend does not support editing records."); - } + replaceZoneRecords(domainInfo, zonename, new_records, qname, qtype); } if (replace_comments) { if (!domainInfo.backend->replaceComments(domainInfo.id, qname, qtype, new_comments)) { @@ -2477,12 +2488,10 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf } } } - else { - throw ApiException("Changetype not understood"); - } } - zone_disabled = (!backend.getSOAUncached(zonename, soaData)); + SOAData soaData; + bool zone_disabled = (!backend.getSOAUncached(zonename, soaData)); // edit SOA (if needed) if (!zone_disabled && !soa_edit_api_kind.empty() && !soa_edit_done) {