feat: PO038 check KeyValueMapOperations MapName and identifier

README.md

@@ -386,6 +386,7 @@ This is the current list:
 |   |:white_check_mark:| PO035 | Quota policy hygiene | In a Quota policy, check element placement and other hygiene. |
 |   |:white_check_mark:| PO036 | ServiceCallout Response element usage | The Response element, when present, should specify a text value and no attributes. |
 |   |:white_check_mark:| PO037 | DataCapture policy hygiene | Checks that a Capture should uses a Source of type request when the policy is attached to the Response flow, and other checks. |
+|   |:white_check_mark:| PO038 | KeyValueMapOperations policy hygiene | Checks that MapName or mapIdentifier is specified, and not both.|
 | FaultRules |   |   |   |   |
 |   |:white_check_mark:| FR001 | No Condition on FaultRule | Use Condition elements on FaultRules, unless it is the fallback rule. |
 |   |:white_check_mark:| FR002 | DefaultFaultRule Structure | DefaultFaultRule should have only supported child elements, at most one AlwaysEnforce element, and at most one Condition element. |

lib/package/plugins/PO038-kvm-mapname.js

@@ -0,0 +1,135 @@
+/*
+  Copyright 2019-2024 Google LLC
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+      https://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+*/
+
+const xpath = require("xpath"),
+  util = require("util"),
+  ruleId = require("../myUtil.js").getRuleId(),
+  debug = require("debug")("apigeelint:" + ruleId);
+
+const plugin = {
+  ruleId,
+  name: "KVM/MapName",
+  fatal: false,
+  severity: 1, // 1=warning, 2=error
+  nodeType: "Policy",
+  enabled: true,
+};
+
+let profile = "apigee";
+const onBundle = function (bundle, cb) {
+  debug(`onBundle()`);
+  profile = bundle.profile;
+  if (typeof cb == "function") {
+    cb(null, false);
+  }
+};
+
+const onPolicy = function (policy, cb) {
+  let flagged = false;
+  const addMessage = (line, column, message) => {
+    policy.addMessage({ plugin, message, line, column });
+    flagged = true;
+  };
+  try {
+    if (policy.getType() === "KeyValueMapOperations") {
+      const mapIdentifier = xpath.select(
+        "/KeyValueMapOperations/@mapIdentifier",
+        policy.getElement(),
+      );
+      const mapNameNodeset = xpath.select(
+        "/KeyValueMapOperations/MapName",
+        policy.getElement(),
+      );
+      debug(
+        `${policy.fileName} found ${mapNameNodeset.length} Set/Payload elements`,
+      );
+      if (profile == "apigeex") {
+        if (mapNameNodeset.length > 1) {
+          mapNameNodeset
+            .slice(1)
+            .forEach((node) =>
+              addMessage(
+                node.lineNumber,
+                node.columnNumber,
+                "use at most one MapName element",
+              ),
+            );
+        }
+        else if (mapNameNodeset.length == 1) {
+          if (mapIdentifier && mapIdentifier[0]) {
+            addMessage(
+              mapNameNodeset[0].lineNumber,
+              mapNameNodeset[0].columnNumber,
+              "use mapIdentifier attribute or MapName element, not both",
+            );
+          }
+          let text = xpath.select('text()', mapNameNodeset[0]);
+          text = text && text[0] && text[0].data;
+          let ref = xpath.select('@ref', mapNameNodeset[0]);
+          debug('ref: ' + util.format(ref));
+          ref = ref && ref[0] && ref[0].value;
+
+          if ( ! text && !ref ) {
+            addMessage(
+              mapNameNodeset[0].lineNumber,
+              mapNameNodeset[0].columnNumber,
+              "The MapName element must specify a @ref attribute, or a text value"
+            );
+          }
+
+        } else {
+          // no MapName element
+          if ( ! mapIdentifier || !mapIdentifier[0]) {
+            addMessage(
+              policy.getElement().lineNumber,
+              policy.getElement().columnNumber,
+              "Specify the map name via either the mapIdentifier attribute, or the MapName element",
+            );
+          }
+        }
+      } else {
+        if (mapNameNodeset.length > 0) {
+          mapNameNodeset.forEach((node) =>
+            addMessage(
+              node.lineNumber,
+              node.columnNumber,
+              "inappropriate MapName element for apigee profile",
+            ),
+          );
+        }
+        if (!mapIdentifier || !mapIdentifier[0]) {
+          addMessage(
+            policy.getElement().lineNumber,
+            policy.getElement().columnNumber,
+            "missing mapIdentifier attribute",
+          );
+        }
+      }
+    }
+
+    if (typeof cb == "function") {
+      cb(null, flagged);
+    }
+  } catch (e) {
+    debug(util.format(e));
+  }
+};
+
+module.exports = {
+  plugin,
+  onBundle,
+  onPolicy,
+};

test/fixtures/resources/PO038/fail/KVM-MapName-apigee.xml

@@ -0,0 +1,10 @@
+<KeyValueMapOperations name="KVM-MapName-apigeex">
+  <MapName ref="OrganizationMapName"/>
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/fail/KVM-both-MapName-and-mapIdentifier-apigee.xml

@@ -0,0 +1,10 @@
+<KeyValueMapOperations name="KVM-both-MapName-and-mapIdentifier-apigee" mapIdentifier='foobar'>
+  <MapName ref="OrganizationMapName"/>
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/fail/KVM-both-MapName-and-mapIdentifier-apigeex.xml

@@ -0,0 +1,10 @@
+<KeyValueMapOperations name="KVM-both-MapName-and-mapIdentifier-apigeex" mapIdentifier='foobar'>
+  <MapName ref="OrganizationMapName"/>
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/fail/KVM-empty-MapName-apigeex.xml

@@ -0,0 +1,10 @@
+<KeyValueMapOperations name="KVM-empty-MapName-apigeex">
+  <MapName/><!-- empty -->
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/fail/KVM-missing-mapIdentifier-apigee.xml

@@ -0,0 +1,9 @@
+<KeyValueMapOperations name="KVM-missing-mapIdentifier-apigee" identifier='foobar'>
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/fail/KVM-neither-MapName-nor-mapIdentifier-apigeex.xml

@@ -0,0 +1,12 @@
+<KeyValueMapOperations name="KVM-neither-MapName-nor-mapIdentifier-apigeex" identifier='foobar'>
+  <!--
+  <MapName ref="OrganizationMapName"/>
+  -->
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/fail/messages.js

@@ -0,0 +1,22 @@
+// These are the error messages only for the failed policies.
+module.exports = {
+  "KVM-empty-MapName-apigeex.xml":
+    "The MapName element must specify a @ref attribute, or a text value",
+
+  "KVM-MapName-apigee.xml": [
+    "inappropriate MapName element for apigee profile",
+    "missing mapIdentifier attribute"
+  ],
+
+  "KVM-neither-MapName-nor-mapIdentifier-apigeex.xml":
+    "Specify the map name via either the mapIdentifier attribute, or the MapName element",
+
+  "KVM-both-MapName-and-mapIdentifier-apigeex.xml":
+    "use mapIdentifier attribute or MapName element, not both",
+
+  "KVM-both-MapName-and-mapIdentifier-apigee.xml":
+    "inappropriate MapName element for apigee profile",
+
+  "KVM-missing-mapIdentifier-apigee.xml":
+    "missing mapIdentifier attribute"
+};

test/fixtures/resources/PO038/pass/KVM-MapName-apigeex.xml

@@ -0,0 +1,10 @@
+<KeyValueMapOperations name="KVM-MapName-apigeex">
+  <MapName ref="OrganizationMapName"/>
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/pass/KVM-just-mapIdentifier-apigee.xml

@@ -0,0 +1,9 @@
+<KeyValueMapOperations name="KVM-just-mapIdentifier-apigee" mapIdentifier='foobar'>
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/fixtures/resources/PO038/pass/KVM-just-mapIdentifier-apigeex.xml

@@ -0,0 +1,9 @@
+<KeyValueMapOperations name="KVM-just-mapIdentifier-apigee" mapIdentifier='foobar'>
+  <ExpiryTimeInSecs>300</ExpiryTimeInSecs>
+  <Get assignTo="JwkURI">
+    <Key>
+      <Parameter>jwk_uri</Parameter>
+    </Key>
+  </Get>
+  <Scope>organization</Scope>
+</KeyValueMapOperations>

test/specs/PO038-test.js

@@ -0,0 +1,119 @@
+/*
+  Copyright 2019-2024 Google LLC
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+      https://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+*/
+
+/* global describe, it */
+
+const testID = "PO038",
+  assert = require("assert"),
+  fs = require("fs"),
+  util = require("util"),
+  path = require("path"),
+  bl = require("../../lib/package/bundleLinter.js"),
+  plugin = require(bl.resolvePlugin(testID)),
+  Policy = require("../../lib/package/Policy.js"),
+  Dom = require("@xmldom/xmldom").DOMParser,
+  rootDir = path.resolve(__dirname, "../fixtures/resources/PO038"),
+  debug = require("debug")(`apigeelint:${testID}-test`);
+
+const loadPolicy = (sourceDir, shortFileName) => {
+  const fqPath = path.join(sourceDir, shortFileName),
+    policyXml = fs.readFileSync(fqPath).toString("utf-8"),
+    doc = new Dom().parseFromString(policyXml),
+    p = new Policy(doc.documentElement, this);
+  p.getElement = () => doc.documentElement;
+  p.fileName = shortFileName;
+  return p;
+};
+
+describe(`${testID} - policy passes KVM hygiene check`, function () {
+  const sourceDir = path.join(rootDir, "pass");
+  const testOne = (shortFileName) => {
+    const policy = loadPolicy(sourceDir, shortFileName);
+    const profile = shortFileName.split(".")[0].split("-").slice(-1)[0];
+    const policyType = policy.getType();
+    it(`check ${shortFileName} passes`, () => {
+      assert.notEqual(policyType, undefined, `${policyType} should be defined`);
+      plugin.onBundle({ profile });
+      plugin.onPolicy(policy);
+      const report = policy.getReport();
+      const foundIssues = report.errorCount != 0 || report.warningCount != 0;
+      const messages = report.messages;
+      debug(`pass ${shortFileName} issues: ${foundIssues}`);
+      debug(`pass ${shortFileName} messages: ${util.format(messages)}`);
+      assert.equal(foundIssues, false, "should be no issues");
+      assert.ok(messages, "messages should exist");
+      assert.equal(messages.length, 0, "unexpected number of messages");
+    });
+  };
+
+  fs.readdirSync(sourceDir)
+    .filter((shortFileName) => shortFileName.endsWith(".xml"))
+    .forEach(testOne);
+});
+
+describe(`${testID} - policy does not pass KVM hygiene check`, () => {
+  const sourceDir = path.join(rootDir, "fail");
+  const expectedErrorMessages = require(path.join(sourceDir, "messages.js"));
+  const testOne = (shortFileName) => {
+    const policy = loadPolicy(sourceDir, shortFileName);
+    const profile = shortFileName.split(".")[0].split("-").slice(-1)[0];
+    const policyType = policy.getType();
+    it(`check ${shortFileName} throws error`, () => {
+      assert.notEqual(policyType, undefined, `${policyType} should be defined`);
+      plugin.onBundle({ profile });
+      plugin.onPolicy(policy);
+      const report = policy.getReport();
+      const foundIssues = report.errorCount != 0 || report.warningCount != 0;
+      debug(`onPolicy: ${policy.getName()}`);
+      assert.equal(true, foundIssues, "should be issues");
+      const messages = report.messages;
+      assert.ok(messages, "messages should exist");
+      debug(util.format(messages));
+      let expectedList = expectedErrorMessages[policy.fileName];
+      assert.ok(
+        expectedList,
+        "test configuration failure: did not find an expected message",
+      );
+      if (typeof expectedList == "string") {
+        expectedList = [expectedList];
+      }
+      assert.equal(
+        expectedList.length,
+        messages.length,
+        "unexpected number of messages",
+      );
+
+      expectedList.forEach((expected, ix) => {
+        debug(`expected: ${expected}`);
+        debug(`actual: ${messages[ix].message}`);
+        assert.ok(messages[ix]);
+        assert.ok(messages[ix].message);
+        assert.equal(typeof messages[ix].message, "string");
+        assert.equal(typeof expected, "string");
+        assert.equal(
+          expected,
+          messages[ix].message,
+          "did not find expected message",
+        );
+      });
+
+    });
+  };
+
+  fs.readdirSync(sourceDir)
+    .filter((shortFileName) => shortFileName.endsWith(".xml"))
+    .forEach(testOne);
+});