From 37109b4917199574aecb76229ad5b502e0ddcfdf Mon Sep 17 00:00:00 2001 From: nspadaccino Date: Tue, 3 Dec 2024 14:45:56 -0500 Subject: [PATCH 1/7] add logging for debug --- .../vinyldns/api/domain/record/RecordSetService.scala | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala b/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala index d61fac1e0..b35414b02 100644 --- a/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala +++ b/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala @@ -27,6 +27,7 @@ import vinyldns.core.domain.zone.{Zone, ZoneCommandResult, ZoneRepository} import vinyldns.core.queue.MessageQueue import cats.data._ import cats.effect.IO +import org.slf4j.{Logger, LoggerFactory} import org.xbill.DNS.ReverseMap import vinyldns.api.config.{ZoneAuthConfigs, DottedHostsConfig, HighValueDomainConfig} import vinyldns.api.domain.DomainValidations.{validateIpv4Address, validateIpv6Address} @@ -94,6 +95,8 @@ class RecordSetService( import RecordSetValidations._ import accessValidation._ + val logger: Logger = LoggerFactory.getLogger(classOf[RecordSetService]) + val approverOwnerShipTransferStatus = List(OwnerShipTransferStatus.ManuallyApproved , OwnerShipTransferStatus.AutoApproved, OwnerShipTransferStatus.ManuallyRejected) val requestorOwnerShipTransferStatus = List(OwnerShipTransferStatus.Cancelled , OwnerShipTransferStatus.Requested, OwnerShipTransferStatus.PendingReview) @@ -155,8 +158,12 @@ class RecordSetService( && !auth.isSuper && !auth.isGroupMember(existing.ownerGroupId.getOrElse("None"))) unchangedRecordSet(existing, recordSet).toResult else ().toResult _ <- if(existing.recordSetGroupChange.map(_.ownerShipTransferStatus).getOrElse("") == OwnerShipTransferStatus.Cancelled - && !auth.isSuper) - recordSetOwnerShipApproveStatus(recordSet).toResult else ().toResult + && !auth.isSuper) { + recordSetOwnerShipApproveStatus(recordSet).toResult + } else ().toResult + _ = logger.debug(s"updated recordsetgroupchange: ${recordSet.recordSetGroupChange}") + _ = logger.debug(s"existing recordsetgroupchange: ${existing.recordSetGroupChange}") +// recordSet <- if(zone.shared) updateRecordSetGroupChangeStatus(recordSet, existing, zone) recordSet <- updateRecordSetGroupChangeStatus(recordSet, existing, zone) change <- RecordSetChangeGenerator.forUpdate(existing, recordSet, zone, Some(auth)).toResult // because changes happen to the RS in forUpdate itself, converting 1st and validating on that From 2e4bf94126552ef03c3a3f6ba415f0e238a46cb2 Mon Sep 17 00:00:00 2001 From: nspadaccino Date: Tue, 3 Dec 2024 15:43:06 -0500 Subject: [PATCH 2/7] fix 422 errors for updating records in private zones from zone view --- .../scala/vinyldns/api/domain/record/RecordSetService.scala | 5 ++--- .../vinyldns/api/domain/record/RecordSetValidations.scala | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala b/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala index b35414b02..3c4b9809f 100644 --- a/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala +++ b/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetService.scala @@ -161,9 +161,8 @@ class RecordSetService( && !auth.isSuper) { recordSetOwnerShipApproveStatus(recordSet).toResult } else ().toResult - _ = logger.debug(s"updated recordsetgroupchange: ${recordSet.recordSetGroupChange}") - _ = logger.debug(s"existing recordsetgroupchange: ${existing.recordSetGroupChange}") -// recordSet <- if(zone.shared) updateRecordSetGroupChangeStatus(recordSet, existing, zone) + _ = logger.info(s"updated recordsetgroupchange: ${recordSet.recordSetGroupChange}") + _ = logger.info(s"existing recordsetgroupchange: ${existing.recordSetGroupChange}") recordSet <- updateRecordSetGroupChangeStatus(recordSet, existing, zone) change <- RecordSetChangeGenerator.forUpdate(existing, recordSet, zone, Some(auth)).toResult // because changes happen to the RS in forUpdate itself, converting 1st and validating on that diff --git a/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetValidations.scala b/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetValidations.scala index 81b7c484e..e3b80a4d3 100644 --- a/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetValidations.scala +++ b/modules/api/src/main/scala/vinyldns/api/domain/record/RecordSetValidations.scala @@ -495,7 +495,7 @@ object RecordSetValidations { existing: RecordSet ): Either[Throwable, Unit] = Either.cond( - updates.recordSetGroupChange == existing.recordSetGroupChange, + updates.recordSetGroupChange == existing.recordSetGroupChange || existing.recordSetGroupChange.isEmpty, (), InvalidRequest("Cannot update RecordSet OwnerShip Status when zone is not shared.") ) From 63577cebb96908ec319b8e42cae3b08af8b9e8ab Mon Sep 17 00:00:00 2001 From: nspadaccino Date: Wed, 4 Dec 2024 11:57:05 -0500 Subject: [PATCH 3/7] fix tests --- .../api/domain/record/RecordSetServiceIntegrationSpec.scala | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/api/src/it/scala/vinyldns/api/domain/record/RecordSetServiceIntegrationSpec.scala b/modules/api/src/it/scala/vinyldns/api/domain/record/RecordSetServiceIntegrationSpec.scala index 74bc18c07..3eaa2885f 100644 --- a/modules/api/src/it/scala/vinyldns/api/domain/record/RecordSetServiceIntegrationSpec.scala +++ b/modules/api/src/it/scala/vinyldns/api/domain/record/RecordSetServiceIntegrationSpec.scala @@ -128,7 +128,10 @@ class RecordSetServiceIntegrationSpec RecordSetStatus.Active, Instant.now.truncatedTo(ChronoUnit.MILLIS), None, - List(AAAAData("fd69:27cc:fe91::60")) + List(AAAAData("fd69:27cc:fe91::60")), + recordSetGroupChange = + Some(OwnerShipTransfer(ownerShipTransferStatus = OwnerShipTransferStatus.None, + requestedOwnerGroupId = None)) ) private val subTestRecordA = RecordSet( zone.id, From e7bc2c0e0d71f70e3deba428598a2e7504660ae8 Mon Sep 17 00:00:00 2001 From: nspadaccino Date: Wed, 4 Dec 2024 12:19:06 -0500 Subject: [PATCH 4/7] update tests --- .../test/functional/tests/recordsets/update_recordset_test.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py index 4139ce260..14f3eae8f 100644 --- a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py +++ b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py @@ -2433,6 +2433,8 @@ def test_update_owner_group_transfer_on_non_shared_zones_in_fails(shared_zone_te try: record_json = create_recordset(ok_zone, "test_update_success", "A", [{"address": "1.1.1.1"}]) + record_json["recordSetGroupChange"] = {"ownerShipTransferStatus": None, + "requestedOwnerGroupId": None} create_response = ok_client.create_recordset(record_json, status=202) update = ok_client.wait_until_recordset_change_status(create_response, "Complete")["recordSet"] From 29b1d6a7dbbad015598e7561f9ebef6ccda2b6cd Mon Sep 17 00:00:00 2001 From: nspadaccino Date: Wed, 4 Dec 2024 12:56:33 -0500 Subject: [PATCH 5/7] update test --- .../tests/recordsets/update_recordset_test.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py index 14f3eae8f..0092de343 100644 --- a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py +++ b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py @@ -303,7 +303,7 @@ def test_update_recordset_replace_2_records_with_1_different_record(shared_zone_ ] } result = client.create_recordset(new_rs, status=202) - + assert_that(result["changeType"], is_("Create")) assert_that(result["status"], is_("Pending")) assert_that(result["created"], is_not(none())) @@ -373,7 +373,7 @@ def test_update_existing_record_set_add_record(shared_zone_test_context): ] } result = client.create_recordset(new_rs, status=202) - + assert_that(result["changeType"], is_("Create")) assert_that(result["status"], is_("Pending")) assert_that(result["created"], is_not(none())) @@ -2432,9 +2432,17 @@ def test_update_owner_group_transfer_on_non_shared_zones_in_fails(shared_zone_te update_rs = None try: - record_json = create_recordset(ok_zone, "test_update_success", "A", [{"address": "1.1.1.1"}]) - record_json["recordSetGroupChange"] = {"ownerShipTransferStatus": None, - "requestedOwnerGroupId": None} + record_json = { + "zoneId": ok_zone, + "name": "test_update_success", + "type": "A", + "ttl": 38400, + "records": [ + {"address": "1.1.1.1"} + ], + "recordSetGroupChange": {"ownerShipTransferStatus": None, + "requestedOwnerGroupId": None} + } create_response = ok_client.create_recordset(record_json, status=202) update = ok_client.wait_until_recordset_change_status(create_response, "Complete")["recordSet"] From 6d799bc78ea955f638d7da6d28fc7e61c7b49abd Mon Sep 17 00:00:00 2001 From: nspadaccino Date: Wed, 4 Dec 2024 13:25:21 -0500 Subject: [PATCH 6/7] update test --- .../test/functional/tests/recordsets/update_recordset_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py index 0092de343..5687c581d 100644 --- a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py +++ b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py @@ -2432,8 +2432,9 @@ def test_update_owner_group_transfer_on_non_shared_zones_in_fails(shared_zone_te update_rs = None try: + # record_json = create_recordset(ok_zone, "test_update_success", "A", [{"address": "1.1.1.1"}], recordSetGroupChange={"ownerShipTransferStatus": None, "requestedOwnerGroupId": None}) record_json = { - "zoneId": ok_zone, + "zoneId": ok_zone["id"], "name": "test_update_success", "type": "A", "ttl": 38400, From 0ac47fcf4240f4d6952435f5197b283deccf0f0b Mon Sep 17 00:00:00 2001 From: nspadaccino Date: Wed, 4 Dec 2024 13:47:06 -0500 Subject: [PATCH 7/7] update test --- .../test/functional/tests/recordsets/update_recordset_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py index 5687c581d..b7f6930f6 100644 --- a/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py +++ b/modules/api/src/test/functional/tests/recordsets/update_recordset_test.py @@ -2441,7 +2441,7 @@ def test_update_owner_group_transfer_on_non_shared_zones_in_fails(shared_zone_te "records": [ {"address": "1.1.1.1"} ], - "recordSetGroupChange": {"ownerShipTransferStatus": None, + "recordSetGroupChange": {"ownerShipTransferStatus": "None", "requestedOwnerGroupId": None} }