From f9ddd7eaedf172a85c57c9fa0eccd2dcc386f6d2 Mon Sep 17 00:00:00 2001 From: Anthony Seure Date: Thu, 21 Mar 2024 17:46:14 +0100 Subject: [PATCH] CUSTESC-36595: Try to prioritize active zone over non-active ones **Problem** As part of an internal issue named CUSTESC-36595, one customer reported us that his Cloudflare Wordpress plugin was reporting that his dashboard was failing with the following error: ```json "errors.noActiveZoneSelected": "It looks like your domain {domain} is not provisioned with Cloudflare. Please continue to {link} to secure and speed up your website.", ``` The problem was that the customer does have two Cloudflare zones with the same name: one being active and the other got purged. **Previous investigations** We initially tried to fix this in the `Cloudflare-Wordpress` plugin itself, via https://github.com/cloudflare/Cloudflare-WordPress/pull/532, by ensuring that the zone listing endpoint `/zones` was requested with proper `status=active` query parameter to only retrieve active zone, if any. However, the problem is not fixed on customers' side. **Current investigation** After looking once again at their HAR files, besides the endpoint itself, we noticed that the initiator of the call was actually coming from the compiled and minifed JavaScript file from this `cloudflare-plugin-frontend` project. This plugin also requests this zone listing endpoint `/zones` but it also seems to use the zone list, so I did not want to change the logic too much and retrieve only the active zones. Instead, I found out the two places where that list of zones was used and from which a single zone was being extracted. **Fixing `src/actions/zoneProvision.js`** For `src/actions/zoneProvision.js`, it seems we pass the list of zones from the Cloudflare API to a dependency called `normalizr` which normalize based on the name field here https://github.com/cloudflare/cloudflare-plugin-frontend/blob/master/src/constants/Schemas.js#L13-L15, which effectively removes duplicates, which was tested locally with: ```js import { normalize, Schema, arrayOf } from 'normalizr'; const zones = [ {'id': 1, 'name': 'cloudflare.com', 'status': 'purged'}, {'id': 2, 'name': 'cloudflare.com', 'status': 'active'}, ]; const zoneSchema = new Schema('zones', { idAttribute: 'name' }); const normalizedZones = normalize(zones, arrayOf(zoneSchema)); console.log(JSON.stringify(normalizedZones, '', null)); ``` which outputs the following: ``` When merging two zones, found unequal data in their "id" values. Using the earlier value. 1 2 When merging two zones, found unequal data in their "status" values. Using the earlier value. purged active {"entities":{"zones":{"cloudflare.com":{"id":1,"name":"cloudflare.com","status":"purged"}}},"result":["cloudflare.com","cloudflare.com"]} ``` This could indeed correlates with our problem considered that the purged zone of the customer was indeed created before the active zone. Should they discriminate based on their `id` field, the active zone would indeed be discarded. To prevent that issue, we are introducing a `deduplicateOnActiveZones()` helper which deduplicates the list of zones whenever at least one active zone is present. By doing so, the invocation to `normalizr` should now only retain the active zone, and the following `asyncZoneSetActiveZone` Redux dispatch receive that zone. **Fixing `src/actions/zones.js`** For `src/actions/zones.js`, it seems that only the first zone from the zone list is being used, if defined. For that one, we simply added an extra logic to attempt at extracting the first active zone first, if any, and fallback to the original logic if no active zone is found. --- src/actions/zoneProvision.js | 4 +- src/actions/zones.js | 7 +++- src/test/utils/deduplicateZonesTest.js | 54 ++++++++++++++++++++++++++ src/utils/utils.js | 28 +++++++++++++ 4 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 src/test/utils/deduplicateZonesTest.js diff --git a/src/actions/zoneProvision.js b/src/actions/zoneProvision.js index e2b4825..0542a60 100644 --- a/src/actions/zoneProvision.js +++ b/src/actions/zoneProvision.js @@ -13,6 +13,7 @@ import * as ActionTypes from '../constants/ActionTypes'; import { asyncZoneSetActiveZone } from './activeZone'; import { normalizeZoneGetAll } from '../constants/Schemas'; import { zoneFetch, zoneFetchSuccess } from './zones'; +import { deduplicateOnActiveZones } from '../utils/utils'; /* * Zone Provision actions still use reducers/zones.js as the reducer @@ -136,7 +137,8 @@ function asyncSetHostAPIProvisionedDomainActive(domainName) { zoneGetAll(function(error, response) { if (response) { dispatch(zoneFetchSuccess(response.body.result)); - let normalizedZoneList = normalizeZoneGetAll(response.body.result); + const zoneList = deduplicateOnActiveZones(response.body.result); + let normalizedZoneList = normalizeZoneGetAll(zoneList); dispatch( asyncZoneSetActiveZone(normalizedZoneList.entities.zones[domainName]) ); diff --git a/src/actions/zones.js b/src/actions/zones.js index 1dc62f4..2a75298 100644 --- a/src/actions/zones.js +++ b/src/actions/zones.js @@ -70,7 +70,12 @@ export function asyncFetchZones() { zoneGetAll(function(error, response) { if (response) { dispatch(zoneFetchSuccess(response.body.result)); - if (response.body.result[0]) { + const activeZone = response.body.result.find( + zone => zone.status === 'active' + ); + if (activeZone) { + dispatch(zoneSetActiveZoneIfEmpty(activeZone)); + } else if (response.body.result[0]) { dispatch(zoneSetActiveZoneIfEmpty(response.body.result[0])); } } else { diff --git a/src/test/utils/deduplicateZonesTest.js b/src/test/utils/deduplicateZonesTest.js new file mode 100644 index 0000000..7e478d0 --- /dev/null +++ b/src/test/utils/deduplicateZonesTest.js @@ -0,0 +1,54 @@ +import { deduplicateOnActiveZones } from '../../utils/utils'; +import expect from 'expect'; + +describe('deduplicateZones', () => { + it('should not change a list of zones without any duplicate', () => { + expect( + deduplicateOnActiveZones([ + { id: '1', name: 'cloudflare.com', status: 'active' }, + { id: '2', name: 'blog.cloudflare.com', status: 'active' } + ]) + ).toEqual([ + { id: '1', name: 'cloudflare.com', status: 'active' }, + { id: '2', name: 'blog.cloudflare.com', status: 'active' } + ]); + }); + + it('should remove non-active zone in case of duplicates', () => { + expect( + deduplicateOnActiveZones([ + { id: '1', name: 'cloudflare.com', status: 'active' }, + { id: '2', name: 'blog.cloudflare.com', status: 'active' }, + { id: '3', name: 'cloudflare.com', status: 'purged' } + ]) + ).toEqual([ + { id: '1', name: 'cloudflare.com', status: 'active' }, + { id: '2', name: 'blog.cloudflare.com', status: 'active' } + ]); + }); + + it('should not remove duplicates when there is no active zone', () => { + expect( + deduplicateOnActiveZones([ + { id: '1', name: 'cloudflare.com', status: 'pending' }, + { id: '2', name: 'blog.cloudflare.com', status: 'active' }, + { id: '3', name: 'cloudflare.com', status: 'purged' } + ]) + ).toEqual([ + { id: '1', name: 'cloudflare.com', status: 'pending' }, + { id: '2', name: 'blog.cloudflare.com', status: 'active' }, + { id: '3', name: 'cloudflare.com', status: 'purged' } + ]); + }); + + it('should remove multiple duplicates when there is one active zone', () => { + expect( + deduplicateOnActiveZones([ + { id: '1', name: 'cloudflare.com', status: 'pending' }, + { id: '2', name: 'cloudflare.com', status: 'purged' }, + { id: '3', name: 'cloudflare.com', status: 'active' }, + { id: '4', name: 'cloudflare.com', status: 'moved' } + ]) + ).toEqual([{ id: '3', name: 'cloudflare.com', status: 'active' }]); + }); +}); diff --git a/src/utils/utils.js b/src/utils/utils.js index ee496b9..dc1bf61 100644 --- a/src/utils/utils.js +++ b/src/utils/utils.js @@ -70,3 +70,31 @@ export function formatMessageForIntegration( return formatMessage({ id: messageId }); } + +// deduplicateOnActiveZones was introduced as a potential fix for CUSTESC-36595. The goal of this function is to +// deduplicate the list of zones returned by the Cloudflare API based on the status of the zone. This way, we hope to +// guarantee that when normalizing the list of zones, only the active zone would show up in case of duplicates. For +// instance, in the mentioned tickets, two zones were potentially returned: a purged as well as an active zone. +export function deduplicateOnActiveZones(zones) { + let filteredZones = []; + const isActive = z => z.status === 'active'; + const isSameZone = (z1, z2) => z1.name === z2.name; + + zones.forEach(zone => { + if (isActive(zone)) { + // If the zone is active, we first remove all existing duplicates and then insert the active zone. + filteredZones = filteredZones.filter(fZone => !isSameZone(zone, fZone)); + filteredZones.push(zone); + } else { + // If the zone is not active, we only insert it if there is no existing active zone. + const alreadyHasActiveZone = filteredZones.some( + fZone => isSameZone(zone, fZone) && isActive(fZone) + ); + if (!alreadyHasActiveZone) { + filteredZones.push(zone); + } + } + }); + + return filteredZones; +}